From a32d2b04cb99d9053296b9c855023fbf537047f4 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Sat, 18 Jul 2026 17:17:39 +0000 Subject: [PATCH] =?UTF-8?q?fix(close-root):=20the=20gate=20judges=20AllowG?= =?UTF-8?q?roups/DenyGroups=20too=20=E2=80=94=20same=20door,=20other=20hin?= =?UTF-8?q?ge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 convergence (codex + claude-bot): sshd enforces the group directives against the candidate's ACTUAL membership, and the gate read only the *Users pair — an admin outside 'AllowGroups sudo' still reached ADMIN_OK=1, and root closed on a false proof. The gate now resolves id -Gn and judges both group directives with the *Users discipline: DenyGroups flags on a held-group literal or ANY pattern/host-qualified token; AllowGroups, when set, passes only on a literal token naming a held group (a pattern that would admit proves nothing — over-refusing stays the safe error). id failing yields no groups, which makes a set AllowGroups flag: fail closed there too. Both requested regressions ride the sourced lib (unmet AllowGroups, DenyGroups naming a held group) plus the pattern/pass cases, and grep guards pin the shipped gate to the verdicts and to real membership. Co-Authored-By: Claude Fable 5 --- README.md | 6 +++-- commands/lib/users-config.sh | 43 ++++++++++++++++++++++++++++++++++++ commands/users-close-root.sh | 26 +++++++++++++++++++--- test/cli.sh | 28 +++++++++++++++++++++++ 4 files changed, 98 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index e3e1aa4..7a1483b 100644 --- a/README.md +++ b/README.md @@ -631,8 +631,10 @@ written, and `sshd -T -C user=` must resolve a per-user effective config that accepts the login (`pubkeyauthentication yes`, no `DenyUsers` 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 +sshd's pattern engine to prove a miss — `AllowUsers`, if set, names them +literally, and the same fail-closed pair for `DenyGroups`/`AllowGroups` +judged against the admin's actual groups), 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 diff --git a/commands/lib/users-config.sh b/commands/lib/users-config.sh index 7ce5578..81c9c15 100644 --- a/commands/lib/users-config.sh +++ b/commands/lib/users-config.sh @@ -176,3 +176,46 @@ deny_verdict() { done return 0 } + +# group_deny_verdict +# +# deny_verdict's sibling for sshd's DenyGroups, judged against the +# candidate's ACTUAL group membership (id -Gn), same fail-closed rule: +# empty output = every token is provably irrelevant — literal and naming +# none of the candidate's groups. A literal token naming a group they are +# in flags, and so does any pattern or host-qualified token, because a +# token this side of sshd cannot prove irrelevant may be the one that +# denies. Pure text→text, sourced by the harness. +group_deny_verdict() { + local groups="$1" tok g; shift + for tok in "$@"; do + case "$tok" in + *[*?]*) printf "sshd DenyGroups has pattern entry '%s' — cannot prove it misses this user's groups; make it literal or remove it, then re-run" "$tok"; return 0 ;; + *@*) printf "sshd DenyGroups 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 ;; + *) for g in $groups; do + if [ "$tok" = "$g" ]; then + printf "sshd DenyGroups names '%s' — a group this user is in" "$g"; return 0 + fi + done ;; + esac + done + return 0 +} + +# group_allow_verdict +# +# AllowGroups' direction: when the directive is set, sshd admits only +# members of a matching group, so the proof must be a LITERAL token +# naming a group the candidate is in. A pattern that would in fact admit +# them proves nothing here (same stance as AllowUsers: over-refusing is +# the safe error), so no literal hit → flag. Pure text→text. +group_allow_verdict() { + local groups="$1" tok g; shift + for tok in "$@"; do + for g in $groups; do + [ "$tok" = "$g" ] && return 0 + done + done + printf "sshd AllowGroups is set and no entry literally names a group this user is in — add their group (or them to a named group), then re-run" + return 0 +} diff --git a/commands/users-close-root.sh b/commands/users-close-root.sh index 893b88f..4330f82 100755 --- a/commands/users-close-root.sh +++ b/commands/users-close-root.sh @@ -34,9 +34,11 @@ 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 — 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. +host-qualified Deny entry counts as a hit, fail closed — AllowUsers, if set, +names them literally, and the same pair of rules for DenyGroups/AllowGroups +judged against the admin's actual groups from id -Gn). 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 @` while this one stays open. Root SSH is the door being welded @@ -180,6 +182,24 @@ while IFS= read -r a; do deny_reason="$(deny_verdict "$a" ${deny_line#* })" [ -n "$deny_reason" ] && flag "$deny_reason" fi + # The group directives close the same door through the other hinge: sshd + # enforces Allow/DenyGroups against the candidate's ACTUAL membership, so + # the gate resolves id -Gn and judges both with the same fail-closed + # discipline as the *Users pair. id failing yields no groups, which makes + # a set AllowGroups flag — the safe direction. + a_groups="$(id -Gn -- "$a" 2>/dev/null)" + denyg_line="$(printf '%s\n' "$perT" | grep -i '^denygroups ' | head -n1)" + if [ -n "$denyg_line" ]; then + # shellcheck disable=SC2086 # word-splitting the tokens is the point + denyg_reason="$(group_deny_verdict "$a_groups" ${denyg_line#* })" + [ -n "$denyg_reason" ] && flag "$denyg_reason" + fi + allowg_line="$(printf '%s\n' "$perT" | grep -i '^allowgroups ' | head -n1)" + if [ -n "$allowg_line" ]; then + # shellcheck disable=SC2086 # word-splitting the tokens is the point + allowg_reason="$(group_allow_verdict "$a_groups" ${allowg_line#* })" + [ -n "$allowg_reason" ] && flag "$allowg_reason" + fi if printf '%s\n' "$perT" | grep -qi '^allowusers ' \ && ! printf '%s\n' "$perT" | grep -i '^allowusers ' | tr ' ' '\n' | grep -qx "$a"; then flag "sshd AllowUsers is set and does not name this user" diff --git a/test/cli.sh b/test/cli.sh index 38bfd90..23a9901 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -588,6 +588,34 @@ check "users close-root: deny_verdict fails closed on USER@HOST forms" \ 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 +# The group directives, same discipline, judged against the candidate's ACTUAL +# membership (the review's regressions: an unmet AllowGroups, a DenyGroups +# naming a group they hold). First arg is the id -Gn word list. +groups_v() { # groups_v + bash -c 'set -euo pipefail + . "$1/commands/lib/users-config.sh"; shift + "$@"' _ "$ROOT" "$@" +} +groups_pass() { [ -z "$(groups_v "$@")" ]; } +check "users close-root: DenyGroups naming a held group flags" \ + 0 "a group this user is in" groups_v group_deny_verdict "dan sudo rig-admin" backup sudo +check "users close-root: DenyGroups fails closed on patterns" \ + 0 "pattern entry 'rig-*'" groups_v group_deny_verdict "dan rig-admin" "rig-*" +check "users close-root: DenyGroups passes provably-irrelevant literals" \ + 0 "" groups_pass group_deny_verdict "dan rig-admin" docker backup +check "users close-root: an unmet AllowGroups flags (fail closed)" \ + 0 "no entry literally names a group this user is in" groups_v group_allow_verdict "dan rig-admin" sudo +check "users close-root: AllowGroups pattern is no proof (fail closed)" \ + 0 "no entry literally names" groups_v group_allow_verdict "dan rig-admin" "rig-*" +check "users close-root: a literally-named held group passes AllowGroups" \ + 0 "" groups_pass group_allow_verdict "dan rig-admin" sudo rig-admin +# ...and the shipped gate consults both, against real membership. +# shellcheck disable=SC2016 +check "users close-root: the gate consults the group verdicts" 0 "" \ + grep -qE '^[[:space:]]*denyg_reason="\$\(group_deny_verdict ' "$ROOT/commands/users-close-root.sh" +# shellcheck disable=SC2016 +check "users close-root: the gate resolves real membership (id -Gn)" 0 "" \ + grep -qF -- 'id -Gn -- "$a"' "$ROOT/commands/users-close-root.sh" # ...and the shipped gate must actually consult it (call, not comment). # shellcheck disable=SC2016 check "users close-root: the gate consults deny_verdict" 0 "" \