fix(auth): fail closed when the bundled breach corpus is unusable (BACKLOG #1438) - #845
fix(auth): fail closed when the bundled breach corpus is unusable (BACKLOG #1438)#845wshallwshall wants to merge 5 commits into
Conversation
…CKLOG #1438) _common_passwords() returned an empty frozenset for an empty, truncated or mis-encoded corpus file, so PasswordPolicy.violations stopped emitting the "not be a common or breached password" clause and breach screening became a silent no-op. Nothing logged. There is no explicit early-out to find by reading: decode(errors= "ignore") swallows the fault and the empty set is produced by the comprehension. password_check_breached ships true, so the shipped configuration would assert a check that was not running on a first deployment. Two halves, at deliberately different levels. _common_passwords() now raises BreachCorpusUnavailable when the file cannot be read or holds fewer than ASVS_6_2_4_MIN_CORPUS_ENTRIES entries, so a password create or change is refused rather than silently accepted. Only those paths screen a password, so login, existing sessions and message flow are untouched. AuthService loads the corpus eagerly beside the existing operator-corpus warning and logs the defect at ERROR. That closes the asymmetry the tree already had: the OPERATOR corpus has a startup guard and the bundled one had none. It does not stop the engine, because HL7 flow does not depend on password screening. The floor reuses ASVS 6.2.4's 3000 against a different quantity than the build-time test pins. The test counts entries that clear the shipped policy; this counts the whole corpus. Clearing is a subset of total, so the runtime check is a necessary condition for the pinned bar and can never be stricter than it. That also means a regenerated corpus cannot trip it. Keying on the clearing subset instead would be wrong rather than merely more expensive. That count tracks min_length hard -- 5,274 entries clear at 15, 3,173 at 16, and 1,735 at 17 -- so such a guard would start refusing every password on a site that raised its minimum to 17, with a sound corpus, punishing an operator for making the policy stronger. The total is invariant to min_length. Measured: 15,256 lines parsing to 15,045 unique entries, identical under LF, CRLF and bare CR. Seven tests in a new tests/test_password_corpus_guard.py, deliberately not in tests/test_auth_core.py, which concurrent work is restructuring. Removing the guard fails four of them and leaves all three negative controls green, which is the positive control a silent-failure guard needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wshallwshall
added
the
reviewed
main moved 58 commits while this branch was held unpushed. Only docs/BACKLOG.md conflicted, as an append collision: both sides added items at the end of the file. Both sides kept, reordered so the ledger stays ascending -- main's 1424 through 1432, then this branch's 1438, then main's 1439. messagefoundry/auth/policy.py and messagefoundry/auth/service.py auto-merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hipped BACKLOG #1432 shipped in 0.3.2 while this branch was held, so two statements written against it went stale on the merge. The #1438 row said the widening sat on an unmerged branch and was claimed rather than measured. It is on main, and its four residual gaps are now verified against shipped code at this merge base: fail_closed_on_drift is False in config/settings.py, the editable-install early-out returns before any hashing, and run_startup_attestation is called once from the API lifespan. integrity.py's _ATTESTED_ASSETS comment described _common_passwords returning an empty set and screening becoming a silent no-op. This branch makes it raise, so that reading would have been wrong on merge. The comment now records that the consequence is contained AND that the entry is still load-bearing, with the case the guard cannot see: a corpus substituted rather than emptied, well-formed and ordinary-sized, minus the one password the attacker intends to use. Only a hash catches that. The converse holds too, so neither control subsumes the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
removed
the
reviewed
…-203d5c # Conflicts: # docs/BACKLOG.md
The
|
| commit | what it does |
|---|---|
e23e4015d |
merge of origin/main, 58 commits. Only docs/BACKLOG.md conflicted, an append collision; both sides kept, reordered so the ledger stays ascending. Both engine files auto-merged. |
2bbee2fee |
reconciles this row with BACKLOG #1432, which shipped in 0.3.2 while this branch was held |
6790048ca |
second merge of origin/main, same append collision with #1435. Ascending order preserved: 1432, 1435, 1438, 1439. |
Why messagefoundry/integrity.py is in this diff
#1432 shipped while this branch was held, and its _ATTESTED_ASSETS comment describes _common_passwords() returning an empty set and screening becoming a silent no-op. This PR makes it raise instead, so that comment would have been wrong on merge.
The comment now records that the consequence is contained and that the entry is still load-bearing, naming the case this guard cannot see: a corpus substituted rather than emptied -- well-formed, ordinary size, minus the one password the attacker intends to use. Only a hash catches that. The converse holds too: a wheel built with an already-truncated corpus matches its own RECORD and drifts from nothing, and there this guard is the only signal. Neither control subsumes the other.
tests/test_startup_attestation.py passes against this branch.
Verification re-run at the current head
ruff format --check, ruff check, mypy strict (267 files), both ledger gates, the tooling-partition gate, and 101 tests across the corpus guard, auth core, startup attestation and ledger suites.
The four #1432 residual gaps in the row are now measured against shipped code rather than claimed against an unreadable branch: fail_closed_on_drift is False in config/settings.py, the editable-install early-out returns before any hashing, and run_startup_attestation is called once from the API lifespan.
Still true from the PR body
No full-suite run completed; the blast radius was run instead, and its 6 failures are pre-existing and order-dependent on main -- settled by a paired run against a detached worktree at the base, identical failure names, delta of exactly this PR's 7 new tests. The two residuals worth a decision are the generic 500 and the fact that a broken corpus blocks minting the first admin account.
…CKLOG #1447) Tracing the #1438 guard's callers turned up a consequence sharper than the residual bullet that recorded it, and one sentence I had already committed was misleading. MEASURED CHAIN. api/app.py awaits auth.initialize() with no try. initialize calls _ensure_bootstrap_admin, which returns early once count_users() > 0 but on an EMPTY store calls _generate_policy_password. That generator screens its own candidate with policy.violations, which now raises when the corpus is unusable. The raise escapes on iteration one, so the generator's defensive fallback is unreachable, and it escapes the lifespan, so a FIRST run does not start at all. Later runs short-circuit at count_users and are unaffected. WHY IT IS ARGUABLY WRONG THERE AND RIGHT EVERYWHERE ELSE. The candidate is a 192-bit secrets.token_urlsafe value, so the breach clause is inert on it by construction. The guard turns a check that can never fire into one that always blocks. On every other path violations screens a human-chosen password, where failing closed is correct. The row files the chain, the four options and a recommendation. It does not decide; the fail-closed reading is a real position and the reason this wants a ruling. Also corrects the scope of three claims already committed on this branch. "Never stops message flow" and "does not stop the engine" were written about the startup reporter, which does only log, but they read as claims about the change as a whole and are false of a first run. Both now say which they cover and cite #1447. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes BACKLOG #1438.
_common_passwords()returned an emptyfrozensetfor an empty, truncated or mis-encoded corpus file.PasswordPolicy.violationsthen stopped emitting the"not be a common or breached password"clause and breach screening became a silent no-op, with nothing logged.password_check_breachedshipstrue, so the shipped configuration would assert a check that was not running on a first deployment.Severity is conditional per CLAUDE.md section 0: zero instances run, so nothing is unscreened today. The defect is in the shipped code.
There is no explicit early-out to find by reading.
decode("utf-8", "ignore")swallows the fault and the empty set is produced by the comprehension, so the fix grades the parsed result rather than any single cause.The change
Two halves, at deliberately different levels.
Contained (
auth/policy.py)._common_passwords()raisesBreachCorpusUnavailablewhen the file cannot be read or holds fewer thanASVS_6_2_4_MIN_CORPUS_ENTRIESentries, so a password create or change is refused rather than silently accepted. Only those paths screen a password, so login, existing sessions and message flow are untouched.Loud (
auth/service.py)._error_if_bundled_corpus_unusableloads the corpus eagerly inAuthService.__init__, beside the existing_warn_if_corpus_unreadable, and logs at ERROR. That closes an asymmetry the tree already had: the OPERATOR corpus has a startup guard and the bundled one had none. It does not stop the engine, because HL7 flow does not depend on password screening.Why the floor is 3000 rather than a non-empty check
It reuses ASVS 6.2.4's number against a different quantity than the build-time test pins. The test counts entries clearing the shipped policy; this counts the whole corpus. Clearing is a subset of total, so the runtime check is a necessary condition for the pinned bar and can never be stricter than it. A regenerated corpus therefore cannot trip it.
Keying on the clearing subset instead would be wrong, not merely more expensive. That count tracks
min_lengthhard:min_lengthThe margin at the shipped floor is one increment, so a clearing-keyed guard would start refusing every password on a site that raised its minimum to 17, with a sound corpus, punishing an operator for making the policy stronger. The total is invariant to
min_length; measured, it reads 15,045 at every floor above. Those counts are measured-at-this-corpus and move together if it is regenerated; the argument does not depend on their values.Tests, and the positive control
A guard against a silent failure needs a reading that would have proved it absent. Removing the guard fails 4 of the 7 and leaves all 3 negative controls green (measured twice, before and after the tests were relocated).
Seven cases in a new
tests/test_password_corpus_guard.py, deliberately not intests/test_auth_core.py, which concurrent work is restructuring. This PR's footprint in that file is zero.Checks run
Run locally:
ruff format --check,ruff check,mypystrict (267 files), both ledger gates,test_tooling_partition.py(this PR's test file imports the engine, so it needs no manifest line), and the measured blast radius of the change: all 92 test files importing the auth package.Not run: no full-suite run completed. Two sessions were running one concurrently on a contended box and neither finished in two hours. Hosted-only legs (for example NSSM under
windows-service-smoke) were never visible to this session and must be read on the PR.The 6 failures in the blast radius are NOT from this PR
The auth blast radius reports 6 failures. They are pre-existing and order-dependent on
main, and they pass in isolation, so the obvious re-check clears them either way. Settled with a paired run against a detached worktree at the base, same 91 files:46ea10a78Identical failure names. The delta of 7 is exactly this PR's new tests. The six are
test_auth_hardening(3),test_client_network_allowlist,test_hop_refusal_329andtest_trust_anchors(1 each), all log-assertion tests. Worth its own item; not filed here.Residuals, named rather than hidden
app.py's catch-all logs the exception type and returns{"detail": "internal error"}. Honest for a server fault, but 503 with a specific message would be better; that means touching the API layer and both/uitwins._generate_policy_passwordscreens its own output. Fail-closed and recoverable by repairing the file, but it blocks minting the first admin account. This one may want an owner ruling.lru_cachedoes not cache exceptions, so a broken install re-reads the file per attempt and recovers without a restart. A deliberate trade.scripts/.Not done here
No
reviewedlabel applied. A relayed instruction said the reviewer step is suspended and self-labelling is allowed; I have not verified that with the owner, and CLAUDE.md is explicit that labelling your own PR unread satisfies the machine and defeats the point. Auto-merge is not armed.Note for whoever lands this:
mainhas not moved for several hours and the merge queue evicts entries at 60 to 65 minutes. Expect this to sit.🤖 Generated with Claude Code