From 4795c6a19942d825c8b25235dff3fe19a673d1d7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 3 Sep 2026 18:50:52 -0500 Subject: [PATCH 1/3] fix(auth): fail closed when the bundled breach corpus is unusable (BACKLOG #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 --- docs/BACKLOG.md | 91 +++++++++++++++++++ messagefoundry/auth/policy.py | 86 +++++++++++++++++- messagefoundry/auth/service.py | 37 +++++++- tests/test_password_corpus_guard.py | 136 ++++++++++++++++++++++++++++ 4 files changed, 344 insertions(+), 6 deletions(-) create mode 100644 tests/test_password_corpus_guard.py diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index a19e6b606..7ad52d574 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -19940,3 +19940,94 @@ That is the same `self._lock` the staged-pipeline handoffs take. On a first depl **PARTLY CLOSED ALREADY, AND THE CLOSURE SITS IN THE WRONG ARTIFACT.** The full record -- both questions, all eight options, both answers quoted -- is [comment 5515263760 on PR 749](https://github.com/MEFORORG/MessageFoundry/pull/749#issuecomment-5515263760), written 2026-09-02. A pull-request comment is a real improvement on a session transcript, which does not survive its session. It is still not the ADR, and the ADR is what a reader consults. **This limb differs from the first two in shape:** closing it needs no decision about the engine, only the record moved into the artifact people actually read. **THE GENERAL PROBLEM, stated once so it is not re-derived per incident.** A decision recorded as an outcome plus a delegation is not reviewable. The inputs -- the question, the options, the answer -- are what let a later reader tell a considered call from an arbitrary one, and they are exactly the part that lives in the least durable place. + +## 1438. Fail closed when the bundled breach corpus is unusable, instead of silently disabling password screening + +> 🚧 **Filed 2026-09-03. Implemented and committed on a feature branch, not merged.** `_common_passwords()` read `messagefoundry/auth/data/common_passwords.txt` and returned a `frozenset` of its lines. An empty or truncated file produced an **empty set**, `PasswordPolicy.violations` stopped emitting the `"not be a common or breached password"` clause, and breach screening became a no-op. Nothing logged. `password_check_breached` ships `true`, so the shipped configuration would assert a check that was not running. +> +> **Severity is conditional (sec. 0).** Zero instances run, so nothing is unscreened today. The defect is in the shipped code: a **first** deployment whose corpus file had been truncated, emptied or replaced would accept every weak password that cleared the length clause, and no log line, metric or audit row would say so. + +**Cluster:** Auth / password policy (ASVS 6.2.4). **Priority:** P2. **Verdict:** fix, shipped on the branch above. + +### The failure was silent in both directions, which is what made it worth a row + +A screen that rejects nothing looks exactly like a screen that found nothing to reject. `violations` returned `[]` either way, and `[]` is the success value. There was no runtime assertion anywhere: `tests/test_auth_core.py::test_breach_corpus_meets_the_asvs_6_2_4_policy_matching_bar` pins the corpus at **build** time (BACKLOG #1134) and nothing checked it again after the wheel was installed. + +The decode is where the silence enters. `data.decode("utf-8", "ignore")` swallows a corrupt, truncated or mis-encoded file and yields a short-or-empty set with no exception, so there is no explicit early-out to find by reading: the empty set is **produced by the comprehension**, not returned by a guard clause. The fix therefore grades the parsed **result**, which covers every cause at once. + +The bundled corpus is the only screen underneath. `password_breach_corpus_file` is optional and unset by default, so on the shipped configuration the bundled file is the whole check. + +### What #1432 covers, and the four reasons it does not close this + +#1432 is the detection half. `messagefoundry/integrity.py` on `main` hashes loaded first-party `.py` files against the wheel's `dist-info/RECORD`; #1432 widens that ADR 0041 D3 startup tripwire to cover shipped security **data** assets, including this corpus. That work sits on its own unmerged branch and is **not on `main`**, so the widening is claimed rather than measured here. Four gaps stand either way, recorded in #1432's own residual: + +1. The tripwire is **alert-only by default**. `[integrity].fail_closed_on_drift` is opt-in, so on the shipped configuration the alert fires and screening still does nothing for the life of the process. +2. It is a **no-op on editable installs and source-tree runs**, which is every developer checkout. +3. It runs **once at startup**, while `_common_passwords()` is `lru_cache`d and read lazily on the first password check. There is a window between the two. +4. It is **detection, not containment**. It never stops a weak password being accepted. + +Detection and containment are different jobs, and this row is the containment one. **There is a case each one misses, which is the argument for having both.** A wheel BUILT with an already-truncated corpus passes attestation: `RECORD` carries the hash of whatever shipped, so a present-but-truncated file matches its own baseline and drifts from nothing. The tripwire is correct to stay silent there, and this guard still fires because it grades the parsed result rather than the bytes. The inverse also holds. Post-install tampering drifts the hash while possibly leaving enough entries to clear this floor, and there the alert is the only signal. + +### The fix + +Two halves, deliberately at different levels. + +**Contained, in `messagefoundry/auth/policy.py`.** `_common_passwords()` now raises `BreachCorpusUnavailable` when the file cannot be read, or when it holds fewer than `ASVS_6_2_4_MIN_CORPUS_ENTRIES` entries. `PasswordPolicy.violations` therefore refuses a password rather than accepting every password. Only the password *create* and *change* paths screen a password, so this never touches login, never invalidates a session, and never stops message flow. + +**Loud, in `messagefoundry/auth/service.py`.** `_error_if_bundled_corpus_unusable` loads the corpus eagerly in `AuthService.__init__`, beside the existing `_warn_if_corpus_unreadable`, and logs the defect at `ERROR`. That closes gap 3 above: the operator learns at boot instead of at somebody's first password change. It does **not** stop the engine. HL7 flow does not depend on password screening, and bricking a message engine over an auth data asset trades a contained failure for an outage. + +The two halves sit at different levels on purpose. The operator corpus degrades to a `WARNING` because the bundled list still screens underneath it. Nothing screens underneath the bundled list. + +### Why the floor is 3000 and not a non-empty check + +`ASVS_6_2_4_MIN_CORPUS_ENTRIES = 3000` is the ASVS 6.2.4 bar, re-used against a **different quantity** than the build-time test measures, and that difference is the reason it is safe. + +The build-time test counts entries that **clear the shipped policy**. The runtime guard counts the **whole corpus**. The clearing subset is a subset of the corpus, so a corpus holding fewer than 3000 entries in total cannot hold 3000 that clear the policy. The runtime check is therefore a **necessary condition** for the pinned bar, never a stricter one, and it cannot fail an install the build-time test would have passed. + +It costs one `len()` on an already-cached `frozenset`. A bare non-empty check would have caught truncation to zero and missed partial corruption. + +**Keying the guard on the clearing subset instead would be wrong, not merely more expensive, and this is the load-bearing reason for the choice.** The clearing count tracks `min_length` hard. Measured on the shipped corpus, independently by two sessions: + +| `min_length` | entries clearing the policy | against the 3000 bar | +|---|---|---| +| 8 | 9,135 | over | +| 10 | 6,020 | over | +| 12 | 5,367 | over | +| 15 (shipped) | 5,274 | over | +| 16 | 3,173 | over, by 173 | +| 17 | 1,735 | **under** | +| 20 | 512 | **under** | + +The margin at the shipped floor is one increment. A runtime guard keyed to clearing would start refusing every password on a site that raised its minimum to 17, with a corpus that is unchanged and sound, punishing an operator for making the policy stronger. The total is invariant to `min_length`; the clearing subset is not. Measured here: the total reads 15,045 at every floor in that table. + +**Read the table as measured-at-this-corpus, not as constants.** Every number in it moves together if the corpus is regenerated, and a regeneration script is in flight on another branch. The argument does not depend on the values: it needs only that the clearing subset shrinks with `min_length` while the total does not, which holds for any corpus. The invariance arm was measured; the rest of the table was produced in one worktree and reproduced in another before being written down. + +Measured 2026-09-03 on this branch: the shipped corpus is **15,256 lines** and parses to **15,045 unique entries** (the difference is case-duplicates, which lower-casing folds), **5,274** of which clear the shipped policy. The guard counts the parsed set, not the lines. Headroom over the floor is about 5x, so ordinary corpus curation cannot trip it. + +### An operator corpus does not excuse a broken bundled one + +Python's `or` evaluates `_common_passwords()` first, so the bundled floor is checked before any operator corpus is consulted. That is deliberate and it is the one judgment call in this row a reviewer should push on. An operator corpus is additive under 6.2.12 and has **no floor of its own**, and a bundled file that has been truncated or replaced is evidence about the **install**, not about the screen. The repair is to reinstall the wheel. + +### The tests, and the positive control + +A guard against a silent failure needs a reading that would have proved it absent, or it is untestable by construction. Seven cases land in a new `tests/test_password_corpus_guard.py` rather than in `tests/test_auth_core.py`, which concurrent work is restructuring; the build-time bar stays where it is and this file holds the runtime guard the same number implies. Measured on this branch by removing the guard and re-running: **four fail, and the two negative-control arms still pass.** + +| arm | what it pins | fails with the guard removed | +|---|---|---| +| truncated corpus refuses | the positive control: a five-entry corpus, and a passphrase the existing tests assert is ACCEPTED against the real corpus | yes | +| empty corpus refuses | truncation to zero bytes, the cheapest way to disable the screen | yes | +| missing corpus refuses | a distinct cause and a distinct message: the read failed rather than returning too little | yes | +| startup reports it at ERROR | the loud half | yes | +| shipped corpus clears the floor | keeps the three above honest: a guard wired to fire unconditionally would pass all of them | no, by design | +| `check_breached=False` stays silent | the deliberate opt-out still works, and the guard did not become an import-time assertion | no, by design | +| startup silent when screening is off | a corpus nobody consults is not a defect | no, by design | + +### Residual + +- **The API answer is a generic 500.** `app.py`'s catch-all handler logs the exception type and returns `{"detail": "internal error"}`. `BreachCorpusUnavailable` is a server fault, not a client one, so 500 is honest, but 503 with a specific message would be better. Mapping it means touching the API layer and both `/ui` twins, which is wider than this row. +- **First-run bootstrap and admin password reset both raise.** `_generate_policy_password` screens its own output, so a broken corpus fails those too. That is fail-closed and correct, and both are recoverable by repairing the file, but it means a broken corpus blocks minting the first admin account. Worth an owner ruling if anyone disagrees. +- **`lru_cache` does not cache exceptions**, so a broken install re-reads the file on each password attempt. That is one file read on an install already refusing them, and it means a repaired file recovers without a restart. Recorded as a deliberate trade, not an oversight. +- **A regenerated corpus cannot trip the floor.** Concurrent work adds a build script that rewrites this file. The floor is 3000 TOTAL entries while the pre-existing build-time test pins 3000 POLICY-CLEARING entries, and clearing is a subset of total, so any corpus passing that test passes this guard. A corpus that did fall under 3000 total could not satisfy ASVS 6.2.4 on its own terms either. +- **The floor is line-ending independent, so a `.gitattributes` pin cannot affect it.** Measured: the loader's `splitlines()` plus `strip()` return **15,045 entries under LF, CRLF and bare CR alike**, against a positive control (a five-line corpus) that correctly reports 5. Byte-level line-ending policy is a **wheel reproducibility** question, not an attestation one, and it changes nothing this guard reads. The startup attestation cannot be split by it on any host: [`integrity.py`](../messagefoundry/integrity.py) takes its baseline from `dist.read_text("RECORD")` and hashes the file in the same install root, and pip writes that RECORD from the bytes it unpacked, so baseline and file come from one install and move together. What the byte policy really costs is that two builds of one commit on different platforms produce different artifacts. An earlier draft of this row said the attestation was affected; that was wrong, and four sessions held that version before anyone read `integrity.py`. +- **The number 3000 now has three independent homes** and no shared constant can join them: `ASVS_6_2_4_MIN_CORPUS_ENTRIES` here, the literal in the build-time test, and a third in the corpus-regeneration script under `scripts/`, which runtime code in `messagefoundry/auth/` must not import. Consolidating them is somebody's later cleanup, not a coupling worth inventing now. +- **#1134's own banner is stale**, unrelated to this fix and not touched here. Its scoring note says the bundled corpus "supplies far fewer than 3000 policy-matching entries", which the 5,274 measured above contradicts. Its pinning test exists and passes. Someone should re-score it. diff --git a/messagefoundry/auth/policy.py b/messagefoundry/auth/policy.py index 89b09dc3f..0ee2cc020 100644 --- a/messagefoundry/auth/policy.py +++ b/messagefoundry/auth/policy.py @@ -13,6 +13,10 @@ Operators can widen it with an offline ``breach_corpus_file`` (6.2.12) — a plaintext list **or** an HIBP-style SHA-1-hash export (``HASH[:count]`` lines, auto-detected), still fully offline. (True HIBP k-anonymity needs a live range query, which this on-prem engine deliberately doesn't make.) +The bundled corpus is **load-bearing, so its loss is loud**: an unreadable, empty or truncated file +raises :class:`BreachCorpusUnavailable` out of :meth:`PasswordPolicy.violations` rather than loading as +an empty set that silently stops screening while ``check_breached`` still reports ``True`` +(BACKLOG #1438). ``AuthService`` loads it once at startup and logs the same defect as an error. """ from __future__ import annotations @@ -53,12 +57,73 @@ ) +#: ASVS 6.2.4 asks for at least the top 3000 breached passwords **that clear the application's own +#: policy**, and ``tests/test_auth_core.py::test_breach_corpus_meets_the_asvs_6_2_4_policy_matching_bar`` +#: pins that policy-clearing subset at build time (BACKLOG #1134). +#: +#: The runtime guard below re-uses the same number against a DIFFERENT quantity -- the whole corpus, +#: not the clearing subset -- because that is a **necessary condition** for the pinned bar rather than +#: a restatement of it: the clearing subset is a subset of the corpus, so a corpus holding fewer than +#: 3000 entries in total cannot possibly hold 3000 that clear the policy. Deliberately the weaker of +#: the two checks. It costs one ``len()`` on an already-cached frozenset and can never be stricter +#: than the build-time test, so it cannot fail an install the test would have passed. +#: +#: **Keying on the clearing subset instead would be WRONG, not merely more expensive**, and this is +#: the load-bearing reason for the choice. The clearing count tracks ``min_length`` hard -- measured +#: on the shipped corpus: 5,274 entries clear at 15, 3,173 at 16, and 1,735 at 17. So a runtime guard +#: keyed to clearing would start refusing every password on a site that had just raised its minimum +#: to 17, with a sound corpus, punishing the operator for making the policy STRONGER. The total is +#: invariant to ``min_length``; the clearing subset is not. +#: +#: Those three counts are measured against THE CORPUS AS IT SHIPS TODAY and move together if it is +#: ever regenerated -- read them as an illustration of the shape, not as constants. The ARGUMENT does +#: not depend on them: it needs only that the clearing subset shrinks with ``min_length`` while the +#: total does not, which is true of any corpus. +ASVS_6_2_4_MIN_CORPUS_ENTRIES = 3000 + + +class BreachCorpusUnavailable(RuntimeError): + """The **bundled** common/breached-password corpus is missing, empty, or truncated. + + Raised from :meth:`PasswordPolicy.violations` when ``check_breached`` is on, so a screen that + cannot run **refuses** the password instead of accepting every password. ``check_breached`` ships + ``True``; before this, an empty or truncated file loaded as an empty ``frozenset``, the + "not be a common or breached password" clause stopped being emitted, and nothing logged -- the + shipped configuration would assert a check that was not happening on a first deployment. + + Blast radius is narrow by construction: only the password *create* and *change* paths screen a + password, so this never touches login, never invalidates a session, and never stops message flow. + ``AuthService`` also loads the corpus eagerly at startup and logs the same defect as an error, so + an operator learns about it from the log rather than from a user's failed password change. + """ + + @functools.lru_cache(maxsize=1) def _common_passwords() -> frozenset[str]: - """The bundled offline common/breached-password set (lower-cased), loaded once and cached.""" - data = (files("messagefoundry.auth") / "data" / "common_passwords.txt").read_bytes() - text = data.decode("utf-8", "ignore") - return frozenset(line.strip().lower() for line in text.splitlines() if line.strip()) + """The bundled offline common/breached-password set (lower-cased), loaded once and cached. + + Raises :class:`BreachCorpusUnavailable` when the file cannot be read or holds fewer than + ``ASVS_6_2_4_MIN_CORPUS_ENTRIES`` entries, rather than returning the empty set that silently + disabled screening. ``lru_cache`` does not cache exceptions, so a broken install re-reads the file + on each attempt and recovers the moment the file is repaired -- one file read per password attempt + on an install that is already refusing them, which is the cheaper side of the trade. + """ + resource = files("messagefoundry.auth") / "data" / "common_passwords.txt" + try: + data = resource.read_bytes() + except OSError as exc: + raise BreachCorpusUnavailable( + f"the bundled breach corpus at {resource} could not be read: {exc}" + ) from exc + entries = frozenset( + line.strip().lower() for line in data.decode("utf-8", "ignore").splitlines() if line.strip() + ) + if len(entries) < ASVS_6_2_4_MIN_CORPUS_ENTRIES: + raise BreachCorpusUnavailable( + f"the bundled breach corpus at {resource} holds {len(entries)} entries, below the floor " + f"of {ASVS_6_2_4_MIN_CORPUS_ENTRIES} (ASVS 6.2.4) -- breach screening cannot run" + ) + return entries @functools.lru_cache(maxsize=4) @@ -107,7 +172,11 @@ def violations(self, password: str, *, username: str | None = None) -> list[str] acceptable. Order: length → opt-in character classes → breach → username → context. ``username`` enables the 6.2.11 own-username check (omit it where there is no user context, - e.g. generating the bootstrap password).""" + e.g. generating the bootstrap password). + + Raises :class:`BreachCorpusUnavailable` when ``check_breached`` is on and the bundled corpus is + unusable (BACKLOG #1438) -- a refusal, not a silent pass. Callers get a list or an exception, + never a list that quietly stopped screening.""" problems: list[str] = [] if len(password) < self.min_length: problems.append(f"be at least {self.min_length} characters") @@ -120,6 +189,13 @@ def violations(self, password: str, *, username: str | None = None) -> list[str] if self.require_symbol and all(c.isalnum() for c in password): problems.append("contain a symbol") lowered = password.lower() + # `_common_passwords()` RAISES on an unusable bundled corpus (BACKLOG #1438), and Python's `or` + # evaluates it first, so the bundled floor is checked before any operator corpus is consulted. + # That is deliberate: an operator corpus is ADDITIVE (6.2.12), it has no floor of its own, and a + # bundled file that has been truncated or replaced is evidence about the INSTALL, not about the + # screen. A large operator export therefore does not excuse it -- the repair is to reinstall the + # wheel, which is cheap, and the alternative is honouring `check_breached=True` with a screen + # nobody has validated. if self.check_breached and ( lowered in _common_passwords() or self._in_operator_corpus(password) ): diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 6b284d856..744951d86 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -56,7 +56,12 @@ is_custom_role_id, validate_custom_role_permissions, ) -from messagefoundry.auth.policy import PasswordPolicy, _operator_corpus +from messagefoundry.auth.policy import ( + BreachCorpusUnavailable, + PasswordPolicy, + _common_passwords, + _operator_corpus, +) from messagefoundry.auth.ratelimit import SlidingWindowRateLimiter from messagefoundry.auth.tokens import hash_bytes, hash_token, mint_token from messagefoundry.config.models import SignatureAlgorithm @@ -95,6 +100,35 @@ def _warn_if_corpus_unreadable(path: str | None) -> None: ) +def _error_if_bundled_corpus_unusable(check_breached: bool) -> None: + """Eagerly load (and cache) the BUNDLED breach corpus at startup, so a truncated or missing file + surfaces in the log at boot rather than as a 500 on somebody's first password change (BACKLOG + #1438). The twin of ``_warn_if_corpus_unreadable`` above, at a higher level for a reason. + + The OPERATOR corpus degrades to a warning because it is optional and the bundled list still screens + underneath it. Nothing screens underneath the BUNDLED list, so its loss is an ERROR: ``check_breached`` + ships ``True``, and a corpus that cannot load means the shipped configuration asserts a check that is + not running. ``PasswordPolicy.violations`` refuses passwords in that state; this is only the loud + half. It deliberately does NOT stop the engine -- HL7 flow does not depend on password screening, and + bricking a message engine over an auth data asset would trade a contained failure for an outage. + + Skipped when the operator has turned screening off: a corpus nobody consults is not a defect. + """ + if not check_breached: + return + try: + entries = _common_passwords() + except BreachCorpusUnavailable as exc: + _log.error( + "%s; local password creation and change will be REFUSED until it is repaired " + "(ASVS 6.2.4). Reinstall the messagefoundry wheel, or set [auth].password_check_breached " + "= false to accept unscreened passwords deliberately", + exc, + ) + return + _log.debug("loaded the bundled breach corpus (%d entries)", len(entries)) + + #: A fixed argon2 hash used to equalize login timing for unknown/disabled accounts (anti-enumeration). _DUMMY_PASSWORD_HASH = hash_password("mf-login-timing-equalizer") @@ -288,6 +322,7 @@ def __init__( lockout_minutes=settings.lockout_minutes, ) _warn_if_corpus_unreadable(settings.password_breach_corpus_file) + _error_if_bundled_corpus_unusable(settings.password_check_breached) if ldap is not None: self._ldap: LdapAuthenticator | None = ldap elif settings.ad_enabled: diff --git a/tests/test_password_corpus_guard.py b/tests/test_password_corpus_guard.py new file mode 100644 index 000000000..d48e0796c --- /dev/null +++ b/tests/test_password_corpus_guard.py @@ -0,0 +1,136 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""BACKLOG #1438 (ASVS 6.2.4): the BUNDLED breach corpus must fail closed, not screen nothing. + +Its own module rather than a section of `test_auth_core.py`, for two reasons. The subject is one +mechanism with one fixture, and `test_auth_core.py` is contested by concurrent work restructuring the +per-number corpus gates that live there. `test_auth_core.py` keeps the BUILD-time bar (BACKLOG #1134: +at least 3000 corpus entries clear the shipped policy); this file holds the RUNTIME guard that the +same number implies. Read them together. + +The shape being defended is the one this project keeps rediscovering: **an empty result and a good +result rendering identically.** `violations` returned `[]` whether the corpus screened the password or +had silently failed to load, and `[]` is the success value, so nothing anywhere could report the +difference. Every arm below therefore has a stated failure reading -- see the table in the #1438 row. +""" + +from __future__ import annotations + +import logging +from collections.abc import Callable, Iterator, Sequence +from pathlib import Path + +import pytest + +from messagefoundry.auth import PasswordPolicy +from messagefoundry.auth import policy as policy_module +from messagefoundry.auth.policy import ASVS_6_2_4_MIN_CORPUS_ENTRIES, BreachCorpusUnavailable + + +@pytest.fixture +def bundled_corpus( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> Iterator[Callable[[Sequence[str] | None], None]]: + """Point the loader at a stand-in bundled corpus holding exactly the entries you pass. + + `policy_module.files` is the module-level name the loader calls, and a `pathlib.Path` already + satisfies the `/` + `read_bytes()` shape of an `importlib.resources` Traversable, so no fake class + is needed. The `lru_cache` is cleared on BOTH sides: a truncated corpus left cached would disarm + every later test in the session, which is the same silent-no-op failure this section exists over. + """ + + def use(entries: Sequence[str] | None) -> None: + """A sequence writes exactly those entries; `None` leaves the file absent entirely.""" + root = tmp_path / "pkg" + (root / "data").mkdir(parents=True, exist_ok=True) + if entries is not None: + (root / "data" / "common_passwords.txt").write_text( + "\n".join(entries), encoding="utf-8" + ) + monkeypatch.setattr(policy_module, "files", lambda _package: root) + policy_module._common_passwords.cache_clear() + + yield use + policy_module._common_passwords.cache_clear() + + +def test_a_truncated_bundled_corpus_refuses_a_password_instead_of_accepting_it( + bundled_corpus: Callable[[Sequence[str] | None], None], +) -> None: + """THE POSITIVE CONTROL for #1438: delete the guard in `_common_passwords` and this test fails. + + `correct-horse-battery-staple-xyz` is asserted ACCEPTED against the real corpus by + `test_breach_corpus_growth_did_not_over_block_or_regress` above, so it clears every other clause + and isolates the breach clause. Against a five-entry corpus the pre-#1438 loader returned an empty + `frozenset`, `violations` returned `[]`, and that is byte-identical to a password which really was + screened. An exception is the only outcome a test can tell apart from that silence. + """ + bundled_corpus(["123456", "password", "qwerty", "letmein", "dragon"]) + with pytest.raises(BreachCorpusUnavailable, match="below the floor"): + PasswordPolicy().violations("correct-horse-battery-staple-xyz") + + +def test_an_empty_bundled_corpus_refuses( + bundled_corpus: Callable[[Sequence[str] | None], None], +) -> None: + """Truncation to zero bytes is the cheapest way to disable the screen, so it gets its own arm.""" + bundled_corpus([]) + with pytest.raises(BreachCorpusUnavailable, match="0 entries"): + PasswordPolicy().violations("correct-horse-battery-staple-xyz") + + +def test_a_missing_bundled_corpus_refuses( + bundled_corpus: Callable[[Sequence[str] | None], None], +) -> None: + """Deleting the file must not read as `nothing is breached` either. A distinct cause from the two + arms above, and a distinct message: the read failed rather than returning too little.""" + bundled_corpus(None) + with pytest.raises(BreachCorpusUnavailable, match="could not be read"): + PasswordPolicy().violations("correct-horse-battery-staple-xyz") + + +def test_the_shipped_bundled_corpus_clears_the_runtime_floor() -> None: + """The arm that keeps the three above honest. + + A guard wired to fire unconditionally would pass every one of them, so the shipped corpus has to + be asserted through the SAME call path and come back clean. Measured at the time of writing: + 15,045 entries against a floor of 3,000. + """ + assert len(policy_module._common_passwords()) >= ASVS_6_2_4_MIN_CORPUS_ENTRIES + assert PasswordPolicy().violations("correct-horse-battery-staple-xyz") == [] + + +def test_the_guard_stays_out_of_the_way_when_screening_is_turned_off( + bundled_corpus: Callable[[Sequence[str] | None], None], +) -> None: + """`check_breached=False` is a deliberate operator choice, so a corpus nobody consults is not a + defect. This also pins that the guard did not become an unconditional import-time assertion.""" + bundled_corpus([]) + assert PasswordPolicy(check_breached=False).violations("correct-horse-battery-staple-xyz") == [] + + +def test_startup_reports_an_unusable_bundled_corpus_as_an_error( + bundled_corpus: Callable[[Sequence[str] | None], None], caplog: pytest.LogCaptureFixture +) -> None: + """The loud half. Without this the operator meets the defect as a 500 on a password change, which + is a window the eager load closes (the loader is `lru_cache`d and otherwise read lazily).""" + from messagefoundry.auth.service import _error_if_bundled_corpus_unusable + + bundled_corpus([]) + with caplog.at_level(logging.ERROR, logger="messagefoundry.auth.service"): + _error_if_bundled_corpus_unusable(True) + assert [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR], ( + "an unusable bundled corpus logged nothing at startup" + ) + assert "REFUSED" in caplog.records[0].getMessage() + + +def test_startup_is_silent_when_screening_is_turned_off( + bundled_corpus: Callable[[Sequence[str] | None], None], caplog: pytest.LogCaptureFixture +) -> None: + from messagefoundry.auth.service import _error_if_bundled_corpus_unusable + + bundled_corpus([]) + with caplog.at_level(logging.ERROR, logger="messagefoundry.auth.service"): + _error_if_bundled_corpus_unusable(False) + assert caplog.records == [] From 2bbee2fee621184001966813f9b4fc1666a45d86 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 4 Sep 2026 17:35:31 -0500 Subject: [PATCH 2/3] docs(auth): reconcile #1438 with #1432 now that the attestation has shipped 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 --- docs/BACKLOG.md | 4 +++- messagefoundry/integrity.py | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 9c1e631c9..08d56031e 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -21956,13 +21956,15 @@ The bundled corpus is the only screen underneath. `password_breach_corpus_file` ### What #1432 covers, and the four reasons it does not close this -#1432 is the detection half. `messagefoundry/integrity.py` on `main` hashes loaded first-party `.py` files against the wheel's `dist-info/RECORD`; #1432 widens that ADR 0041 D3 startup tripwire to cover shipped security **data** assets, including this corpus. That work sits on its own unmerged branch and is **not on `main`**, so the widening is claimed rather than measured here. Four gaps stand either way, recorded in #1432's own residual: +#1432 is the detection half, and it **shipped in 0.3.2** while this row was held on a branch. [`integrity.py`](../messagefoundry/integrity.py) now attests an explicit `_ATTESTED_ASSETS` set alongside the loaded engine modules, and `auth/data/common_passwords.txt` is in it. Its rationale comment names this row's exact defect. Four gaps stand anyway, and they are now **measured against the shipped code** rather than claimed against a branch nobody here could read: 1. The tripwire is **alert-only by default**. `[integrity].fail_closed_on_drift` is opt-in, so on the shipped configuration the alert fires and screening still does nothing for the life of the process. 2. It is a **no-op on editable installs and source-tree runs**, which is every developer checkout. 3. It runs **once at startup**, while `_common_passwords()` is `lru_cache`d and read lazily on the first password check. There is a window between the two. 4. It is **detection, not containment**. It never stops a weak password being accepted. +Verified 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. + Detection and containment are different jobs, and this row is the containment one. **There is a case each one misses, which is the argument for having both.** A wheel BUILT with an already-truncated corpus passes attestation: `RECORD` carries the hash of whatever shipped, so a present-but-truncated file matches its own baseline and drifts from nothing. The tripwire is correct to stay silent there, and this guard still fires because it grades the parsed result rather than the bytes. The inverse also holds. Post-install tampering drifts the hash while possibly leaving enough entries to clear this floor, and there the alert is the only signal. ### The fix diff --git a/messagefoundry/integrity.py b/messagefoundry/integrity.py index b1b106051..10689600e 100644 --- a/messagefoundry/integrity.py +++ b/messagefoundry/integrity.py @@ -72,12 +72,21 @@ #: #: Attesting ``.py`` alone left a gap the module walk cannot see: an admin with venv-write plus #: restart rights could truncate ``auth/data/common_passwords.txt`` to zero bytes, and -#: :func:`~messagefoundry.auth.policy._common_passwords` would then return an empty set, so -#: ``PasswordPolicy.violations`` stops emitting "not be a common or breached password" and breach -#: screening becomes a silent no-op. No engine ``.py`` changed, so attestation reported clean. Same +#: :func:`~messagefoundry.auth.policy._common_passwords` returned an empty set, so +#: ``PasswordPolicy.violations`` stopped emitting "not be a common or breached password" and breach +#: screening became a silent no-op. No engine ``.py`` changed, so attestation reported clean. Same #: shape for ``security/semgrep/handler-security.yml``: emptying the rules file makes the operator's #: opt-in CI leg pass everything. #: +#: **That consequence is now contained, and this entry is still load-bearing** (BACKLOG #1438). The +#: loader raises rather than returning an empty set, so the truncation above is refused rather than +#: silently accepted. What the guard cannot see is a corpus **substituted rather than emptied** -- a +#: well-formed replacement of ordinary size, minus the one password the attacker intends to use -- +#: because it grades the parsed result and that result looks entirely healthy. Only a hash catches +#: that one. The two are complementary in both directions: a wheel BUILT with a truncated corpus +#: matches its own ``RECORD`` and drifts from nothing, and there the loader's guard is the only +#: signal. +#: #: Version skew is impossible by construction: this list and the ``RECORD`` it is compared against #: ship in the *same* wheel, so a declared asset always has a baseline row. #: From d630398878dc9f8cd1857c6ac614c63118c388d9 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 4 Sep 2026 18:35:32 -0500 Subject: [PATCH 3/3] docs(auth): file the first-run startup failure this guard creates (BACKLOG #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 --- docs/BACKLOG.md | 49 ++++++++++++++++++++++++++++++++-- messagefoundry/auth/policy.py | 11 ++++++-- messagefoundry/auth/service.py | 7 +++-- 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index fed637679..efd6e43b4 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -22030,9 +22030,9 @@ Detection and containment are different jobs, and this row is the containment on Two halves, deliberately at different levels. -**Contained, in `messagefoundry/auth/policy.py`.** `_common_passwords()` now raises `BreachCorpusUnavailable` when the file cannot be read, or when it holds fewer than `ASVS_6_2_4_MIN_CORPUS_ENTRIES` entries. `PasswordPolicy.violations` therefore refuses a password rather than accepting every password. Only the password *create* and *change* paths screen a password, so this never touches login, never invalidates a session, and never stops message flow. +**Contained, in `messagefoundry/auth/policy.py`.** `_common_passwords()` now raises `BreachCorpusUnavailable` when the file cannot be read, or when it holds fewer than `ASVS_6_2_4_MIN_CORPUS_ENTRIES` entries. `PasswordPolicy.violations` therefore refuses a password rather than accepting every password. On an ESTABLISHED instance only the password *create* and *change* paths screen a password, so this never touches login, never invalidates a session, and never stops message flow. A **first** run is the exception and is not narrow: see BACKLOG #1447. -**Loud, in `messagefoundry/auth/service.py`.** `_error_if_bundled_corpus_unusable` loads the corpus eagerly in `AuthService.__init__`, beside the existing `_warn_if_corpus_unreadable`, and logs the defect at `ERROR`. That closes gap 3 above: the operator learns at boot instead of at somebody's first password change. It does **not** stop the engine. HL7 flow does not depend on password screening, and bricking a message engine over an auth data asset trades a contained failure for an outage. +**Loud, in `messagefoundry/auth/service.py`.** `_error_if_bundled_corpus_unusable` loads the corpus eagerly in `AuthService.__init__`, beside the existing `_warn_if_corpus_unreadable`, and logs the defect at `ERROR`. That closes gap 3 above: the operator learns at boot instead of at somebody's first password change. That reporter does **not** stop the engine. HL7 flow does not depend on password screening, and bricking a message engine over an auth data asset trades a contained failure for an outage. **Read that as scoped to the reporter, not to this change as a whole.** On a first run the bootstrap generator screens its own candidate, so the raise from `violations` escapes an unguarded lifespan call and startup fails. BACKLOG #1447 carries that, with the chain measured and the options weighed. The two halves sit at different levels on purpose. The operator corpus degrades to a `WARNING` because the bundled list still screens underneath it. Nothing screens underneath the bundled list. @@ -22158,3 +22158,48 @@ So `test_the_script_prefers_its_own_repo_over_an_earlier_path_entry` supplies th **Verification:** 8 passed in `tests/test_webconsole_seam_snapshot.py`. Mutation check run rather than argued -- with the `sys.path.insert` line deleted, the decoy test reds naming the decoy import, and the by-path digest test **stays green**, which is the luck described above measured rather than predicted. Anchor restored, 8 passed again. **Adjacent and NOT fixed here, named rather than numbered.** `docs/WEBCONSOLE-PACKAGE.md`'s seam-refresh procedure is stale in three steps left behind by #1220: it says to bump `ENGINE_UI_SEAM` by hand (`1` to `2`) when the value is a derived digest, it says to update curated lists in this script that #1220 retired, and its step 5 prescribes `python scripts/webconsole_seam_snapshot.py > tests/golden/...`, the shell redirect this script's own docstring forbids because PowerShell's `>` writes UTF-16LE with a BOM into a file the test reads as UTF-8. That is doc drift with its own cause and it wants its own item; folding a documentation rewrite into a `sys.path` fix would make both harder to review. + +## 1447. an unusable breach corpus blocks minting the first admin account, because the bootstrap generator screens its own random token + +> 🔢 **Filed 2026-09-04. The heading understates it, and this line is the finding: on a FIRST run an unusable bundled corpus does not merely block the admin account, it stops the engine starting at all.** `AuthService.initialize` is awaited unguarded in the API lifespan, and on an empty store it mints the bootstrap admin through a generator that screens its own candidate. With BACKLOG #1438's guard in place that screen raises, the exception escapes the lifespan, and startup fails. On any later run `count_users() > 0` short-circuits the whole path and the engine starts normally. +> +> **Severity is conditional (sec. 0).** Zero instances run. Written as what a first deployment would meet: an operator whose wheel shipped or acquired a truncated corpus would get a process that refuses to come up, with the cause in the log from #1438's startup reporter but no admin account and no obvious link between the two. +> +> **Residual of #1438, which is not yet on `main`** as this is filed. The behaviour described here does not exist until that lands, so this row is filed ahead of its own subject deliberately rather than describing shipped code. + +**Cluster:** Auth / password policy (ASVS 6.2.4). **Priority:** P2. **Verdict:** needs an owner ruling, then a small fix. + +### The chain, measured + +Each link verified at this branch's merge base: + +1. [`api/app.py`](../messagefoundry/api/app.py) -- the lifespan awaits `auth.initialize()` with no `try`. +2. [`auth/service.py`](../messagefoundry/auth/service.py) -- `initialize` calls `_ensure_bootstrap_admin`. +3. Same file -- `_ensure_bootstrap_admin` returns early when `count_users() > 0`, so **only a first run continues**, then calls `_generate_policy_password`. +4. Same file -- that generator loops 16 times over `secrets.token_urlsafe`, calling `self._policy.violations(candidate)` on each. +5. [`auth/policy.py`](../messagefoundry/auth/policy.py) -- `violations` consults `_common_passwords()`, which raises `BreachCorpusUnavailable` when the corpus is unusable. + +The raise escapes on **iteration one**, so the generator's defensive `token_urlsafe(length) + "aA1!"` fallback is unreachable in this state. `admin_reset_password` reaches the same generator, so an admin-initiated password reset fails the same way, but that one is a 500 on a running engine rather than a failure to start. + +### Why the guard is arguably wrong HERE specifically, and right everywhere else + +The candidate is `secrets.token_urlsafe(max(24, min_length))` -- at least **192 bits** of entropy, rendered as roughly 32 URL-safe characters. It is not a human-chosen password and cannot appear in a corpus of human-chosen passwords. **The breach clause is inert on this input by construction.** + +So on this one path #1438's guard converts a check that can never fire into a check that always blocks. That is the precise shape of a false positive: the control is refusing an operation it was never able to evaluate in the first place, and the refusal is total. + +This does **not** generalise. On every other path `violations` screens an operator-supplied or user-supplied password, where the corpus is the whole point and failing closed is correct. Do not "fix" this by weakening the guard. + +### The options, not decided here + +| option | what it costs | +|---|---| +| screen the generated token with `check_breached=False` | one line and a comment; the screen it drops is provably inert on a 192-bit token. Keeps every other path failing closed. **Recommended.** | +| catch `BreachCorpusUnavailable` inside the generator and fall through | keeps the screen wired but makes the generator quietly tolerate a broken corpus; a reader then has to reason about which failures it swallows | +| guard `auth.initialize()` in the lifespan | wrong layer. It would let the engine start with no admin and no way to make one, trading a loud failure for a confusing one | +| leave as is | defensible fail-closed reading: an install whose shipped security data is corrupt should not mint the highest-value credential in the system. Costs a first deployment a startup failure whose cause is two files away from its symptom | + +**The first option is the recommendation** because it removes a refusal that carries no security value while leaving every refusal that does. The fourth is a real position and the reason this is filed for a ruling rather than just fixed. + +### What would make this concrete + +A test that builds an `AuthService` against an empty store and an unusable corpus, and asserts `initialize()` does not raise once the chosen option lands. There is no such test today, which is why the interaction was found by reading the call graph rather than by a red leg. diff --git a/messagefoundry/auth/policy.py b/messagefoundry/auth/policy.py index 40490e425..7cadbbbe1 100644 --- a/messagefoundry/auth/policy.py +++ b/messagefoundry/auth/policy.py @@ -94,8 +94,15 @@ class BreachCorpusUnavailable(RuntimeError): "not be a common or breached password" clause stopped being emitted, and nothing logged -- the shipped configuration would assert a check that was not happening on a first deployment. - Blast radius is narrow by construction: only the password *create* and *change* paths screen a - password, so this never touches login, never invalidates a session, and never stops message flow. + Blast radius on an ESTABLISHED instance is narrow by construction: only the password *create* and + *change* paths screen a password, so this never touches login, never invalidates a session, and + never stops message flow. + + **A FIRST RUN IS THE EXCEPTION, and it is not narrow.** On an empty store ``AuthService.initialize`` + mints the bootstrap admin, whose generator screens its own candidate, so this raises out of an + unguarded lifespan call and the engine does not start at all. That is fail-closed but arguably + disproportionate, because the candidate is a 192-bit random token the breach clause can never match. + Tracked separately; do not read the paragraph above as covering a first run. ``AuthService`` also loads the corpus eagerly at startup and logs the same defect as an error, so an operator learns about it from the log rather than from a user's failed password change. """ diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index b2b5120f4..7b77ffc59 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -110,8 +110,11 @@ def _error_if_bundled_corpus_unusable(check_breached: bool) -> None: underneath it. Nothing screens underneath the BUNDLED list, so its loss is an ERROR: ``check_breached`` ships ``True``, and a corpus that cannot load means the shipped configuration asserts a check that is not running. ``PasswordPolicy.violations`` refuses passwords in that state; this is only the loud - half. It deliberately does NOT stop the engine -- HL7 flow does not depend on password screening, and - bricking a message engine over an auth data asset would trade a contained failure for an outage. + half. THIS FUNCTION deliberately does not stop the engine -- HL7 flow does not depend on password + screening, and bricking a message engine over an auth data asset would trade a contained failure for + an outage. Read that as scoped to this function and not to the change as a whole: on a FIRST run + ``initialize`` mints the bootstrap admin, whose generator screens its own candidate, so the raise + from ``violations`` escapes an unguarded lifespan call and startup fails. Tracked separately. Skipped when the operator has turned screening off: a corpus nobody consults is not a defect. """