diff --git a/docs/ASVS-L2-PHASE0-CHANGES.md b/docs/ASVS-L2-PHASE0-CHANGES.md index 6c8898e13..3f6130e39 100644 --- a/docs/ASVS-L2-PHASE0-CHANGES.md +++ b/docs/ASVS-L2-PHASE0-CHANGES.md @@ -446,8 +446,8 @@ tables in [`CONNECTIONS.md`](CONNECTIONS.md) §"Resource management & limits" (A | SMART Backend Services token endpoint (ADR 0024) | outbound | HTTPS POST to the operator-pinned token URL (no discovery); routed through the connection's forward proxy when one resolves for the **token** host | the REST verifying no-redirect opener; a cleartext-`http` token URL is **refused** unless `MEFOR_ALLOW_INSECURE_TLS` (the client assertion is a credential) | a signed `client_assertion` JWT (`smart_private_key`, optionally passphrase-protected) | **yes** — `smart_token_url` per Connection | `smart_token_url`, `smart_client_id`, `smart_private_key`, `smart_scope`, `smart_timeout_seconds`, `smart_expiry_skew_seconds` | | Generic OAuth2 client-credentials token endpoint | outbound | HTTPS POST for the non-SMART REST/SOAP/FHIR/DICOMweb bearer path; also proxy-routed per token host | the same verifying no-redirect opener; a cleartext-`http` credential hop is posture-keyed and refused | `oauth2_client_id` + `oauth2_client_secret`, sent `basic` or `post` per `oauth2_auth_style` | **yes** — `oauth2_token_url` per Connection | `oauth2_token_url`, `oauth2_client_id`, `oauth2_client_secret`, `oauth2_scope`, `oauth2_auth_style`, `oauth2_timeout_seconds` | | Engine-brokered AI assistance (ADR 0135) | outbound | HTTPS POST of a `code_only` assist prompt to a **customer-managed / self-hosted** LLM endpoint; runs off the event loop | the REST verifying no-redirect opener; a cleartext-`http` endpoint carrying the key is refused unless `MEFOR_ALLOW_INSECURE_TLS` | `MEFOR_AI_API_KEY`, sent as the `x-api-key` header | **yes** — verbatim the requirement's "the end user provides an external location" | `[ai].endpoint`, `[ai].api_key`, `[ai].allowed_endpoints` (a **dedicated fail-closed** SSRF allowlist — an EMPTY list refuses everything; deliberately **not** `[egress].allowed_http`), `[ai].provider`, `[ai].model` | -| HashiCorp Vault Transit — store DEK envelope-decrypt (ADR 0019) | outbound | HTTPS via `hvac` (the `[vault]` extra); port from the address | TLS verification is `hvac`/`requests`' own default — the engine sets no explicit client TLS options here | a Vault token — `MEFOR_STORE_VAULT_TOKEN` (`hvac` falls back to `VAULT_TOKEN` when unset) | **yes** — `MEFOR_STORE_VAULT_ADDR` (opt-in; fail-closed) | `MEFOR_STORE_VAULT_ADDR`, `MEFOR_STORE_VAULT_TOKEN`, `MEFOR_STORE_VAULT_TRANSIT_KEY` | -| HashiCorp Vault Transit — **bulk at-rest cipher** (`[store].cipher_provider = vault_transit`, ADR 0138) | outbound (**per store operation**) | HTTPS via the same shared `hvac` client build; port from the address. **One `encrypt_data` / `decrypt_data` round trip per encrypted CELL** on every store write and read, plus one `generate_hmac` per audit row — not a startup-only hop | as the DEK hop: `hvac`/`requests` defaults, no engine-set client TLS options | the same Vault token (`MEFOR_STORE_VAULT_TOKEN`) | **yes** — `MEFOR_STORE_VAULT_ADDR` (shared with the DEK hop) | `[store].cipher_provider`, `MEFOR_STORE_TRANSIT_KEY`, `MEFOR_STORE_TRANSIT_AUDIT_KEY` | +| HashiCorp Vault Transit — store DEK envelope-decrypt (ADR 0019) | outbound | HTTPS via `hvac` (the `[vault]` extra); port from the address | TLS verification defaults to `hvac`/`requests`' own **public** bundle. `MEFOR_STORE_VAULT_CA_FILE` (BACKLOG #1180, ASVS 12.3.4) narrows it to one internal CA, resolved through the same `resolve_trust_anchor` every other outbound hop uses; unset, the client is constructed exactly as before | a Vault token — `MEFOR_STORE_VAULT_TOKEN` (`hvac` falls back to `VAULT_TOKEN` when unset) | **yes** — `MEFOR_STORE_VAULT_ADDR` (opt-in; fail-closed) | `MEFOR_STORE_VAULT_ADDR`, `MEFOR_STORE_VAULT_TOKEN`, `MEFOR_STORE_VAULT_TRANSIT_KEY`, `MEFOR_STORE_VAULT_CA_FILE` | +| HashiCorp Vault Transit — **bulk at-rest cipher** (`[store].cipher_provider = vault_transit`, ADR 0138) | outbound (**per store operation**) | HTTPS via the same shared `hvac` client build; port from the address. **One `encrypt_data` / `decrypt_data` round trip per encrypted CELL** on every store write and read, plus one `generate_hmac` per audit row — not a startup-only hop | as the DEK hop — same shared client build, so `MEFOR_STORE_VAULT_CA_FILE` narrows this hop too | the same Vault token (`MEFOR_STORE_VAULT_TOKEN`) | **yes** — `MEFOR_STORE_VAULT_ADDR` (shared with the DEK hop) | `[store].cipher_provider`, `MEFOR_STORE_TRANSIT_KEY`, `MEFOR_STORE_TRANSIT_AUDIT_KEY` | | DR backup destination (ADR 0049) | outbound (scheduled + on-demand) | local filesystem, or **SMB/CIFS over TCP when `[backup].destination` is a UNC path** (the OS redirector owns the port); a cloud URL is **rejected at load** | n/a — no engine-terminated TLS on this hop; SMB dialect security is the OS's | the engine service account's **own** identity — `[backup]` exposes no `credential_*` impersonation knob, unlike the FILE connector | **yes** — `[backup].destination` | `[backup].enabled`, `[backup].destination`, `schedule_at`, `retention_keep`, `snapshot_method`, `allow_unencrypted` | | Security-event notification email, per user | outbound | SMTP through the **same** `[alerts]` transport and default port 587, but a **second, independent** background dispatcher — its own 1000-item queue and its own drain task — mailing each affected USER's own address, not the operator `email_to` list | STARTTLS **and certificate verification**, as the operator sink — plumbed at this call site in its own right (`pipeline/security_notify.py`), not inherited implicitly | as the operator sink | **yes** — the same `[alerts].email_smtp_host` | `[auth].notify_security_events`, `[alerts].email_*` | | HashiCorp Vault KV v2 — connector-credential secrets provider (ADR 0019) | outbound | HTTPS via `hvac`, a **separate client** from the Transit one behind the same extra | as above | a Vault token — `MEFOR_SECRETS_VAULT_TOKEN` (falls back to `VAULT_TOKEN`) | **yes** — `MEFOR_SECRETS_VAULT_ADDR` (opt-in; fail-closed) | `MEFOR_SECRETS_VAULT_ADDR`, `MEFOR_SECRETS_VAULT_TOKEN`, `[secrets].provider` | diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 55cc19edd..550a5e8ce 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -10637,6 +10637,16 @@ Nothing here touches the TLS/FTPS context in the same module, which was already **Still not an honest pass:** the item's two traps hold -- shipping `trust_anchor_mode = "pinned"` as the default is refused at config load without an internal CA (`config/settings.py:1025-1031`) so every stock configuration would fail to start, and arguing that a hospital's OS trust store IS the enterprise CA is a property of the operator's image build laundered into a product control. The sharpest trap is a third the item does not name: the cell's own re-score trigger says to watch for the anchor resolver or a CA parameter appearing on those HTTP factories, so **the expressibility work trips that trigger by construction, and if it lands and the cell is re-scored while every default is still permissive, that is wiring an existing toolkit into the pipeline so an absence claim stops firing -- wearing the hat of a trigger the cell wrote for itself.** Expressibility must never be scored without the forcing control. Two more, both reachable from the record: re-scoring on the discovery that the generic ODBC dialect passes operator keywords through (a newly FOUND opt-in capability is not a newly BUILT control, and that dialect is not the default), and re-scoring on the syslog hop (one hop of roughly a dozen, reached only by opting into a non-default protocol). Proposed work, unallocated and by subject: the HTTP-egress trust-anchor threading across ALL FOUR factories that lack a certificate parameter, including the lookup executor's own opener map and the separate webhook opener; the Vault client CA argument and scheme gate, with the honest note that it needs new settings and a posture handle on a lazy environment-fed path rather than reusing the shipped ladder; the FTPS factory's lost CA and verification parameters; the DATABASE default-dialect server-certificate parity fix, which the store code already emits against the identical driver; the tray probe CA pin; the IDE certificate-authority setting AND a separate IDE-side forcing rule with its own publicly-certified-peer arm, since no engine-side gate can reach a separate process; the anchor-discipline posture gate, owner-gated and specified against a public-chain-probe predicate wherever measurement is possible with the declaration arm confined to the ODBC and Schannel hops; posture reporting of an unnarrowed internal hop as a first-class loosening; an explicit disposition for the augmenting trust mode, which keeps the OS roots and so does not satisfy the word ONLY; an ECH-sidecar egress ruling rather than leaving it in the unknowns while a pass is claimed; and extending the anchor-integrity preflight beyond the three auth-path anchors. +**Progress 2026-09-04 (builder). THE EXPRESSIBILITY LIMB IS SHIPPED. The default flip and the posture ruling stay open, and this item stays open with them.** Re-measured by execution at HEAD `a2eef0f37`, with a positive control in the same run so an all-negative probe could not pass for a clean repo: `resolve_trust_anchor` was absent from `transports/rest.py`, `soap.py`, `fhir.py` and `dicomweb.py` on all four probes (imported symbol, source mention, context builder, policy plumbing) while `mllp.py`, `dicom.py` and `remotefile.py` returned true on all four; and a recording fake `hvac` showed both `_build_client` sites passing exactly `{url, token, allow_redirects}` and no CA-bearing keyword, with the recorder demonstrably working because it saw those three. Both stated shortfalls reproduce. + +**What landed.** One construction point, `build_anchored_https_handler` in `config/tls_policy.py`, now builds every HTTP-family https handler, so the call sites cannot drift. A hop with no CA gets `build_asserted_https_handler` verbatim -- urllib's own context, asserted in place. `augment` loads the internal CA into that same context, so nothing is rebuilt and urllib's ALPN and post-handshake auth are untouched. Only `pinned` (and a per-connection CA, the same shape) substitutes a context, because it must trust ONLY the internal CA and an `SSLContext` cannot unload roots urllib has already loaded; that one arm replays urllib's two measured deltas so pinning a hop does not silently change its handshake. It is reached by `RestDestination`, `SoapDestination` (including the mutual-TLS opener, which builds its own context), `FhirDestination`, `DicomWebDestination`, and the `fhir_lookup` executor's per-connection opener map -- the last has no `Destination` to carry the policy, so `RegistryRunner` threads it in explicitly at both the live and the `build_check` site. Both hvac hops take `MEFOR_STORE_VAULT_CA_FILE` / `MEFOR_SECRETS_VAULT_CA_FILE` through the same resolver, mapped onto `requests`' single `verify=` bundle path; `augment` is REFUSED there rather than silently narrowed, because one path cannot say "public roots plus this CA". **The default is unchanged and it is asserted, not asserted-about:** a stock https destination is still handed the shared `_NO_REDIRECT_OPENER` **by object identity**, and a hop takes a per-connection opener only when it carries an extra handler or an anchor that narrows. Twelve mutations were run against the new assertions and all twelve go red, including two negative controls -- one making an unanchored hop substitute a context, one giving every hop a per-connection opener. + +**Two of this item's own claims are corrected by the work.** The severity clause says the Vault hop "would be verified against the full OS store"; the 2026-08-20 research already corrected that to a fail-closed public-bundle hop, and the shipped code confirms it -- `requests` defaults to certifi, not the OS store, so what was missing there was the ability to reach an internal-CA Vault **at all**. And the per-connection `tls_ca_file` this work reads is still unreachable from either authoring surface on these five factories, exactly as this item's own factory probe found for `Ftp()`: precedence rule 1 of `resolve_trust_anchor` is live code on an unspeakable key until those signatures gain the parameter. + +**Measured and deliberately NOT done, so nobody reads this as the limb closed.** (1) The `tls_ca_file` parameter on `Rest()`, `Soap()`, `FHIR()`, `DICOMweb()` and `FhirLookup()` -- the connection-scoped half of the same shortfall; the instance `[tls]` anchor is what this pass made expressible. (2) **The SMART token-endpoint hop**, `transports/smart.py`, built from inside the very `FhirDestination.__init__` this pass changed: in `pinned` mode a FHIR destination now anchors its data hop while the hop carrying its `client_assertion` still verifies against the OS store. It is not a one-line thread-through -- the anchor must be resolved for the TOKEN host, which has its own loopback and proxy decisions (ADR 0126 already splits it), so it needs the policy passed into `token_provider_from_settings`, not the destination's resolved anchor. (3) The alert webhook opener in `pipeline/alert_sinks.py`, whose `notifier_from_settings` already receives a `trust_anchor_policy` and threads it to `EmailTransport` only. (4) `[tls].internal_ca_file` reaching the two Vault hops: neither provider has that section in scope -- they hold a `StoreSettings` / `SecretsSettings` and build from the environment -- so it needs threading through `resolve_key_provider` / `resolve_secret_provider`, which is the "new settings" this item's research already named. (5) A per-anchor context cache: with an internal CA set, each anchored lane builds its own `SSLContext` at construction, and `build_check` builds a second one it discards. Startup-only, and caching a mutable `SSLContext` across connectors is its own hazard, so it was left alone rather than done quietly. + +**Neither trap was touched and the third one holds.** `trust_anchor_mode` still defaults to `"system"`; nothing was flipped, and the pinned-without-a-CA validator is untouched. The refuse-versus-fall-back posture decision is an owner ruling and is not built. **And this item's own sharpest warning applies to this commit by construction:** the cell's re-score trigger watches for the anchor resolver or a CA parameter appearing on these HTTP factories, and this work fires that trigger deliberately. **Expressibility is not a pass. Do not re-score 12.3.4 on this alone** -- every default is still permissive, and scoring it here would be wiring an existing toolkit into the pipeline so an absence claim stops firing, which is exactly what the record says not to do. + ## 1181. research an honest pass for ASVS 12.3.5 -- intra-service endpoint authentication on an engine whose coordination is store-mediated > 🔢 **Re-scored 2026-08-20 -> P2.** Value **6/10** · Difficulty **7/10** · _big bet_. The setting is still attestation-only: its sole consumer at settings.py:850 collapses four values to a boolean, so a declared "mtls" changes nothing. Value 6 rather than 7 because the hop the item's own severity names is genuinely closable today -- setting [api].tls_client_ca_file makes the API listener CERT_REQUIRED-verify the proxy's client cert (api/tls.py:58-62) -- so the gap is an unverified declaration beside a working control rather than an unavailable one; difficulty 7 because an honest pass must decide whether an attestation-only setting should exist at all, whether mutual PKI is reachable for the store and Vault hops given the drivers in use, and whether the IDE, tray and apiclient fall inside the requirement's scope. _(was 7/10 · 7/10.)_ diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 5a2b79129..6f19fe651 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1175,7 +1175,11 @@ A provider is consulted **only** for a credential whose per-credential `*_secret store password is seam-only (managed identity is preferred there). A reference is `""` or `"#"` for `vault` (field defaults to `value`; KV mount from `MEFOR_SECRETS_VAULT_KV_MOUNT`, default `secret`); Vault address/token come from `MEFOR_SECRETS_VAULT_ADDR` / `MEFOR_SECRETS_VAULT_TOKEN` -(falling back to hvac's `VAULT_ADDR` / `VAULT_TOKEN`). **Fail-closed:** a reference with `provider = none`, +(falling back to hvac's `VAULT_ADDR` / `VAULT_TOKEN`). Point `MEFOR_SECRETS_VAULT_CA_FILE` at the PEM of +the CA that issued your Vault server's certificate to verify that hop against your own PKI instead of the +public bundle `requests` ships with (BACKLOG #1180; the store KeyProvider's twin is +`MEFOR_STORE_VAULT_CA_FILE`) — a path, not a secret, and unset leaves the hop exactly as it was. +**Fail-closed:** a reference with `provider = none`, an unknown provider, a missing `[vault]` extra, or an unresolvable/empty secret raises at load/connect — never a blank credential; the value is never logged. diff --git a/messagefoundry/config/secretprovider_vault.py b/messagefoundry/config/secretprovider_vault.py index ffabb92d8..d83e4a7e3 100644 --- a/messagefoundry/config/secretprovider_vault.py +++ b/messagefoundry/config/secretprovider_vault.py @@ -32,6 +32,7 @@ from typing import TYPE_CHECKING, Any from messagefoundry.config.secretprovider import SecretProviderError +from messagefoundry.config.tls_policy import vault_client_verify_kwargs if TYPE_CHECKING: from messagefoundry.config.settings import SecretsSettings @@ -47,6 +48,13 @@ _ENV_TOKEN = "MEFOR_SECRETS_VAULT_TOKEN" # nosec B105 — the env-var NAME, not a token value #: KV v2 mount point the connector secrets live under (Vault's conventional default is ``secret``). _ENV_KV_MOUNT = "MEFOR_SECRETS_VAULT_KV_MOUNT" +#: PEM path to the CA that issued the Vault server's certificate (#1180, ASVS 12.3.4). A PATH, not a +#: secret. The twin of the store KeyProvider's ``MEFOR_STORE_VAULT_CA_FILE``, kept separate for the +#: same reason the address and token are: the two providers may point at different Vaults. Unset = +#: hvac's own default, which is ``requests``' PUBLIC certifi bundle. +#: (:func:`~messagefoundry.config.tls_policy.vault_client_verify_kwargs` records why ``[tls]`` does +#: not reach this hop yet.) +_ENV_CA_FILE = "MEFOR_SECRETS_VAULT_CA_FILE" #: Default field read from a KV secret when a reference omits ``#``. _DEFAULT_FIELD = "value" @@ -67,6 +75,26 @@ def _import_hvac() -> Any: return hvac +def _vault_ca_kwargs(addr: str | None) -> dict[str, str]: + """This Vault hop's ``verify=`` keyword arguments — ``{}`` when no anchor is configured. + + #1180 (ASVS 12.3.4) — the twin of ``store/keyprovider_vault.py``'s, sharing + :func:`~messagefoundry.config.tls_policy.vault_client_verify_kwargs`. This client reads connector + credentials out of Vault KV, so the anchor that verifies the server is the only thing standing + between a spoofed Vault and every partner credential the engine holds. + + Fails closed here rather than in the shared helper, because the error type and cell name are this + module's: ``requests`` would otherwise raise deep inside the first KV read, surfacing as an opaque + resolution failure that names no cause.""" + ca = os.environ.get(_ENV_CA_FILE) or None + if ca is not None and not os.path.isfile(ca): + raise SecretProviderError( + f"[secrets].provider={_EXTRA!r}: {_ENV_CA_FILE} names {ca!r}, which is not a readable " + f"file — point it at the PEM of the CA that issued the Vault server certificate." + ) + return vault_client_verify_kwargs(ca_file=ca, addr=addr, cell=f"[secrets].provider={_EXTRA!r}") + + def _build_client(addr: str | None, token: str | None) -> Any: """Construct an ``hvac.Client``. Factored out so tests can substitute a fake KV backend without a live Vault. ``addr``/``token`` pass through; when ``None``, hvac falls back to its own VAULT_ADDR/VAULT_TOKEN @@ -77,7 +105,11 @@ def _build_client(addr: str | None, token: str | None) -> Any: # egress does. `token` rides as an `X-Vault-Token` header on every KV read, so a 3xx from an # on-path attacker (absent TLS integrity) or a spoofed Vault would otherwise relocate the # request carrying it. See store/keyprovider_vault.py's twin for the measurement. - client: Any = hvac.Client(url=addr, token=token, allow_redirects=False) + # #1180 (ASVS 12.3.4): narrow the trust anchor when the operator named one. The keyword is OMITTED + # when they did not, so the stock construction is unchanged rather than passed an explicit default. + client: Any = hvac.Client( + url=addr, token=token, allow_redirects=False, **_vault_ca_kwargs(addr) + ) return client diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index 500dfad19..56ff4cf99 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -35,6 +35,7 @@ import os import ssl import time +import urllib.parse import urllib.request from collections.abc import Callable, Iterator, Mapping from contextlib import contextmanager @@ -66,6 +67,8 @@ "active_hop_posture", "APPROVED_SMTP_AUTH_MECHANISMS", "assert_ldap3_tls_suites", + "urllib_handler_context", + "build_anchored_https_handler", "build_asserted_https_handler", "build_smtp_tls_context", "smtp_login_approved", @@ -78,6 +81,9 @@ "harden_verify_flags", "kex_groups_report", "relax_verify_expiry", + "requests_verify_from_anchor", + "vault_client_verify_kwargs", + "SYSTEM_TRUST_ANCHOR", "in_process_tls_revocation_refused", "insecure_hop_disposition", "is_loopback_hop_host", @@ -602,15 +608,28 @@ def build_asserted_https_handler(*, connector: str) -> urllib.request.HTTPSHandl same class ``build_opener`` would have instantiated itself, built the same way, and supplying an instance only stops urllib adding a second one. + ``connector`` is the operator-recognisable label :func:`harden_cipher_suites` names in its error. + Lives here rather than beside each opener so the two call sites (the HTTP-family destinations and + the alert webhook) cannot drift onto different constructions. The guarded read of urllib's private + context lives in :func:`urllib_handler_context`.""" + handler = urllib.request.HTTPSHandler() + harden_cipher_suites(urllib_handler_context(handler, connector=connector), connector=connector) + return handler + + +def urllib_handler_context( + handler: urllib.request.HTTPSHandler, *, connector: str +) -> ssl.SSLContext: + """The :class:`ssl.SSLContext` ``handler`` built for itself, or refuse. + Reads the handler's private ``_context`` deliberately, and **fails closed** if it is not there. A ``getattr(..., None)`` that shrugged and returned would be a security control reporting success forever — exactly the failure :func:`harden_kex_groups` documents. A CPython that renames the attribute must break loudly at construction, not go quiet. - ``connector`` is the operator-recognisable label :func:`harden_cipher_suites` names in its error. - Lives here rather than beside each opener so the two call sites (the HTTP-family destinations and - the alert webhook) cannot drift onto different constructions.""" - handler = urllib.request.HTTPSHandler() + One function so the two readers of that private attribute — the forward-secrecy assertion above + and :func:`build_anchored_https_handler`'s ``augment`` arm, which loads an extra root into the + same context — cannot drift onto different guards.""" ctx = getattr(handler, "_context", None) if not isinstance(ctx, ssl.SSLContext): raise ValueError( @@ -618,8 +637,7 @@ def build_asserted_https_handler(*, connector: str) -> urllib.request.HTTPSHandl f"(no `_context` attribute on this runtime), so the forward-secrecy assertion " f"(ASVS 12.1.2) cannot run on this hop. Refusing rather than crossing unchecked." ) - harden_cipher_suites(ctx, connector=connector) - return handler + return ctx #: The ``ldap3.Tls`` keyword arguments :func:`assert_ldap3_tls_suites` can faithfully replicate. @@ -1190,6 +1208,22 @@ class TrustAnchor: cafile: str | None load_system_roots: bool + @property + def narrows(self) -> bool: + """Whether this anchor names a CA of its own, i.e. whether it changes anything. + + The one predicate a caller needs: an anchor that names no CA resolves to the OS trust store, + which is what every hop had before an anchor was resolvable at all. Callers that must choose + between a shared, unanchored client and a per-connection one read THIS rather than spelling + out the ``cafile is not None`` test, so the five HTTP-family call sites cannot drift apart.""" + return self.cafile is not None + + +#: The anchor a hop that configures nothing resolves to: the OS trust store, no private CA. The +#: default for every ``trust_anchor`` parameter, so "no anchor" and "an anchor that narrows nothing" +#: are ONE value rather than two spellings of it (``None`` used to be a second). +SYSTEM_TRUST_ANCHOR = TrustAnchor(cafile=None, load_system_roots=True) + def resolve_trust_anchor( *, @@ -1247,6 +1281,116 @@ def build_verifying_client_context( return ssl.create_default_context(purpose, cafile=anchor.cafile) +#: urllib's ALPN advertisement — see :func:`build_anchored_https_handler` for why it is replayed. +_URLLIB_HTTPS_ALPN_PROTOCOLS = ["http/1.1"] + + +def build_anchored_https_handler( + *, anchor: TrustAnchor, connector: str +) -> urllib.request.HTTPSHandler: + """The HTTP-family https handler for a hop whose client trust anchor is ``anchor`` (#1180, ADR 0093). + + The HTTP egress family (REST / SOAP / FHIR / DICOMweb / the ``fhir_lookup`` read path) exposed only + a ``verify_tls`` boolean, so :func:`resolve_trust_anchor` could not be *spoken* there at all: an + operator who set ``[tls].internal_ca_file`` had it honoured on the MLLP / DICOM / FTPS hops and + silently ignored on every https one. This is the single construction point that closes that, so the + call sites cannot drift onto different constructions. + + **An anchor that narrows nothing changes nothing, and that is the point.** A hop with no internal + CA — ``system`` mode, a loopback hop, or a connection that named no CA of its own — returns + :func:`build_asserted_https_handler` verbatim, i.e. urllib's OWN context, asserted in place. Nothing + about a stock hop's handshake moves. **This is the one place that claim is made; the call sites do + not restate it.** + + **``augment`` keeps urllib's own context too**, and simply loads the internal CA into it — adding + a root needs no new context, so that arm changes the trust store and nothing else. + + Only ``pinned`` (and a per-connection CA, which is the same shape) substitutes a context, and it + substitutes for a measured reason rather than a stylistic one: it must trust ONLY the internal CA, + and an :class:`ssl.SSLContext` cannot unload the default roots urllib's context has already + loaded. That one arm gets :func:`build_verifying_client_context` plus the two deltas urllib + applies over ``create_default_context`` (``set_alpn_protocols(["http/1.1"])`` and + ``post_handshake_auth``, measured on CPython 3.14.6 in :func:`build_asserted_https_handler`) — + otherwise pinning a hop would quietly drop its ALPN advertisement and post-handshake auth, which + is exactly the silent handshake change that function was written to avoid.""" + if not anchor.narrows: + return build_asserted_https_handler(connector=connector) + if anchor.load_system_roots: + # augment: urllib's OWN context, with the internal CA loaded on top. Nothing is replayed + # because nothing is rebuilt — the only change is one more trusted root. + handler = build_asserted_https_handler(connector=connector) + urllib_handler_context(handler, connector=connector).load_verify_locations( + cafile=anchor.cafile + ) + return handler + ctx = build_verifying_client_context(anchor) + ctx.set_alpn_protocols(_URLLIB_HTTPS_ALPN_PROTOCOLS) + if ctx.post_handshake_auth is not None: # urllib guards it the same way + ctx.post_handshake_auth = True + harden_cipher_suites(ctx, connector=connector) # assert forward secrecy (ASVS 12.1.2) + return urllib.request.HTTPSHandler(context=ctx) + + +def requests_verify_from_anchor(anchor: TrustAnchor, *, cell: str) -> str | None: + """The ``verify=`` argument a ``requests``-based client needs to honour ``anchor`` (#1180). + + For the two ``hvac`` clients, which ride ``requests`` rather than stdlib ``urllib``. ``requests`` + takes ONE bundle path and trusts only what that path holds, so the mapping is exact for two of the + three anchor shapes and impossible for the third: + + * no CA (``system``, no internal CA, loopback) → ``None``, meaning **pass nothing**. The caller + omits the keyword entirely, so the client is constructed exactly as it was. + * a pinned / per-connection CA → that path. ``requests`` then trusts ONLY it, which is precisely + what ``load_system_roots=False`` asks for. + * ``augment`` (OS roots **plus** the internal CA) → **refused**, because a single ``verify=`` path + cannot say it. Silently passing the path would narrow a hop the operator asked to widen, and + silently dropping it would ignore the anchor — the failure this whole item is about. So it says + so instead. + + Worth stating plainly, because the direction is counter-intuitive: this hop is not one of the + broadly-trusting ones. ``requests`` defaults to the PUBLIC certifi bundle, not the OS store, so an + internal-CA Vault fails closed today rather than being widely trusted. What was missing here is + the ability to reach such a Vault at all.""" + if not anchor.narrows: + return None + if anchor.load_system_roots: + raise ValueError( + f"{cell}: trust_anchor_mode='augment' (OS roots plus an internal CA) cannot be expressed " + f"to a requests-based client, which trusts exactly one bundle path. Use a pinned anchor " + f"(a CA file naming the issuer for this hop) or leave the anchor unset." + ) + return anchor.cafile + + +def vault_client_verify_kwargs( + *, ca_file: str | None, addr: str | None, cell: str +) -> dict[str, str]: + """The ``verify=`` keyword arguments an ``hvac.Client`` needs for a Vault hop (#1180). + + Returns ``{}`` when nothing is configured — the caller splats it, so the client is constructed + with exactly the arguments it carried before rather than an explicit default — and + ``{"verify": }`` when an anchor narrows the hop. + + Shared by ``config/secretprovider_vault.py`` and ``store/keyprovider_vault.py``, which resolve the + same anchor for two different Vaults; the fail-closed check on ``ca_file`` stays with each caller, + because each has its own error type and its own operator-facing cell name. + + The policy is the default :class:`TrustAnchorPolicy` today, so the anchor reduces to whatever + ``ca_file`` names. That is a real limit and not an oversight: neither provider has the instance + ``[tls]`` section in scope — they hold a ``StoreSettings`` / ``SecretsSettings`` and are built + from the environment — so making ``[tls].internal_ca_file`` reach these two hops means threading + the policy through ``resolve_key_provider`` / ``resolve_secret_provider``, which is a separate + plumbing change. Routing through :func:`resolve_trust_anchor` anyway means that change moves one + argument rather than rewriting the hop.""" + anchor = resolve_trust_anchor( + connection_ca_file=ca_file, + host=urllib.parse.urlsplit(addr or "").hostname or "", + policy=TrustAnchorPolicy(), + ) + verify = requests_verify_from_anchor(anchor, cell=cell) + return {} if verify is None else {"verify": verify} + + def build_smtp_tls_context( *, host: str, diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index fcd806b95..e469919d6 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -1564,7 +1564,10 @@ def _build_fhir_lookup_executor(self) -> FhirLookupExecutor | None: # the executor here outside build_check_registry's active_hop_posture scope, so an unstamped build # would either fail-closed a legit dev read or (via the send-time re-assertion) mis-key the hop. with active_hop_posture(self._hop_posture): - return FhirLookupExecutor(resolved) + # #1180 (ADR 0093): a FhirLookup connection has no Destination to carry the instance + # [tls] anchor policy, so it is threaded explicitly here — the sanctioned live read + # against an internal FHIR server could otherwise never name an internal CA. + return FhirLookupExecutor(resolved, trust_anchor_policy=self._trust_anchor_policy) def _run_fhir_lookup( self, @@ -6492,7 +6495,8 @@ def _build_check_connectors( resolved_fhir_lookups[fname] = fsettings if resolved_fhir_lookups: # Construct (and discard): validates each FHIR URL/TLS/SMART-auth without issuing a read. - FhirLookupExecutor(resolved_fhir_lookups) + # #1180: the same anchor policy the live build uses, so build_check resolves what serve does. + FhirLookupExecutor(resolved_fhir_lookups, trust_anchor_policy=trust_anchor_policy) def check_pt_backend_supported(registry: Registry, store: QueueStore) -> None: diff --git a/messagefoundry/store/keyprovider_vault.py b/messagefoundry/store/keyprovider_vault.py index e7737bb47..53ccf4ace 100644 --- a/messagefoundry/store/keyprovider_vault.py +++ b/messagefoundry/store/keyprovider_vault.py @@ -32,6 +32,7 @@ from collections.abc import Sequence from typing import TYPE_CHECKING, Any +from messagefoundry.config.tls_policy import vault_client_verify_kwargs from messagefoundry.store.keyprovider import KeyProviderError, _split_retired if TYPE_CHECKING: @@ -52,6 +53,12 @@ #: The wrapped DEK ciphertext (``vault:v1:…``). Not itself a secret (it is KEK-encrypted), but supplied #: via env alongside the token so a deployment keeps all Vault wiring in one place. _ENV_WRAPPED_DEK = "MEFOR_STORE_VAULT_WRAPPED_DEK" +#: PEM path to the CA that issued the Vault server's certificate (#1180, ASVS 12.3.4). A PATH, not a +#: secret — the same status as ``[tls].internal_ca_file`` and ``tls_cert_file``. Env-fed, beside the +#: address and token it belongs with; ``[tls].internal_ca_file`` cannot reach this hop yet, and +#: :func:`~messagefoundry.config.tls_policy.vault_client_verify_kwargs` records what that would take. +#: Unset = hvac's own default, which is ``requests``' PUBLIC certifi bundle. +_ENV_CA_FILE = "MEFOR_STORE_VAULT_CA_FILE" def _import_hvac() -> Any: @@ -70,6 +77,30 @@ def _import_hvac() -> Any: return hvac +def _vault_ca_kwargs(addr: str | None) -> dict[str, str]: + """This Vault hop's ``verify=`` keyword arguments — ``{}`` when no anchor is configured. + + #1180 (ASVS 12.3.4): the client that hands out the store's data-encryption key took no CA argument + at all, so an operator running Vault behind their own PKI could not say so. The env-supplied CA is + this hop's own anchor and so wins verbatim — trust ONLY it, no public bundle — exactly as a + connection's ``tls_ca_file`` does; see + :func:`~messagefoundry.config.tls_policy.vault_client_verify_kwargs` for the shared resolution and + for what still has to be threaded before ``[tls].internal_ca_file`` can reach this hop. + + Fails closed here rather than there, because the error type and cell name are this module's: + ``requests`` would otherwise raise deep inside the first Transit call, surfacing as an opaque + store-open failure that names no cause.""" + ca = os.environ.get(_ENV_CA_FILE) or None + if ca is not None and not os.path.isfile(ca): + raise KeyProviderError( + f"[store].key_provider={_EXTRA!r}: {_ENV_CA_FILE} names {ca!r}, which is not a readable " + f"file — point it at the PEM of the CA that issued the Vault server certificate." + ) + return vault_client_verify_kwargs( + ca_file=ca, addr=addr, cell=f"[store].key_provider={_EXTRA!r}" + ) + + def _build_client(addr: str | None, token: str | None) -> Any: """Construct an ``hvac.Client``. Factored out so tests can substitute a fake Transit backend without a live Vault. ``addr``/``token`` are passed through; when ``None``, hvac falls back to its own @@ -94,7 +125,11 @@ def _build_client(addr: str | None, token: str | None) -> Any: # header on a same-host redirect. Measured against hvac 2.4.0: the kwarg lands on the adapter, # which passes it to requests.Session.request. Shared with crypto_transit.py, so the Transit # cipher inherits the policy from this one construction point. - client: Any = hvac.Client(url=addr, token=token, allow_redirects=False) + # #1180 (ASVS 12.3.4): narrow the trust anchor when the operator named one. The keyword is OMITTED + # when they did not, so the stock construction is unchanged rather than passed an explicit default. + client: Any = hvac.Client( + url=addr, token=token, allow_redirects=False, **_vault_ca_kwargs(addr) + ) return client diff --git a/messagefoundry/transports/dicomweb.py b/messagefoundry/transports/dicomweb.py index 54cd002f2..0003d789b 100644 --- a/messagefoundry/transports/dicomweb.py +++ b/messagefoundry/transports/dicomweb.py @@ -67,6 +67,7 @@ _redact_url, egress_route_from_settings, enforce_outbound_length_limits, + http_family_trust_anchor, refuse_cleartext_credentials, refuse_cleartext_egress, refuse_unrevoked_verified_hop, @@ -264,8 +265,14 @@ def __init__(self, config: Destination) -> None: connector="DICOMweb destination", revocation_attested=config.tls_revocation_attested, ) + # #1180 (ADR 0093): the client trust anchor for this STOW-RS hop. + anchor = http_family_trust_anchor( + s, url=self.base_url, trust_anchor_policy=config.trust_anchor_policy + ) self._opener: urllib.request.OpenerDirector = ( - _no_redirect_opener(*proxy_handlers) if proxy_handlers else _NO_REDIRECT_OPENER + _no_redirect_opener(*proxy_handlers, trust_anchor=anchor) + if proxy_handlers or anchor.narrows + else _NO_REDIRECT_OPENER ) else: # verify_tls=false makes the https hop MITM-able — a posture-keyed insecure hop (#200). diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index c6eb6e9ca..f79971f28 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -47,6 +47,7 @@ from typing import Any from messagefoundry.config.models import ConnectorType, Destination +from messagefoundry.config.tls_policy import TrustAnchorPolicy from messagefoundry.controlchars import has_control_char from messagefoundry.parsing.fhir import FhirPeek, FhirPeekError from messagefoundry.transports.base import ( @@ -76,6 +77,7 @@ enforce_send_time_length_limits, enforce_signature_header_limits, find_outbound_length_violation, + http_family_trust_anchor, normalize_header_allowlist, outbound_headers_from_metadata, refuse_cleartext_credentials, @@ -418,13 +420,20 @@ def __init__(self, config: Destination) -> None: ) # #129 (ADR 0094): granular expiry-only relaxation — verify chain + hostname but tolerate an # expired FHIR-server cert (opt-in; default off = the shared verifying opener, byte-identical). + # #1180 (ADR 0093): the client trust anchor for this https hop. + anchor = http_family_trust_anchor( + s, url=self.base_url, trust_anchor_policy=config.trust_anchor_policy + ) if bool(s.get("tls_allow_expired", False)): self._opener: urllib.request.OpenerDirector = _expiry_relaxed_opener( - urllib.parse.urlsplit(self.base_url).hostname or "", *proxy_handlers + urllib.parse.urlsplit(self.base_url).hostname or "", + *proxy_handlers, + trust_anchor=anchor, ) - elif proxy_handlers: + elif proxy_handlers or anchor.narrows: # A forward proxy → a per-connection verifying opener carrying it (never the shared one). - self._opener = _no_redirect_opener(*proxy_handlers) + # A narrowed trust anchor needs its own opener for the same reason. + self._opener = _no_redirect_opener(*proxy_handlers, trust_anchor=anchor) else: self._opener = _NO_REDIRECT_OPENER else: @@ -838,8 +847,17 @@ class FhirLookupExecutor: ``OperationOutcome`` issue code, a redacted host) — never the returned body, the query's parameter values, or the SMART token.""" - def __init__(self, connections: Mapping[str, Mapping[str, Any]]) -> None: + def __init__( + self, + connections: Mapping[str, Mapping[str, Any]], + *, + trust_anchor_policy: TrustAnchorPolicy | None = None, + ) -> None: # connections: name -> already-env-resolved settings (the runner substitutes env() first). + # trust_anchor_policy (#1180, ADR 0093): the instance [tls] client anchor policy, threaded by + # the runner. A FhirLookup connection has no Destination to carry it (unlike every other + # HTTP-family hop), which is why this executor's opener map could not name an internal CA at + # all. `None` (a direct test build) = the OS trust store, byte-identical. from messagefoundry.transports.smart import token_provider_from_settings self._base: dict[str, str] = {} @@ -919,8 +937,16 @@ def __init__(self, connections: Mapping[str, Mapping[str, Any]]) -> None: self._headers[cname] = headers self._token[cname] = token if bool(s.get("verify_tls", True)): + # #1180 (ADR 0093): the sanctioned live read-only lookup against an internal FHIR + # server is the most on-point instance of 12.3.4's condition in the product, and it + # could not name an anchor at all. + lookup_anchor = http_family_trust_anchor( + s, url=url, trust_anchor_policy=trust_anchor_policy + ) self._opener[cname] = ( - _no_redirect_opener(*proxy_handlers) if proxy_handlers else _NO_REDIRECT_OPENER + _no_redirect_opener(*proxy_handlers, trust_anchor=lookup_anchor) + if proxy_handlers or lookup_anchor.narrows + else _NO_REDIRECT_OPENER ) else: # verify_tls=false makes the https hop MITM-able — a posture-keyed insecure hop (#200). diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index 7f9ef4e91..9c388d863 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -43,11 +43,15 @@ from messagefoundry.config.models import ConnectorType, Destination from messagefoundry.config.settings import hop_insecure_escape_downgrades from messagefoundry.config.tls_policy import ( + SYSTEM_TRUST_ANCHOR, HopDisposition, HopPosture, InsecureHopRefused, RevocationHopGuard, - build_asserted_https_handler, + TrustAnchor, + TrustAnchorPolicy, + build_anchored_https_handler, + build_verifying_client_context, cleartext_acceptance_audit_sink, current_hop_posture, enforce_insecure_hop, @@ -55,6 +59,7 @@ insecure_hop_disposition, is_loopback_hop_host, relax_verify_expiry, + resolve_trust_anchor, ) from messagefoundry.controlchars import strip_control_chars from messagefoundry.transports.base import ( @@ -80,6 +85,7 @@ "proxy_auth_handler_from_settings", "proxy_config_from_settings", "enforce_outbound_length_limits", + "http_family_trust_anchor", "normalize_header_allowlist", "outbound_headers_from_metadata", "refuse_cleartext_credential_hop", @@ -228,34 +234,56 @@ def redirect_request( return None -def _asserted_https_handler() -> urllib.request.HTTPSHandler: - """urllib's own default https handler for this connector family, with its context ASSERTED. +#: The operator-recognisable connector label the HTTP-family TLS assertions name in their errors. The +#: default REST / FHIR / DICOMweb / ``fhir_lookup`` opener names its ``HTTPSHandler`` rather than +#: leaving ``build_opener`` to fill one in, so ``harden_cipher_suites`` can run on the context that +#: handler carries — see :func:`~messagefoundry.config.tls_policy.build_asserted_https_handler`. +_HTTP_FAMILY_CELL = "HTTP-family destination (REST/FHIR/DICOMweb)" - ``build_opener(_NoRedirectHandler)`` alone leaves urllib to fill in the ``HTTPSHandler``, and the - context that handler builds is one the engine never names — so the default REST / FHIR / - DICOMweb / ``fhir_lookup`` egress path, which every HTTP-family destination falls back to unless - it needs a proxy, an escape or an expiry relaxation, had an inherited suite list that nothing - checked. Naming the handler here lets - :func:`~messagefoundry.config.tls_policy.harden_cipher_suites` run on the context it carries. - It does NOT substitute a context: urllib's own is asserted in place. See - :func:`~messagefoundry.config.tls_policy.build_asserted_https_handler` for the measurement that - forced that choice — a hand-built ``ssl.create_default_context()`` differs from urllib's on ALPN - and post-handshake auth, so passing one would have changed the handshake.""" - return build_asserted_https_handler(connector="HTTP-family destination (REST/FHIR/DICOMweb)") +def http_family_trust_anchor( + settings: Mapping[str, Any], + *, + url: str, + trust_anchor_policy: TrustAnchorPolicy | None, +) -> TrustAnchor: + """Resolve the client trust anchor for an https hop in the HTTP egress family (#1180, ADR 0093). + + Shared by ``RestDestination`` / ``SoapDestination`` / ``FhirDestination`` / ``DicomWebDestination`` + and the ``fhir_lookup`` executor's per-connection opener map, so all five resolve the SAME anchor + from the same two inputs, on the precedence :func:`resolve_trust_anchor` defines: the connection's + own ``tls_ca_file``, and the instance-wide ``[tls]`` policy behind it. + + Total by construction — a policy-less build (a direct test construction) resolves to + :data:`~messagefoundry.config.tls_policy.SYSTEM_TRUST_ANCHOR`, which is the same value the shipped + default resolves to, so callers have ONE "nothing configured" value to handle rather than two.""" + ca = settings.get("tls_ca_file") + return resolve_trust_anchor( + connection_ca_file=str(ca) if ca else None, + host=urllib.parse.urlsplit(url).hostname or "", + policy=trust_anchor_policy if trust_anchor_policy is not None else TrustAnchorPolicy(), + ) def _no_redirect_opener( *extra_handlers: urllib.request.BaseHandler, + trust_anchor: TrustAnchor = SYSTEM_TRUST_ANCHOR, ) -> urllib.request.OpenerDirector: """A PER-CONNECTION verifying, no-redirect opener carrying ``extra_handlers`` (a forward-proxy ``ProxyHandler``, a reactive ``ProxyDigestAuthHandler``, …) — used in place of the shared :data:`_NO_REDIRECT_OPENER` whenever a connection needs a handler the shared one lacks, so the shared opener is **never** mutated (ADR 0126). Passing a ``ProxyHandler`` here also suppresses urllib's default env-reading ProxyHandler (``build_opener`` skips a default whose class a supplied handler - already covers), so there is never a competing double-proxy.""" + already covers), so there is never a competing double-proxy. + + ``trust_anchor`` (#1180) narrows the client trust store to a resolved internal CA. An anchor that + narrows nothing — the default — leaves the opener handler-for-handler what it was, which is why a + connection needs one of these openers only when it carries an extra handler OR an anchor that + ``narrows``; see :func:`~messagefoundry.config.tls_policy.build_anchored_https_handler`.""" return urllib.request.build_opener( - _NoRedirectHandler, _asserted_https_handler(), *extra_handlers + _NoRedirectHandler, + build_anchored_https_handler(anchor=trust_anchor, connector=_HTTP_FAMILY_CELL), + *extra_handlers, ) @@ -284,7 +312,9 @@ def _insecure_opener( def _expiry_relaxed_opener( - host: str, *extra_handlers: urllib.request.BaseHandler + host: str, + *extra_handlers: urllib.request.BaseHandler, + trust_anchor: TrustAnchor = SYSTEM_TRUST_ANCHOR, ) -> urllib.request.OpenerDirector: """A no-redirect opener that verifies chain + hostname but tolerates an EXPIRED server cert (#129, ADR 0094). Built per connection (not the shared module-level verifying opener) only when @@ -292,8 +322,13 @@ def _expiry_relaxed_opener( ``check_hostname=True``) and relaxes ONLY the validity-period check via :func:`~messagefoundry.config.tls_policy.relax_verify_expiry`. Verification stays ON, so this is the granular alternative to ``verify_tls=false`` — a MITM-able peer (wrong host / untrusted chain) is - still rejected. Shared verbatim by the SOAP destination.""" - ctx = ssl.create_default_context() + still rejected. Shared verbatim by the SOAP destination. + + ``trust_anchor`` (#1180) selects the roots that chain is validated against; the default resolves to + the OS trust store, which is the ``ssl.create_default_context()`` this line used to be. Expiry + tolerance and anchor narrowing are independent — relaxing the validity window never widens the + trust store, and vice versa.""" + ctx = build_verifying_client_context(trust_anchor) relax_verify_expiry(ctx, host=host) # chain + hostname stay enforced; only expiry is relaxed harden_cipher_suites(ctx, connector="HTTP-family destination (expired-certificate tolerance)") return urllib.request.build_opener( @@ -1353,14 +1388,22 @@ def __init__(self, config: Destination) -> None: # #129 (ADR 0094): granular expiry-only relaxation — verify chain + hostname but tolerate an # expired server cert (opt-in; default off = the shared verifying opener, byte-identical). It # keeps verification ON, so it is NOT an insecure hop in the #200 sense (no refusal keys on it). + # #1180 (ADR 0093): the client trust anchor for this https hop. + anchor = http_family_trust_anchor( + s, url=self.url, trust_anchor_policy=config.trust_anchor_policy + ) if bool(s.get("tls_allow_expired", False)): self._opener = _expiry_relaxed_opener( - urllib.parse.urlsplit(self.url).hostname or "", *proxy_handlers + urllib.parse.urlsplit(self.url).hostname or "", + *proxy_handlers, + trust_anchor=anchor, ) - elif proxy_handlers: + elif proxy_handlers or anchor.narrows: # A forward proxy is configured → a PER-CONNECTION verifying opener carrying it (never the - # shared one; ADR 0126). No proxy → the shared opener stays (byte-identical). - self._opener = _no_redirect_opener(*proxy_handlers) + # shared one; ADR 0126). A narrowed trust anchor needs its own opener for the same reason: + # the shared one carries the OS trust store and must never be mutated. Neither → the + # shared opener stays. + self._opener = _no_redirect_opener(*proxy_handlers, trust_anchor=anchor) else: self._opener = _NO_REDIRECT_OPENER else: diff --git a/messagefoundry/transports/soap.py b/messagefoundry/transports/soap.py index 9ffede72e..094c128ea 100644 --- a/messagefoundry/transports/soap.py +++ b/messagefoundry/transports/soap.py @@ -65,7 +65,13 @@ from xml.sax.xmlreader import InputSource # nosec B406 — fed only the hardened, no-DTD parser from messagefoundry.config.models import ConnectorType, Destination -from messagefoundry.config.tls_policy import harden_cipher_suites, relax_verify_expiry +from messagefoundry.config.tls_policy import ( + SYSTEM_TRUST_ANCHOR, + TrustAnchor, + build_verifying_client_context, + harden_cipher_suites, + relax_verify_expiry, +) from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -90,6 +96,7 @@ enforce_outbound_length_limits, enforce_send_time_length_limits, enforce_signature_header_limits, + http_family_trust_anchor, normalize_header_allowlist, refuse_cleartext_credential_hop, refuse_cleartext_credentials, @@ -188,6 +195,7 @@ def _client_cert_opener( *extra_handlers: urllib.request.BaseHandler, allow_expired: bool = False, host: str = "", + trust_anchor: TrustAnchor = SYSTEM_TRUST_ANCHOR, ) -> urllib.request.OpenerDirector: """A no-redirect opener that presents a **client certificate** for mutual TLS (ADR 0015 §3). @@ -198,8 +206,14 @@ def _client_cert_opener( ``allow_expired`` (#129, ADR 0094) relaxes ONLY the peer cert's validity-period check (chain + hostname stay enforced) — the granular expiry tolerance, composable with mTLS. Default off = - byte-identical.""" - ctx = ssl.create_default_context() + byte-identical. + + ``trust_anchor`` (#1180, ADR 0093) selects the roots that verify the SERVER; the client identity + loaded below is a separate direction and is untouched by it. The default resolves to the OS trust + store, which is the ``ssl.create_default_context()`` this line used to be. mTLS is exactly the + deployment where an internal CA is likeliest, so leaving this hop unable to name one was the + sharpest edge of the inexpressible slice.""" + ctx = build_verifying_client_context(trust_anchor) ctx.minimum_version = ssl.TLSVersion.TLSv1_2 ctx.load_cert_chain(certfile, keyfile, password) if allow_expired: @@ -407,6 +421,11 @@ def __init__(self, config: Destination) -> None: revocation_attested=config.tls_revocation_attested, ) + # #1180 (ADR 0093): the client trust anchor, shared by every VERIFYING branch below. Not + # resolved on the verify_tls=false branch, which is CERT_NONE and has no roots to choose. + anchor = http_family_trust_anchor( + s, url=self.url, trust_anchor_policy=config.trust_anchor_policy + ) if self.client_cert_file and self.client_key_file: # NEW — mutual TLS, takes precedence self._opener: urllib.request.OpenerDirector = _client_cert_opener( self.client_cert_file, @@ -415,17 +434,21 @@ def __init__(self, config: Destination) -> None: *proxy_handlers, # ADR 0126: forward proxy threaded through the mTLS opener too allow_expired=bool(s.get("tls_allow_expired", False)), # #129 (ADR 0094) host=urllib.parse.urlsplit(self.url).hostname or "", + trust_anchor=anchor, ) elif bool(s.get("verify_tls", True)): # #129 (ADR 0094): granular expiry-only relaxation — verify chain + hostname but tolerate an # expired peer cert (opt-in; default off = the shared verifying opener, byte-identical). if bool(s.get("tls_allow_expired", False)): self._opener = _expiry_relaxed_opener( - urllib.parse.urlsplit(self.url).hostname or "", *proxy_handlers + urllib.parse.urlsplit(self.url).hostname or "", + *proxy_handlers, + trust_anchor=anchor, ) - elif proxy_handlers: + elif proxy_handlers or anchor.narrows: # A forward proxy → a per-connection verifying opener carrying it (never the shared one). - self._opener = _no_redirect_opener(*proxy_handlers) + # A narrowed trust anchor needs its own opener for the same reason. + self._opener = _no_redirect_opener(*proxy_handlers, trust_anchor=anchor) else: self._opener = _NO_REDIRECT_OPENER else: diff --git a/tests/test_soap_wssecurity.py b/tests/test_soap_wssecurity.py index 3b59ab96a..7f3c08498 100644 --- a/tests/test_soap_wssecurity.py +++ b/tests/test_soap_wssecurity.py @@ -93,7 +93,10 @@ def get_ciphers(self) -> list[dict[str, object]]: def test_client_cert_opener_loads_chain_and_floors_tls(monkeypatch: pytest.MonkeyPatch) -> None: fake = _FakeCtx() - monkeypatch.setattr(soap_mod.ssl, "create_default_context", lambda: fake) + # The stub takes the real signature's arguments. #1180 routed this opener's context through + # `build_verifying_client_context`, which passes the ssl.Purpose positionally the way the stdlib + # is called everywhere else; a zero-argument stub only ever matched one of the two call shapes. + monkeypatch.setattr(soap_mod.ssl, "create_default_context", lambda *args, **kwargs: fake) opener = _client_cert_opener("client.pem", "key.pem", "pw") assert fake.cert_args == ("client.pem", "key.pem", "pw") assert fake.minimum_version == ssl.TLSVersion.TLSv1_2 # ADR 0002 floor diff --git a/tests/test_tls_trust_anchor.py b/tests/test_tls_trust_anchor.py index 278759e0a..303119dcf 100644 --- a/tests/test_tls_trust_anchor.py +++ b/tests/test_tls_trust_anchor.py @@ -11,7 +11,11 @@ from __future__ import annotations import datetime +import importlib import ssl +import sys +import types +import urllib.request from pathlib import Path import pytest @@ -25,9 +29,13 @@ from messagefoundry.config.tls_policy import ( TrustAnchor, TrustAnchorPolicy, + build_anchored_https_handler, build_verifying_client_context, + requests_verify_from_anchor, resolve_trust_anchor, ) +from messagefoundry.transports import rest, soap +from messagefoundry.transports.fhir import FhirLookupExecutor from messagefoundry.transports.mllp import _mllp_ssl_context @@ -266,3 +274,316 @@ def test_augment_mode_without_internal_ca_is_allowed() -> None: # augment-without-CA equals system (harmless), so it loads — only pinned needs the anchor. s = TlsSettings(trust_anchor_mode="augment") assert s.policy() == TrustAnchorPolicy(internal_ca_file=None, mode="augment") + + +# --- the HTTP egress family: the anchor was INEXPRESSIBLE there (#1180, ASVS 12.3.4) --------------- +# +# Measured at a2eef0f37: `resolve_trust_anchor` reached mllp / dicom / remotefile and NOTHING in +# rest.py, soap.py, fhir.py or dicomweb.py, which exposed a `verify_tls` boolean and no CA parameter +# of any kind. So an operator who set `[tls].internal_ca_file` had it honoured on some hops and +# silently ignored on every https one — REST, SOAP, FHIR, DICOMweb and the fhir_lookup read. +# +# Two halves are under test below and they pull in opposite directions, which is why both are here: +# a hop that names NO anchor must be constructed exactly as it was (identity with the shared opener, +# and a context matching urllib's own), and a hop that DOES name one must actually get it. + + +def _opener_context(opener: urllib.request.OpenerDirector) -> ssl.SSLContext | None: + """The ``SSLContext`` ``opener``'s https handler will hand every connection it opens.""" + for handler in opener.handlers: + if hasattr(handler, "https_open"): + ctx = getattr(handler, "_context", None) + if isinstance(ctx, ssl.SSLContext): + return ctx + return None + + +def _ca_subjects(ctx: ssl.SSLContext) -> set[str]: + """Every CA common name loaded into ``ctx``'s trust store.""" + names: set[str] = set() + for cert in ctx.get_ca_certs(): + for rdn in cert.get("subject", ()): + for attr, value in rdn: + if attr == "commonName": + names.add(value) + return names + + +def _internal_policy(ca: str, mode: str = "pinned") -> TrustAnchorPolicy: + return TrustAnchorPolicy(internal_ca_file=ca, mode=mode) # type: ignore[arg-type] + + +def _client_cert_pair(tmp_path: Path) -> tuple[str, str]: + """A self-signed client cert + its key, as PEM paths — a loadable mTLS identity.""" + key = ec.generate_private_key(ec.SECP256R1()) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "mefor-client")]) + cert = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(datetime.datetime(2020, 1, 1, tzinfo=datetime.UTC)) + .not_valid_after(datetime.datetime(2040, 1, 1, tzinfo=datetime.UTC)) + .sign(key, hashes.SHA256()) + ) + cert_path = tmp_path / "client.crt" + key_path = tmp_path / "client.key" + cert_path.write_bytes(cert.public_bytes(serialization.Encoding.PEM)) + key_path.write_bytes( + key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + ) + return str(cert_path), str(key_path) + + +def _http_dest(ctype: ConnectorType, settings: dict[str, object], **over: object) -> object: + from messagefoundry.transports.base import build_destination + + return build_destination( + Destination(name="OB", type=ctype, settings=settings, **over) # type: ignore[arg-type] + ) + + +# --- the negative: nothing configured, nothing changed --------------------------------------------- + + +def test_default_http_hop_still_gets_the_shared_opener_object() -> None: + """No ``[tls]`` block, no per-connection CA → the SHARED module-level opener, by IDENTITY. + + The strongest available statement that the default did not move: not "an equivalent opener" but + the very object every stock https destination has always been handed.""" + dest = _http_dest( + ConnectorType.REST, + {"url": "https://partner.example.org/api", "method": "POST"}, + ) + assert dest._opener is rest._NO_REDIRECT_OPENER # type: ignore[attr-defined] + + +def test_default_http_hop_with_a_system_policy_still_gets_the_shared_opener() -> None: + """A ``[tls]`` block in ``system`` mode (or with no internal CA) is a no-op here too. + + This is the shipped default the item describes as "the mechanism ships off" — the resolver runs, + resolves to the OS trust store, and the hop is handed the same shared opener as before.""" + for policy in ( + TrustAnchorPolicy(), + TrustAnchorPolicy(internal_ca_file="/org/internal-ca.pem", mode="system"), + TrustAnchorPolicy(internal_ca_file=None, mode="augment"), + ): + dest = _http_dest( + ConnectorType.REST, + {"url": "https://partner.example.org/api", "method": "POST"}, + trust_anchor_policy=policy, + ) + assert dest._opener is rest._NO_REDIRECT_OPENER, policy # type: ignore[attr-defined] + + +def test_loopback_hop_is_exempt_on_the_http_family_too(tmp_path: Path) -> None: + """The resolver's loopback exemption reaches these hops: an on-box hop needs no org-PKI anchor.""" + dest = _http_dest( + ConnectorType.REST, + {"url": "https://127.0.0.1:8443/api", "method": "POST"}, + trust_anchor_policy=_internal_policy(_ca_pem(tmp_path)), + ) + assert dest._opener is rest._NO_REDIRECT_OPENER # type: ignore[attr-defined] + + +def test_unanchored_opener_context_still_matches_urllibs_own() -> None: + """An unanchored opener still carries urllib's OWN context, not a look-alike. + + ``build_asserted_https_handler`` documents the measurement that forced this: urllib's context adds + ALPN and post-handshake auth that ``ssl.create_default_context()`` does not, so substituting one + would silently change the handshake. Threading a trust anchor through these builders must not + start doing that on the hops that named no anchor.""" + engine = _opener_context(rest._no_redirect_opener()) + unanchored = _opener_context( + rest._no_redirect_opener(trust_anchor=TrustAnchor(cafile=None, load_system_roots=True)) + ) + stock = _opener_context(urllib.request.build_opener(rest._NoRedirectHandler)) + assert engine is not None and unanchored is not None and stock is not None + for ctx in (engine, unanchored): + assert ctx.post_handshake_auth == stock.post_handshake_auth + assert ctx.verify_mode == stock.verify_mode + assert ctx.check_hostname == stock.check_hostname + assert ctx.minimum_version == stock.minimum_version + assert len(ctx.get_ca_certs()) == len(stock.get_ca_certs()) + + +# --- the positive: an anchor named is an anchor honoured ------------------------------------------- + + +def test_anchored_https_handler_trusts_only_the_internal_ca(tmp_path: Path) -> None: + """A pinned anchor yields a context holding EXACTLY the internal CA — no public bundle.""" + ca = _ca_pem(tmp_path, "mefor-only-anchor") + handler = build_anchored_https_handler( + anchor=TrustAnchor(cafile=ca, load_system_roots=False), connector="test" + ) + ctx = handler._context + assert _ca_subjects(ctx) == {"mefor-only-anchor"} + # Verification is NEVER turned off by anchoring — it only chooses which roots do the verifying. + assert ctx.verify_mode == ssl.CERT_REQUIRED + assert ctx.check_hostname is True + # and the handshake deltas urllib applies are replayed, not silently dropped. + stock = urllib.request.HTTPSHandler()._context + assert ctx.post_handshake_auth == stock.post_handshake_auth + + +def test_anchored_https_handler_augment_keeps_the_public_roots(tmp_path: Path) -> None: + """``augment`` loads the internal CA INTO urllib's own context rather than replacing it. + + Adding a root needs no new context, so this arm rebuilds nothing and replays nothing — the only + difference from a stock hop is one more trusted issuer.""" + ca = _ca_pem(tmp_path, "mefor-augmenting-ca") + ctx = build_anchored_https_handler( + anchor=TrustAnchor(cafile=ca, load_system_roots=True), connector="test" + )._context + stock = urllib.request.HTTPSHandler()._context + assert "mefor-augmenting-ca" in _ca_subjects(ctx) + assert len(ctx.get_ca_certs()) == len(stock.get_ca_certs()) + 1 + assert ctx.post_handshake_auth == stock.post_handshake_auth + assert ctx.verify_mode == stock.verify_mode + + +@pytest.mark.parametrize( + ("ctype", "settings"), + [ + (ConnectorType.REST, {"url": "https://partner.example.org/api", "method": "POST"}), + (ConnectorType.SOAP, {"url": "https://partner.example.org/svc", "soap_action": "Send"}), + (ConnectorType.FHIR, {"url": "https://fhir.internal.example.org/fhir"}), + (ConnectorType.DICOMWEB, {"url": "https://pacs.internal.example.org/dicom-web"}), + ], +) +def test_every_http_family_destination_honours_the_internal_ca( + ctype: ConnectorType, settings: dict[str, object], tmp_path: Path +) -> None: + """THE expressibility assertion, over the four destinations the item names. + + Before #1180 each of these built the shared OS-trust-store opener and there was no configuration + an operator could write to change that.""" + ca = _ca_pem(tmp_path, "mefor-org-ca") + dest = _http_dest(ctype, settings, trust_anchor_policy=_internal_policy(ca)) + opener = dest._opener # type: ignore[attr-defined] + assert opener is not rest._NO_REDIRECT_OPENER + ctx = _opener_context(opener) + assert ctx is not None + assert _ca_subjects(ctx) == {"mefor-org-ca"} + + +def test_the_fhir_lookup_executor_honours_the_internal_ca(tmp_path: Path) -> None: + """The sanctioned live read-only lookup (ADR 0043) is the most on-point instance of the verb's + condition in the product, and it has no ``Destination`` to carry the policy — the runner threads + it in explicitly.""" + ca = _ca_pem(tmp_path, "mefor-lookup-ca") + ex = FhirLookupExecutor( + {"L": {"url": "https://fhir.internal.example.org/fhir"}}, + trust_anchor_policy=_internal_policy(ca), + ) + ctx = _opener_context(ex._opener["L"]) + assert ctx is not None + assert _ca_subjects(ctx) == {"mefor-lookup-ca"} + + +def test_the_fhir_lookup_executor_default_is_the_shared_opener() -> None: + ex = FhirLookupExecutor({"L": {"url": "https://fhir.example.org/fhir"}}) + assert ex._opener["L"] is rest._NO_REDIRECT_OPENER + + +def test_soap_mutual_tls_opener_honours_the_internal_ca(tmp_path: Path) -> None: + """mTLS is the deployment likeliest to sit behind an internal CA, and its opener builds its own + context — so it needs the anchor threaded separately from the shared verifying path.""" + ca = _ca_pem(tmp_path, "mefor-mtls-server-ca") + cert, key = _client_cert_pair(tmp_path) + ctx = _opener_context( + soap._client_cert_opener( + cert, + key, + None, + trust_anchor=TrustAnchor(cafile=ca, load_system_roots=False), + ) + ) + assert ctx is not None + assert _ca_subjects(ctx) == {"mefor-mtls-server-ca"} + + +def test_expiry_relaxed_opener_honours_the_internal_ca(tmp_path: Path) -> None: + """Expiry tolerance and anchor narrowing are independent: relaxing the validity window must not + quietly widen the trust store back to the OS roots.""" + ca = _ca_pem(tmp_path, "mefor-expiry-ca") + ctx = _opener_context( + rest._expiry_relaxed_opener( + "partner.example.org", trust_anchor=TrustAnchor(cafile=ca, load_system_roots=False) + ) + ) + assert ctx is not None + assert _ca_subjects(ctx) == {"mefor-expiry-ca"} + assert ctx.verify_mode == ssl.CERT_REQUIRED + + +# --- the two hvac clients: no CA argument reached them at all -------------------------------------- + + +def test_requests_verify_maps_the_three_anchor_shapes() -> None: + """``requests`` takes ONE bundle path, so two shapes map exactly and the third must be refused.""" + assert requests_verify_from_anchor(TrustAnchor(None, True), cell="c") is None + assert requests_verify_from_anchor(TrustAnchor("/org/ca.pem", False), cell="c") == "/org/ca.pem" + with pytest.raises(ValueError, match="augment"): + # Silently passing the path would NARROW a hop the operator asked to widen; silently dropping + # it would ignore the anchor. Refusing is the only reading that is not a lie. + requests_verify_from_anchor(TrustAnchor("/org/ca.pem", True), cell="c") + + +@pytest.mark.parametrize( + ("module", "env_var"), + [ + ("messagefoundry.config.secretprovider_vault", "MEFOR_SECRETS_VAULT_CA_FILE"), + ("messagefoundry.store.keyprovider_vault", "MEFOR_STORE_VAULT_CA_FILE"), + ], +) +def test_hvac_clients_take_a_ca_only_when_one_is_configured( + module: str, env_var: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Both hvac sites — including the one that hands out the store's data-encryption key. + + Measured before this change: each passed exactly ``url``/``token``/``allow_redirects`` and no CA + argument of any kind, so an internal-CA Vault could not be reached at all (``requests`` defaults + to the PUBLIC certifi bundle, so that hop fails closed rather than trusting broadly).""" + calls: list[dict[str, object]] = [] + + class _Client: + def __init__(self, **kwargs: object) -> None: + calls.append(dict(kwargs)) + + fake = types.ModuleType("hvac") + fake.Client = _Client # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "hvac", fake) + mod = importlib.import_module(module) + + monkeypatch.delenv(env_var, raising=False) + mod._build_client("https://vault.internal:8200", "s.token") + assert "verify" not in calls[-1], "an unconfigured hop must be constructed exactly as before" + assert set(calls[-1]) == {"url", "token", "allow_redirects"} + + ca = _ca_pem(tmp_path, "mefor-vault-ca") + monkeypatch.setenv(env_var, ca) + mod._build_client("https://vault.internal:8200", "s.token") + assert calls[-1]["verify"] == ca + + +@pytest.mark.parametrize( + ("module", "env_var"), + [ + ("messagefoundry.config.secretprovider_vault", "MEFOR_SECRETS_VAULT_CA_FILE"), + ("messagefoundry.store.keyprovider_vault", "MEFOR_STORE_VAULT_CA_FILE"), + ], +) +def test_hvac_ca_path_that_is_not_a_file_fails_closed( + module: str, env_var: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A typo'd anchor must name itself, not surface as an opaque failure on the first Vault call.""" + mod = importlib.import_module(module) + monkeypatch.setenv(env_var, str(tmp_path / "absent.pem")) + with pytest.raises(Exception, match=env_var): + mod._vault_ca_kwargs("https://vault.internal:8200")