fix: harden account lifecycle, schedulers and signed releases - #45
Merged
Merged
Conversation
DisableLogin issued chage -E 1970-01-01. chage stores that field as days since the epoch, so the literal date encoded to 0 in the eighth /etc/shadow field — verified against this host's chage, which writes 0 for 1970-01-01 and 1 for 1970-01-02. shadow(5) on the same host says of that value: "The value 0 should not be used as it is interpreted as either an account with no expiration, or as an expiration on Jan 1, 1970." shadow's isexpired() takes the first reading; it requires sp_expire > 0 before it will call an account expired. DisableLogin's own comment states why that matters: expiry is the effective gate for a key-based account, because locking the password does not stop a public-key login. Writing the one encoding shadow calls ambiguous into the field that gate depends on is the whole defect. The constant's comment shows the intent was already there — it declined to pass a literal "0" as chage's argument because that reads ambiguously next to -E -1. It simply moved the same ambiguity one layer down, into the stored value. The date is now one day past the epoch: equally and unambiguously in the past, and stored as 1. The new test pins the invariant rather than the literal: it decodes the constant to the stored day count and requires it to be both positive and in the past. It was mutation-checked against the old value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ensureSubordinateIDsAbsent compared only the login name, while subuid(5) and subgid(5) define the first field as "login name or UID" — and the manual page recommends the numeric form on hosts with many entries, so a config-management or container tool writing it is ordinary rather than exotic. A numeric-owner entry therefore read as clean, ReconcileAccountDatabaseAfterDeletion reported the database reconciled, and the caller dropped the registry row that was the residue's last recovery pointer. The callers already hold the account's numeric identity, so no exported signature changes. Grant's pre-reload rollback skipped the reload whenever no removal marker predated the call, on the invariant that the running daemon cannot have seen a file this call created. WriteRootFile replaces an existing drop-in in place, and one left by an earlier granted-and-reloaded call may already be in daemon memory. Unlinking it without asking sshd to re-read left the daemon enforcing a grant whose file this tool had removed — a divergence between what it reports and what the host does. The rollback now recognises the replaced-file case. Both tests were mutation-checked. The sshd one also documents a trap it hit first: Grant validates once before writing and once after, and failing the pre-check refuses the call before anything is written, so no rollback runs at all and the test proves nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sshPortFromSshdT read only `port` from sshd -T. OpenSSH's ListenAddress with an explicit port creates a listener without adding to options->ports, and fill_default_server_options still leaves ports[0] = 22, so a host configured with only `ListenAddress 0.0.0.0:2222` reports `port 22`. Verified against OpenSSH 9.2 on this host: that config prints port 22 alongside listenaddress 0.0.0.0:2222, while `Port 2222` prints both as 2222. The invite therefore told the collaborator to connect to a port nothing was listening on — confidently, with none of the UNVERIFIED signalling the tool uses when it cannot establish something. sshd -T renders every listener as a listenaddress carrying an explicit port, including the defaults, so the listener set is complete and authoritative. One distinct port is the answer; several are the operator's decision and fail closed for an explicit --port; an entry that cannot be parsed voids the whole set rather than silently narrowing it. RejectUnder split mountinfo with strings.Fields, which splits on unicode.IsSpace. The kernel escapes exactly four bytes inside path fields — fs/proc_namespace.c passes " \t\n\\" to seq_dentry and seq_path_root — so \v, \f and \r arrive literally and added a field, shifting the positional mountpoint. The line "... /home/u/x\v/etc /home/u/hidden ..." then read /etc as the mountpoint, an unrelated path, and the check reported no mount under /home/u for a root that had one. That check exists to stop a recursive removal at a mount boundary. The mountinfo test earned its payload the hard way: an earlier version used a byte sequence whose split halves were not valid paths, so the old parser tripped its mountpoint validation and the test passed against both implementations. Only a payload whose shifted field is itself a plausible path outside the root demonstrates the false negative. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An sshd config line beginning with '=' carries an empty keyword through parseSSHDDirective, and the Match/Include scan skipped it with the branch meant for blank lines. sshd honours such a line: verified on OpenSSH 9.2, a config carrying "=Match Address 203.0.113.0/24" then "PubkeyAuthentication no" reports pubkeyauthentication no for a matching connection and yes without it. The scan therefore missed a connection-scoped Match and let an invite claim a login sshd would deny. A line with arguments but no keyword is now unmodellable rather than blank, and the scan fails closed. ensureAtd's systemd branch uses `enable --now`; the OpenRC and sysvinit branches only started atd. Those branches are reached exactly on hosts with no systemd, where the at fallback IS the auto-revoke mechanism, so every queued revocation stopped firing after a reboot. Both now also arm it, best effort, with the status probe still deciding the result. CheckIntegrity required the sequence to cover the rows as soon as a sequence file existed beside a legacy header, contradicting its own doc comment and Init, whose migration path reseeds from the highest recorded UID. The reported error is not ErrIdentitySequenceMissing and the registry is not v5, so neither recover-identity-sequence nor RepairMissingIdentitySequence would act on it: the operator was told to restore from backup for a host Init fixes on the next run. Two invite registry writes treated a *fsutil.DurabilityError as a plain failure. AtomicWriteFileAt renames the file into place and only then fsyncs the directory, so that error means the row is already on disk. The creation-intent row was abandoned with tx.registered still false and no release cleanup registered, and the pending-to-completed row left tx.rec on the superseded shape that persistDeletionStarted compares strictly — aborting invite's own rollback. Both adopt the committed row and then fail, matching how sudoers, sshdconf and schedule already handle this primitive. The invite pair has no test: App.Registry is a concrete *registry.Store and fsutil exposes no durability hook, so reaching the case would mean adding a seam to a privileged path solely for testing. The other three are covered and were mutation-checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pgrade deleteAndFinalize discarded the QuarantineUntil parse error, leaving the zero time, which is before every clock reading — so a corrupt deadline chose the quarantined teardown, the one that skips the synchronous drain. honorExistingQuarantine already refuses the same value; this reading must not be the weaker of the two. The quarantine handoff reported "the account is disabled and retained" for every failure, including the one from beginIdentityQuarantine's first step, which is DisableLogin. A distinguishable error now separates them, so the operator is not told the door is shut when it may still be open. shouldAskLang compared argv against the bare "--yes"/"-y" tokens. Go's flag package also accepts --yes=true, -y=1 and so on for the boolean flag every mutating subcommand registers, so those runs still met the first-run language prompt, which stops them. The --remove-users gate counted every entry in plan.accounts, which is a union of witnesses rather than live accounts: a stale v1 row, an orphaned sudoers drop-in, an orphaned timer and a stale v2 row all appear without a passwd entry. A host with no live account was told to pass the mass-deletion flag to authorize deletions that were not going to happen. The official-mirror upgrade path treated latest.json as the sole version selector, so an index offering something older than the installed release was indistinguishable from being current: silent, exit 0, no record. It now warns and audits, while still declining to install. main indexed os.Args[1:] unguarded; the runtime builds that slice as make([]string, argc), so an exec with argc == 0 panicked with a goroutine dump instead of printing usage. cmd/lta-release already guards the same way. Three of the six have mutation-checked tests. The uninstall gate, the mirror downgrade report and the argc guard do not: the first two need fixtures well beyond the change, and main() is not reachable from a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…giene The audit record's actor is whatever SUDO_USER says, and its uid field records geteuid(), which is 0 for every run. Neither identifies the human when the name is in question. The record now also carries /proc/self/loginuid, set once per login session by PAM, with the unset sentinel (4294967295) recorded as absent rather than as a uid. ReconcileAccountDatabaseAfterDeletion authorized groupdel on any positive GID. The account protection and the identity allocator share a floor at 1000; the group removal is just as destructive and now reads the same constant. schedule's orphan scan used os.ReadDir, which resolves the path, while the sudoers and sshd managers open every directory with O_NOFOLLOW|O_DIRECTORY. A replaced unit directory would have been enumerated as if it were ours. install.sh executes the downloaded candidate twice with stdin still attached to the pipe carrying the rest of the installer, on the documented curl | sh path. Both invocations now read /dev/null. bounded_copy computed `ulimit -f` blocks as 1024 bytes. Bash uses 512-byte blocks in POSIX or sh mode — measured on this host: 1024 under bash, 512 under `bash --posix` — so the effective cap was halved and files within the limit were rejected. It now probes /proc/self/limits in a command-substitution child, the way install.sh documents and already does; the caller's own limit is unaffected. The change was made in the canonical .inc and re-synced into the three release scripts. publish-release.sh runs chmod without listing it in its preflight, and scrubs an exhaustive list of trust-affecting variables without GNUPGHOME. atomic_write's handler closed fd after fdopen had already closed it, so a descriptor opened in between — directory_fd, immediately below — could inherit that number and be closed out from under its owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eanup helper integrationtest.removeUser runs `userdel -r -f` as root from roughly thirty fixtures — the exact flag combination internal/user documents as unsafe for the production path — and deleted whatever name it was handed. It now refuses anything outside this suite's own naming namespace, so a typo or a fixture reusing a real login cannot take that account and its home directory with it. uninstallApp built its Scheduler with only the v1 legacy prefix while production schedule.New() also sweeps QuarantineUnitPrefix, leaving the one namespace that names a disabled-but-still-present account untested by every uninstall case. expiry.LockInstant restates Date's own assumption that chage reads the date as UTC, so every test built on it agreed with Date by construction. A new test asks chage instead, against a throwaway root, under TZ=UTC, Pacific/Kiritimati (UTC+14) and Pacific/Honolulu (UTC-10). Measured: this shadow-utils stores the same sp_expire in all three, so the UTC model holds here — but a shadow that parsed the date locally would now be caught rather than assumed away. TestDropInRestoresScopeForLaterIncludedFiles could not fail: deleting the managed file's trailing `Match all` leaves its query answering "no" all the same, because OpenSSH 9.2 on this host ends Match scope at the file boundary instead of carrying it into the next file of the Include glob — the opposite of the behaviour the package doc gives as the guard's reason. The measurement is now recorded in the test, the guard is kept for older sshd, and its presence in the rendered drop-in is pinned directly so removing it fails something. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The group, gshadow and subordinate-ID parsers treated every line that was not exactly N colon-separated fields as malformed. glibc's nss_files skips blank and '#' lines, and both glibc and shadow-utils accept the '+'/'-' NIS compatibility entries, so a host carrying one hard-failed every sequential invite over a line that names no group. Writing a test for the integration suite's own group reader found the identical rule missing there too. revoke's picker printed the raw seven-column table while status, manageUsers and cleanupExpired all fall back to the numbered vertical view — it was the one picker that could garble the rows it asks the operator to choose from. The uninstall plan table listed an --ignore-foreign-markers account as live under "The uninstall will remove:", because it had no access to the options that decide what is excused. published_at was validated for shape and then used only to rebuild canonical bytes, constraining nothing about which release the mirror names. A timestamp ahead of now cannot describe a published release; staleness itself stays a judgement this code cannot make without a trusted clock, and the version lower bound added earlier covers a rolled-back index. The receiver's stable-installer scan hashed one file per published release under the deployment lock with no deadline, and rrsync ran without -munge while the mandated profile enables --links. The invite password was minted as an immutable Go string and copied into several more, none of which can be zeroed, while the key path clears everything it can. It is bytes end to end now: cleared after rendering, and the chpasswd line built and cleared in the function that owns it. probeVersion is the only privileged write-and-exec that works by name rather than through a pinned directory descriptor. Restructuring it to openat/fexecve is a larger change than this warrants; binding its two resolutions of the install directory to one inode closes the window between the safety verdict and the write without that risk, and the deviation is now recorded where it happens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Two CI failures, both mine. Narrowing the --remove-users gate to live accounts was wrong. It broke TestUninstallWithAccountsRefusesNonInteractivelyWithoutTheFlag, whose fixture carries a registry row with no passwd entry behind it and whose prose states the intent: the gate mirrors --fix-sshd, so a run nobody is watching never removes a registry row, a sudoers drop-in or a scheduled task implicitly either. The audit finding read the demand as naming deletions that will not happen; the test says the flag is about what it authorizes, not about what each host happens to hold. Reverted, with that reasoning recorded where the count is made. TestGrantRollbackReloadsWhenItReplacedALiveDropIn went into the untagged test file, so CI's non-root job ran it against a t.TempDir() owned by the runner and Grant refused the directory before reaching anything under test. It moves to the root fixtures, which skip when not root and chown the directory. Local runs were all root, which is exactly the trap the repo's own guidance warns about; the whole changed set has now been re-run as an unprivileged user the way CI does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xxvcc
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Account creation, rollback and revocation now retain identity and retry evidence until cleanup is confirmed. The change also fixes expired-account encoding, accounts for real SSH listener ports and conditional policy, preserves pending sshd removals, creates delivered key files privately without overwriting existing paths, and requires complete process credentials before declaring a UID empty.
Automatic revocation now verifies both the recorded deadline and a persistent scheduler backend. Non-systemd hosts can use their native atd service even when systemctl is installed. Helper and mirror-transfer failures terminate owned process groups before reaping their leaders, including output-limit, inherited-pipe and signal paths. Release downloads consistently enforce file-size limits in normal and POSIX Bash, and x/crypto is updated to v0.56.0.
Validation: full non-root race suite; isolated root race integration with real account tools and bind mounts; real atd enablement, cancellation and overdue dispatch after daemon restart; vet and Staticcheck with and without integration tags; shell syntax/ShellCheck, all workflow actionlint checks, receiver signal/process tests, dependency verification and vulnerability scan; static amd64/arm64 builds and version probes. A separate disposable systemd/sshd canary checks SSH PTY, passwordless sudo, container reboot, timer restoration, automatic revoke and cleanup.