fix: preflight every admin binary a command uses, not only useradd
Some checks failed
ci / check (pull_request) Successful in 55s
ci / install (pull_request) Successful in 3s
ci / db-integration (pull_request) Successful in 3s
labels / labels (pull_request) Failing after 7s

Addresses codex (1538) and kimi (1539): the sweep caught the reported
incident and not the class. Both are right, and there were four sites, not
three.

  forgejo-runner-install  useradd -> useradd usermod   (usermod -aG docker,
                          reached only after the token has been spent)
  users-apply             useradd usermod -> + groupadd (called two lines
                          into convergence), and visudo when a role needs it
  bootstrap-tenant        NEW site (kimi) — usermod -aG docker runs AFTER
                          docker and node are installed, so an unguarded
                          PATH fails it mid-convergence on a changed machine
  runner-install          unchanged: useradd is the only admin binary it
                          calls, and declaring more would refuse boxes that
                          are fine

visudo is checked after the sudo-install block rather than beside the root
check, because until sudo is installed its absence has an innocent cause.
Below that block it does not: sudo is present, so a missing visudo means
/usr/sbin is off PATH. That case is the quiet one — the sudoers block reads
`command -v visudo` as "no sudo on the box means no role needed it", so
apply reported success having granted roles without the escalation those
roles exist for. The other three sites at least crash.

Measured which binaries this covers (Debian 13): useradd, usermod, groupadd,
userdel, groupdel and visudo are /usr/sbin; gpasswd is /usr/bin and so is
NOT affected and deliberately not preflighted. visudo shares the directory
but ships in `sudo`, not `passwd` — which is why it needs its own treatment.

Tests: the sbin-less fixtures could only ever prove the FIRST binary is
named, since useradd wins every race. Six new checks use partial PATHs that
resolve the earlier binaries and withhold exactly one, plus the ordering
assertions (no token prompt, no group created) and the negative case — a
users file needing no sudo must NOT be refused for a missing visudo.

Refs #139
This commit is contained in:
cluade-reviewer-andresmgsl 2026-08-02 00:05:02 +00:00
parent 5187b74fa0
commit 7aed6ea098
6 changed files with 135 additions and 3 deletions

View file

@ -1,3 +1,4 @@
### Fixed
- `rig forgejo-runner install`, `rig runner install` and `rig users apply` refuse with a named remedy when root's `PATH` carries no `/usr/sbin`, instead of dying on `useradd: command not found` after prompting for a token (#139)
- `rig forgejo-runner install`, `rig runner install`, `rig users apply` and `rig bootstrap <tenant>` refuse with a named remedy when root's `PATH` carries no `/usr/sbin`, instead of dying on `useradd: command not found` after prompting for a token (#139)
- `rig users apply` no longer reports success having silently skipped the sudoers drop-in when `visudo` is off `PATH` (#139)

View file

@ -31,6 +31,8 @@ HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)"
. "$HERE/lib/templates.sh" # templates_resolve / template_parse_env / render_tenant_context
# shellcheck source=SCRIPTDIR/lib/users-config.sh
. "$HERE/lib/users-config.sh" # read_role_marker / root_door_of
# shellcheck source=SCRIPTDIR/lib/admin-path.sh
. "$HERE/lib/admin-path.sh" # require_admin_bins
# shellcheck source=SCRIPTDIR/lib/sshd.sh
. "$HERE/lib/sshd.sh" # harden_sshd (the staging-box tenant)
# shellcheck source=SCRIPTDIR/lib/manifest.sh
@ -200,6 +202,15 @@ else
fi
[ "$(id -u)" -eq 0 ] || die "must run as root"
# Root is not enough here either (#139). This mint adds the tenant user to the
# docker group with `usermod` far below — AFTER installing docker and node,
# which is what makes it the worst-placed of the four call sites: on a
# PATH-shorn root it dies mid-convergence with a bare `usermod: command not
# found`, having already changed the machine, rather than before touching it.
#
# Unconditional because the docker block below is unconditional — "every tenant
# gets docker" is the stated rule there, so every tenant reaches the usermod.
require_admin_bins usermod
if [ -r /etc/os-release ]; then
# Sourced in a subshell: os-release defines VERSION, NAME, ID, etc. —
# sourcing it in the main shell silently clobbers same-named script vars.

View file

