From 256ded2e922c651189ee3e352c23f0ed2ae83d6b Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 10:02:09 +0000 Subject: [PATCH] fix: the racing-reader sweep guards the class, and revoke-user captures the trust store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep added in #107 matched `ufw status[^|]*\| *grep` — 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 same 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. Readers are deliberately not narrowed to the early-exit spellings: telling `grep -q` from `grep -c` by regex is precision that rots, and all six `ufw status` sites in the tree already capture first, so banning the pipe outright costs nothing real. Writers gain `incus config trust list`. 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 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 called the purge complete: fail-open, on the path whose job is to prove access is gone. Now captured into `trust_csv` and matched with `[[ ]]`, with a leading newline so the first CSV row anchors like the `^` it replaces. Writers are enumerated rather than generalised — ~150 legitimate `| grep` sites exist under host/ and drill/, nearly all re-reading a captured string — so the sweep claims only what it can check. The `id -nG | tr | grep -qx` shapes in grant/revoke/setup are left alone: single tiny writes. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 27 ++++++++++++++++++++++++++ host/revoke-user.sh | 15 ++++++++++++++- test/cli.sh | 47 +++++++++++++++++++++++++++++++++++++++------ 3 files changed, 82 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3d8866..927a48e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -399,6 +399,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 fd641a7..9dcde9a 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -2008,9 +2008,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. # @@ -2022,13 +2022,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 @@ -2042,6 +2065,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"