From 0ff520c850890c6d8985552620e5003ee951bcb1 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Sun, 19 Jul 2026 16:18:21 +0000 Subject: [PATCH] fix: dropping the box role revokes through box, not behind its back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `users apply` converged group `incus` with a bare `gpasswd -d`, the same move it makes for `rig-admin` and `rig`. Those two are rig's. `incus` is box's, and `box revoke` does strictly more with it: it says out loud that supplementary groups are read AT LOGIN, so a session the dropped operator already holds keeps the Incus socket until that session dies, and it hands over `loginctl terminate-user ` as the remedy. rig logged "removed from incus" and moved on. An operator who dropped someone from the users file and watched apply succeed believed the VM access was gone — and was wrong for as long as that user held a session. Both removal paths — the per-user convergence loop and the dropped-user sweep — now route the incus group through one `drop_incus` helper that calls `box revoke`, keeping a single owner for the group. Never `--purge`: that deletes the user's boxes, images and project, and destroying someone's running machines is not a convergence step; it stays an explicit admin act. The exit code is not trusted (the #12 lesson bootstrap already applies to box's installer): a revoke that returns 0 with the membership still standing has not closed the socket, so the effective state is checked and rig falls back to removing the group itself — as it also does on a host where box is not installed. Every fallback path carries the session warning in rig's own voice, because the silence was the bug. The absent-group case needs no new guard: `id -nG` cannot report a group that does not exist, so the existing `in_group` test at both call sites is already false on a host=no box or one where `box setup-host` never ran. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 25 ++++++- README.md | 14 ++++ commands/users-apply.sh | 80 +++++++++++++++++++-- test/cli.sh | 150 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 264 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 318a733..f77e10a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ History before 0.1.0 lives in git — rig grew its version surface (`VERSION`, `rig --version`, the side-by-side `versions/` install layout; #35/#36) on the way to cutting its first release, and this file starts there. -## 0.1.0 — 2026-07-19 +## Unreleased ### Fixed @@ -27,6 +27,29 @@ on the way to cutting its first release, and this file starts there. exact-membership convergence now strips box-role users out of `incus`, on the same reasoning: a membership inherited from a previous life is the same half-grant as a freshly added one. +- **Dropping the box role revokes through `box`, not behind its back** + (#50) — `users apply` converged group `incus` with a bare `gpasswd -d`, + the same move it makes for `rig-admin` and `rig`. Those two are rig's; + `incus` is box's, and `box revoke` does strictly more with it: it says + out loud that supplementary groups are read at LOGIN, so a session the + dropped operator already holds keeps the Incus socket until it dies, and + hands over `loginctl terminate-user ` as the remedy. rig logged + `removed from incus` and moved on, so an operator who dropped + someone from the users file and watched apply succeed believed the VM + access was gone — and was wrong for as long as that user held a session. + Both removal paths (the per-user convergence and the dropped-user sweep) + now call `box revoke`, which keeps one owner for the group. Never + `--purge`: that deletes the user's boxes, images and project, and + destroying someone's running machines is not a convergence step — it + stays an explicit admin act. The exit code is not trusted (#12's lesson): + a revoke that returns 0 with the membership still standing has not closed + the socket, and rig falls back to removing the group itself, as it also + does where box is not installed. Every fallback path carries the session + warning, because the silence was the bug. + +## 0.1.0 — 2026-07-19 + +### Fixed - **The release suite accepts the ceremony's own tree** (#44) — `test/release.sh` demanded a literal `## Unreleased` heading in the real diff --git a/README.md b/README.md index 31919d3..5618d31 100644 --- a/README.md +++ b/README.md @@ -784,6 +784,20 @@ apply dies pointing at `box setup-host` rather than conjure a group nothing would consult. `incus-admin` is deliberately **not** a role: that group is host-root-equivalent, break-glass by hand only. +Losing the `box` role goes back through box, too. `rig-admin` and `rig` are +rig's groups and a `gpasswd -d` is the whole story for them; `incus` is box's, +and `box revoke` does more with it than remove a membership — it says out loud +that supplementary groups are read **at login**, so a session the dropped +operator already holds keeps the Incus socket until it dies, and names +`loginctl terminate-user ` as the way to end it now. So apply calls +`box revoke` and lets box speak. Never `--purge`: that deletes the user's +boxes, images and project, and destroying someone's running machines is not a +convergence step — `box revoke --purge` stays a deliberate admin act. +Where box is not installed (or the revoke returns success with the membership +still standing — an exit code is not effective state) rig removes the group +itself **and carries the session warning**, because a silent removal is what +lets an operator believe access ended when it has not. + **All passwords stay locked, always** — created or found. The SSH key at the door is the authentication, and NOPASSWD sudo does not weaken it: there was never a password to guess or rotate. diff --git a/commands/users-apply.sh b/commands/users-apply.sh index d0a1838..e0c1449 100755 --- a/commands/users-apply.sh +++ b/commands/users-apply.sh @@ -47,7 +47,10 @@ roles: admin group rig-admin — full NOPASSWD sudo rig group rig — NOPASSWD sudo for /usr/local/bin/rig only box group incus — Incus restricted tier, no sudo (box's setup-host - owns the Incus install; rig only asserts it) + owns the Incus install; rig only asserts it). + Dropping this role hands the group back through + 'box revoke' — never --purge: convergence removes + access, never someone's running boxes. All passwords stay locked, always — the SSH key at the door is the authentication, and NOPASSWD sudo does not weaken it. Convergent: membership @@ -222,6 +225,60 @@ fi in_group() { id -nG "$1" 2>/dev/null | tr ' ' '\n' | grep -qx "$2"; } +# --- dropping role box: give the group back to the tool that owns it (#50) --- +# rig-admin and rig are rig's own groups, and `gpasswd -d` is the whole story +# for them. incus is not rig's: box's setup-host creates it, `box grant` hands +# it out, and `box revoke` takes it back — doing strictly MORE than removing +# the membership. It says out loud what removing the membership does not do: +# supplementary groups are read at LOGIN, so a session the dropped operator +# already holds keeps the Incus socket until that session dies, and the remedy +# is `loginctl terminate-user `. rig's bare `gpasswd -d` logged "removed +# from incus" and left it there, so an operator who dropped someone from +# the users file and watched apply succeed believed the VM access was gone — +# and was wrong for as long as that user held a session. +# +# So box takes its own group back. The fallback below exists for the host where +# box is not installed (someone else built the Incus stack, or box was removed +# from under it), and it carries the session warning ITSELF: the silence is the +# bug being fixed, not the gpasswd call. +# +# NEVER --purge from here. A bare revoke ends access and leaves the user's +# project and boxes intact — still RUNNING, because revoking a person does not +# kill their workloads. Deleting them is irreversible and is not a convergence +# step: an edit to the users file must never destroy someone's machines behind +# an admin's back. `box revoke --purge`, run deliberately, is where that +# lives. +# +# The absent-group case needs no guard of its own: `id -nG` cannot report a +# group that does not exist, so every caller's `in_group` test is already false +# on a host=no box or one where `box setup-host` never ran — there is nothing +# to revoke, and apply says nothing and moves on. +drop_incus() { # drop_incus — hand group 'incus' back to box + local u="$1" + if command -v box >/dev/null 2>&1; then + # Don't trust the exit code — prove the effective state (the #12 lesson, + # the same one bootstrap applies to box's installer). A revoke that exits + # 0 having left the membership in place would otherwise be reported as a + # removal; the membership is what closes the socket, so it gets checked. + if box revoke "$u" && ! in_group "$u" incus; then + log "removed $u from incus (via 'box revoke' — box owns that group)" + return 0 + fi + warn "'box revoke $u' did not remove the incus group — removing it directly; check box on this host" + fi + if in_group "$u" incus; then + gpasswd -d "$u" incus >/dev/null + log "removed $u from incus" + fi + # box's warning, in rig's voice, because rig is the one that took the group + # here. pgrep decides whether it applies; if pgrep is absent rig cannot tell, + # and an unnecessary warning costs an operator one command while a missing + # one costs them a wrong belief about who can reach the daemon. + if ! command -v pgrep >/dev/null 2>&1 || pgrep -u "$u" >/dev/null 2>&1; then + warn "$u may hold live sessions, and group membership is read at login — those sessions keep the Incus socket until they end. To end them now: loginctl terminate-user $u" + fi +} + # --- converge each user ------------------------------------------------------ for u in "${USERS[@]}"; do if ! id -u "$u" >/dev/null 2>&1; then @@ -263,8 +320,12 @@ for u in "${USERS[@]}"; do fi ;; *) if in_group "$u" "$g"; then - gpasswd -d "$u" "$g" >/dev/null - log "removed $u from $g" + if [ "$g" = incus ]; then + drop_incus "$u" + else + gpasswd -d "$u" "$g" >/dev/null + log "removed $u from $g" + fi CHANGED=1 fi ;; esac @@ -324,8 +385,19 @@ if [ -r "$LEDGER" ]; then if [ -f "$prevhome/.ssh/authorized_keys" ]; then mv "$prevhome/.ssh/authorized_keys" "$prevhome/.ssh/authorized_keys.revoked-by-rig" fi + # incus goes back through box here too — a user dropped from the file + # entirely is exactly the offboarding this warning was written for, and it + # would be perverse for the full revocation to be the quiet one. It fires + # once, on the transition: the membership is gone on the next run, so + # in_group is false and an already-revoked user stays a clean no-op. for g in rig-admin rig incus; do - if in_group "$prev" "$g"; then gpasswd -d "$prev" "$g" >/dev/null; fi + if in_group "$prev" "$g"; then + if [ "$g" = incus ]; then + drop_incus "$prev" + else + gpasswd -d "$prev" "$g" >/dev/null + fi + fi done REVOKED+=("$prev") # Warn on the TRANSITION only: an already-revoked user is converged above diff --git a/test/cli.sh b/test/cli.sh index a6483c3..49c96d4 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -814,6 +814,156 @@ check "users apply: the incus want is gated on the host= verdict, not just the g check "users apply: a marker/reality mismatch warns and names the repair" 0 "" \ grep -q "marker and this box's reality disagree" "$ROOT/commands/users-apply.sh" +# --- dropping role box goes through box, not behind its back (#50) ----------- +# The 'incus' group is box's: box's setup-host creates it and `box revoke` +# takes it back, warning that supplementary groups are read at LOGIN so a +# session the user already holds keeps the socket until it dies. rig's old bare +# `gpasswd -d` took the group and said nothing, so an operator watching apply +# succeed believed VM access had ended when it had not. +# +# Both removal paths route through one function, so both are covered by the one +# set of assertions below. +check "users apply: both removal paths route incus through drop_incus" 0 "" \ + test "$(grep -c 'drop_incus "\$' "$ROOT/commands/users-apply.sh")" -eq 2 +# Convergence removes access, never someone's running machines: '--purge' +# deletes the user's boxes, images and project, and must stay an explicit admin +# act. Asserted by the SHAPE of the one line that invokes box — a bare revoke +# whose effective state is then checked — rather than by grepping the file for +# '--purge', which the rationale comments and --help text mention on purpose, +# to say where it does belong. The captures below prove the same thing at +# runtime, on every path. +# shellcheck disable=SC2016 # the literal source line is the pattern, unexpanded +check "users apply: the box invocation is a bare revoke" 0 "" \ + grep -qF 'if box revoke "$u" && ! in_group "$u" incus; then' \ + "$ROOT/commands/users-apply.sh" + +# drop_incus is exercised, not argued about. users-apply.sh EXECUTES when +# sourced (and dies at the root check), so the function is lifted out of the +# real file verbatim — column-0 'drop_incus() {' through column-0 '}' — and +# driven against stub log/warn/in_group and a stub PATH holding only box, +# gpasswd and pgrep. The extraction is asserted first: if that shape ever +# changes the lift comes back empty and every case below fails loudly rather +# than passing vacuously. +DROP_FN="$(sed -n '/^drop_incus() {/,/^}/p' "$ROOT/commands/users-apply.sh")" +# shellcheck disable=SC2016 # $1 is the inner bash -c's positional, deliberately +check "users apply: drop_incus lifts out of the real file whole" 0 "" \ + bash -c '[ -n "$1" ] && printf %s "$1" | grep -q "^}$"' _ "$DROP_FN" + +# The driving shell is named by absolute path: PATH below is REPLACED by the +# stub directory (not prefixed) so that 'absent' means absent even on a host +# that really has box installed — which also puts bash itself out of reach of +# a PATH lookup. +BASH_BIN="$(command -v bash)" +# drive_drop — run the real drop_incus against stubs. +# 'ok': box revokes and the membership is gone afterwards. 'hollow': box exits +# 0 and leaves the membership standing (the #12 lesson — an exit code is not +# effective state). 'fail': box exits non-zero. 'absent': no box on the host. +drive_drop() { + local mode="$1" d + d="$(mktemp -d)" + mkdir -p "$d/bin" + : > "$d/member" # 'dan is in incus' — removed when taken + # The stub reports on STDERR: the real call is 'gpasswd -d ... >/dev/null', + # so a stub that spoke on stdout would be silenced by the code under test and + # every fallback assertion below would pass vacuously. + # Each stub restores a real PATH for itself: the caller's PATH is REPLACED by + # the stub dir (that is what makes 'absent' mean absent), which would other- + # wise leave the stubs unable to find 'rm'. + cat > "$d/bin/gpasswd" <&2 +rm -f "$d/member" +EOF + printf '%s\n' '#!/bin/sh' 'exit 0' > "$d/bin/pgrep" # dan holds a session + if [ "$mode" != absent ]; then + cat > "$d/bin/box" <&2; } + in_group() { [ -e "$MEMBER" ]; } + '"$DROP_FN"' + drop_incus dan' 2>&1 + rm -rf "$d" +} +DROP_OK="$(drive_drop ok)" +DROP_HOLLOW="$(drive_drop hollow)" +DROP_FAIL="$(drive_drop fail)" +DROP_ABSENT="$(drive_drop absent)" +in_out() { printf '%s' "$1" | grep -qF -e "$2"; } # in_out + +# The happy path: box takes its own group back, bare, and rig does not reach +# for gpasswd behind it. +check "drop_incus: calls 'box revoke '" 0 "" in_out "$DROP_OK" "CALL: box revoke dan" +# Bare on EVERY path box is reached on, not just the one that works: a retry +# or a fallback must never escalate to the destructive verb. +check "drop_incus: the box call is bare — no --purge" 1 "" in_out "$DROP_OK" "--purge" +check "drop_incus: a hollow success never retries with --purge" 1 "" in_out "$DROP_HOLLOW" "--purge" +check "drop_incus: a failed revoke never retries with --purge" 1 "" in_out "$DROP_FAIL" "--purge" +check "drop_incus: box's success needs no gpasswd" 1 "" in_out "$DROP_OK" "CALL: gpasswd" +check "drop_incus: names box as the one that revoked" 0 "" in_out "$DROP_OK" "via 'box revoke'" + +# Effective state, not exit codes: a revoke that returns 0 with the membership +# still standing has not closed the socket, and apply must not report that it +# has. +check "drop_incus: a hollow box success is caught" 0 "" \ + in_out "$DROP_HOLLOW" "did not remove the incus group" +check "drop_incus: a hollow box success falls back to gpasswd" 0 "" \ + in_out "$DROP_HOLLOW" "CALL: gpasswd -d dan incus" +check "drop_incus: a hollow box success never claims box did it" 1 "" \ + in_out "$DROP_HOLLOW" "via 'box revoke'" +check "drop_incus: a failing box revoke falls back to gpasswd" 0 "" \ + in_out "$DROP_FAIL" "CALL: gpasswd -d dan incus" + +# No box on the host: rig takes the group itself and carries box's warning, +# because the silence is the bug — an operator must not read "removed" as +# "their sessions are gone too". +check "drop_incus: no box on PATH still removes the group" 0 "" \ + in_out "$DROP_ABSENT" "CALL: gpasswd -d dan incus" +check "drop_incus: the fallback warns that groups are read at login" 0 "" \ + in_out "$DROP_ABSENT" "group membership is read at login" +check "drop_incus: the fallback hands over the remedy" 0 "" \ + in_out "$DROP_ABSENT" "loginctl terminate-user dan" +# Every fallback path carries it, not just the box-less one. +check "drop_incus: the hollow-success fallback warns too" 0 "" \ + in_out "$DROP_HOLLOW" "loginctl terminate-user dan" +check "drop_incus: the failed-revoke fallback warns too" 0 "" \ + in_out "$DROP_FAIL" "loginctl terminate-user dan" +# box only warns when the user has live processes, and rig mirrors that — but +# an absent pgrep means rig cannot tell, and a wrong belief about who reaches +# the daemon costs more than one unnecessary command. Proven by running the +# fallback with a PATH that has no pgrep at all. +NOPGREP_D="$(mktemp -d)" +mkdir -p "$NOPGREP_D/bin" +printf '%s\n' '#!/bin/sh' 'PATH=/usr/bin:/bin' 'echo "CALL: gpasswd $*" >&2' \ + "rm -f $NOPGREP_D/member" > "$NOPGREP_D/bin/gpasswd" +chmod +x "$NOPGREP_D/bin/gpasswd" +: > "$NOPGREP_D/member" +# shellcheck disable=SC2016 # $MEMBER/$* resolve inside the driving shell +DROP_NOPGREP="$(MEMBER="$NOPGREP_D/member" PATH="$NOPGREP_D/bin" "$BASH_BIN" -c ' + set -euo pipefail + log() { printf "rig-users: %s\n" "$*"; } + warn() { printf "rig-users: WARNING: %s\n" "$*" >&2; } + in_group() { [ -e "$MEMBER" ]; } + '"$DROP_FN"' + drop_incus dan' 2>&1)" +check "drop_incus: an absent pgrep warns rather than guessing" 0 "" \ + in_out "$DROP_NOPGREP" "loginctl terminate-user dan" +rm -rf "$NOPGREP_D" + # --- users close-root: the human-class root-door shutter --------------------- check "users close-root: --help exits 0" 0 "usage:" "$ROOT/commands/users-close-root.sh" --help check "users close-root: unknown flag exits 2" 2 "unknown flag" "$ROOT/commands/users-close-root.sh" --nope