@ -234,7 +234,14 @@ fi
# Root is not enough: the admin binaries must also be reachable (#139). This
# sits beside the root check so identity and capability are asserted together,
# and BEFORE any prompt or download — a token typed for a doomed run is waste.
require_admin_bins useradd
#
# BOTH binaries this command goes on to call: useradd at the user-create below,
# usermod at the docker-group add further down. Naming only the first would
# still consume the token on a PATH that happened to resolve useradd but not
# usermod — the same failure one step later, which is the shape #75 exists to
# refuse (a sweep that covers most of its call sites is the hole the next bug
# arrives through).
require_admin_bins useradd usermod
if [ -r /etc/os-release ]; then
# Sourced in a subshell: os-release defines VERSION (e.g. "13 (trixie)"),
# which would clobber this script's $VERSION.

View file

@ -10,6 +10,24 @@
# versioned install root, emitted AFTER a registration token had been read off
# the operator's terminal (#139).
#
# Which binaries this covers is measured, not assumed (Debian 13, 2026-08-01):
#
# useradd usermod groupadd userdel groupdel /usr/sbin package: passwd
# visudo /usr/sbin package: sudo
# gpasswd /usr/bin package: passwd
#
# Two consequences worth keeping written down. `gpasswd` is in the same PACKAGE
# as useradd but a different DIRECTORY, so it is reachable on a PATH-shorn root
# and does not belong in any of these preflights — do not add it for symmetry.
# And `visudo` shares the directory but not the package, so its absence has a
# second, innocent cause (sudo simply not installed) that the others do not, so
# `rig users apply` checks it separately, after the point where that cause is
# ruled out. See the comment there.
#
# (Spelled without the `.sh` on purpose: test/cli.sh pins that exactly one file
# under commands/ names that script, to catch a second caller appearing. A
# comment is not a caller, but the pin is deliberately blunt and cheap.)
#
# It REFUSES rather than repairing PATH itself. A command that quietly prepends
# /usr/sbin teaches the operator nothing and leaves a misconfigured host
# misconfigured; the same reason bootstrap refuses rather than guessing. The

View file

@ -141,7 +141,7 @@ done <<< "$PARSED"
# Root is not enough: the admin binaries must also be reachable (#139). This
# sits beside the root check so identity and capability are asserted together,
# and BEFORE any prompt or download — a token typed for a doomed run is waste.
require_admin_bins useradd usermod
require_admin_bins useradd usermod groupadd
# Identity management gates its INVOKER, not just its uid: %rig's sudoers rule
# is binary-scoped but not argument-scoped, so without this gate a rig-role
@ -201,6 +201,26 @@ if [ "$NEED_SUDO" -eq 1 ] && ! command -v sudo >/dev/null 2>&1; then
DEBIAN_FRONTEND=noninteractive apt-get install -y -qq sudo
CHANGED=1
fi
# visudo is checked HERE and not beside the root check, because until the block
# above has run there is a legitimate reason for it to be absent: sudo is not
# installed yet, and apply is what installs it. Above, a missing visudo would
# be indistinguishable from that, so the refusal would fire on a healthy box.
#
# Below, it is unambiguous. sudo is present, so `visudo` missing means only one
# thing: /usr/sbin is off PATH. And it MUST refuse here rather than be left to
# the sudoers block further down, because that block asks `command -v visudo`
# and treats false as "no sudo on the box means no role needed it" — which on a
# PATH-shorn root is FALSE TWICE. A role does need it, sudo is installed, and
# apply would finish reporting success having silently never written the
# sudoers drop-in: the users get their roles and not the escalation the roles
# are FOR. That is the failure this whole issue is about (a wrong effective
# state reported as success, #12), in its quietest form — the other three sites
# at least crash. Refusing before the first mutation is what keeps it loud.
#
# It sits before `groupadd` below, so nothing has been converged when it fires.
if [ "$NEED_SUDO" -eq 1 ]; then
require_admin_bins visudo
fi
# --- groups ------------------------------------------------------------------
groupadd -f rig-admin

View file

