From 59782dca91a6fff73b8d02fb13c8a8b71ce121bb Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 12:16:35 -0500 Subject: [PATCH] feat(tls): assert forward secrecy on every built context, don't inherit it (ASVS 12.1.2) validate_tls_ciphers already rejects a CONFIGURED tls_ciphers/proxy_tls_ciphers that admits static RSA/DH -- but it only fires when an operator sets the knob. A context built without one inherits the interpreter's default suite list and nothing checked it. That inheritance-without-assertion is the real residual. The residual of record -- "No cipher knob on MLLP/DICOM/FTPS/REST/SOAP at all" -- OVERSTATES the defect, and correcting our own residual is part of this cell's evidence: an assessor who runs get_ciphers() sees the correction came from us. harden_cipher_suites(ctx, connector=...) asserts and raises at all six context sites (API/UI listener, DICOM listener + destination, MLLP listener + destination, FTPS). MEASURED, not asserted: on CPython 3.14.6 / OpenSSL 3.5.7 all four shipped context shapes resolve to 17 suites with ZERO non-forward-secret. So this raises on no supported configuration today -- it converts an inherited property into a checked one, at zero interop risk. Deliberately NOT shipped: the obvious set_ciphers("ECDHE+AESGCM:ECDHE+CHACHA20: DHE+AESGCM:!aNULL:!eNULL") preference string. Measured against the shipped default it REMOVES six CBC-SHA2 suites that real MLLP/DICOM hospital peers still speak -- a genuine interop regression -- and ADDS two DSS suites the default did not enable. The default order already leads with TLS_AES_256_GCM_SHA384, so "strongest first" holds without touching it. Dropping that item is what makes this S rather than M. The evidence test PRINTS what it measured ("17 suites examined, 0 non-forward-secret" per shape) rather than swallowing it through capsys: capturing would verify the report was produced while hiding it from the reader, which defeats the point. `pytest -s -k forward_secret` IS the artefact. Four mutations, each red for its own reason. Two of them exist because the first pass MISSED them: with the assertion wired at six sites and two tests passing, deleting the call from an MLLP site stayed GREEN -- both tests exercised the FUNCTION, not its WIRING, so a new listener could have shipped an unasserted context unnoticed, which is exactly how the original residual arose. Added a DERIVED call-site guard (every site pinning kex groups must also assert; harden_kex_groups already marks every place the engine builds a context) plus a liveness receipt so the rglob cannot pass over nothing. Untouched on purpose: harden_kex_groups. `ssl.SSLContext.set_groups` does not exist on 3.14.6, so it pins nothing at any of its six call sites; the proposed "call it on the five uncovered contexts" fix would add five more no-ops behind a test that can self-skip. That is a separate, honest correction -- not this cell. --- messagefoundry/api/tls.py | 7 +- messagefoundry/config/tls_policy.py | 34 +++++++ messagefoundry/transports/dicom.py | 3 + messagefoundry/transports/mllp.py | 3 + messagefoundry/transports/remotefile.py | 4 + tests/test_tls_policy.py | 124 ++++++++++++++++++++++++ 6 files changed, 174 insertions(+), 1 deletion(-) diff --git a/messagefoundry/api/tls.py b/messagefoundry/api/tls.py index 95788422..471d8230 100644 --- a/messagefoundry/api/tls.py +++ b/messagefoundry/api/tls.py @@ -14,7 +14,11 @@ from messagefoundry.auth.trust_anchors import api_client_anchor_spec, enforce_anchor from messagefoundry.config.settings import ApiSettings -from messagefoundry.config.tls_policy import harden_kex_groups, harden_verify_flags +from messagefoundry.config.tls_policy import ( + harden_cipher_suites, + harden_kex_groups, + harden_verify_flags, +) __all__ = ["build_api_ssl_context"] @@ -49,6 +53,7 @@ def build_api_ssl_context(api: ApiSettings, *, enforcing: bool = True) -> ssl.SS if api.tls_ciphers: ctx.set_ciphers(api.tls_ciphers) harden_kex_groups(ctx) # pin approved ECDHE groups where the runtime supports it (ASVS 11.6.2) + harden_cipher_suites(ctx, connector="API/UI listener") # assert forward secrecy (ASVS 12.1.2) harden_verify_flags(ctx) # strict RFC 5280 cert validation (ASVS 12.1.4) client_ca = api_client_anchor_spec(api) if client_ca is not None: diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index d3d9a709..219aee0d 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -64,6 +64,7 @@ "cleartext_acceptance_audit_sink", "current_hop_posture", "enforce_insecure_hop", + "harden_cipher_suites", "harden_kex_groups", "harden_verify_flags", "relax_verify_expiry", @@ -297,6 +298,39 @@ def validate_tls_ciphers(value: str) -> str: return value +def harden_cipher_suites(ctx: ssl.SSLContext, *, connector: str) -> None: + """**Assert** that every suite ``ctx`` would negotiate is forward-secret, and raise if not. + + ASVS 12.1.2 / 11.6.2. ``validate_tls_ciphers`` already rejects a *configured* ``tls_ciphers`` / + ``proxy_tls_ciphers`` that admits static RSA/DH — but that validator only fires when an operator + sets the knob. A context built without one **inherits the interpreter's default suite list and + nothing checked it**, which is the real residual: inheritance without assertion, not (as the + residual of record says) "no cipher knob at all". + + This is an assertion rather than a ``set_ciphers`` preference string, deliberately. Measured + against the shipped default on CPython 3.14.6 / OpenSSL 3.5.7, all four context shapes resolve to + **17 suites, zero non-forward-secret**, so this raises on no supported configuration today — it + converts an inherited property into a checked one. The obvious alternative, + ``set_ciphers("ECDHE+AESGCM:ECDHE+CHACHA20:...")``, was measured and REJECTED: against the shipped + default it *removes* six CBC-SHA2 suites that real MLLP/DICOM hospital peers still speak (an + interop regression) and *adds* two DSS suites the default did not enable. The default order + already leads with ``TLS_AES_256_GCM_SHA384``, so "strongest first" holds without touching it. + + Raises :class:`ValueError` at construction — the same class the surrounding TLS config errors use, + so it surfaces at ``check`` / dry-run / ``serve`` rather than as a wire-time surprise. + """ + non_fs = sorted( + {str(c.get("name", "?")) for c in ctx.get_ciphers() if not _is_forward_secret(c)} + ) + if non_fs: + raise ValueError( + f"{connector}: the TLS context would negotiate non-forward-secret suite(s) " + f"{', '.join(non_fs)} (ASVS 12.1.2). Forward secrecy is required on every hop; a suite " + f"list that admits static RSA/DH key exchange lets a future key compromise decrypt " + f"recorded PHI traffic." + ) + + def _is_forward_secret(cipher: Mapping[str, object]) -> bool: """Whether a ``SSLContext.get_ciphers()`` entry uses an (EC)DHE — forward-secret — key exchange.""" name = str(cipher.get("name", "")) diff --git a/messagefoundry/transports/dicom.py b/messagefoundry/transports/dicom.py index abf017a8..5fc60372 100644 --- a/messagefoundry/transports/dicom.py +++ b/messagefoundry/transports/dicom.py @@ -59,6 +59,7 @@ from messagefoundry.config.tls_policy import ( TrustAnchorPolicy, build_verifying_client_context, + harden_cipher_suites, harden_kex_groups, harden_verify_flags, relax_verify_expiry, @@ -141,6 +142,7 @@ def _server_ssl_context(s: dict[str, Any]) -> ssl.SSLContext | None: ctx.load_verify_locations(cafile=str(ca)) ctx.verify_mode = ssl.CERT_REQUIRED harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites(ctx, connector="DICOM listener") # assert forward secrecy (ASVS 12.1.2) harden_verify_flags(ctx) # strict RFC 5280 validation of any mTLS client cert (ASVS 12.1.4) return ctx @@ -439,6 +441,7 @@ def _client_ssl_context( ) ctx.load_cert_chain(certfile=str(cert), keyfile=str(key) if key else None, password=pw_arg) harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites(ctx, connector="DICOM destination") # assert forward secrecy (ASVS 12.1.2) harden_verify_flags(ctx) # strict RFC 5280 validation of the peer's server cert (ASVS 12.1.4) # #129 (ADR 0094): opt-in granular expiry-only relaxation — honour an expired downstream PACS cert # while STILL validating chain + hostname (verification stays ON; default off = byte-identical). diff --git a/messagefoundry/transports/mllp.py b/messagefoundry/transports/mllp.py index a34e1215..c08a02fb 100644 --- a/messagefoundry/transports/mllp.py +++ b/messagefoundry/transports/mllp.py @@ -48,6 +48,7 @@ cleartext_acceptance_audit_sink, current_hop_posture, enforce_insecure_hop, + harden_cipher_suites, harden_kex_groups, harden_verify_flags, insecure_hop_disposition, @@ -540,6 +541,7 @@ def _mllp_ssl_context( ctx.load_verify_locations(cafile=ca) ctx.verify_mode = ssl.CERT_REQUIRED harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites(ctx, connector="MLLP listener") # assert forward secrecy (ASVS 12.1.2) harden_verify_flags(ctx) # strict RFC 5280 validation of any mTLS client cert (ASVS 12.1.4) return ctx # Outbound (client): verify the server cert unless explicitly — and loudly — disabled. #200 (ADR @@ -578,6 +580,7 @@ def _mllp_ssl_context( if cert: # optional client identity for mTLS ctx.load_cert_chain(certfile=cert, keyfile=key, password=pw_arg) harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites(ctx, connector="MLLP destination") # assert forward secrecy (ASVS 12.1.2) if verify: # skip the tls_verify=false / CERT_NONE path — nothing to validate (ASVS 12.1.4) harden_verify_flags(ctx) # strict RFC 5280 validation of the server cert # #129 (ADR 0094): granular expiry-only relaxation — honour a partner cert whose notAfter has diff --git a/messagefoundry/transports/remotefile.py b/messagefoundry/transports/remotefile.py index 0b1b5d25..ae737648 100644 --- a/messagefoundry/transports/remotefile.py +++ b/messagefoundry/transports/remotefile.py @@ -59,6 +59,7 @@ from messagefoundry.config.tls_policy import ( TrustAnchorPolicy, build_verifying_client_context, + harden_cipher_suites, harden_kex_groups, harden_verify_flags, relax_verify_expiry, @@ -210,6 +211,9 @@ def _ftps_ssl_context( pw_arg = key_password if key_password is not None else (lambda: b"") ctx.load_cert_chain(certfile=cert, keyfile=key, password=pw_arg) harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites( + ctx, connector="remote-file (FTPS) connection" + ) # assert forward secrecy (ASVS 12.1.2) if verify: # nothing to strict-validate on the CERT_NONE path (ASVS 12.1.4) harden_verify_flags(ctx) # #129 (ADR 0094): opt-in granular expiry-only relaxation — accept an expired server cert while diff --git a/tests/test_tls_policy.py b/tests/test_tls_policy.py index 457998c1..8e5dd758 100644 --- a/tests/test_tls_policy.py +++ b/tests/test_tls_policy.py @@ -8,6 +8,7 @@ import ssl import types from itertools import product +from pathlib import Path import pytest @@ -417,3 +418,126 @@ def test_active_hop_posture_stamps_and_restores() -> None: assert current_hop_posture() is None assert current_hop_posture() is posture assert current_hop_posture() is None + + +# --- ASVS 12.1.2: forward secrecy is ASSERTED on every shipped context, not inherited --------------- + + +def test_every_shipped_context_shape_negotiates_only_forward_secret_suites() -> None: + """The evidence an assessor actually wants: the suite count and the non-FS count, PRINTED. + + ``validate_tls_ciphers`` already rejects a *configured* string that admits static RSA/DH — but it + only fires when an operator sets the knob. A context built without one inherits the interpreter's + default list and nothing checked it. That inheritance-without-assertion is the real 12.1.2 + residual (the residual of record overstates it as "no cipher knob at all", which is false). + + This prints rather than merely asserting because a green dot proves nothing to a reader: the + output states what was examined, so "0 non-forward-secret" is a measurement rather than a claim. + """ + from messagefoundry.config.tls_policy import _is_forward_secret + + shapes = { + "PROTOCOL_TLS_SERVER": ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER), + "default(SERVER_AUTH)": ssl.create_default_context(ssl.Purpose.SERVER_AUTH), + "default(CLIENT_AUTH)": ssl.create_default_context(ssl.Purpose.CLIENT_AUTH), + "PROTOCOL_TLS_CLIENT": ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT), + } + findings: list[str] = [] + examined = 0 + for name, ctx in shapes.items(): + suites = ctx.get_ciphers() + non_fs = sorted({str(c.get("name", "?")) for c in suites if not _is_forward_secret(c)}) + print(f"{name}: {len(suites)} suites examined, {len(non_fs)} non-forward-secret") + assert suites, f"{name} resolved to NO suites — the measurement is vacuous" + examined += 1 + if non_fs: + findings.append(f"{name}: {non_fs}") + # Deliberately NOT capsys: capturing the report would verify it was produced while hiding it from + # the reader, which defeats the point. pytest shows these lines with -s (and on failure), so + # `pytest -s -k forward_secret` IS the evidence artefact. + assert examined == len(shapes), f"expected {len(shapes)} shapes measured, got {examined}" + assert not findings, ( + f"shipped TLS context shape(s) would negotiate non-forward-secret suites: {findings}. " + f"A suite admitting static RSA/DH lets a future key compromise decrypt recorded PHI traffic." + ) + + +def test_harden_cipher_suites_raises_on_a_non_forward_secret_context() -> None: + """The assertion must actually fire — proven by building a context that violates it rather than + by trusting that it would. + + Mutation: delete the raise in ``harden_cipher_suites``. Red: DID NOT RAISE. Without this test the + function is only ever exercised on contexts that pass, so it could be a no-op and every call site + would still look green. + """ + from messagefoundry.config.tls_policy import harden_cipher_suites + + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + try: + ctx.set_ciphers("AES256-SHA:@SECLEVEL=0") # static-RSA kx, no forward secrecy + except ssl.SSLError: + pytest.skip( + "this OpenSSL build cannot enable a static-RSA suite — nothing to assert against" + ) + if all(_fs(c) for c in ctx.get_ciphers()): # pragma: no cover - build-dependent + pytest.skip("this OpenSSL build resolved the static-RSA request to FS suites only") + with pytest.raises(ValueError, match="non-forward-secret"): + harden_cipher_suites(ctx, connector="test listener") + + +def _fs(cipher: object) -> bool: + from messagefoundry.config.tls_policy import _is_forward_secret + + assert isinstance(cipher, dict) + return _is_forward_secret(cipher) + + +def test_every_context_that_pins_kex_groups_also_asserts_forward_secrecy() -> None: + """Call-site coverage — the half the function-level tests cannot see. + + Mutation-proven gap: deleting ``harden_cipher_suites`` from one MLLP context site left the whole + TLS suite GREEN, because the other tests exercise the FUNCTION, not its wiring. A new listener or + destination could ship a context with no forward-secrecy assertion and nothing would notice — + which is precisely how the original 12.1.2 residual arose (inheritance without assertion). + + Derived, not a hardcoded site list: ``harden_kex_groups(ctx)`` already marks every place the + engine builds and hardens a TLS context, so the two must be co-located. A new context that pins + groups but skips the assertion fails here. + """ + pkg = Path(tls_policy.__file__).resolve().parent.parent + problems: list[str] = [] + for path in sorted(pkg.rglob("*.py")): + if path.name == "tls_policy.py": + continue # the definitions themselves + text = path.read_text(encoding="utf-8") + kex = [ + n + for n, ln in enumerate(text.splitlines(), 1) + if "harden_kex_groups(" in ln and not ln.lstrip().startswith(("#", "*")) + ] + assertions = text.count("harden_cipher_suites(") + if kex and assertions < len(kex): + rel = path.relative_to(pkg.parent).as_posix() + problems.append( + f"{rel}: {len(kex)} kex-pin site(s) at lines {kex}, {assertions} assertion(s)" + ) + assert not problems, ( + f"TLS context site(s) that pin key-exchange groups but do NOT assert forward secrecy: " + f"{problems}. Every built context must be checked (ASVS 12.1.2) — the suite list is inherited " + f"from the interpreter unless something asserts it." + ) + + +def test_the_call_site_scan_examined_real_files() -> None: + """Liveness receipt for the scan above: it is a `not found` over an rglob, so a moved package or a + changed helper name would make it pass over nothing.""" + pkg = Path(tls_policy.__file__).resolve().parent.parent + sites = sum( + 1 + for p in pkg.rglob("*.py") + for ln in p.read_text(encoding="utf-8").splitlines() + if "harden_kex_groups(ctx)" in ln and not ln.lstrip().startswith("#") + ) + assert sites >= 5, ( + f"expected the known TLS context sites, found {sites} — the scan is not landing" + )