diff --git a/docs/deprovision-os-account.md b/docs/deprovision-os-account.md index f3eeddd9..e8f8828c 100644 --- a/docs/deprovision-os-account.md +++ b/docs/deprovision-os-account.md @@ -174,9 +174,15 @@ implementation calls is a restatement of its own beliefs, not an audit. Two mode ``` ./scripts/assert-uid-free.sh --capture green # BEFORE: prints "green 1001 165536 65536" -sudo ./scripts/assert-uid-free.sh --check green 1001 165536 65536 # AFTER: exit 1 unless clean +sudo DATA_PATH="$DATA_PATH" ./scripts/assert-uid-free.sh --check green 1001 165536 65536 # AFTER: exit 1 unless clean ``` +**Pass `DATA_PATH` through explicitly.** sudo's `env_reset` drops it, so the plain `sudo ./assert-uid-free.sh` +this used to say fell back to a hardcoded default — and every check here reports `ok` on finding nothing, so +a wrong root reports `CLEAN — uid safe to reissue` without having looked at a single member tree. The ACL +check is the one that failed silently and completely, because it is the only one scoped to `DATA_PATH` alone. +The script now refuses to run when a search root is missing rather than passing vacuously. + The range has to be captured **before** deletion, because `userdel` removes the `/etc/subuid` entry with the account. After that there is no way to ask what range it held — and a check that silently skips that half is the exact failure this section exists to prevent. @@ -293,7 +299,7 @@ To validate on the production host, against a throwaway account: 4. Confirm the range check *fails* on that tree while the account still exists. A checker that has never failed has not been tested. 5. Delete through the UI, then - `sudo ./scripts/assert-uid-free.sh --check `. + `sudo DATA_PATH="$DATA_PATH" ./scripts/assert-uid-free.sh --check `. The delete handler logs that exact command line with the captured values after a successful deprovision, because after `userdel` nothing else on the machine remembers the range. diff --git a/scripts/assert-uid-free.sh b/scripts/assert-uid-free.sh index 4b6600be..25b437f1 100755 --- a/scripts/assert-uid-free.sh +++ b/scripts/assert-uid-free.sh @@ -19,6 +19,35 @@ SEARCH_ROOTS=("$DATA_PATH" /home) usage() { echo "usage: $0 --capture | --check " >&2; exit 2; } +# ── A search root that does not exist makes this whole script lie ── +# +# Every check below is "look for X; report ok when nothing is found", so a root that is missing reports +# clean without having looked. That is not hypothetical here: this script is documented to run under +# `sudo`, and sudo's env_reset DROPS DATA_PATH, so the fallback above is what actually gets used. On a +# host where the fallback is wrong, `--check` scans a directory that does not exist, finds nothing, and +# prints "CLEAN — uid safe to reissue". +# +# The ACL check is the one that fails silently and completely, because it is scoped to DATA_PATH alone. +# The exact check added to catch the hazard ownership cannot see is the one a missing DATA_PATH disables. +# +# So: refuse to run rather than pass vacuously. Same posture the subuid section of the spec argues for. +require_roots() { + local missing=() + for root in "${SEARCH_ROOTS[@]}"; do + [[ -d "$root" ]] || missing+=("$root") + done + if (( ${#missing[@]} )); then + echo "refusing to check: these search roots do not exist: ${missing[*]}" >&2 + echo "" >&2 + echo "DATA_PATH is currently '$DATA_PATH'. sudo strips it from the environment, so pass it through:" >&2 + echo " sudo DATA_PATH=/path/to/data $0 --check ..." >&2 + echo " (or: sudo -E $0 --check ...)" >&2 + echo "" >&2 + echo "Every check here reports 'ok' on finding nothing, so a wrong root reports CLEAN without looking." >&2 + exit 2 + fi +} + if [[ "${1:-}" == "--capture" ]]; then user="${2:?user required}" uid="$(id -u "$user" 2>/dev/null)" || { echo "no such account: $user" >&2; exit 1; } @@ -30,6 +59,16 @@ fi [[ "${1:-}" == "--check" ]] || usage user="${2:?}"; uid="${3:?}"; sub_start="${4:?}"; sub_count="${5:?}" +require_roots + +# The range arithmetic has to be numbers. `deprovisionOsAccount` logs '' in this position +# when the account had no /etc/subuid entry, and pasting that log line straight in — which is exactly how +# it is meant to be used — would otherwise make sub_end empty and turn the range scan into a no-op. +[[ "$uid" =~ ^[0-9]+$ && "$sub_start" =~ ^[0-9]+$ && "$sub_count" =~ ^[0-9]+$ ]] || { + echo "uid, subuid_start and subuid_count must all be numbers (got: '$uid' '$sub_start' '$sub_count')" >&2 + echo "an account with no /etc/subuid range has nothing to scan for — verify the uid half by hand" >&2 + exit 2 +} sub_end=$(( sub_start + sub_count - 1 )) fails=0 diff --git a/src/servers/api/users/manage-users.ts b/src/servers/api/users/manage-users.ts index d3eda6c2..8646a2f6 100644 --- a/src/servers/api/users/manage-users.ts +++ b/src/servers/api/users/manage-users.ts @@ -4,6 +4,7 @@ import type { UserRole } from 'officerdb'; import * as errors from '@@/custom-errors'; import { OS_USERS_ENABLED } from '@@/os-user'; import { deprovisionOsAccount } from '@@/os-user-deprovision'; +import { DATA_PATH } from '@@/data-path'; // Owner-only management of the other accounts. Everything here is gated by ownerGate in // users-router.ts; these handlers assume the caller is the Super Admin. @@ -139,10 +140,19 @@ export const deleteUserHandler: Handler = async function (ctx) { if (deprovisioned.freed) { // The audit line. `scripts/assert-uid-free.sh --check` takes exactly these arguments, and after // `userdel` this log is the only place the freed subuid range still exists. + // + // DATA_PATH is spelled out rather than left to the operator, because this line exists to be COPIED + // and sudo's env_reset drops it — the version without it fell back to a hardcoded default and made + // the checker report CLEAN without reading a single member tree. const { osUser, uid, subUid } = deprovisioned.freed; + const range = subUid ? `${subUid.start} ${subUid.count}` : null; console.info( - `[users] deprovisioned ${osUser} — verify with: sudo ./scripts/assert-uid-free.sh --check ` + - `${osUser} ${uid} ${subUid?.start ?? ''} ${subUid?.count ?? ''}`, + range + ? `[users] deprovisioned ${osUser} — verify with: sudo DATA_PATH=${DATA_PATH} ` + + `./scripts/assert-uid-free.sh --check ${osUser} ${uid} ${range}` + : `[users] deprovisioned ${osUser} (uid ${uid}) — it had no /etc/subuid range, so only the uid ` + + `half is verifiable: sudo DATA_PATH=${DATA_PATH} ./scripts/assert-uid-free.sh --check ` + + `${osUser} ${uid} will refuse without real numbers`, ); } }