From 251498ad587d46ec62f83ec77225a17256eb28fb Mon Sep 17 00:00:00 2001 From: Malin Date: Sat, 15 Aug 2026 21:28:05 +0200 Subject: [PATCH] bastille-jail-provisioning: add FreeBSD-adapted script safety patterns Merge in a checklist-style "provisioning script safety" section, adapted from wshobson/agents' bash-defensive-patterns skill (fetched directly from GitHub, MIT licensed) rather than trusted from its catalog summary. Covers: shell-choice caution (#!/bin/bash isn't guaranteed on FreeBSD; bash is a package at /usr/local/bin/bash, not base), a contextual error-handling philosophy instead of a blanket `set -Eeuo pipefail`, trap-based cleanup/logging for scripts interrupted mid-jail-creation (without auto-rollback, which can worsen the known destroy/create IP churn issue), jail-name/IP input validation before destructive `bastille` commands, and FreeBSD `mktemp`/`sed -i`/`date` syntax differences from GNU. Deliberately dropped the upstream's interactive `rm -rI` cleanup pattern (wrong for unattended SSH automation) and did not import retry/locking/ShellCheck-gate advice that wasn't actually present in the real upstream files. Cross-references remote-shell-quoting-safety instead of duplicating its nested-shell-quoting content. Also documents the /usr/local/bastille vs /www/bastille bastille_prefix split already present across this fleet's hosts (gringo/staging vs granja), which the new script-safety guidance assumes readers already know not to hardcode. Full worked patterns and code examples live in the new references/script-safety.md; SKILL.md keeps the load-bearing summary. Co-Authored-By: Claude Sonnet 5 --- skills/bastille-jail-provisioning/SKILL.md | 60 +++++ .../references/script-safety.md | 241 ++++++++++++++++++ 2 files changed, 301 insertions(+) create mode 100644 skills/bastille-jail-provisioning/references/script-safety.md diff --git a/skills/bastille-jail-provisioning/SKILL.md b/skills/bastille-jail-provisioning/SKILL.md index 4290047..5dc5399 100644 --- a/skills/bastille-jail-provisioning/SKILL.md +++ b/skills/bastille-jail-provisioning/SKILL.md @@ -265,3 +265,63 @@ exist under that name in the FreeBSD ports tree) — and remember Valkey's ACL means any code connecting to it needs an explicit `AUTH` step, not just `connect()`; a `connect()`-only implementation will silently fail every subsequent command with `NOAUTH` against this fleet's standard Valkey setup. + +## Bastille filesystem prefix varies by host — check before hardcoding a path + +Don't assume `/usr/local/bastille/jails//root/...` — that's only +the convention where `bastille.conf` uses the default `bastille_prefix` +(confirmed on gringo and staging). A host with a custom +`bastille_prefix` (e.g. `granja`, where storage was repointed at a +dedicated `wwwpool` with `bastille_prefix=/www/bastille`) puts jails +under `/www/bastille/jails//root/...` instead — writing to the +`/usr/local/bastille/...` path on that host silently succeeds (creates a +new, wrong directory) rather than erroring, since nothing stops you from +writing to an arbitrary path. **Check the host's actual +`bastille_prefix`** (`grep bastille_prefix /etc/rc.conf` or +`bastille.conf`) before hardcoding either path in a script. + +## Provisioning script safety + +Applies to any shell script written to automate part of this +provisioning flow (not the one-off interactive commands above). Full +worked patterns, including FreeBSD-specific `mktemp`/`sed`/`date` syntax, +a trap-based cleanup template, and jail-name/IP input validation, live in +`references/script-safety.md` — read that before writing a new +provisioning script. Summary of the load-bearing points: + +- **Don't assume `#!/bin/bash` exists.** FreeBSD base ships `/bin/sh` + (POSIX-ish ash), not bash — bash is a package that installs to + `/usr/local/bin/bash`, never `/bin/bash`, and many freshly cloned jails + won't have it installed at all. Default to `#!/bin/sh` and POSIX + constructs unless bash-only features are genuinely needed and you've + confirmed bash is present on the target. +- **Error handling is contextual, not a blanket `set -Eeuo pipefail`.** + Turn on `set -eu` for the whole script, but explicitly tolerate steps + that are allowed to fail (e.g. an already-installed package, an + optional service restart) with a commented `|| true` rather than either + aborting the whole provisioning run on a non-fatal hiccup or suppressing + errors wholesale. `-o pipefail`/`-E` are bash-only and unavailable under + `/bin/sh`. +- **Trap cleanup, don't trap auto-rollback.** A script that dies partway + through jail creation should log exactly which step it reached (jail + created? DB granted? vhost written?) so a human can clean up — see the + create/destroy IP-alias churn gotcha above for why an automatic + `bastille destroy` inside a failure-path trap can make things worse, not + better. +- **Validate jail names and IPs before they reach a destructive `bastille` + command.** `bastille destroy ` has no confirmation prompt. + Validate with plain POSIX `case`/`[` pattern matching (works under both + `/bin/sh` and bash) before any value — especially one derived from + outside the script — is interpolated into a command. + +For quoting content through nested shell layers (`ssh` → `bastille cmd` +→ `su`), use the separate `remote-shell-quoting-safety` skill — that +problem is not duplicated here. + +Provenance: the script-safety patterns above are adapted from +wshobson/agents' `bash-defensive-patterns` skill +(https://github.com/wshobson/agents, +`plugins/shell-scripting/skills/bash-defensive-patterns`, MIT licensed, +fetched 2026-08-15) — rewritten for FreeBSD/Bastille where upstream +assumed GNU/Linux; see `references/script-safety.md` for exactly what was +kept, rewritten, or dropped. diff --git a/skills/bastille-jail-provisioning/references/script-safety.md b/skills/bastille-jail-provisioning/references/script-safety.md new file mode 100644 index 0000000..b491e1f --- /dev/null +++ b/skills/bastille-jail-provisioning/references/script-safety.md @@ -0,0 +1,241 @@ +# Provisioning script safety — FreeBSD/Bastille worked patterns + +Deeper worked examples for the "Provisioning script safety" section of +`SKILL.md`. Read this when the summary there isn't enough to actually +write the script. + +**Provenance**: adapted from wshobson/agents' `bash-defensive-patterns` +skill (https://github.com/wshobson/agents, +`plugins/shell-scripting/skills/bash-defensive-patterns/{SKILL.md,references/details.md}`, +MIT licensed, fetched 2026-08-15). That upstream skill is written for +Linux/GNU and knows nothing about FreeBSD base tools, `/usr/local/bin/bash`, +Bastille, rc.d, ZFS, or PF. Every pattern below was checked against FreeBSD +15.1-RELEASE behavior and either kept, rewritten, or explicitly dropped — +see "What was deliberately left out" at the end. Nothing here was copied +from the upstream examples unchanged. + +For shell-quoting-through-multiple-hops (`ssh` → `bastille cmd` → `su`), +see the `remote-shell-quoting-safety` skill instead — not duplicated here. + +## Shell choice: don't assume `#!/bin/bash` exists + +FreeBSD base's `/bin/sh` is a modified `ash` (POSIX-ish), not bash. Bash +is **not** part of FreeBSD base — it's a package (`pkg install bash`) +that installs to `/usr/local/bin/bash`, **never** `/bin/bash`. A +`#!/bin/bash` shebang written for a Linux-oriented example fails +immediately with "No such file or directory" on a bare FreeBSD host or a +freshly cloned jail — many base jail images don't have bash installed at +all. + +- If bash-only features are genuinely needed (arrays, `[[ ]]`, `mapfile`, + process substitution `< <(...)`), use `#!/usr/bin/env bash` **and** + verify bash is actually present on the target first — + `bastille cmd which bash` or `pkg info bash` on the host. Don't + assume. +- For anything that runs early in provisioning (before you know what's on + the target) or where plain POSIX does the job, prefer `#!/bin/sh` — it's + guaranteed present on both the host and every jail, no package + dependency. Drop arrays, `[[ ]]`, `mapfile`/`readarray`, and process + substitution; use `case`, `$( )`, and POSIX `[ ]` test instead. + +## Error-handling philosophy: contextual, not a blanket `set -Eeuo pipefail` + +The upstream checklist's rule #1 is "always use strict mode." Treating +that as an absolute is wrong for provisioning scripts: + +- `set -e` (and `set -u`) are genuinely valuable for early validation — + bad jail name, IP already taken, missing base release — where any + failure should hard-stop before anything destructive happens. +- Once a script is deep into a multi-step provisioning sequence, some + steps are legitimately allowed to fail without aborting the whole run — + e.g. `pkg install -y arp-scan` when it may already be installed, or + restarting an optional service that doesn't exist on every base image + yet. Blanket `-e` turns those into full-script aborts, which is exactly + how you get the half-created-jail mess this skill warns about elsewhere. +- Practical rule for this fleet: turn strict mode on for the whole script + (`set -eu`; add `-o pipefail` only if it's genuinely running under + bash), but explicitly wrap any step that's allowed to fail — + `cmd || true` or `if ! cmd; then log_warn "..."; fi` — with a comment + saying *why* it's non-fatal. Silent blanket suppression (`set +e` + everywhere) is worse than blanket strict mode; the goal is a script + where every deliberately-tolerated failure is visible in a diff, not one + with no error handling at all. +- `-o pipefail` and `-E` (ERR trap inherited into functions) are bash + extensions, unavailable under POSIX `/bin/sh`. If the script is + `/bin/sh`, restructure instead of relying on them — e.g. write a + pipeline's output to a temp file and check that command's own exit + status, rather than depending on an upstream stage's exit code surviving + a pipe. + +## Trap-based cleanup for interrupted provisioning + +A half-created jail is a real mess: `bastille create` partway through, a +DB created but no grant, an nginx vhost written but not enabled, a pf +table entry added but the jail never started. A script that dies partway +(Ctrl-C, a dropped SSH session, a later step's genuine failure) should at +minimum log exactly how far it got — automatically rolling back a jail +creation from inside a trap is its own risk (see this skill's +create/destroy-churn IP-alias gotcha; automating teardown under failure +conditions can make the mess worse, not better). + +```sh +# POSIX-compatible: /bin/sh, no bash-only trap features needed +PROVISION_LOG="/tmp/provision-${JAIL_NAME}.log" +CREATED_JAIL="" +CREATED_DB="" +CURRENT_STEP="" + +cleanup() { + status=$? + if [ "$status" -ne 0 ]; then + { + echo "FAILED at step: ${CURRENT_STEP:-unknown} (exit $status)" + echo "Partial state: jail=${CREATED_JAIL:-none} db=${CREATED_DB:-none}" + echo "Manual cleanup needed before retrying with the same name/IP." + } >> "$PROVISION_LOG" + echo "Provisioning failed — see $PROVISION_LOG" >&2 + fi +} +trap cleanup EXIT INT TERM + +CURRENT_STEP="bastille create" +bastille create "$JAIL_NAME" 15.1-RELEASE "$JAIL_IP" vtnet0 +CREATED_JAIL="$JAIL_NAME" + +CURRENT_STEP="db grant" +# ... etc, set CURRENT_STEP before each major step +``` + +Set `CURRENT_STEP` before each major step so the log names the actual +failed step, not just "something failed." + +## Input validation before jail names/IPs reach a destructive command + +Bastille commands take a jail name and/or IP as a bare positional +argument with no confirmation prompt — `bastille destroy ` is +irreversible. Validate **before** the value is interpolated into any +`bastille` command, especially if it ever originates from outside the +script (a ticket description, a generated name, pasted text): + +```sh +validate_jail_name() { + name="$1" + case "$name" in + ''|*[!a-zA-Z0-9_-]*) + echo "ERROR: invalid jail name: '$name' (letters, digits, - and _ only)" >&2 + return 1 + ;; + esac + if bastille list all 2>/dev/null | grep -qx "$name"; then + echo "NOTE: jail '$name' already exists" >&2 + fi +} + +validate_ipv4() { + ip="$1" + case "$ip" in + *[!0-9.]*|'') echo "ERROR: not an IPv4 literal: '$ip'" >&2; return 1 ;; + esac + IFS=. read -r o1 o2 o3 o4 </dev/null || { + echo "ERROR: octet out of range in '$ip'" >&2 + return 1 + } + done +} +``` + +This is intentionally plain `case`/`[` pattern matching, not a regex +call — it works identically under `/bin/sh` and bash, and avoids needing +`[[ =~ ]]` (bash-only) or GNU `grep -P`. Run `arp-scan` (see the main +SKILL.md's IP-conflict section) only **after** `validate_ipv4` passes, not +before. + +## Safe temporary files: FreeBSD `mktemp` syntax + +FreeBSD's base `mktemp` is BSD mktemp, not GNU coreutils mktemp — +long options like `--tmpdir=DIR` or `--suffix=` don't exist there. + +```sh +# Portable on FreeBSD and Linux: bare mktemp / mktemp -d, no custom template +TMPFILE=$(mktemp) || { echo "ERROR: mktemp failed" >&2; exit 1; } +TMPDIR=$(mktemp -d) || { echo "ERROR: mktemp -d failed" >&2; exit 1; } + +# A custom prefix/path needs an explicit trailing run of X's on BOTH +# platforms — a prefix alone (no X's) is not enough on either: +TMPFILE=$(mktemp "/tmp/provision-${JAIL_NAME}.XXXXXX") || exit 1 +``` + +Clean up with a trap (see above), and always quote and use `--`: +`rm -rf -- "$TMPDIR"` — the `--` guards against a temp path that could +otherwise be parsed as an option if it somehow started with `-`. + +## `sed -i` and `date`: don't copy GNU-only invocations into jail.conf/pf.conf edits + +Two gotchas that bite specifically because provisioning steps edit +`jail.conf` and `pf.conf` in place: + +- **`sed -i`**: FreeBSD/BSD sed requires an explicit (possibly empty) + backup-suffix argument as its own token: + `sed -i '' -e 's/old/new/' file`. GNU sed's `-i` only takes the suffix + glued to the flag (`-i.bak` or bare `-i`) — passing `-i ''` as two + separate arguments to GNU sed does **not** mean "no backup"; it's + parsed differently and can silently do the wrong thing. These two forms + are not cross-compatible. Use the BSD form on this fleet's FreeBSD + hosts, don't copy a Linux-written example verbatim. +- **`date`**: FreeBSD date has no GNU-style free-text parsing + (`date -d "1 day ago"` doesn't work). Use `-v` adjustments + (`date -v-1d +%F`) or `-j -f +` to parse + a fixed format. A script that only timestamps a log line + (`date +'%Y-%m-%d %H:%M:%S'`, fixed format, no relative math) is fine + as-is on both platforms — the trap is only when a script tries to + compute a relative date. + +## Structured logging (portable as written) + +The upstream timestamped-log-level pattern is fine to reuse as-is — +fixed-format `date`, no relative parsing, identical under FreeBSD and GNU +date: + +```sh +log_info() { echo "[$(date +'%Y-%m-%d %H:%M:%S')] INFO: $*" >&2; } +log_warn() { echo "[$(date +'%Y-%m-%d %H:%M:%S')] WARN: $*" >&2; } +log_error() { echo "[$(date +'%Y-%m-%d %H:%M:%S')] ERROR: $*" >&2; } +``` + +## What was deliberately left out + +- **Arrays, `mapfile`/`readarray`, `[[ ]]`, process substitution + (`< <(...)`)** — all bash-only, and bash isn't guaranteed present on a + freshly cloned jail. Only reach for them once you've confirmed the + target will always run under a real installed bash; default to POSIX + `/bin/sh` constructs for anything that has to run early or on an + unknown target. +- **The upstream `rm -rI` "safe cleanup" pattern** — `-I` prompts for + interactive confirmation, which is the opposite of what an unattended + provisioning script run over SSH (often with no TTY) needs; a script + waiting on a prompt that can never be answered just hangs. Validate + inputs hard (see above) instead, then remove non-interactively + (`rm -rf --`). +- **Retry/backoff and file-locking patterns** from the general upstream + checklist — not reproduced here because the fetched upstream content + (`SKILL.md` + `references/details.md`, both fetched directly from + GitHub) doesn't actually contain a worked retry or locking pattern + specific enough to adapt; those topics only appear as chapter-title-level + mentions in secondhand catalog summaries, not in the real file. If a + real retry-needing case shows up in provisioning (a flaky `pkg install` + in a fresh jail before DNS is confirmed working — see this skill's + resolver-failure-after-destroy/create-churn gotcha), write a + fleet-specific pattern then, rather than importing a generic one now on + spec. +- **ShellCheck as a hard gate** — worth running locally before landing a + new provisioning script (`pkg install shellcheck`; point it at a + `/bin/sh` script with `# shellcheck shell=sh` since its default dialect + guess is bash), but not made mandatory here since this fleet has no CI + wired to enforce it on these scripts yet. +- **The upstream "always use `[[ ]]`" advice as a blanket rule** — true + only once bash is confirmed present; POSIX `[ ]` is required, not just + "more portable," for any `/bin/sh` script.