diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c6d1db..9b9b995 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -468,6 +468,33 @@ which records not just what changed but what each drill run proved. exists. (`drill/doctor.sh` was checked and needs nothing: it already reads into `ufw_out` and is safe by construction, not by absent `pipefail`.) +- **The racing-reader sweep guards the class, not one spelling — and now + names a second writer** (#124) — the sweep #107 added matched + `ufw status[^|]*\| *grep`, which caught every historical instance and none + of the equivalent spellings. `head -n1`, `sed -n '1p;q'`, + `awk '/x/ {print; exit}'` and `read` all close the pipe early, SIGPIPE the + writer, and yield the identical 141 under `pipefail`; the pin guarded the + instance spelling of the very thing it existed to generalise. Both halves + of the matcher are alternations now. The reader half is deliberately not + narrowed to the early-exit spellings — separating `grep -q` from `grep -c` + by regex is precision that rots, and every `ufw status` site in the tree + already captures first, so banning the pipe outright costs nothing. + The writer half gains `incus config trust list`, because + **`host/revoke-user.sh:206` piped it into `grep -q` as the `--purge` + leftover assert** — under `set -euo pipefail`, so unlike `drill/wipe.sh` + nothing but the writer's size was holding it, and sitting left of `&&` a + 141 is `set -e`-exempt too. It would have read as "no leftover cert" on a + host that still trusts the revoked user's certificate and reported the + purge complete: fail-open, on the path whose whole job is to prove access + is gone. Now captured into `trust_csv` and matched with `[[ ]]` (leading + newline so the first CSV row still anchors like the `^` it replaces — + verified equivalent to the old `grep` across nine anchoring cases). Writers + are enumerated rather than generalised: ~150 legitimate `| grep` sites + exist under `host/` and `drill/`, nearly all re-reading an already-captured + string, so the sweep claims only what it can check and grows one named + writer at a time. The `id -nG | tr | grep -qx` shapes in grant/revoke/setup + are deliberately untouched — single tiny writes, not realistically racy. + ## 0.8.0 — 2026-07-19 ### Added diff --git a/host/revoke-user.sh b/host/revoke-user.sh index 9533899..fa46c20 100644 --- a/host/revoke-user.sh +++ b/host/revoke-user.sh @@ -200,10 +200,23 @@ fi # and a promise the header makes is a promise this block checks. The # incus-user state directory too — it was purged for releases without being # re-checked, which is exactly the gap this block exists to close. +# The trust store is read into a capture rather than piped into a reader that +# stops at its first match — #102's shape, and this file is `set -euo pipefail` +# already, so unlike drill/wipe.sh (#107) nothing but the writer's size is +# holding it. A reader that exits early SIGPIPEs incus mid-table and the +# pipeline yields 141; sitting left of `&&` that is also set -e-exempt, so it +# would read as "no leftover cert" on a host that still trusts the revoked +# user's certificate and the purge would report success. Fail-open, on the +# cleanup path whose entire job is to prove access is gone. +# Un-racy in practice today — the trust store is small and likely one write — +# so this is defensive, not a live defect. Captured so it cannot become one. +trust_csv="$(incus config trust list --format csv --columns nf 2>/dev/null || true)" + leftover="" incus project show "$project" >/dev/null 2>&1 /dev/null 2>&1 /dev/null | grep -q "^incus-user-$uid," \ +# Leading newline so the first CSV row anchors like the `^` this replaces. +[[ $'\n'"$trust_csv" == *$'\n'"incus-user-$uid,"* ]] \ && leftover="$leftover cert:incus-user-$uid" $SUDO test -d "/var/lib/incus/users/$uid" 2>/dev/null \ && leftover="$leftover /var/lib/incus/users/$uid" diff --git a/test/cli.sh b/test/cli.sh index 135723a..e02201c 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -2333,9 +2333,9 @@ check "teardown-host: refuses without a TTY and names the override (#113)" 2 \ # #102's race, pinned as a CLASS rather than at the one site that had it # (#107). A daemon-free run cannot exercise a UFW teardown, so the shape is -# pinned instead: nowhere under host/ or drill/ may `ufw status` be piped into -# a reader that exits on its first match. `Status: active` is ufw's FIRST line, -# so the reader matches, closes the pipe, ufw takes SIGPIPE, and the pipeline +# pinned instead: nowhere under host/ or drill/ may a known multi-line writer +# be piped into a line reader. `Status: active` is ufw's FIRST line, so the +# reader matches, closes the pipe, ufw takes SIGPIPE, and the pipeline # yields 141 — under pipefail the branch silently reads false and the whole # firewall block is skipped on a host the operator was told is clean. # @@ -2347,13 +2347,36 @@ check "teardown-host: refuses without a TTY and names the override (#113)" 2 \ # Comment lines are stripped before matching: each fix's own commentary quotes # the racing shape to explain it, and a pin that cannot tell prose from code # would fail on the very comment documenting why it exists. +# +# BOTH halves of the matcher are alternations, and both were widened in #124: +# +# · READERS. Pinning `| grep` guarded the instance spelling, not the class. +# `head -n1`, `sed -n '1p;q'` and `awk '/x/ {print; exit}'` all close the +# pipe early and produce the identical wrong answer under pipefail. The +# alternation is deliberately NOT restricted to the early-exit spellings +# (`grep -q` but not `grep -c`, `sed …q` but not `sed s///`): telling +# those apart by regex is exactly the kind of precision that rots, and +# the house idiom is to capture first anyway — all six `ufw status` sites +# in the tree already do. Banning the pipe outright costs nothing real +# and cannot be defeated by a spelling nobody enumerated. +# +# · WRITERS. Enumerated, not generalised. `incus config trust list` joins +# `ufw status` because host/revoke-user.sh used it as a leftover-detection +# condition under `set -euo pipefail` (#124). A generic "no multi-line +# writer feeds a reader" matcher is unwritable here: ~150 legitimate +# `| grep` sites exist across host/ and drill/, nearly all reading an +# already-captured string back out of `printf '%s\n' "$var"`. So the +# sweep claims exactly what it can check — THESE writers are never piped +# — and grows one named writer at a time. # shellcheck disable=SC2016 # "$1" is the subshell's positional, passed below -check "no 'ufw status' is piped into an early-exit reader under host/ or drill/" 0 "" \ +check "no multi-line writer is piped into a line reader under host/ or drill/" 0 "" \ bash -c 'bad="" for f in "$1"/host/*.sh "$1"/drill/*.sh; do - grep -vE "^[[:space:]]*#" "$f" | grep -qE "ufw status[^|]*\| *grep" && bad="$bad ${f#"$1"/}" + grep -vE "^[[:space:]]*#" "$f" \ + | grep -qE "(ufw status|incus config trust list)[^|]*\| *(grep|head|sed|awk|read)" \ + && bad="$bad ${f#"$1"/}" done - [ -z "$bad" ] || { printf "racing ufw reads in:%s\n" "$bad"; exit 1; }' \ + [ -z "$bad" ] || { printf "racing reads in:%s\n" "$bad"; exit 1; }' \ _ "$ROOT" # The other direction, per file that removes UFW rules: the capture present and @@ -2367,6 +2390,18 @@ for f in host/teardown-host.sh drill/wipe.sh; do check "$f: the numbered-delete loop breaks on absence, not on a pipe" 0 "" \ grep -qF '[ -n "$line" ] || break' "$ROOT/$f" done + +# Same other-direction pin for the non-ufw writer the sweep now names: the +# --purge leftover assert must match a captured trust store, so the sweep +# cannot be satisfied by deleting the assert instead of fixing it. That assert +# is the last thing standing between "purge INCOMPLETE" and a silent claim of +# success on a host that still trusts the revoked user's certificate. +# shellcheck disable=SC2016 # the $-strings are literals in the target file +check "revoke-user: the purge leftover assert reads a captured trust store" 0 "" \ + grep -qF 'trust_csv="$(incus config trust list' "$ROOT/host/revoke-user.sh" +# shellcheck disable=SC2016 # ditto +check "revoke-user: the cert leftover check matches the capture, not a pipe" 0 "" \ + grep -qF '"$trust_csv" == *$' "$ROOT/host/revoke-user.sh" check "drill: reads the installed tree through current/" 0 "" \ grep -qF '.local/share/box/current/VERSION' "$ROOT/drill/drill.sh"