the acl check could not fail, because sudo drops DATA_PATH
Review of 76cd7c2. The ACL finding is right and the fix is correct — verified here that `setfacl -R -P -b`
removes the default entries as well as the access ones, which the man page splits between -b and -k and
does not settle. `acl` is already a core package in setup.sh, so the new hard dependency is real.
But the checker it added cannot fail in the way it is documented to be run.
`assert-uid-free.sh` is invoked as `sudo ./assert-uid-free.sh --check ...`, and sudo's env_reset DROPS
DATA_PATH, so the script falls back to the hardcoded `/home/pastilhas/officerdev/data` — which is not this
machine's data directory and does not exist. Every check in the file is "look for X, report ok when nothing
is found", so a missing root reports clean without looking. Demonstrated: a tree carrying both
`user:65534:rwx` and `default:user:65534:rwx` was reported as `ok no ACL entries naming uid 65534`.
The ACL check is the one that fails silently and completely, because it is the only one scoped to DATA_PATH
alone — the uid and subuid scans still walk /home and would catch something. So the check just added to
catch the hazard ownership cannot see is the check a wrong DATA_PATH disables.
Fixed by refusing rather than passing:
require_roots every search root must exist, or exit 2 naming it and showing the sudo invocation
that preserves DATA_PATH
numeric guard uid/start/count must be numbers. deprovisionOsAccount logs '<no-subuid-range>' in
that position for an account with no /etc/subuid entry, and pasting that log line in
— which is exactly how it is meant to be used — made sub_end empty and turned the
range scan into a no-op.
The handler's audit line now prints DATA_PATH inside the command it tells the operator to copy, and says
so explicitly when there is no subuid range rather than emitting a command that cannot work.
Verified: bogus root exits 2, non-numeric range exits 2, and the ACL check FAILS on a specimen tree
carrying the entries — the "make it fail before trusting it to pass" step from the spec's own subuid
section, now done for the ACL half too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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"
|
./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
|
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
|
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.
|
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
|
4. Confirm the range check *fails* on that tree while the account still exists. A checker that has never
|
||||||
failed has not been tested.
|
failed has not been tested.
|
||||||
5. Delete through the UI, then
|
5. Delete through the UI, then
|
||||||
`sudo ./scripts/assert-uid-free.sh --check <user> <uid> <start> <count>`.
|
`sudo DATA_PATH="$DATA_PATH" ./scripts/assert-uid-free.sh --check <user> <uid> <start> <count>`.
|
||||||
|
|
||||||
The delete handler logs that exact command line with the captured values after a successful deprovision,
|
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.
|
because after `userdel` nothing else on the machine remembers the range.
|
||||||
|
|||||||
@@ -19,6 +19,35 @@ SEARCH_ROOTS=("$DATA_PATH" /home)
|
|||||||
|
|
||||||
usage() { echo "usage: $0 --capture <user> | --check <user> <uid> <subuid_start> <subuid_count>" >&2; exit 2; }
|
usage() { echo "usage: $0 --capture <user> | --check <user> <uid> <subuid_start> <subuid_count>" >&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
|
if [[ "${1:-}" == "--capture" ]]; then
|
||||||
user="${2:?user required}"
|
user="${2:?user required}"
|
||||||
uid="$(id -u "$user" 2>/dev/null)" || { echo "no such account: $user" >&2; exit 1; }
|
uid="$(id -u "$user" 2>/dev/null)" || { echo "no such account: $user" >&2; exit 1; }
|
||||||
@@ -30,6 +59,16 @@ fi
|
|||||||
|
|
||||||
[[ "${1:-}" == "--check" ]] || usage
|
[[ "${1:-}" == "--check" ]] || usage
|
||||||
user="${2:?}"; uid="${3:?}"; sub_start="${4:?}"; sub_count="${5:?}"
|
user="${2:?}"; uid="${3:?}"; sub_start="${4:?}"; sub_count="${5:?}"
|
||||||
|
require_roots
|
||||||
|
|
||||||
|
# The range arithmetic has to be numbers. `deprovisionOsAccount` logs '<no-subuid-range>' 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 ))
|
sub_end=$(( sub_start + sub_count - 1 ))
|
||||||
|
|
||||||
fails=0
|
fails=0
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import type { UserRole } from 'officerdb';
|
|||||||
import * as errors from '@@/custom-errors';
|
import * as errors from '@@/custom-errors';
|
||||||
import { OS_USERS_ENABLED } from '@@/os-user';
|
import { OS_USERS_ENABLED } from '@@/os-user';
|
||||||
import { deprovisionOsAccount } from '@@/os-user-deprovision';
|
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
|
// 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.
|
// 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) {
|
if (deprovisioned.freed) {
|
||||||
// The audit line. `scripts/assert-uid-free.sh --check` takes exactly these arguments, and after
|
// 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.
|
// `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 { osUser, uid, subUid } = deprovisioned.freed;
|
||||||
|
const range = subUid ? `${subUid.start} ${subUid.count}` : null;
|
||||||
console.info(
|
console.info(
|
||||||
`[users] deprovisioned ${osUser} — verify with: sudo ./scripts/assert-uid-free.sh --check ` +
|
range
|
||||||
`${osUser} ${uid} ${subUid?.start ?? '<no-subuid-range>'} ${subUid?.count ?? ''}`,
|
? `[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} <start> <count> will refuse without real numbers`,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user