feat(tls): measure the declared front door instead of trusting it (ASVS 12.1.1) - #53
Merged
Merged
Conversation
…ext banner test_startup_dual_control_arm_is_documented_as_warn_only asserts that the #189 approvals-at-exposure arm still WARNS rather than refusing, by slicing that arm out of __main__.py and checking it contains no `return 2`. The slice ran from the arm's `if` to the next `\n # ---` section banner. That boundary is not reference-invariant: it measures whatever happens to sit between the arm and the next banner, not the arm. Insert any refusal after the arm without a banner of its own and the guard goes red, blaming the approvals arm for a `return 2` that is nowhere near it. That is exactly what happened -- the ASVS 12.1.1 TLS-floor probe landed in that gap and this test failed with "the approvals-at-exposure arm now REFUSES to start" while the arm was byte-identical. A gate whose answer depends on unrelated neighbouring code is not measuring its subject. Slice by the `if`'s own indentation instead: the block is the marker line plus every following line indented deeper than it. Note the slice now starts at the LINE START, not at the marker offset -- source.index lands past the leading whitespace, so computing the indent from it would have produced a 4-space body_indent for an 8-space body and swallowed the whole file. Added a liveness receipt, because the failure mode of a boundary bug is a slice too SHORT, which makes the assertion pass vacuously: the test now asserts the slice actually contains the arm's `warning:` print and more than five lines before trusting the `return 2` check. A guard that could return "nothing found" because it looked at nothing is the same defect in the other direction. Both directions mutation-proven, since neither is evidence on its own: M1 `return 2` inserted into the approvals arm -> RED (still catches its actual subject) M2 the 12.1.1 section banner removed from the neighbouring block -> GREEN (no longer sensitive to neighbours) Before the fix M2 was RED, which is the whole defect.
…VS 12.1.1)
[api].proxy_tls_min_version is an ATTESTATION: the operator types "1.2" and
nothing checks it. Off-loopback the browser TLS is terminated at their proxy, so
the engine negotiates none of it -- that is the residual of record. Making an
unverified declaration mandatory does not close the requirement; it just makes
the unchecked claim compulsory. A probe converts the declaration into a
measurement, which is why the probe IS this cell and the WARN->REFUSE flip alone
is not.
New config/tls_probe.py asks the declared public_origin three questions at
startup:
1. Do you still speak TLS 1.0? Offered with minimum==maximum==TLSv1 and
ALL:@SECLEVEL=0. A SUCCESSFUL handshake is the failure -- it proves the front
door accepts a protocol NIST SP 800-52r2 withdrew.
2. Do you still speak TLS 1.1? Same shape.
3. What do you actually CHOOSE? A default-capability client, asserting the
negotiated version. Deliberately a full offer, not a 1.3-only probe: version
selection is server-driven, so a full offer landing on 1.3 proves PREFERENCE,
whereas a 1.3-only handshake proves only SUPPORT. Preference is the property.
SECLEVEL=0 is load-bearing and mutation-proven: without it modern OpenSSL will
not even send a TLS 1.0 ClientHello, so the probe would measure OUR refusal to
ask rather than THEIR refusal to answer -- a permissive door reading as clean.
The deprecated enum is ASSERTED, never skipped. A probe that quietly skipped
when ssl.TLSVersion.TLSv1 disappears becomes a gate that cannot fail and reports
success forever. That is precisely the harden_kex_groups failure mode this repo
already carries -- six call sites, zero effect, because set_groups does not exist
on 3.14.6 and it returns silently. TlsProbeUnavailable is raised instead, and
resolved BEFORE any dial so a build defect never looks like a proxy problem.
Certificate validation is deliberately off in the probe (CERT_NONE): it measures
the PROTOCOL FLOOR, and an untrusted internal CA would abort the handshake
before the version was settled -- reporting "TLS 1.0 refused" for a door never
actually knocked on. Chain validation is a different control (12.1.4). The
context handles no application data and never leaves the module.
This also retires the loopback carve-out. That arm warns because "the engine
cannot distinguish loopback-behind-a-declared-proxy from loopback-and-genuinely-
unexposed beyond the declaration itself" -- true of a declaration, false of a
measurement. A reachable front door speaking TLS 1.0 is a fact, loopback or not.
OPERATIONAL COST, stated rather than buried: on the posture where this refuses,
startup now depends on the proxy being reachable. That is a real tension for an
interface engine and it is deliberate -- warn-on-unreachable is defeated by
start ordering (bring the engine up first and the check never runs), so it is
not a gate at all. Blast radius is bounded to exactly the posture the
requirement is about: a declared upstream terminator, PHI, under enforce. Every
other posture never reaches the probe and is byte-identical. A regression
detected LATER at runtime must not kill the engine and the hospital's feed with
it; that path belongs to an AlertSink event and is not built here.
Tests run against REAL handshakes -- a loopback TLS listener with a throwaway
cert -- because a mocked ssl would test the mock. Verified live during
development against pypi.org: "negotiated TLSv1.3; refused TLS 1.0 and 1.1".
Five mutations red: invert the finding so a permissive door reads clean; drop
the version pin; report unreachable as reachable; let the missing enum skip;
drop SECLEVEL=0. Simulating the enum removal needs a stand-in type -- enum
members cannot be deleted ("cannot reassign member") -- which is itself worth
knowing for anyone maintaining this.
Three things the existing guards caught, all real:
- The crypto-inventory gate refused tls_probe.py as an undocumented ssl import.
Correct -- a new ssl/hashlib/hmac/cryptography root needs an inventory row.
Added, with the warning that these are DELIBERATELY WEAKENED client contexts
(SECLEVEL=0, CERT_NONE) for measurement only and must never be reused for a
data path.
- test_serve_ui_declared_proxy_requires_mfa_on_prod_phi showed the probe
PRE-EMPTING the MFA-at-exposure refusal. Ordering defect, not a test problem:
the probe is the only gate that makes network calls, so firing it before the
config-only refusals means an operator fixes the TLS floor, restarts, and only
then learns MFA was off -- two round trips for one boot. Moved after the
auth-off / /ui-exposure / MFA gates. Cheap refusals first.
- test_startup_dual_control_arm_is_documented_as_warn_only went red claiming
"the approvals-at-exposure arm now REFUSES to start" -- an arm this change does
not touch. It sliced that arm from its `if` to the next `# ---` section banner,
and this block landed in the gap without one, so THIS refusal was attributed to
THAT arm. Half a real finding: the guard's boundary was not
reference-invariant (repaired in the preceding commit, mutation-proven in both
directions), and this block was genuinely missing the section banner that every
sibling in the serve ladder carries. Both fixed; the banner comment records why
it is load-bearing so it does not get tidied away later.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
wshallwshall
added a commit
that referenced
this pull request
Jul 29, 2026
… pin that never pinned (ASVS 11.6.2) (#56) * fix(auth): an EMPTY oidc_client_secret is a MISSING one (ASVS 6.x config validation) The guard that makes a missing OIDC client secret fail at config load read if self.oidc_client_secret is None and self.oidc_client_secret_ref is None which is not the shape a missing secret usually takes. `MEFOR_AUTH_OIDC_CLIENT_SECRET=` in a service wrapper, or an NSSM environment entry with an empty value, yields `""` -- and `"" is not None`, so oidc_enabled loaded happily with no client secret at all. Reproduced before the fix, with a working control so the repro proves something: secret='client-s3cret' ACCEPTED (control) secret='' ACCEPTED oidc_client_secret='' secret=' ' ACCEPTED oidc_client_secret=' ' secret absent refused "oidc_enabled requires a client secret" Only the ABSENT case fired. Note the `missing` list ten lines above in the same validator already uses `if not value` for all five pinned endpoints -- this one line was the only place in the validator that disagreed with its own siblings, which is why it read as correct. Severity, stated accurately rather than inflated: this is NOT an auth bypass. An empty client_secret reaches the IdP at the token exchange and is rejected there, so the effect is a federated login that is broken instead of refused, diagnosed from a proxy log rather than from a startup message. The defect is that a fail-at-load guard silently became a fail-at-first-login one. Whitespace is stripped for the emptiness TEST only; the value is never rewritten, and a secret with meaningful leading/trailing whitespace still binds byte-for-byte (asserted). The same test now applies to oidc_client_secret_ref, or the fix would be half a fix -- an empty ref would otherwise satisfy "one of the two is set" while naming no provider entry. Both mutations killed against the shipped, formatted source: M1 restore `is None` -> RED (4 failed) on the empty cases M2 make the guard reject EVERY secret -> RED on the POSITIVE CONTROL M2 is the one that matters: without a positive control, both refusal tests would stay green under a guard that refuses everything, and "a raise happened" is not the same claim as "the right input was refused". * fix(tls): the approved KEX groups were never pinned -- stop claiming they were (ASVS 11.6.2) harden_kex_groups pins SSLContext.set_groups "where available (Python 3.13+)". set_groups is a Python 3.15 API. typeshed guards it at sys.version_info >= (3, 15), alongside the get_groups that will finally make the pin assertable. Measured on this tree -- Python 3.14.6 / OpenSSL 3.5.7, hasattr(ctx, "set_groups") is False -- so APPROVED_KEX_GROUPS reaches ZERO of its six call sites and every built context inherits OpenSSL's default group list. That much was already suspected. What was not: client pinned to result (real handshakes vs build_api_ssl_context, identical at tls_min_version 1.2 and 1.3) X25519 accepted <- positive control secp384r1 accepted <- positive control prime256v1 accepted <- positive control ffdhe2048 ACCEPTED ffdhe3072 ACCEPTED secp521r1 ACCEPTED secp224r1 refused (NO_SUITABLE_GROUPS) sect571r1 refused (NO_SUITABLE_GROUPS) ADR 0092 section 4(b) asserts the opposite: "a real handshake test proves a client offering only a non-approved FFDHE group is refused -- runtime enforcement". That test reached its assertion only through a CLIENT-side set_groups, so it hit pytest.skip on every interpreter this project runs on. A skip was concealing a false assertion, and the ADR cited it as proof. That is worse than an untested claim, because it reads as evidence. Method, stated because the probe could have lied too: the client pins ONE group via set_ecdh_curve, which was validated as a genuine constraint FIRST -- a server pinned to prime256v1 refuses a client pinned to secp384r1 (NO_SUITABLE_KEY_SHARE) while the unpinned-server control accepts it. Without that check, a probe that constrained nothing would have reported "everything accepted" for a context that was in fact restrictive. SEVERITY, stated accurately rather than inflated: the inherited list is forward-secret, so the property the TLS 1.2+ floor exists to guarantee still holds, and the genuinely weak curves are refused. This is a conformance gap against an internal allow-list -- wider than policy, not weak. ASVS 11.6.2 is Partial in the scorecard of record and stays Partial. The headline count does not move. What changes is whether it can be defended. What lands: harden_kex_groups now RETURNS the list it actually pinned (None today). A security control that cannot report whether it did anything reports success forever -- that is how a call at six sites with zero effect survived three assessments. The failure path returns None too: previously a pin that RAISED logged a warning and then fell off the end, so a caller could not distinguish "pinned" from "tried and failed". Its `# pragma: no cover` is gone, because a stand-in context now drives that branch instead of leaving it to an unusual OpenSSL build. The three unit tests that stood over this asserted (a) the call does not raise, (b) it no-ops on an object without the API, (c) a string constant has three colon-separated names. All three pass identically whether or not a single group is ever pinned. Replaced with an unconditional receipt asserting the None, whose failure message is the re-derivation instruction for whoever trips it. Written unconditionally on purpose: an `if hasattr(ctx, "set_groups")` branch would restore exactly the property being removed. The skipping handshake test is replaced by one that MEASURES the accepted-group set and cannot skip. Asserted as invariants rather than an exact table, since the accepted set comes from the linked OpenSSL and a CI leg may differ: every approved group must get in (else the server is over-restricted and the next assertion would pass for the wrong reason) and some non-approved group must get in (proving the list is not enforced). The measured table prints on failure. Five mutations, both directions: M1 report a pin never made -> inertness receipt RED M2 revert to the old no-return shape -> reporting test RED M3 failed pin reports the list -> raising test RED M4 probe accepts everything -> measurement RED M5 probe refuses everything -> measurement RED M2 is the one that justifies its own test: under M2 the INERTNESS test stays GREEN (measured: rc=0), because a function with no return statement also yields None. Asserting `is None` therefore does not prove the reporting works, which is why the pinning path is driven separately with a stand-in context. Documents corrected in the same change, because the claim had spread to five: - config/tls_policy.py -- the "3.13+" docstring and the module summary. - docs/PHI.md section 4 -- "Approved groups pinned where supported" -> inherited, with the measured accepted set. - docs/ASVS-L2-PHASE0-CHANGES.md -- the same sentence, same error. - docs/adr/0092 section 4(b) -- struck through in place and WITHDRAWN by a dated amendment naming all three wrong assertions. The ADR's decision is unaffected: the forward secrecy it relies on comes from the TLS 1.2+ floor, which IS enforced. Sections 4(a) and 4(c) were re-confirmed and stand. - docs/BACKLOG.md -- the "runtime-KEX enforcement ... handshake-tested" clause inside a shipped banner. Prose-only edit, banner invariant untouched, test_backlog_status_check.py green. Deliberately NOT touched: docs/security/ASVS-L3-ASSESSMENT.md and the other dated assessments carry the same claim and are marked "Retained unmodified for diffing and audit history". They stay unmodified; the scorecard-of-record and register corrections are vaulted separately (docs/security/ is git-ignored here). Also NOT done, and it is a real option rather than an oversight: SSLContext.set_ecdh_curve exists and genuinely pins, but takes exactly ONE OpenSSL curve short name -- it cannot express a preference list, and pinning through it would refuse two of the three approved groups. It is a costed hardening knob for a future lane, not a drop-in, and shipping it as though it closed 11.6.2 would be the kind of lenient reading this project has already retracted twice. Also note secp256r1 is a valid group-list alias but NOT a valid EC curve name (that spelling is prime256v1) -- do not "normalise" the constant. Swept the sibling helpers, because "is anything else inert?" is the obvious next question and a claim is not an answer. tls_policy.py has exactly three getattr-guarded best-effort call sites; all three were driven on this interpreter: set_groups (harden_kex_groups) INERT -- the subject of this commit VERIFY_X509_STRICT (harden_verify_flags) LIVE -- present (32), and the flag is verifiably ORed into verify_flags (32768 -> 32800) _hashlib.get_fips_mode (fips_attestation) LIVE -- returns False, not None; None would have meant undeterminable, i.e. a vacuous attestation So this was the only one of the three, and that is measured rather than assumed. * docs(adr-0034): record the py/insecure-protocol dismissal on the 12.1.1 TLS probe CodeQL alert 145 -- py/insecure-protocol, HIGH, messagefoundry/config/tls_probe.py -- was raised by PR #53 and dismissed `won't fix` on the repo. ADR 0034's policy is that every finding is triaged to Fix or Dismiss-with-a-recorded-reason, with the per-alert rationale in the GitHub comment and the class rationale in the ADR. The GitHub comment is capped at 280 characters, so the ADR is where the real argument has to live; this is that entry. The rule is factually right and does not apply: tls_probe.py offers TLS 1.0/1.1 BECAUSE a successful handshake at a withdrawn version is the finding it exists to produce. Two flagged settings are load-bearing and mutation-proven -- SECLEVEL=0 (without it OpenSSL sends no ClientHello, so the probe would measure our refusal to ask rather than their refusal to answer) and CERT_NONE (an untrusted internal CA would abort before the version was settled, reporting "TLS 1.0 refused" for a door never knocked on). What makes the dismissal safe is scope, so the row states it: client contexts only, built in that module, one handshake, never returned to a caller, no application data, no PHI. Not a data path, and the settings must never be reused for one -- which the module docstring and the crypto-inventory row both already say, at the two places a future author would actually look. Also recorded per this ADR's own convergence rule: the anchor is tls_probe.py:146 inside _offer_context, and that module is new and small, so this WILL re-fire as a fresh alert number the first time anything is inserted above line 146. Re-dismiss with this rationale rather than re-triaging from scratch.
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.
ASVS 12.1.1 — measure the declared front door instead of trusting it
[api].proxy_tls_min_versionis an attestation: the operator types1.2and nothing checks it.Off-loopback the browser TLS is terminated at their reverse proxy, so the engine negotiates none of it —
that is the residual of record. Making an unverified declaration mandatory does not close the
requirement; it just makes the unchecked claim compulsory. The probe is the cell; the WARN→REFUSE
flip alone is not.
New
messagefoundry/config/tls_probe.pyasks the declaredpublic_originthree questions at startup:minimum == maximum == TLSv1,ALL:@SECLEVEL=0TLSv1_1TLSv1.3Question 3 is deliberately a full offer, not a 1.3-only probe: version selection is server-driven, so
a full offer landing on 1.3 proves the proxy prefers it, whereas a 1.3-only handshake proves only that
it supports it. Preference is the property 12.1.1 is about.
Verified live against
pypi.org:443during development:negotiated TLSv1.3; refused TLS 1.0 and 1.1.Three design points that are load-bearing
SECLEVEL=0is not decoration. Without it modern OpenSSL will not even send a TLS 1.0ClientHello, so the probe would measure our refusal to ask rather than their refusal to answer —
a permissive front door would read clean. Mutation-proven.
The deprecated enum is asserted, never skipped.
ssl.TLSVersion.TLSv1/TLSv1_1will eventually beremoved. A probe that skipped at that point would become a gate that cannot fail and would report success
forever — the exact
harden_kex_groupsfailure mode this repo already carries (six call sites, zeroeffect).
TlsProbeUnavailableis raised instead, and resolved before any dial so a build defect neverpresents as a proxy problem.
Certificate validation is off (
CERT_NONE), on purpose. The probe measures the protocol floor. Anuntrusted internal CA would abort the handshake before the version was settled, reporting "TLS 1.0
refused" for a door that was never actually knocked on. Chain validation is a different control (12.1.4).
The context carries no application data and never leaves the module.
This also retires the loopback carve-out, which warned because "the engine cannot distinguish
loopback-behind-a-declared-proxy from loopback-and-genuinely-unexposed beyond the declaration itself" —
true of a declaration, false of a measurement.
On the posture where this refuses, startup now depends on the proxy being reachable. That is a real
tension for an interface engine and it is deliberate: warn-on-unreachable is defeated by start ordering
(bring the engine up before nginx and the check never runs), so it would not be a gate at all.
Blast radius is bounded to exactly the posture the requirement is about — a declared upstream
terminator, on PHI, under
enforce. Every other posture never reaches the probe and is byte-identical.A regression detected later, at runtime, must not kill the engine and the hospital's feed with it;
that path belongs to an AlertSink event and is not built here.
Testing
Real handshakes against a loopback TLS listener with a throwaway self-signed cert — a mocked
sslwouldtest the mock. Simulating the enum removal needs a stand-in type, because enum members cannot be deleted
(
AttributeError: cannot reassign member).Five mutations red: invert the finding so a permissive door reads clean; drop the version pin; report
unreachable as reachable; let the missing enum skip; drop
SECLEVEL=0.Three things the existing guards caught, all real
tls_probe.pyas an undocumentedsslimport. Correct. Row added,with the warning that these are deliberately weakened client contexts (
SECLEVEL=0,CERT_NONE)for measurement only and must never be reused for a data path.
test_serve_ui_declared_proxy_requires_mfa_on_prod_phishowed the probe pre-empting theMFA-at-exposure refusal. Ordering defect, not a test problem — the probe is the only gate that makes
network calls, so firing it first means an operator fixes the TLS floor, restarts, and only then
learns MFA was off. Moved after the config-only refusals. Cheap refusals first.
test_startup_dual_control_arm_is_documented_as_warn_onlywent red claiming "theapprovals-at-exposure arm now REFUSES to start" — an arm this change does not touch. Half a real
finding, fixed in the first commit: that guard sliced the arm from its
ifto the next# ---section banner, a boundary that is not reference-invariant, so it measured whatever landed in the gap.
This block was also genuinely missing the section banner every sibling in the serve ladder carries.
The guard now slices by the arm's own indentation and is mutation-proven both ways — a
return 2in the arm still goes red; removing the neighbouring banner no longer does. Before the fix, the second
of those was red, which is the whole defect.
Verification
ruff check·ruff format --check(975 files) ·mypy --strict(257 modules) · crypto-inventory (57documented sites, no drift) · full suite 9610 passed, 720 skipped.
One pre-existing environmental failure remains and is unrelated:
test_installed_metadata_matches_dunder_versioncompares the editable install's metadata (0.3.0)against
__version__(0.3.2) — the test's own comment names this case and its remedy(
pip install -e . --no-deps). CI installs fresh.