diff --git a/docs/ASVS-L2-PHASE0-CHANGES.md b/docs/ASVS-L2-PHASE0-CHANGES.md index 1ddc19c7..c432f7b6 100644 --- a/docs/ASVS-L2-PHASE0-CHANGES.md +++ b/docs/ASVS-L2-PHASE0-CHANGES.md @@ -218,9 +218,12 @@ value. Under `[security].enforcement=ENFORCE` a DEK past its max-age + grace **e For the off-loopback transports the TLS floor is **1.2+** (`tls_min_version`), which constrains key exchange to forward-secret **(EC)DHE** suites. The **code-half is now built** (WP-L3-10 free lift): -[config/tls_policy.py](../../messagefoundry/config/tls_policy.py) pins the approved key-exchange groups -(`X25519:secp384r1:secp256r1` via `SSLContext.set_groups` on Python ≥3.13; OpenSSL already defaults to -these on 3.11/3.12) on every built API + MLLP TLS context (`harden_kex_groups`), and a `[api].tls_ciphers` +[config/tls_policy.py](../../messagefoundry/config/tls_policy.py) *attempts* the approved key-exchange +group pin (`X25519:secp384r1:secp256r1` via `SSLContext.set_groups`) on every built API + MLLP TLS +context (`harden_kex_groups`) — but **corrected 2026-07-29: `set_groups` is a Python 3.15 API, not 3.13, +so today it pins nothing and those contexts inherit OpenSSL's default group list** (forward-secret, but +wider than the approved three — see [PHI.md](PHI.md) §4 for the measured accepted set). The helper now +reports what it pinned so the gap is visible rather than assumed. A `[api].tls_ciphers` settings validator (`validate_tls_ciphers`) rejects any non-forward-secret (static-RSA/DH) operator cipher string at config load. On the default `127.0.0.1` bind no TLS is presented, so this is immaterial today; the controls take effect once the engine terminates TLS off-loopback. diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 102ca00e..cbd0440b 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -6293,7 +6293,7 @@ Two findings are worth surfacing here. **Posture B scores worse on Fails than Po ## 200. Transport enforcement: make the code refuse the insecure hop -> ✅ **SHIPPED 2026-07-13 (ADR 0092 + 2026-07-13 amendment) — the posture-keyed transport-hop refusal and ALL its DEFERRED residuals are closed.** The core (2026-07-11): the #200 cleartext-hop refusal **enforces on the primary `serve`/`reload` path, not only at `build_check`** — the live connector-build sites in `pipeline/wiring_runner.py` (`_start_outbound`, `_start_inbound_unsafe`, `_reconcile_outbounds`) stamp the derived `active_hop_posture`, so the raw-TCP/X12/MLLP/DICOM/anon-ftp guards **refuse a production-PHI cleartext outbound at serve**, and the strict verify-off cells (engine⇄store weakened TLS, MLLP/FTPS `tls_verify=false`, credentialed plain-ftp) route `MEFOR_ALLOW_INSECURE_TLS` through the **production-PHI clamp** (`config.settings.weakened_tls_escape_permitted`) so the escape can no longer relax a production-PHI hop. **Residuals now closed (2026-07-13):** (1) the **API PHI-read data-path guard** — `create_app` derives the API serve-hop disposition via the new pure `tls_policy.phi_read_hop_disposition` (reusing the ONE authority + the production-PHI clamp) and `api/security.enforce_phi_read_hop` (folded into `require_phi_read`; explicit on the step-up `search` route) **refuses (403, PHI-free)** a raw-view/attachment-download/summary read on a prod-PHI instance whose serve hop is not proven secure — loopback/TLS/proxy-terminated/synthetic/no-`[ai]` stay byte-identical; (2) the **`db_lookup`/`fhir_lookup` live-read posture stamp** — `_build_lookup_executor`/`_build_fhir_lookup_executor` now wrap construction in `active_hop_posture(self._hop_posture)`, so a prod-PHI weakened-TLS live read is refused (it previously keyed on the UNCLAMPED escape, posture unstamped) and a synthetic cleartext read is no longer false-closed; (3) **`messagefoundry check`** now runs the posture-stamped `build_check_registry` (new required `build-check` in `checks.py`; fail-safe SKIP with no `messagefoundry.toml`), so a prod-PHI cleartext hop is caught at commit/CI; (4) the **Posture-B tails** — a cert-authenticated `GET /service/identity` writes a `service_cert_auth` audit row, runtime-KEX enforcement (`harden_kex_groups` on the in-process context) + a real mutual-TLS handshake are handshake-tested. All compose with the #201 revocation guard / #199 cleartext-egress / #129 expiry-relaxation and never double-refuse a legitimate lane. Tests: `tests/test_hop_refusal_residuals.py` + `tests/test_api_tls.py`. **Genuinely deferred (infra-bound):** a full uvicorn-on-a-real-socket mTLS handshake through the live serve bind (Windows TLS CI legs) — the handshake tests exercise the same `build_api_ssl_context` context, so only the uvicorn wiring is uncovered. _(Re-scored 2026-07-10 → P2; filed by the ASVS 5.0 L3 re-score, PR #854.)_ +> ✅ **SHIPPED 2026-07-13 (ADR 0092 + 2026-07-13 amendment) — the posture-keyed transport-hop refusal and ALL its DEFERRED residuals are closed.** The core (2026-07-11): the #200 cleartext-hop refusal **enforces on the primary `serve`/`reload` path, not only at `build_check`** — the live connector-build sites in `pipeline/wiring_runner.py` (`_start_outbound`, `_start_inbound_unsafe`, `_reconcile_outbounds`) stamp the derived `active_hop_posture`, so the raw-TCP/X12/MLLP/DICOM/anon-ftp guards **refuse a production-PHI cleartext outbound at serve**, and the strict verify-off cells (engine⇄store weakened TLS, MLLP/FTPS `tls_verify=false`, credentialed plain-ftp) route `MEFOR_ALLOW_INSECURE_TLS` through the **production-PHI clamp** (`config.settings.weakened_tls_escape_permitted`) so the escape can no longer relax a production-PHI hop. **Residuals now closed (2026-07-13):** (1) the **API PHI-read data-path guard** — `create_app` derives the API serve-hop disposition via the new pure `tls_policy.phi_read_hop_disposition` (reusing the ONE authority + the production-PHI clamp) and `api/security.enforce_phi_read_hop` (folded into `require_phi_read`; explicit on the step-up `search` route) **refuses (403, PHI-free)** a raw-view/attachment-download/summary read on a prod-PHI instance whose serve hop is not proven secure — loopback/TLS/proxy-terminated/synthetic/no-`[ai]` stay byte-identical; (2) the **`db_lookup`/`fhir_lookup` live-read posture stamp** — `_build_lookup_executor`/`_build_fhir_lookup_executor` now wrap construction in `active_hop_posture(self._hop_posture)`, so a prod-PHI weakened-TLS live read is refused (it previously keyed on the UNCLAMPED escape, posture unstamped) and a synthetic cleartext read is no longer false-closed; (3) **`messagefoundry check`** now runs the posture-stamped `build_check_registry` (new required `build-check` in `checks.py`; fail-safe SKIP with no `messagefoundry.toml`), so a prod-PHI cleartext hop is caught at commit/CI; (4) the **Posture-B tails** — a cert-authenticated `GET /service/identity` writes a `service_cert_auth` audit row, a real mutual-TLS handshake is handshake-tested (**corrected 2026-07-29:** the companion "runtime-KEX enforcement" claim here was wrong — `SSLContext.set_groups` is a Python 3.15 API, so `harden_kex_groups` pins nothing on today's interpreters and the test that asserted the FFDHE refusal self-skipped; measured, the context ACCEPTS ffdhe2048. Both fixed and the residual is now asserted — see the ADR 0092 2026-07-29 amendment and PHI.md §4). All compose with the #201 revocation guard / #199 cleartext-egress / #129 expiry-relaxation and never double-refuse a legitimate lane. Tests: `tests/test_hop_refusal_residuals.py` + `tests/test_api_tls.py`. **Genuinely deferred (infra-bound):** a full uvicorn-on-a-real-socket mTLS handshake through the live serve bind (Windows TLS CI legs) — the handshake tests exercise the same `build_api_ssl_context` context, so only the uvicorn wiring is uncovered. _(Re-scored 2026-07-10 → P2; filed by the ASVS 5.0 L3 re-score, PR #854.)_ **Cluster:** Security & Compliance. **Priority:** P2. **Verdict:** build. **Severity:** medium. diff --git a/docs/PHI.md b/docs/PHI.md index 08c5d525..5b3b3660 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -635,17 +635,27 @@ and a reverse-proxy / forwarded-header alternative are designed in [ADR 0002](adr/0002-phase2-transport-security-and-strong-auth.md) (*Proposed* — build gated on a scheduled off-loopback exposure). -**Key-exchange parameters `[BUILT — WP-L3-10 code half]` (ASVS 11.6.2).** Every TLS context the engine +**Key-exchange parameters `[PARTIAL — the 1.2+ floor and the cipher validator are enforced; the group +pin is INERT until Python 3.15]` (ASVS 11.6.2).** Every TLS context the engine builds — the API/WebSocket listener ([api/tls.py](../messagefoundry/api/tls.py)) and the per-connection MLLP server/client contexts ([transports/mllp.py](../messagefoundry/transports/mllp.py)) — enforces a **TLS 1.2+ floor**, which constrains 1.2 to **(EC)DHE** key exchange and makes 1.3 ECDHE-only: forward- -secret key establishment, never static RSA/DH. Two controls in -[config/tls_policy.py](../messagefoundry/config/tls_policy.py) pin the *parameters*: - -- **Approved groups pinned where supported.** Built contexts call `harden_kex_groups`, which sets the - approved ECDHE groups `X25519:secp384r1:secp256r1` via `SSLContext.set_groups` on Python ≥ 3.13. On - 3.11/3.12 there is no public group-pinning API and OpenSSL's defaults already lead with exactly these - curves, so it is a deliberate no-op, not a downgrade. +secret key establishment, never static RSA/DH. **That floor is the enforced control.** Two further +controls in [config/tls_policy.py](../messagefoundry/config/tls_policy.py) address the *parameters* — and +only the second of them actually takes effect on today's interpreters: + +- **Approved groups are *inherited*, not pinned — corrected 2026-07-29.** Built contexts call + `harden_kex_groups`, which pins the approved ECDHE groups `X25519:secp384r1:secp256r1` via + `SSLContext.set_groups` — an API that lands in **Python 3.15**. This bullet previously said "≥ 3.13", + and the practical effect of the error is that on every interpreter this project currently runs on + (measured: 3.14.6 / OpenSSL 3.5.7) the helper pins **nothing** and every built context inherits + OpenSSL's default group list. That default *is* forward-secret — the property the TLS 1.2+ floor + above exists to guarantee — but it is **wider than the approved list**: measured against the real API + context, it also accepts `ffdhe2048`, `ffdhe3072` and `secp521r1`. It refuses `secp224r1` and + `sect571r1`, so the gap is *wider than policy*, not *weak*. `harden_kex_groups` now **returns the + list it actually pinned** — `None` today — and `tests/test_tls_policy.py` asserts that `None` + unconditionally, so the first interpreter with the API turns the test red instead of letting the + claim drift back. - **`tls_ciphers` is validated, not trusted.** An operator `[api].tls_ciphers` string is rejected at config load if it would admit a **non-forward-secret** (static-RSA/DH) suite, so a misconfiguration cannot widen the key exchange below policy. diff --git a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md index 8333f3e3..c376bcbd 100644 --- a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md +++ b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md @@ -221,3 +221,35 @@ an execution. Before the next tag, run `security.yml`'s sbom job via `workflow_d log — the install command there is byte-identical to `release.yml`'s, and `test_sbom_install_is_byte_identical_in_release_and_security` now enforces that identity, because the dry-run is evidence about the release step only for as long as the two commands are the same command. + +## Amendment — 2026-07-29: `py/insecure-protocol` on the ASVS 12.1.1 TLS-floor probe + +**Alert 145 — `py/insecure-protocol`, HIGH, `messagefoundry/config/tls_probe.py`. Dismissed `won't fix`.** + +CodeQL is factually right and the finding does not apply. `tls_probe.py` is the **measurement** for ASVS +12.1.1: at startup, on a PHI instance behind a *declared* upstream TLS terminator under `enforce`, it dials +the operator's own `public_origin` and offers TLS 1.0 and 1.1. **A successful handshake is the finding** — +it proves the front door accepts a protocol NIST SP 800-52r2 withdrew, and the engine refuses to serve. +Offering the withdrawn version *is* the control; there is no implementation that measures whether a peer +accepts TLS 1.0 without asking it to. + +Two settings the rule flags are load-bearing and mutation-proven in `tests/test_tls_floor_probe.py`: + +- `minimum_version == maximum_version == TLSv1` **plus `ALL:@SECLEVEL=0`** — without the security-level + drop, modern OpenSSL will not even *send* the ClientHello, so the probe would measure **our** refusal to + ask rather than **their** refusal to answer, and a permissive front door would read as clean. Dropping + `SECLEVEL=0` is one of the five mutations that turn the suite red. +- `CERT_NONE` — the probe measures the **protocol floor**. An internal CA the engine does not trust would + abort the handshake *before the version was settled*, reporting "TLS 1.0 refused" for a door that was + never knocked on. Chain validation is a separate control (12.1.4 / `harden_verify_flags`). + +**Scope, which is what makes the dismissal safe:** client contexts only, constructed in this module, used +for exactly one handshake, never returned to a caller, carrying no application data and no PHI. This is +**not a data path** and these settings must never be reused for one — the module docstring says so, and +the crypto-inventory row (`scripts/security/crypto_inventory_check.py`) repeats the warning at the place a +future author would look. Every TLS scanner (`testssl.sh`, `sslyze`, `nmap ssl-enum-ciphers`) is built the +same way; suppressing this rule for a scanner is the industry-standard disposition, not a local shortcut. + +**Convergence note (per the rule above):** the anchor is `tls_probe.py:146`, inside `_offer_context`. That +module is new and small, so expect this to 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. diff --git a/docs/adr/0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md b/docs/adr/0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md index 06e90a7b..ef48ba82 100644 --- a/docs/adr/0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md +++ b/docs/adr/0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md @@ -166,10 +166,11 @@ authority above: 4. **Posture-B tails.** (a) A **cert-authenticated intra-service auth** is now **audited**: the `GET /service/identity` route (the only `require_service_cert` surface) writes a `service_cert_auth` row into the tamper-evident chain naming the mapped principal (PHI/secret-free — auth plane + route - only). (b) **Runtime KEX enforcement** is verified: when the engine terminates TLS in-process, + only). (b) ~~**Runtime KEX enforcement** is verified: when the engine terminates TLS in-process, `build_api_ssl_context` pins the approved forward-secret groups (`harden_kex_groups`), and a real handshake test proves a client offering only a non-approved FFDHE group is refused — runtime - enforcement, not the operator attestation the proxy-terminated (Posture-B) case still relies on. (c) A + enforcement, not the operator attestation the proxy-terminated (Posture-B) case still relies on.~~ + **WITHDRAWN 2026-07-29 — see the amendment below.** (c) A real **mutual-TLS handshake** test exercises the exact server context the serve path builds (`CERT_REQUIRED`): a trusted client cert completes, a missing one is refused. **Genuinely deferred (infra-bound):** a full uvicorn-on-a-real-socket mTLS handshake through the live serve bind is left to @@ -207,3 +208,40 @@ every escape-clamp is **byte-identical** to the former production-PHI behaviour two carve-outs of the 2026-07-20 amendment re-key to `enforcement`, and the keyless-PHI ack is renamed `allow_unencrypted_phi_in_production` → `allow_unencrypted_phi_under_strict_enforcement` (see the ADR 0140 amendment). The four hard-refused floor items and the unconditional ePHI audit are unchanged. + +## Amendment (2026-07-29) — §4(b) "runtime KEX enforcement" is WITHDRAWN as a measurement error + +The 2026-07-13 amendment's §4(b) made three assertions. **All three are wrong**, re-measured on the +shipping interpreter (Python 3.14.6 / OpenSSL 3.5.7): + +1. *"`build_api_ssl_context` pins the approved forward-secret groups"* — it does not. + `SSLContext.set_groups` is a **Python 3.15** addition (typeshed guards it at + `sys.version_info >= (3, 15)`), so `harden_kex_groups` returns without pinning at **all six** of its + call sites and the built contexts inherit OpenSSL's default group list. +2. *"a real handshake test proves…"* — that test + (`test_kex_allow_list_enforced_at_runtime`) reached its assertion only through a **client-side** + `set_groups`, so on every interpreter this project runs on it hit `pytest.skip`. It never executed. +3. *"…a client offering only a non-approved FFDHE group is refused"* — the opposite is true. Measured by + pinning a client to one group at a time via `set_ecdh_curve` and handshaking against the real + `build_api_ssl_context`, at both `tls_min_version` 1.2 and 1.3: `ffdhe2048`, `ffdhe3072` and + `secp521r1` are all **accepted**. (`secp224r1` and `sect571r1` are refused.) + +A skip was concealing a false assertion, and this ADR cited it as proof. That is worse than an untested +claim, because it reads as evidence. + +**What this ADR relies on is unaffected.** The forward-secrecy property comes from the **TLS 1.2+ floor**, +which *is* enforced, and the inherited default list is itself forward-secret — the residual is *wider than +policy*, not *weak*. §4(a) (the `service_cert_auth` audit row) and §4(c) (the real mutual-TLS handshake +against the built `CERT_REQUIRED` context) were re-confirmed and stand. + +**Fixed alongside this amendment:** `harden_kex_groups` now **returns the group list it actually pinned** +(`None` today), so the inertness is observable rather than silent; a failed pin returns `None` too, where +it previously fell through as if it had succeeded. The skipping test is replaced by one that *measures* +the accepted-group set and cannot skip, plus an unconditional receipt asserting the `None` — so the first +interpreter to grow the API turns the suite red and forces this amendment, `PHI.md` §4 and the ASVS +11.6.2 row to be re-derived together. See [PHI.md](../PHI.md) §4. + +**Do not re-assert a KEX pin anywhere until `harden_kex_groups` returns non-`None`.** +`SSLContext.set_ecdh_curve` does exist and genuinely constrains the TLS 1.3 `supported_groups`, but it +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 option, not a drop-in. diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index ca4ddec4..7cee18a4 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -2120,10 +2120,22 @@ def _require_oidc_fields(self) -> AuthSettings: if missing: raise ValueError(f"oidc_enabled requires: {', '.join(missing)}") - if self.oidc_client_secret is None and self.oidc_client_secret_ref is None: + # EMPTY, not just absent. `is None` was the test here, and it let the most common shape of a + # missing secret straight through: an env var exported with no value. `MEFOR_AUTH_OIDC_CLIENT_SECRET=` + # in a service wrapper or an NSSM environment entry produces `""`, which is not None, so the guard + # that exists to make a missing client secret fail at CONFIG LOAD did not fire — the failure moved + # to the first token exchange, as an IdP rejection an operator has to go read a proxy log to + # understand. `if not value` is already the emptiness test used by the `missing` list ten lines + # above; this line was the only one in the validator that disagreed. Whitespace is stripped for the + # test only — the value itself is never rewritten. + if ( + not (self.oidc_client_secret or "").strip() + and not (self.oidc_client_secret_ref or "").strip() + ): raise ValueError( - "oidc_enabled requires a client secret: set oidc_client_secret (via " - "MEFOR_AUTH_OIDC_CLIENT_SECRET) or oidc_client_secret_ref (a [secrets].provider reference)" + "oidc_enabled requires a NON-EMPTY client secret: set oidc_client_secret (via " + "MEFOR_AUTH_OIDC_CLIENT_SECRET) or oidc_client_secret_ref (a [secrets].provider " + "reference). An env var exported with no value counts as missing." ) # Every pinned URL must be https (no dev escape — this is an off-box trust boundary) and its diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index 219aee0d..66ab3b16 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -8,9 +8,11 @@ * :func:`validate_tls_ciphers` — reject an operator ``tls_ciphers`` string that would admit a non-forward-secret (non-ECDHE/DHE) key exchange, so a misconfiguration cannot widen the suite below policy. Run from the ``[api].tls_ciphers`` settings validator, so a bad value fails loud at load. -* :func:`harden_kex_groups` — pin the approved ECDHE groups on a built context where the runtime - supports it (``SSLContext.set_groups``, Python 3.13+); on older interpreters OpenSSL already leads - with these groups, so it is a best-effort no-op rather than a downgrade. +* :func:`harden_kex_groups` — *attempt* to pin the approved ECDHE groups on a built context, and + **report whether it managed to**. ``SSLContext.set_groups`` is a **Python 3.15** API (this said + "3.13+" and was wrong), so today it pins nothing on every supported runtime and the contexts inherit + OpenSSL's default group list — forward-secret, but wider than policy. See its docstring for the + measured accepted set; do not cite the groups as "pinned" while it returns ``None``. * :func:`harden_verify_flags` — OR ``ssl.VERIFY_X509_STRICT`` into a verifying context's ``verify_flags`` so a presented chain must be RFC 5280-conformant (ASVS 12.1.4 strict path validation). Revocation itself is delegated to the org PKI / OCSP-must-staple proxy + OS trust store @@ -109,23 +111,54 @@ def fips_attestation() -> tuple[bool | None, str]: return fips_mode, ssl.OPENSSL_VERSION -def harden_kex_groups(ctx: ssl.SSLContext) -> None: - """Best-effort pin ``ctx`` to :data:`APPROVED_KEX_GROUPS`. - - Uses ``SSLContext.set_groups`` where available (Python 3.13+). On older interpreters there is no - public API to pin groups and OpenSSL's defaults already lead with X25519/P-256/P-384, so this is a - deliberate no-op rather than a weakening. A runtime that rejects the group list (an unusual OpenSSL - build) is logged and left at its secure defaults.""" +def harden_kex_groups(ctx: ssl.SSLContext) -> str | None: + """Pin ``ctx`` to :data:`APPROVED_KEX_GROUPS`; return the list pinned, or ``None`` if nothing was. + + **On every runtime this project currently supports this pins NOTHING and returns ``None``.** + ``SSLContext.set_groups`` is a **Python 3.15** addition — typeshed guards it at + ``sys.version_info >= (3, 15)``, alongside the ``get_groups`` that will finally make the pin + *assertable*. This docstring previously said "Python 3.13+"; that was wrong, and it was repeated + into `docs/PHI.md`, `docs/ASVS-L2-PHASE0-CHANGES.md`, ADR 0092 §4(b) and the ASVS scorecard. + Measured on this tree: Python 3.14.6 / OpenSSL 3.5.7, ``hasattr(ctx, "set_groups")`` is ``False``, + so :data:`APPROVED_KEX_GROUPS` reaches **zero** of this function's six call sites and every built + context falls back to OpenSSL's default group list. + + That default *is* forward-secret — which is the property the TLS 1.2+ floor guarantees and the one + ASVS 11.6.2's first clause is about — but it is **wider than the approved list**. Measured against + the real ``build_api_ssl_context``, at both ``tls_min_version`` 1.2 and 1.3, it also accepts + ``ffdhe2048``, ``ffdhe3072`` and ``secp521r1``; it does refuse ``secp224r1`` and ``sect571r1``. So + the residual is *wider than policy*, not *weak*. Until this returns non-``None``, do not describe + the engine's key-exchange groups as "pinned" anywhere — they are **inherited**. + + The **return value is the point.** 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. ``tests/test_tls_policy.py`` asserts the ``None`` *unconditionally*, so the first + interpreter that grows the API turns that test red — which is the signal to re-derive this + docstring, `docs/PHI.md` §4 and the 11.6.2 row, and to switch the test to ``get_groups()``. + + Two traps for whoever does that work: + + * ``SSLContext.set_ecdh_curve`` *does* exist and genuinely constrains the TLS 1.3 + ``supported_groups`` (verified: a server pinned to ``prime256v1`` refuses a client pinned to + ``secp384r1`` with ``NO_SUITABLE_KEY_SHARE``, while the unpinned control accepts it). It is not a + substitute here — it takes exactly ONE OpenSSL curve short name, so it cannot express a + preference list, and pinning through it would refuse two of the three approved groups. + * ``secp256r1`` is a valid OpenSSL *group-list* alias but **not** a valid EC curve name (that + spelling is ``prime256v1``, and ``set_ecdh_curve("secp256r1")`` raises). Both names are correct + in their own API. Do not "normalise" the constant. + """ set_groups = getattr(ctx, "set_groups", None) if set_groups is None: - return + return None try: set_groups(APPROVED_KEX_GROUPS) - except ( - ssl.SSLError, - ValueError, - ) as exc: # pragma: no cover - depends on the linked OpenSSL build + except (ssl.SSLError, ValueError) as exc: + # None, not the list: a pin that RAISED must never read back as a pin that took. (The former + # `# pragma: no cover` here is gone — this branch is now driven by a stand-in context in + # tests/test_tls_policy.py rather than left to an unusual OpenSSL build to exercise.) logger.warning("Could not pin TLS key-exchange groups %r: %s", APPROVED_KEX_GROUPS, exc) + return None + return APPROVED_KEX_GROUPS def harden_verify_flags(ctx: ssl.SSLContext) -> None: diff --git a/tests/test_api_tls.py b/tests/test_api_tls.py index 8106b819..deb0efbe 100644 --- a/tests/test_api_tls.py +++ b/tests/test_api_tls.py @@ -1253,28 +1253,80 @@ def _verifying_client_ctx(ca: Path) -> ssl.SSLContext: return ssl.create_default_context(ssl.Purpose.SERVER_AUTH, cafile=str(ca)) -def test_kex_allow_list_enforced_at_runtime(tmp_path: Path) -> None: - # The engine terminates TLS in-process, so it ENFORCES the approved forward-secret KEX groups at - # runtime (harden_kex_groups on the built context), not merely as an operator attestation. Prove it: - # a client pinned to ONLY a non-approved FFDHE group cannot complete against the pinned server, while - # a default client negotiates a forward-secret TLS 1.3 suite. +#: Key-exchange groups OUTSIDE `APPROVED_KEX_GROUPS` that a client can pin via `set_ecdh_curve`. +#: `x448` and `X25519MLKEM768` are deliberately absent — they are TLS groups but not EC curve NAMES, so +#: `set_ecdh_curve` raises on them and this technique cannot measure them. +_NON_APPROVED_KEX_PROBES = ("ffdhe2048", "ffdhe3072", "secp521r1", "secp224r1", "sect571r1") + +#: The approved groups, spelled for `set_ecdh_curve` — `prime256v1`, never `secp256r1` (see +#: APPROVED_KEX_GROUPS: the group-list alias and the EC curve name differ for exactly this curve). +_APPROVED_KEX_PROBES = ("X25519", "secp384r1", "prime256v1") + + +def _kex_group_accepted(server_ctx: ssl.SSLContext, ca: Path, group: str) -> bool: + """Does ``server_ctx`` complete a handshake with a client offering ONLY ``group``?""" + client = _verifying_client_ctx(ca) + # A ValueError here is a bug in this test's group table, not a finding — let it raise loudly rather + # than degrade into "not accepted", which would silently manufacture the result we want. + client.set_ecdh_curve(group) + try: + return bool(_handshake(server_ctx, client, client_cert=None)) + except OSError: + return False + + +def test_which_kex_groups_the_built_context_actually_accepts(tmp_path: Path) -> None: + """MEASURE the built context's key-exchange groups (ASVS 11.6.2). + + This replaces a test that asserted the opposite of the truth and stayed green by skipping. It read: + "a client offering ONLY ffdhe2048 ... shares no key-exchange group with the pinned server, so the + handshake is refused — runtime enforcement". It reached that assertion only through client-side + ``set_groups``, a **Python 3.15** API, so on every interpreter this project runs on it hit + ``pytest.skip``. And the claim was false: measured, this context ACCEPTS ffdhe2048, because + ``harden_kex_groups`` pins nothing. A skip was concealing a wrong assertion, which is worse than no + test — ADR 0092 §4(b) cited it as proof. + + ``set_ecdh_curve`` is the API that does exist, and it genuinely constrains the TLS 1.3 + ``supported_groups``: verified separately that a server pinned to ``prime256v1`` refuses a client + pinned to ``secp384r1`` (``NO_SUITABLE_KEY_SHARE``) while the unpinned control accepts it. One group + per handshake is enough to enumerate what the server will take. + + Asserted as INVARIANTS, not as an exact table: the accepted set comes from the linked OpenSSL's + default group list and a CI leg may link a different build. What must hold is that *every* approved + group gets in (else the server is over-restricted, or the harness is broken, and the second + assertion would pass for the wrong reason) and that *some* non-approved group gets in (proving the + approved list is not enforced). The measured table is printed on failure either way. + + Mutation: make ``_kex_group_accepted`` return True unconditionally. Red — the "genuinely weak curves + stay out" assertions fail, so a probe that cannot distinguish anything is caught. + """ ca, cert, key = _strict_ca_and_leaf(tmp_path) server_ctx = build_api_ssl_context( ApiSettings(tls_cert_file=str(cert), tls_key_file=str(key), tls_min_version="1.3") ) - # Positive control: a normal client (approved groups) handshakes and negotiates a TLS 1.3 suite. + + # Positive control first: a default client must negotiate a forward-secret TLS 1.3 suite at all. cipher = _handshake(server_ctx, _verifying_client_ctx(ca), client_cert=None) - assert cipher and cipher.startswith("TLS_") # a TLS 1.3 (ECDHE, forward-secret) suite + assert cipher and cipher.startswith("TLS_") - # Negative: a client offering ONLY ffdhe2048 (a valid TLS 1.3 group the server does NOT list) shares - # no key-exchange group with the pinned server, so the handshake is refused — runtime enforcement. - bad_client = _verifying_client_ctx(ca) - try: - bad_client.set_groups("ffdhe2048") - except (AttributeError, ssl.SSLError, ValueError): - pytest.skip("runtime does not support set_groups on the client side") - with pytest.raises(OSError): - _handshake(server_ctx, bad_client, client_cert=None) + approved = {g: _kex_group_accepted(server_ctx, ca, g) for g in _APPROVED_KEX_PROBES} + non_approved = {g: _kex_group_accepted(server_ctx, ca, g) for g in _NON_APPROVED_KEX_PROBES} + table = f"approved={approved} non_approved={non_approved}" + + assert all(approved.values()), f"the built context refuses an APPROVED group — {table}" + + # The residual of record: the approved list is NOT enforced, because a group outside it gets in. + leaked = sorted(g for g, ok in non_approved.items() if ok) + assert leaked, ( + "no non-approved key-exchange group was accepted, so the built context now DOES constrain " + "groups to the approved list. That is an improvement, not a test failure — re-score ASVS " + f"11.6.2, update docs/PHI.md §4 and the register, then tighten this test. {table}" + ) + + # ...and the genuinely weak curves stay out, so the residual is "wider than policy", not "insecure". + # If either of these fails it is a real finding on that OpenSSL build, not a flake. + assert not non_approved["secp224r1"], f"a 112-bit-strength curve was accepted — {table}" + assert not non_approved["sect571r1"], f"a binary-field curve was accepted — {table}" def test_real_mutual_tls_handshake_on_built_context(tmp_path: Path) -> None: diff --git a/tests/test_settings.py b/tests/test_settings.py index a7ce7df9..ba8edd84 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -605,6 +605,63 @@ def test_auth_oidc_refusals_name_the_key( load_settings(config_path=cfg, environ=env) +@pytest.mark.parametrize( + ("secret", "label"), + [ + ("", "an env var exported with NO value"), + (" ", "whitespace only"), + ("\t\n", "whitespace only, non-space"), + ], +) +def test_auth_oidc_empty_client_secret_is_refused(tmp_path: Path, secret: str, label: str) -> None: + """An EMPTY client secret must be refused at config load, not just an absent one. + + The guard used to read ``if self.oidc_client_secret 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 federated login + started up "configured" with no client secret and failed later at the token exchange as an IdP + rejection. Every sibling field in the same validator already used ``if not value``; this one line + disagreed. + + Reproduced before the fix: ``""`` and ``" "`` both loaded successfully with + ``oidc_client_secret == ''``. Mutation: restore ``is None``. Red on all three cases. + """ + cfg = _write(tmp_path / "messagefoundry.toml", _OIDC_AD + _OIDC_BLOCK) + env = dict(_OIDC_ENV) | {"MEFOR_AUTH_OIDC_CLIENT_SECRET": secret} + with pytest.raises(ValidationError, match="NON-EMPTY client secret"): + load_settings(config_path=cfg, environ=env) + + +def test_auth_oidc_empty_secret_ref_is_refused(tmp_path: Path) -> None: + """The same emptiness test must apply to the ``_ref`` alternative, or the fix is half a fix. + + An empty ``oidc_client_secret_ref`` would otherwise satisfy the "one of the two is set" guard while + naming no provider entry at all. + """ + cfg = _write( + tmp_path / "messagefoundry.toml", + _OIDC_AD + _OIDC_BLOCK + 'oidc_client_secret_ref = ""\n', + ) + env = {k: v for k, v in _OIDC_ENV.items() if k != "MEFOR_AUTH_OIDC_CLIENT_SECRET"} + with pytest.raises(ValidationError, match="NON-EMPTY client secret"): + load_settings(config_path=cfg, environ=env) + + +def test_auth_oidc_a_real_secret_still_loads(tmp_path: Path) -> None: + """Positive control for the two refusals above. + + Without it, tightening the emptiness test to something that rejects EVERY secret would leave both + refusal tests green — they only assert that a raise happens. + """ + cfg = _write(tmp_path / "messagefoundry.toml", _OIDC_AD + _OIDC_BLOCK) + s = load_settings(config_path=cfg, environ=dict(_OIDC_ENV)) + assert s.auth.oidc_client_secret == "client-s3cret" + # A secret with meaningful surrounding whitespace is NOT rewritten — stripping is for the + # emptiness test only, because an IdP secret can legitimately contain any byte. + padded = dict(_OIDC_ENV) | {"MEFOR_AUTH_OIDC_CLIENT_SECRET": " s3cret "} + assert load_settings(config_path=cfg, environ=padded).auth.oidc_client_secret == " s3cret " + + def test_auth_oidc_requires_public_origin(tmp_path: Path) -> None: cfg = _write(tmp_path / "messagefoundry.toml", _OIDC_AD + _OIDC_BLOCK) env = {k: v for k, v in _OIDC_ENV.items() if k != "MEFOR_SECURITY_WEB_CONSOLE_PUBLIC_ADDRESS"} diff --git a/tests/test_tls_policy.py b/tests/test_tls_policy.py index 8e5dd758..014639af 100644 --- a/tests/test_tls_policy.py +++ b/tests/test_tls_policy.py @@ -111,17 +111,87 @@ def test_validate_rejects_non_forward_secret() -> None: # --- harden_kex_groups ------------------------------------------------------------------------- def test_harden_does_not_raise_on_real_context() -> None: ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) - harden_kex_groups(ctx) # no-op pre-3.13, set_groups on 3.13+ — either way must not raise + harden_kex_groups(ctx) # must never raise, whatever the runtime can or cannot pin -def test_harden_is_noop_without_set_groups() -> None: - # A runtime/object lacking set_groups is handled gracefully (older interpreters). +def test_the_group_pin_is_inert_on_this_runtime_and_says_so() -> None: + """A liveness receipt for ASVS 11.6.2 — written to FAIL on the interpreter upgrade. + + ``SSLContext.set_groups`` is a **Python 3.15** addition, so ``harden_kex_groups`` pins nothing on + any interpreter this project runs on and ``APPROVED_KEX_GROUPS`` reaches zero of its six call + sites. That was already true; what was missing was any way to NOTICE. The tests that stood here + asserted (a) that the call does not raise, (b) that it no-ops on an object without the API, and (c) + the contents of a string constant — all three pass identically whether or not a single group is + ever pinned. That is how the docstring came to claim "Python 3.13+" and how ADR 0092 §4(b), PHI.md + §4 and the ASVS scorecard all came to assert a control that does not execute. + + Asserted **unconditionally** on purpose: an ``if hasattr(ctx, "set_groups")`` branch here would + restore exactly the property being removed — a test that cannot fail. + """ + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + pinned = harden_kex_groups(ctx) + assert pinned is None, ( + f"harden_kex_groups pinned {pinned!r}, so this interpreter HAS a group-list API and the ASVS " + f"11.6.2 residual has CHANGED. This is good news, not a bug. Re-derive, in one change: the " + f"harden_kex_groups docstring, docs/PHI.md §4, docs/ASVS-L2-PHASE0-CHANGES.md, the ADR 0092 " + f"§4(b) amendment and the 11.6.2 register row — then rewrite this test to assert the pin TOOK, " + f"via ctx.get_groups(), which lands in the same Python version." + ) + assert not hasattr(ctx, "set_groups"), ( + "set_groups EXISTS but harden_kex_groups still returned None, so the pin is failing silently — " + "worse than being unavailable, because the docs would read as satisfied. See the " + "logger.warning path." + ) + + +def test_harden_reports_none_without_set_groups() -> None: + # An object lacking the API must be handled gracefully AND report that nothing was pinned. fake = types.SimpleNamespace() - harden_kex_groups(fake) # type: ignore[arg-type] + assert harden_kex_groups(fake) is None # type: ignore[arg-type] + + +def test_harden_reports_the_list_it_pinned_when_the_api_exists() -> None: + """The return value must be REAL, not incidentally-``None``. + + ``test_the_group_pin_is_inert_on_this_runtime_and_says_so`` asserts ``is None`` — which a function + with no ``return`` statement at all also satisfies. On its own it therefore does NOT prove the + reporting works: reverting this helper to its old ``-> None`` signature would leave it green. Drive + the pinning path with a stand-in context that *has* ``set_groups`` so both halves of the contract + are covered, and assert the group list actually reached it. + """ + + class _Pinnable: + def __init__(self) -> None: + self.pinned: list[str] = [] + + def set_groups(self, grouplist: str) -> None: + self.pinned.append(grouplist) + + ctx = _Pinnable() + assert harden_kex_groups(ctx) == APPROVED_KEX_GROUPS # type: ignore[arg-type] + assert ctx.pinned == [APPROVED_KEX_GROUPS], "the group list never reached set_groups" + + +def test_a_pin_that_raises_reports_nothing_pinned() -> None: + """An OpenSSL build that REJECTS the group list must report ``None``, not the list. + + This was the shipped bug: the helper logged the warning and then fell off the end, so the caller + could not distinguish "pinned" from "tried and failed" — and once the helper started reporting, the + failure path returning the list would have been a lie with a warning line next to it. + """ + + class _Rejecting: + def set_groups(self, grouplist: str) -> None: + raise ValueError("this OpenSSL build rejects the group list") + + assert harden_kex_groups(_Rejecting()) is None # type: ignore[arg-type] def test_approved_groups_are_ecdhe_curves() -> None: assert APPROVED_KEX_GROUPS.split(":") == ["X25519", "secp384r1", "secp256r1"] + # NB `secp256r1` is a valid OpenSSL group-list alias but NOT a valid EC curve name — that spelling + # is `prime256v1`, and set_ecdh_curve("secp256r1") raises ValueError. Both are correct in their own + # API; do not "normalise" them to one. # --- harden_verify_flags -----------------------------------------------------------------------