docs: correct six security and configuration documents that state things the shipped code refutes - #435
Merged
wshallwshall merged 6 commits intoAug 19, 2026
Conversation
…ampling it Both places that described the local-password context check called it "app/vendor/HL7 terms" and showed four of the twelve enforced terms behind a "like" / "e.g." hedge. The category was wrong, not just partial: five of the twelve members are changeme, bootstrap, admin, administrator and password, which belong to no vendor and to no protocol. An operator sizing a password standard against that sentence would expect Bootstrap-Winter-2026! to pass, and it is refused with no indication of which rule fired. SECURITY.md now lists all twelve verbatim from CONTEXT_WORDS in auth/policy.py, states that the match is case-insensitive SUBSTRING containment rather than equality or a prefix, and names the retired description rather than quietly widening the sample. It also records what the knob cannot do: password_check_context is whole-list on/off, no setting adds or removes a term, and password_breach_corpus_file is not a substitute because it matches the whole password only. CONFIGURATION.md's settings row carries the same correction and links rather than repeating the list, so the two cannot drift apart again.
…names Four claims about the second-factor requirement were refuted by the shipped code, and the document contradicted itself on two of them. Scope. The pathway-strength paragraph said require_mfa "scopes to local Administrator accounts". The shipped default is require_mfa_scope = "every_local_account" (config/settings.py), whose own comment records that the default widens the gate past the Administrator role. The Local pathway row sixty lines earlier already said this correctly. Enforcement point. The same paragraph, and the MFA-state row of the enforcement table, placed the refusal "at the step-up boundary, not as an access gate". api/security.py evaluates it above the permission loop and refuses every authorized route with 403 + X-MFA-Required: 1. Setting names. [auth].require_mfa and [auth].require_mfa_scope are in _RELOCATED_TO_SECURITY; the loader raises on them from file and environment alike and serve exits 2. The documented opt-out therefore named a config that cannot start. All sites now name the [security] keys and say the old spelling is rejected. [auth].oidc_require_mfa_claim is NOT relocated and keeps its spelling. Delegated pathways. "_complete_ad_login mints AD, Kerberos and OIDC sessions mfa_verified=True unconditionally" was wrong about the mechanism: it is a keyword argument decided per mechanism, and the federated leg passes oidc_require_mfa_claim, reached only after the claim gate has refused a token carrying no configured amr/acr. Under the shipped default the outcome is the same, which is why the wrong mechanism survived; turning the claim gate off does change it, and the old text implied otherwise. Two smaller fixes in the same cells: browser sessions are redirected to /ui/mfa, not confined to it (the account and enrolment routes are MFA-pending-exempt, which is what stops a zero-factor user being stranded), and an AD principal is exempt even with a factor enrolled, so the "always required once enrolled" clause is scoped to local accounts. PHI.md's control row asserted the same retired step-up/Administrator framing and is corrected with it, so the two documents agree. tests/test_docs_security_pathways.py pinned the literal "[auth].require_mfa defaults on" inside the section being corrected. The model field is unchanged (AuthSettings.require_mfa is the internal desugared field); only the rendered operator-facing key moves to [security], because the guard was pinning the documentation to a key the loader rejects.
…issing surfaces
The section opened by claiming "a single, change-controlled inventory of every
key, algorithm, and certificate the engine relies on". The word every was
false: four shipped crypto surfaces had no row. Rows are added for all four
and the opening is reworded to an at-least shape, so the next connector does
not falsify it again.
- the verifying SMTP hop shared by the [alerts] sink and the EMAIL and
DIRECT destinations, whose one build_smtp_tls_context factory is what
stands between the engine and smtplib's own unverified fallback
- XML-DSig verification via signxml, which appeared once in the whole
document, inside the CI blockquote, with no algorithms or trust anchor
- the SFTP connector's SSH host-key trust and its RSA client key
- the SMART Backend Services client assertion
Storage/access for the store DEK said "environment only". That is refuted by
three further shipped routes: a DPAPI-protected key file, the vault key
provider (keyprovider_vault.py ships and is dispatched by name behind the
[vault] extra), and cipher_provider = vault_transit, where no local key
material exists at all. Only aws_kms, azure_kv, gcp_kms and pkcs11 are
unbuilt; the settings comment still says all five are, which is what the old
bullet was written from. A site hardening key handling from that sentence
would not learn it can keep the plaintext key out of the service environment
entirely.
A distribution bullet is added, and stated as the true asymmetry rather than
a cap: the engine needs one holder, escrow is a deliberate second holder with
a stated reason, and further copies are unaccounted for by construction.
Asserting "exactly one entity" would have contradicted the escrow mandate in
the very next bullet.
A new subsection covers the five asymmetric private keys the engine loads but
does not mint. They previously had a rotation cadence and nothing else: no
generation, storage, holder or destruction policy. The DEK's rules read across
badly in both directions, which is why the split is stated explicitly -
escrowing a signing key would copy a credential whose loss costs only a
re-issue, and the DEK's discard-is-erasure property describes no other key
here.
The JWS row listed three of the five algorithms the enum ships, so a partner
requiring SHA-384 looked unsupported.
tests/test_key_usage_scope_inventory.py's curated registry classifies every
parsed row; the four new labels are added to the key-material set or the
guard fails on unclassified rows.
…ize ceilings The ASVS 5.1.1 policy block opened with a closed count - "MessageFoundry's file surface has three parts" - that omitted the DICOM C-STORE SCP, a fourth surface on which a remote modality pushes whole objects into the engine. A reader auditing the file-receiving surface from that sentence would have missed it, and would have had no signal that the drop-directory policy below does not govern it. Reworded to an at-least shape with the SCP named and its controls pointed at the DICOM section. The decompression bullet claimed that with no decompress= set "there is no unpacked-size surface". False for a shipped payload type: a Deflated Explicit VR LE DICOM object carries its own DEFLATE stream, the drop's content sniff accepts it on the DICM magic alone, and the inflate is bounded elsewhere - at 16 MiB with no per-connection knob when a Router or Handler parses it, and at max_object_bytes when an outbound SCU forwards it. Neither ceiling appeared anywhere in operator documentation. Measured over all tracked paths under docs/ with per-file grep -c: guard_part10_deflate, MAX_INFLATED and "Deflated Explicit VR" each matched exactly one file, the ledger, against a positive control of max_object_bytes matching five files in the same run. The inbound max_object_bytes row now records the second duty that setting carries and the counter-intuitive consequence of disabling it: 0/None does not widen the limit, it removes the object-size check and tightens the inflate ceiling to the 16 MiB codec default the guard falls back to. The bullet stops short of claiming an exposure: max_file_bytes still caps the compressed bytes at ingest where that cap is set, and whether a Handler parses the object is site-specific, so the text says where the bound lives.
…cement dials
Three documents stated the refuse / auto-bound split backwards, and this is
the one correction in the set where a site acting on the old text loses PHI.
They said a PHI instance under [security].enforcement = enforce refuses to
start (exit 2) when a PHI-body window is unbounded, and that the 30-day
auto-bound applied only to a non-enforcing instance. The shipped gate in
__main__.py does the opposite for an UNSET window: the defaulting loop is
guarded by `if not settings.retention.allow_unbounded_phi:` alone, with no
enforcement branch, so an unset window is defaulted to 30 days and the
instance starts on both dials. tests/test_cli.py already pins this at --env
prod on the shipped enforce dial, asserting rc == 0 and a 30-day window.
The practical harm is specific: a site could deliberately leave a window unset
as a hold, expecting the boot to stop until someone chose a number, and would
instead get a started instance that begins purging PHI bodies at 30 days. The
old sentences are named as retired rather than silently swapped, because a
reader who acted on them needs to see that they changed.
What survives is the fail-closed path for an EXPLICIT 0, which is not
auto-bounded and still refuses under enforce (warns under warn). The closing
claim that a PHI instance "cannot run with PHI-body retention off without a
loud, audited opt-out" is narrowed accordingly - it holds under enforce, but
under warn an explicitly-zeroed window warns and starts with no opt-out.
Related corrections in the same gate:
- the set of auto-bounded windows is three, not the two the prose implied,
and reference_snapshot_days was missing. The windows that carry no
auto-bound are described as an open set with the two clearest cases and
their reason, rather than a count: six of the nine classified windows
carry auto_bound_days=None, each for its own recorded reason, so naming
two as "the" exclusions would have been a fresh false enumeration.
- the [retention] block is no longer one of the blocks a stock PHI instance
must configure to boot, so the worked example's gate count and its
dead_letter_days comment (REQUIRED, "its own exit 2") are corrected to
RECOMMENDED with the reason to set it anyway.
- the loosening register credited enforcement = warn with the auto-bound.
It is dial-independent; what warn actually changes for retention is that
an explicit 0 warns instead of refusing.
Both files also now say plainly that 30 days is the engine's floor against an
accidentally unbounded window and not the site's retention policy, since an
auto-bounded window is a number nobody decided.
wshallwshall
enabled auto-merge (squash)
August 17, 2026 21:01
wshallwshall
disabled auto-merge
August 18, 2026 04:19
Collaborator
Author
Do not merge without rebasing first: this would silently DELETE content from
|
Collaborator
Author
Correction to my previous comment: I quoted the wrong side of the statMy earlier comment said 259 deletions. That number is what this branch ADDS, not what merging would lose. I read The corrected figure: merging would lose 56 line(s) that The finding itself stands and so does the remedy -- rebase before merging, do not clear BEHIND and merge on green -- because a clean merge still drops those 56 lines with no marker. But the magnitude was overstated and the number in my first comment should not be quoted. |
Collaborator
Author
RETRACTION: my "would silently delete content" comments on this PR were wrongBoth of my earlier comments are withdrawn. The finding does not survive a discriminator, and the remedy I attached to it was justified by a defect that is not there. What I did wrongI counted removed lines in a merge result and reported them as lost content. Those are different questions. A line-for-line REPLACEMENT scores identically to a DELETION under a line count -- and this branch's changes are overwhelmingly rewrites. The discriminator, run on this PRFor every line Zero. Every removed line's payload is present in the merged result. The concrete case that broke my own claimI said this branch deletes Not deleted -- expanded, plus a new mechanism (ADR 0164, What still stands, and what does notDoes not stand: any claim that merging this drops content, and the "never update-branch-and-merge-on-green" remedy insofar as it rested on that. Still true, but unremarkable: this branch is BEHIND, and branch protection is Credit where due: a peer session caught this by verifying one file in #437 -- |
wshallwshall
enabled auto-merge (squash)
August 19, 2026 13:45
wshallwshall
deleted the
docs-accuracy-crypto-mfa-retention-file-surface
branch
August 19, 2026 16:54
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
1 participant
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.
Six documents asserted things the shipped code refutes. Each correction was written by opening the
code first and citing it; where a claim could not be verified from public source it was weakened or
dropped rather than restated.
The one a deploying site could have been harmed by
docs/PHI.mdand two companions described a PHI retention startup gate that does not fire. A sitereading that text and deliberately leaving a retention window unset -- expecting the engine to refuse
to start rather than silently apply a default -- would instead have got a started instance purging
message bodies at thirty days. The text now states what the gate actually does on both enforcement
dials.
The rest
MFA. The documented opt-out named a configuration key the loader rejects; following the page as
written produces exit 2. Scope, enforcement point and setting names are corrected together, because
they were wrong as a set rather than individually.
Password context deny-list.
docs/SECURITY.mdcalled it "app/vendor/HL7 terms" and showed fourexamples. Five of the twelve enforced terms are
changeme,bootstrap,admin,administratorandpassword-- generic credential words belonging to no vendor and no protocol -- and eight of thetwelve appeared in no document at all. So the category description was wrong, not merely incomplete,
and an operator whose passphrase was refused could not tell which rule fired. The full list is now
enumerated, the hedge is gone, and the code is named as the authority on drift. Replacing four
illustrations with twelve illustrations would not have fixed this; the hedge was the defect.
Key management. The scope sentence overstated what the policy covers, and four shipped surfaces
were missing from the inventory.
File surface and DICOM. The file-handling inventory and the size ceilings did not match the
connectors.
How the corrections are written
Where a statement was false it is retired in place, not silently swapped: the text says what the
document previously asserted and that it was wrong. A reader who acted on the old sentence needs to
know it changed, and a silent correction hides that anyone was ever misled.
Datestamps were deliberately left off the retractions. Fourteen dated corrections across six security
documents read as a remediation log, which is a different artifact than a set of accurate documents,
and dates go stale while the corrections do not.
Verification
1,769 tests passed across four tranches, no failures. Several tests in this repository parse
documentation from disk, so a docs-only change here is not test-neutral; the affected suites were run
rather than assumed. Two suites SKIP because a file they read is not present in this checkout -- they
are reported as skipped and nothing was created to satisfy them.
ruff,ruff format --checkandmypyare unaffected by this change and were confirmed unchangedagainst the pre-change baseline.
Scope
Documentation only. No engine behaviour changes. No new files. No assessment record was touched --
correcting a document and grading anything against it are separate acts, and only the first is in
this branch.