@ -3372,6 +3372,81 @@ check "preflight: the GitHub runner installer refuses too" 1 "useradd" \
adm_run "$ROOT/commands/runner-install.sh" --repo o/r
check "preflight: users apply refuses before it converges anything" 1 "useradd" \
adm_run "$ROOT/commands/users-apply.sh" --file /dev/null
# A sbin-less PATH loses ALL of /usr/sbin at once, so the checks above can only
# ever prove the FIRST binary is named — `useradd` wins every race and would
# hide a preflight that forgot the others. These fixtures resolve the earlier
# binaries and withhold exactly one, which is the only way to show the sweep
# covers what each command actually calls (the #75 lesson: a sweep that misses
# one call site is how the next bug gets in).
#
# The withheld binary is real: these are PATHs, not stubs of the tools
# themselves — a stub that silently succeeded would let convergence run on.
admstub() { # admstub <dir> <bin>... — a bindir resolving id(0) plus <bin>...
local d="$1"; shift
mkdir -p "$d"
# shellcheck disable=SC2016 # shell source being written, not expanded
printf '#!/usr/bin/env bash\nif [ "${1:-}" = -u ]; then printf "0\\n"; else exec /usr/bin/id "$@"; fi\n' > "$d/id"
chmod +x "$d/id"
local b
for b in "$@"; do printf '#!/usr/bin/env bash\nexit 0\n' > "$d/$b"; chmod +x "$d/$b"; done
}
adm_with() { # adm_with <bindir> <cmd>...
local d="$1"; shift
env PATH="$d:/usr/local/bin:/usr/bin:/bin" "$@" 2>&1
}
# adm_saw <bindir> <pattern> <cmd>... — exit 0 if the run printed <pattern>.
# Used with `check ... 1 ""` to assert a thing did NOT happen (a token prompt,
# a group creation), the same shape as adm_prompted above.
adm_saw() {
local d="$1" pat="$2"; shift 2
adm_with "$d" "$@" | grep -qi -- "$pat"
}
# useradd resolves, usermod does not — the forgejo installer calls both, and
# only reaches usermod after the token has been spent.
admstub "$ADMPATH_DIR/no-usermod" useradd
check "preflight: forgejo-runner install names usermod when only useradd resolves" 1 "usermod" \
adm_with "$ADMPATH_DIR/no-usermod" "$ROOT/commands/forgejo-runner-install.sh" --instance https://f.example.com
check "preflight: …and still refuses before the token prompt" 1 "" \
adm_saw "$ADMPATH_DIR/no-usermod" 'registration token:' \
"$ROOT/commands/forgejo-runner-install.sh" --instance https://f.example.com
# users apply calls groupadd unconditionally, two lines into its convergence.
admstub "$ADMPATH_DIR/no-groupadd" useradd usermod
check "preflight: users apply names groupadd when useradd and usermod resolve" 1 "groupadd" \
adm_with "$ADMPATH_DIR/no-groupadd" "$ROOT/commands/users-apply.sh" --file /dev/null
# visudo is the quiet one. With sudo INSTALLED and /usr/sbin off PATH, the
# sudoers block reads `command -v visudo` as "no sudo on the box" and skips the
# drop-in — apply then reports success having granted roles without the
# escalation those roles exist for. So this fixture needs a role that wants
# sudo, and asserts the refusal by name rather than a silent success.
ADM_USERS="$ADMPATH_DIR/users"
printf '%s\n' 'dan admin ssh-ed25519 AAAAC3fixture dan@laptop' > "$ADM_USERS"
admstub "$ADMPATH_DIR/no-visudo" useradd usermod groupadd
check "preflight: users apply refuses a sudo-needing role when visudo is unreachable" 1 "visudo" \
adm_with "$ADMPATH_DIR/no-visudo" "$ROOT/commands/users-apply.sh" --file "$ADM_USERS" --yes
# …and the refusal must land BEFORE the groups are converged, or the machine is
# already half-changed when the operator reads it.
check "preflight: …before any group is created" 1 "" \
adm_saw "$ADMPATH_DIR/no-visudo" 'rig-admin' \
"$ROOT/commands/users-apply.sh" --file "$ADM_USERS" --yes
# A users file needing no sudo must NOT be refused for a missing visudo: the
# guard has to be as narrow as the need, or it refuses healthy boxes.
printf '%s\n' 'maria ops ssh-ed25519 AAAAC3fixture maria@mac' > "$ADMPATH_DIR/users-nosudo"
check "preflight: …but a file with no sudo-backed role is not refused for visudo" 1 "" \
adm_saw "$ADMPATH_DIR/no-visudo" 'visudo' \
"$ROOT/commands/users-apply.sh" --file "$ADMPATH_DIR/users-nosudo" --yes
# The fourth call site (#139 review): bootstrap-tenant adds the tenant user to
# the docker group AFTER installing docker and node, so an unguarded PATH fails
# it mid-convergence on a machine it has already changed.
# staging-box because it is the one tenant role defined in rig's own tree: the
# others resolve through the template registry, and a preflight test must not
# depend on the network to reach the guard it is testing.
admstub "$ADMPATH_DIR/no-any"
check "preflight: bootstrap-tenant refuses a sbin-less PATH before it converges" 1 "usermod" \
adm_with "$ADMPATH_DIR/no-any" "$ROOT/commands/bootstrap-tenant.sh" staging-box
rm -rf "$ADMPATH_DIR"
# --- rig forgejo-runner (#109) ----------------------------------------------