Merge pull request 'fix: refuse a PATH without /usr/sbin, before the token prompt' (#140) from build/139-sbin-path-preflight into main

Reviewed-on: #140
Reviewed-by: grok-reviewer-andresmgsl <andres+3@heavyduty.builders>
Reviewed-by: kimi-reviewer-andresmgsl <andres+4@heavyduty.builders>
Reviewed-by: codex-reviewer-andresmgsl <andres+2@heavyduty.builders>

changelog.d/139.md

@@ -0,0 +1,4 @@
+### Fixed
+
+- `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)

commands/bootstrap-tenant.sh

@@ -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.

commands/forgejo-runner-install.sh

@@ -17,6 +17,8 @@ set -euo pipefail
 HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)"
 # shellcheck source=SCRIPTDIR/lib/forgejo-runner-config.sh
 . "$HERE/lib/forgejo-runner-config.sh"
+# shellcheck source=SCRIPTDIR/lib/admin-path.sh
+. "$HERE/lib/admin-path.sh"
 
 log()  { printf 'rig-forgejo-runner: %s\n' "$*"; }
 warn() { printf 'rig-forgejo-runner: WARNING: %s\n' "$*" >&2; }
@@ -229,6 +231,17 @@ fi
 
 # --- guards ----------------------------------------------------------------
 [ "$(id -u)" -eq 0 ] || die "must run as root"
+# 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.
+#
+# 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.

commands/lib/admin-path.sh

@@ -0,0 +1,47 @@
+#!/usr/bin/env bash
+# admin-path.sh — assert the admin binaries are REACHABLE, not merely that we
+# are root.
+#
+# Being uid 0 and being able to find useradd are different facts, and rig
+# asserted only the first. `su` without `-`, sudo with a sanitised secure_path,
+# and several container images all hand you a root shell whose PATH carries no
+# /usr/sbin — which is where useradd, usermod and groupadd live on Debian. The
+# result was a bare `useradd: command not found` naming a line number inside a
+# 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
+# message carries the fix so the refusal costs one paste, not an investigation.
+
+# require_admin_bins <bin>... — die unless every one resolves on PATH.
+require_admin_bins() {
+  local missing=() b
+  for b in "$@"; do
+    command -v "$b" >/dev/null 2>&1 || missing+=("$b")
+  done
+  [ "${#missing[@]}" -eq 0 ] && return 0
+  # Names the REMEDY, not this script: the operator typed a `rig ...` command,
+  # and echoing the internal path back at them is the unhelpful half of the
+  # original `useradd: command not found`.
+  die "cannot find ${missing[*]} on PATH — it lives in /usr/sbin, which this root shell does not carry (a 'su' without '-' does this, and so do some container images). Re-run the same rig command with: PATH=/usr/sbin:/sbin:\$PATH"
+}

commands/runner-install.sh

@@ -9,6 +9,8 @@ set -euo pipefail
 HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)"
 # shellcheck source=SCRIPTDIR/lib/runner-config.sh
 . "$HERE/lib/runner-config.sh"
+# shellcheck source=SCRIPTDIR/lib/admin-path.sh
+. "$HERE/lib/admin-path.sh"
 
 log()  { printf 'rig-runner: %s\n' "$*"; }
 warn() { printf 'rig-runner: WARNING: %s\n' "$*" >&2; }
@@ -85,6 +87,10 @@ VERSION="${VERSION#v}"
 
 # --- guards ----------------------------------------------------------------
 [ "$(id -u)" -eq 0 ] || die "must run as root"
+# 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
 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.

commands/users-apply.sh

@@ -11,6 +11,8 @@ set -euo pipefail
 HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)"
 # shellcheck source=SCRIPTDIR/lib/users-config.sh
 . "$HERE/lib/users-config.sh"
+# shellcheck source=SCRIPTDIR/lib/admin-path.sh
+. "$HERE/lib/admin-path.sh"
 
 log()  { printf 'rig-users: %s\n' "$*"; }
 warn() { printf 'rig-users: WARNING: %s\n' "$*" >&2; }
@@ -136,6 +138,10 @@ done <<< "$PARSED"
 
 # --- guards ------------------------------------------------------------------
 [ "$(id -u)" -eq 0 ] || die "must run as root"
+# 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 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
@@ -195,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

test/cli.sh

@@ -3334,6 +3334,121 @@ check "ci-box: its install.sh does not register" 1 "" \
 check "ci-box: bootstrap-tenant does not read the staging dir" 1 "" \
   grep -q 'docs/templates' "$ROOT/commands/bootstrap-tenant.sh"
 
+# --- admin binaries must be reachable, not just root (#139) ------------------
+# Being root and being able to FIND the admin binaries are different facts, and
+# rig asserted only the first. `su` without `-`, sudo with a sanitised
+# secure_path, and several container images all give a root shell whose PATH
+# carries no /usr/sbin — where useradd lives. Reported from a real ci-box:
+#
+#   root@ci-forgejo-box:/home/dev# rig forgejo-runner install --instance …
+#   forgejo runner registration token:
+#   …/forgejo-runner-install.sh: line 250: useradd: command not found
+#
+# Note where it died: AFTER reading a registration token off the operator's
+# terminal. A secret typed for a run that could never succeed is the avoidable
+# half of the bug, so the refusal has to come before the prompt.
+#
+# The root check fires first and correctly, so these stub `id -u` to 0 — the
+# idiom the bootstrap --undo block above already uses — to reach the preflight.
+ADMPATH_DIR="$(mktemp -d)"
+mkdir -p "$ADMPATH_DIR/bin"
+# shellcheck disable=SC2016 # the body is 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' \
+  > "$ADMPATH_DIR/bin/id"
+chmod +x "$ADMPATH_DIR/bin/id"
+# A PATH with the stub and the ordinary bindirs, but deliberately no /usr/sbin.
+SBINLESS="$ADMPATH_DIR/bin:/usr/local/bin:/usr/bin:/bin"
+adm_run() { env PATH="$SBINLESS" "$@" 2>&1; }
+adm_prompted() {  # did it read a token before refusing?
+  adm_run "$@" | grep -qi 'registration token:'
+}
+check "preflight: forgejo-runner install refuses a PATH with no /usr/sbin" 1 "useradd" \
+  adm_run "$ROOT/commands/forgejo-runner-install.sh" --instance https://f.example.com
+check "preflight: …and names PATH as the cause, not just the missing binary" 1 "PATH" \
+  adm_run "$ROOT/commands/forgejo-runner-install.sh" --instance https://f.example.com
+check "preflight: …and refuses BEFORE prompting for a token" 1 "" \
+  adm_prompted "$ROOT/commands/forgejo-runner-install.sh" --instance https://f.example.com
+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) ----------------------------------------------
 FR="$ROOT/commands/forgejo-runner-install.sh"
 check "forgejo-runner: bare subcommand shows usage, exit 2" 2 "usage:" "$ROOT/bin/rig" forgejo-runner