forked from heavy-duty/rig
fix(close-root): DenyUsers judged fail-closed — patterns and USER@HOST flag
All three reviewers, same substance, and they were right that it was
lockout-adjacent: the literal grep passed a candidate whom a DenyUsers
PATTERN really denies ('DenyUsers dan*' vs admin 'dan'), and the door
closed on a false proof. The judgment now lives in the lib as a pure
deny_verdict: a literal hit flags, and so does ANY pattern or
host-qualified token — a token the check cannot prove irrelevant counts
as a hit, never as a pass. The asymmetry with AllowUsers is now the
same direction on both sides: every error closes toward repair, never
toward a welded-shut door.
Also (claude-bot): the -C probe resolves Match blocks against a
synthetic addr=127.0.0.1, so Match Address is out of the local proof's
scope — named in --help, the README, and the gate's comment, so the
separate-session advisory reads as load-bearing, not ceremony.
Regressions ride the sourced lib: wildcard (the review's dan* case),
'?', USER@HOST, literal hit, irrelevant-literals pass, plus a grep
guard that the shipped gate consults deny_verdict.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
2396880f09
commit
a5b48d5b2c
4 changed files with 88 additions and 15 deletions
15
README.md
15
README.md
|
|
@ -629,11 +629,16 @@ account — and then two **reachability** proofs (#17): `sudo -n true` under
|
|||
`runuser` must answer, so NOPASSWD sudo is effective rather than merely
|
||||
written, and `sshd -T -C user=<admin>` must resolve a per-user effective
|
||||
config that accepts the login (`pubkeyauthentication yes`, no `DenyUsers`
|
||||
hit, `AllowUsers` — if set — names them), so a `Match` block elsewhere cannot
|
||||
quietly exclude the admin while every file looks right. The refusal names
|
||||
which check failed, per candidate. What no local check can prove is that you
|
||||
*hold* the private key — which is why the separate-session verification below
|
||||
stays load-bearing. Never close the only door.
|
||||
hit — where any pattern or `USER@HOST` entry counts as a hit, fail closed,
|
||||
since `DenyUsers dan*` really denies admin `dan` and rig will not re-implement
|
||||
sshd's pattern engine to prove a miss — and `AllowUsers`, if set, names them
|
||||
literally), so a `Match` block elsewhere cannot quietly exclude the admin
|
||||
while every file looks right. The refusal names which check failed, per
|
||||
candidate. What no local check can prove: that you *hold* the private key,
|
||||
and how a `Match Address` rule treats your real client address (the probe
|
||||
resolves against a synthetic `addr=127.0.0.1`) — which is why the
|
||||
separate-session verification below stays load-bearing. Never close the only
|
||||
door.
|
||||
|
||||
Before running it, prove the admin door in a **separate** session — `ssh
|
||||
<admin>@<box>` while this one stays open. Root SSH is being welded shut; the
|
||||
|
|
|
|||
|
|
@ -145,3 +145,34 @@ assert_marker_human() {
|
|||
return 1 ;;
|
||||
esac
|
||||
}
|
||||
|
||||
# deny_verdict <user> <denyusers token...>
|
||||
#
|
||||
# Judge sshd's effective DenyUsers list against ONE candidate, fail closed.
|
||||
# Empty output = every token is PROVABLY irrelevant to <user>: literal (no
|
||||
# sshd pattern metacharacters, no host qualifier) and not this username.
|
||||
# Anything else prints the reason and the caller flags the candidate:
|
||||
#
|
||||
# - a literal hit — DenyUsers really names them;
|
||||
# - ANY pattern token (* or ?) — 'DenyUsers dan*' genuinely denies admin
|
||||
# 'dan', and this side of sshd cannot re-implement its pattern engine
|
||||
# just to prove a miss, so an unprovable token counts as a hit;
|
||||
# - ANY host-qualified token (USER@HOST) — whether it bites depends on the
|
||||
# client's address, which no local probe knows.
|
||||
#
|
||||
# The asymmetry with AllowUsers is deliberate and points the same direction:
|
||||
# AllowUsers must name the admin literally (a pattern that WOULD admit them
|
||||
# still refuses — over-refusing is safe), DenyUsers refuses on anything it
|
||||
# cannot prove misses. Both errors close toward "repair first", never toward
|
||||
# a welded-shut root door. Pure text→text, sourced by the harness.
|
||||
deny_verdict() {
|
||||
local u="$1" tok; shift
|
||||
for tok in "$@"; do
|
||||
case "$tok" in
|
||||
"$u") printf 'sshd DenyUsers names this user'; return 0 ;;
|
||||
*[*?]*) printf "sshd DenyUsers has pattern entry '%s' — cannot prove it misses this user; make it literal or remove it, then re-run" "$tok"; return 0 ;;
|
||||
*@*) printf "sshd DenyUsers has host-qualified entry '%s' — whether it bites depends on the client address, which no local check can prove; make it literal or remove it, then re-run" "$tok"; return 0 ;;
|
||||
esac
|
||||
done
|
||||
return 0
|
||||
}
|
||||
|
|
|
|||
|
|
@ -33,13 +33,17 @@ owned by the user and not group/world-writable, a real login shell, account
|
|||
not expired), then two reachability proofs (#17) — `sudo -n true` under
|
||||
runuser must answer (NOPASSWD sudo is effective, not merely written), and
|
||||
`sshd -T -C user=...` must resolve a per-user effective config that accepts
|
||||
the login (pubkeyauthentication yes, no DenyUsers hit, AllowUsers — if set —
|
||||
names them). The refusal names which check failed, per candidate. Run rig
|
||||
users apply first; never close the only door.
|
||||
the login (pubkeyauthentication yes, no DenyUsers hit — where any pattern or
|
||||
host-qualified Deny entry counts as a hit, fail closed — and AllowUsers, if
|
||||
set, names them literally). The refusal names which check failed, per
|
||||
candidate. Run rig users apply first; never close the only door.
|
||||
|
||||
Before running, verify your admin login in a SEPARATE session — `ssh
|
||||
<admin>@<box>` while this one stays open. Root SSH is the door being welded
|
||||
shut; the admin door must be proven, not presumed.
|
||||
shut; the admin door must be proven, not presumed. This is not ceremony: the
|
||||
local probe resolves Match blocks against a synthetic loopback client
|
||||
(addr=127.0.0.1), so a `Match Address` rule that treats real inbound clients
|
||||
differently is invisible to it — only a real login proves the real door.
|
||||
|
||||
Run as root. Convergent: once root is closed, a re-run is a clean no-op.
|
||||
EOF
|
||||
|
|
@ -157,16 +161,24 @@ while IFS= read -r a; do
|
|||
# Reachability proof 2 — sshd's per-user EFFECTIVE config accepts them.
|
||||
# `sshd -T -C user=...` resolves Match blocks for exactly this login, so an
|
||||
# exclusion the global `sshd -T` never shows is caught. Allow/Deny entries
|
||||
# are matched LITERALLY: a pattern that would in fact admit the admin still
|
||||
# flags here — fail closed, the operator proves patterns by hand — while a
|
||||
# denying pattern this literal match misses is one more reason the
|
||||
# separate-session verification stays load-bearing.
|
||||
# are judged fail-closed in BOTH directions: AllowUsers must name the admin
|
||||
# literally (a pattern that would in fact admit them still flags — the
|
||||
# operator proves patterns by hand), and DenyUsers flags on a literal hit
|
||||
# OR on any pattern/host-qualified token (deny_verdict, in the lib) —
|
||||
# 'DenyUsers dan*' really denies admin 'dan', and a token this check cannot
|
||||
# prove irrelevant must count as a hit, never as a pass. What no local
|
||||
# probe can resolve is a Match on the CLIENT's address — the -C probe pins
|
||||
# addr=127.0.0.1 — which is why the separate-session verification stays
|
||||
# load-bearing.
|
||||
if perT="$(sshd -T -C "user=$a,host=$(hostname),addr=127.0.0.1" 2>/dev/null)"; then
|
||||
if ! printf '%s\n' "$perT" | grep -qx 'pubkeyauthentication yes'; then
|
||||
flag "sshd resolves pubkeyauthentication != yes for this user"
|
||||
fi
|
||||
if printf '%s\n' "$perT" | grep -i '^denyusers ' | tr ' ' '\n' | grep -qx "$a"; then
|
||||
flag "sshd DenyUsers names this user"
|
||||
deny_line="$(printf '%s\n' "$perT" | grep -i '^denyusers ' | head -n1)"
|
||||
if [ -n "$deny_line" ]; then
|
||||
# shellcheck disable=SC2086 # word-splitting the tokens is the point
|
||||
deny_reason="$(deny_verdict "$a" ${deny_line#* })"
|
||||
[ -n "$deny_reason" ] && flag "$deny_reason"
|
||||
fi
|
||||
if printf '%s\n' "$perT" | grep -qi '^allowusers ' \
|
||||
&& ! printf '%s\n' "$perT" | grep -i '^allowusers ' | tr ' ' '\n' | grep -qx "$a"; then
|
||||
|
|
|
|||
25
test/cli.sh
25
test/cli.sh
|
|
@ -567,6 +567,31 @@ check "users close-root: per-user sshd resolve precedes the drop-in install" \
|
|||
# never die on a missing prover. Grep the graceful branch.
|
||||
check "users close-root: a missing runuser skips the sudo proof, loudly" 0 "" \
|
||||
grep -q "runuser not found" "$ROOT/commands/users-close-root.sh"
|
||||
# DenyUsers judged fail-closed through the lib's pure deny_verdict — sshd
|
||||
# accepts patterns and USER@HOST forms, and 'DenyUsers dan*' REALLY denies
|
||||
# admin 'dan', so a token the check cannot prove irrelevant must flag, never
|
||||
# pass (the review's regression: a wildcard denial). Empty output is the only
|
||||
# pass; every hit names its reason.
|
||||
deny_v() { # deny_v <user> <token...>
|
||||
bash -c 'set -euo pipefail
|
||||
. "$1/commands/lib/users-config.sh"; shift
|
||||
deny_verdict "$@"' _ "$ROOT" "$@"
|
||||
}
|
||||
check "users close-root: deny_verdict flags a literal hit" \
|
||||
0 "names this user" deny_v admin root admin
|
||||
check "users close-root: deny_verdict fails closed on a wildcard (dan* vs dan)" \
|
||||
0 "pattern entry 'dan*'" deny_v dan "dan*"
|
||||
check "users close-root: deny_verdict fails closed on '?' patterns" \
|
||||
0 "pattern entry" deny_v admin "admi?"
|
||||
check "users close-root: deny_verdict fails closed on USER@HOST forms" \
|
||||
0 "host-qualified" deny_v admin "admin@10.0.0.1"
|
||||
deny_pass() { [ -z "$(deny_v "$@")" ]; } # empty verdict IS the pass
|
||||
check "users close-root: deny_verdict passes provably-irrelevant literals" \
|
||||
0 "" deny_pass admin root git backup
|
||||
# ...and the shipped gate must actually consult it (call, not comment).
|
||||
# shellcheck disable=SC2016
|
||||
check "users close-root: the gate consults deny_verdict" 0 "" \
|
||||
grep -qE '^[[:space:]]*deny_reason="\$\(deny_verdict ' "$ROOT/commands/users-close-root.sh"
|
||||
# Marker-gate refusals through the sourced lib against fixture markers: the CLI
|
||||
# path sits behind the root check, so the gate is a pure lib function on
|
||||
# purpose (repo precedent: parse_users_file, assert_runner_repo). The command
|
||||
|
|
|
|||
Loading…
Reference in a new issue