diff --git a/changelog.d/139.md b/changelog.d/139.md new file mode 100644 index 0000000..7ce5028 --- /dev/null +++ b/changelog.d/139.md @@ -0,0 +1,4 @@ +### Fixed + +- `rig forgejo-runner install`, `rig runner install`, `rig users apply` and `rig bootstrap ` 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) diff --git a/commands/bootstrap-tenant.sh b/commands/bootstrap-tenant.sh index ee251c6..3799eb6 100755 --- a/commands/bootstrap-tenant.sh +++ b/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. diff --git a/commands/forgejo-runner-install.sh b/commands/forgejo-runner-install.sh index d80d44a..3e4c376 100755 --- a/commands/forgejo-runner-install.sh +++ b/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. diff --git a/commands/lib/admin-path.sh b/commands/lib/admin-path.sh new file mode 100644 index 0000000..45fd818 --- /dev/null +++ b/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 ... — 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" +} diff --git a/commands/runner-install.sh b/commands/runner-install.sh index 6dcd97f..e8800f6 100755 --- a/commands/runner-install.sh +++ b/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. diff --git a/commands/users-apply.sh b/commands/users-apply.sh index 67fca52..b69fd89 100755 --- a/commands/users-apply.sh +++ b/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 diff --git a/test/cli.sh b/test/cli.sh index 948a79b..9c93855 100644 --- a/test/cli.sh +++ b/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 ... — a bindir resolving id(0) plus ... + 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 ... + local d="$1"; shift + env PATH="$d:/usr/local/bin:/usr/bin:/bin" "$@" 2>&1 +} +# adm_saw ... — exit 0 if the run printed . +# 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