diff --git a/CHANGELOG.md b/CHANGELOG.md index ea13f161..5b794a21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,29 @@ All notable changes to MessageFoundry are documented here. The format follows `block_unlisted_outbound = false` is unchanged and still refused, because there the other six transports stay allow-any; the refusal now names that override as the reason. **No shipped refusal stops firing.** +- **BREAKING — a non-loopback DICOM C-STORE SCP now requires a *verifiable* peer control; + `calling_ae_allowlist` no longer satisfies the gate on its own.** The fail-closed peer-control check + refuses a remotely-reachable SCP that has no peer control, and it accepted any one of three: + `calling_ae_allowlist`, `source_ip_allowlist`, or mTLS. It **counted** them rather than weighing + them. But a Calling AE Title is a string the caller asserts about **itself** in the association + request — no key, no signature, nothing to verify — and AE Titles are published in conformance + statements and visible in any capture. An SCP whose only control was an AE-title list was therefore + reachable by anyone who could route to it and knew one string, while passing a check named + "fail-closed peer controls". Server TLS does not close this: without `tls_ca_file` there is no client + certificate, so the cleartext bind guard (confidentiality) and this gate (authentication) are + orthogonal. + **What changed:** off-loopback, the gate now requires `source_ip_allowlist` **or** mTLS + (`tls` + `tls_ca_file`). `calling_ae_allowlist` is **kept and still enforced** at association time — + it is a genuinely useful filter that catches a misrouted sender and pins intent — it simply has to be + **paired** with one of the two. Measured: AE-title-only off-loopback goes from starting to refused; + AE-title **paired** with an IP allowlist starts; IP-only and mTLS-only are unchanged; and every + loopback bind (the common dev/single-box case) is unchanged. + **Who this bites:** a site running a non-loopback SCP whose only peer control is + `calling_ae_allowlist`. It starts today and will refuse after upgrading. The fix is one line — add + `source_ip_allowlist=[...]` to the `inbound(...)` call, which for a DICOM SCP is the only authoring + surface — and the refusal names it. Keep the AE list; it is still doing work. + Tracked as **BACKLOG #316**. Options considered and declined: an audited opt-out switch, and + documenting the weakness without changing the gate. ### Fixed - **The DICOM C-STORE SCP's fail-closed refusal named a settings key that does not exist.** It told diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 795ccf53..38738b25 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -7219,9 +7219,9 @@ The existing test is too weak to catch any of it: `test_insert_comment_reads_bac **Related:** #250 (the ops API this would sit on), #238, #26 (the declined visual-authoring line — why a declarative artifact interpreted by a second execution path is out). **Source:** Windmill/Kestra evaluation (2026-07-30), "Anvil Ops Tasks" design. Recorded here because the design memo holding the conditions has been deleted. -## 252. DICOM SCP peer-control gate counts a spoofable AE-title list as sufficient +## 316. DICOM SCP peer-control gate counts a spoofable AE-title list as sufficient -> 🔢 **Filed 2026-07-30.** P2. Surfaced while fixing the refusal message that named a non-existent settings key (that half is fixed; this half is a contract change and was deliberately not ridden in on it). +> ✅ **SHIPPED — 2026-07-30. Option (a): pair, do not remove.** Off-loopback, the gate now requires a **verifiable** control — `source_ip_allowlist` or mTLS. `calling_ae_allowlist` no longer satisfies it alone, but is **kept and still enforced** at association time as a filter, so nothing that was useful about it is lost. Measured: AE-title-only off-loopback goes STARTS → REFUSED; AE-title **paired** with an IP allowlist starts; IP-only and mTLS-only are unchanged; every loopback case is unchanged. Breaking for a site relying on AE-title-alone off-loopback — see CHANGELOG. Options (b) audited-opt-out and (c) document-only were declined by the owner in favour of (a). **Type:** security hardening — authentication strength of a fail-closed gate. diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 5c8e2b00..ce8abc3c 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -1536,7 +1536,9 @@ MWL, Query/Retrieve (C-FIND/C-MOVE/C-GET), and pixel-data handling. The **bind interface** is the service-level `[inbound].bind_host` (or a per-connection `bind_address`) and the **peer-IP gate** is the per-connection **`source_ip_allowlist`** — both are set on the `inbound(...)` call, not as `DICOM()` arguments. ⚠️ **`source_ip_allowlist` is *not* a key of the `[inbound]` section in `messagefoundry.toml`.** That section carries only `bind_host`, `ack_after` and `stream_inflight_budget_bytes`, and every settings section is pydantic `extra="ignore"` — so writing `source_ip_allowlist` under `[inbound]` in the service TOML is **accepted silently and does nothing**. (Verified: `InboundSettings.model_fields` is exactly those three; a loaded `[inbound].source_ip_allowlist` leaves no attribute behind, while a sibling `bind_host` survives.) `bind_address` is the same story — a per-connection keyword, not a `[inbound]` key. The reachable forms are `inbound("IB_…", DICOM(...), source_ip_allowlist=["10.20.0.0/16"])` and, for the transports available as data, the **top-level** `source_ip_allowlist` key in `connections.toml` (shown in the [`connections.toml` example](#connections-as-data--connectionstoml-adr-0007) above) — `DICOM()` is code-first only, so for a SCP it is the `inbound(...)` keyword. A non-loopback cleartext SCP is **refused at startup** unless `tls=true` (the generalized [cleartext] bind-guard — `check_dimse_tls_exposure`). `serve --allow-insecure-bind` downgrades that refusal to a warning, but the flag is **clamped** exactly as it is for the MLLP/HTTP/TCP listeners: on a PHI-classified instance under the default `[security].enforcement = enforce` the bind is refused *even with it*, so on a stock instance `tls=true` is the only way to bind off-loopback. (`host` / `called_ae_title` / `connect_timeout` on `DICOM()` are for the **Phase-2 outbound SCU** and are unused by the inbound SCP.) -> **Fail-closed peer controls (deny-by-default, ADR 0025 §9).** DICOM has no transport authentication on its own, so a **non-loopback** SCP **MUST** set at least one peer control — `calling_ae_allowlist` (a `DICOM()` argument), a per-connection `source_ip_allowlist` (an `inbound(...)` keyword — **not** a `[inbound]` service-TOML key, see the ⚠️ above), or **mTLS** (`tls=true` **and** `tls_ca_file`, which makes the SCP require + verify a client cert). With **none** of the three set, a non-loopback SCP is **refused at construction** (the connection degrades per ADR 0031 startup fault isolation; surfaced under `check`/dry-run). This is the **authentication** analog of the `check_dimse_tls_exposure` cleartext bind-guard above (which is the orthogonal **confidentiality** guard): TLS-without-mTLS encrypts the channel but does **not** authenticate the peer. A **loopback** bind (`127.0.0.1`/`localhost`/`::1`, the common dev/single-box case) is exempt. +> **Fail-closed peer controls (deny-by-default).** DICOM has no transport authentication on its own, so a **non-loopback** SCP **MUST** set a **verifiable** peer control — either a per-connection `source_ip_allowlist` (an `inbound(...)` keyword — **not** a `[inbound]` service-TOML key, see the ⚠️ above), or **mTLS** (`tls=true` **and** `tls_ca_file`, which makes the SCP require + verify a client cert). With **neither** set, a non-loopback SCP is **refused at construction** (the connection degrades per ADR 0031 startup fault isolation; surfaced under `check`/dry-run). This is the **authentication** analog of the `check_dimse_tls_exposure` cleartext bind-guard above (which is the orthogonal **confidentiality** guard): TLS-without-mTLS encrypts the channel but does **not** authenticate the peer. A **loopback** bind (`127.0.0.1`/`localhost`/`::1`, the common dev/single-box case) is exempt. +> +> ⚠️ **`calling_ae_allowlist` does not satisfy this gate on its own (BACKLOG #316).** It used to: the three controls were counted as co-equal. But a Calling AE Title is a string the caller asserts about **itself** in the association request — no key, no signature, nothing to verify — and AE Titles are published in conformance statements and visible in any capture. An SCP whose only control was an AE-title list was reachable by anyone who could route to it and knew one string, while passing a check named "fail-closed peer controls". **Keep it — it is still enforced at association time and is a genuinely useful filter** (it catches a misrouted sender and pins intent). It simply has to be **paired** with `source_ip_allowlist` or mTLS off-loopback. > > ⚠️ **The construction gate counts controls, so the wrong spelling passes it.** Set > `calling_ae_allowlist` (AE titles are attacker-chosen strings on an unauthenticated association — diff --git a/docs/SECURITY.md b/docs/SECURITY.md index d5b43b2c..aab44b52 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -1118,7 +1118,7 @@ listen source. The refusal action differs materially per listener, so each has i | **MLLP / HTTP / DICOM** — peer client certificate | the TLS peer certificate presented at handshake | `tls = true` **and** `tls_ca_file` set → `ssl.CERT_REQUIRED` plus strict RFC 5280 verify flags; no client certificate, or one not issued by that CA | **DENY** — the TLS handshake fails and the connection **never reaches the accept path**, so there is **no** connection event and no allow-list evaluation. `tls_ca_file` unset → server-only TLS and no peer-certificate decision. TCP and X12 have no inbound TLS at this release | | **DICOM** — calling AE | the requesting AE's Calling AE Title, at **association negotiation** | `calling_ae_allowlist` set and the title is not in it | **DENY** — the association is rejected by pynetdicom before any C-STORE callback runs (`ae.require_calling_aet`). `None` = any AE the peer-IP allow-list admits | | **DICOM** — called AE | the AE Title the peer addressed the association to | not this engine's own `ae_title` | **DENY** at negotiation (`ae.require_called_aet`); **default `require_called_ae_title = true`** | -| **DICOM** — peer-control construction gate | the SCP's bind host × the presence of any peer control | non-loopback bind with **none** of `calling_ae_allowlist`, `source_ip_allowlist` (an `inbound(...)` keyword — for a DICOM SCP the ONLY surface, since `DICOM()` is not authorable in `connections.toml`), or mTLS (`tls` + `tls_ca_file` → `CERT_REQUIRED`) | **DENY at construction** (ValueError). The connection degrades per ADR 0031 startup fault isolation and the fault surfaces under `messagefoundry check` / dry-run. Loopback hosts are exempt | +| **DICOM** — peer-control construction gate | the SCP's bind host × the presence of a **verifiable** peer control | non-loopback bind with **neither** `source_ip_allowlist` (an `inbound(...)` keyword — for a DICOM SCP the ONLY surface, since `DICOM()` is not authorable in `connections.toml`) **nor** mTLS (`tls` + `tls_ca_file` → `CERT_REQUIRED`). ⚠️ `calling_ae_allowlist` does **not** satisfy this gate alone (BACKLOG #316): an AE Title is caller-asserted with no cryptographic binding, so it is still enforced as a filter but must be **paired** with one of the two above | **DENY at construction** (ValueError). The connection degrades per ADR 0031 startup fault isolation and the fault surfaces under `messagefoundry check` / dry-run. Loopback hosts are exempt | > **Telemetry honesty.** The `peer_not_allowlisted` connection event is durable when the connection's > `capture_connection_errors` is `true`, **or is unset (`None`, the default) and the diff --git a/messagefoundry/transports/dicom.py b/messagefoundry/transports/dicom.py index 5530d7c4..7409f189 100644 --- a/messagefoundry/transports/dicom.py +++ b/messagefoundry/transports/dicom.py @@ -177,30 +177,42 @@ def __init__(self, config: Source) -> None: self._timeout = float(s.get("timeout_seconds", 30.0)) # Build the TLS context now so a bad cert/key fails at build, not at bind (like MLLP/LDAPS). self._ssl = _server_ssl_context(s) - # Fail-closed peer controls (SEC-012, deny-by-default per ADR 0025 §9): a non-loopback SCP with - # NO peer authentication is refused at construction. DIMSE has no transport auth on its own, so - # a remotely-reachable SCP must gate peers by at least one of: the calling-AE allowlist, the - # per-connection source_ip_allowlist (an inbound(...) keyword — NOT an [inbound] service-settings - # key, which does not exist and is discarded silently), or mTLS (tls + tls_ca_file → CERT_REQUIRED in + # Fail-closed peer controls (SEC-012, deny-by-default; tightened by BACKLOG #316): + # a non-loopback SCP with no VERIFIABLE peer control is refused at construction. DIMSE has no + # transport auth of its own, so a remotely-reachable SCP must gate peers by the per-connection + # source_ip_allowlist (an inbound(...) keyword — NOT an [inbound] service-settings key, which does + # not exist and is discarded silently) or mTLS (tls + tls_ca_file → CERT_REQUIRED in # _server_ssl_context). This is the AUTHENTICATION analog of check_dimse_tls_exposure's cleartext - # bind guard (which is the orthogonal CONFIDENTIALITY guard). Raising here integrates with - # ADR-0031 startup fault isolation (the connection degrades, not the engine) and surfaces under - # check/dry-run. Loopback binds (dev/single-box) are exempt. + # bind guard (the orthogonal CONFIDENTIALITY guard). Raising here integrates with ADR-0031 startup + # fault isolation (the connection degrades, not the engine) and surfaces under check/dry-run. + # Loopback binds (dev/single-box) are exempt. + # + # calling_ae_allowlist deliberately does NOT satisfy this gate on its own. The original rule + # counted the three controls as co-equal, but a Calling AE Title is a string the caller asserts + # about ITSELF in the association request — no key, no signature, nothing to verify, and AE Titles + # are published in conformance statements and visible in any capture. An SCP whose only control + # was an AE-title list was reachable by anyone who could route to it and knew one string, while + # passing a check named "fail-closed peer controls". It remains a useful FILTER (it catches a + # misrouted sender and pins intent) and is still enforced at association time — it just has to be + # PAIRED with a control that can actually be verified. mtls_on = bool(s.get("tls")) and bool(s.get("tls_ca_file")) - if self._host not in _LOOPBACK_HOSTS and not ( - self._calling_ae_allowlist or self._source_ip_allowlist or mtls_on - ): + if self._host not in _LOOPBACK_HOSTS and not (self._source_ip_allowlist or mtls_on): + unpaired = ( + " You set calling_ae_allowlist, but an AE Title is asserted by the caller and cannot be" + " verified, so it no longer satisfies this gate alone (BACKLOG #316) — keep it as a" + " filter and add one of the two controls above." + if self._calling_ae_allowlist + else "" + ) raise ValueError( - f"DICOM C-STORE SCP bound non-loopback host {self._host!r} with no peer controls: " - "set at least one of mTLS (tls + tls_ca_file), source_ip_allowlist, or " - "calling_ae_allowlist to fail closed (egress deny-by-default ethos, ADR 0025 §9), or " - "bind 127.0.0.1. Authoring surface: pass them to inbound(...), e.g. " - 'inbound("pacs_in", DICOM(...), source_ip_allowlist=["10.0.0.0/8"]). That is the ONLY ' - "surface for a DICOM SCP — the [inbound] section of messagefoundry.toml has no " - "source_ip_allowlist key and discards it silently, and while connections.toml " - "[[inbound]] tables do accept the key, none of their transports is DICOM. Note " - "calling_ae_allowlist alone gates on a caller-asserted AE Title with no cryptographic " - "binding; prefer mTLS or the IP allowlist where the peer is not trusted." + f"DICOM C-STORE SCP bound non-loopback host {self._host!r} with no verifiable peer " + "control: set source_ip_allowlist, or mTLS (tls + tls_ca_file), to fail closed " + f"(deny-by-default, ADR 0025 §9), or bind 127.0.0.1.{unpaired} Authoring surface: pass " + 'them to inbound(...), e.g. inbound("pacs_in", DICOM(...), ' + 'source_ip_allowlist=["10.0.0.0/8"]). That is the ONLY surface for a DICOM SCP — the ' + "[inbound] section of messagefoundry.toml has no source_ip_allowlist key and discards it " + "silently, and while connections.toml [[inbound]] tables do accept the key, none of " + "their transports is DICOM." ) self._handler: InboundHandler | None = None self._loop: asyncio.AbstractEventLoop | None = None diff --git a/tests/test_dicom_scp_security.py b/tests/test_dicom_scp_security.py index b0989776..475bd127 100644 --- a/tests/test_dicom_scp_security.py +++ b/tests/test_dicom_scp_security.py @@ -158,13 +158,30 @@ def test_nonloopback_scp_without_any_peer_control_fails_closed() -> None: with pytest.raises(ValueError) as exc: _scp("0.0.0.0") msg = str(exc.value) - assert "calling_ae_allowlist" in msg assert "source_ip_allowlist" in msg assert "mTLS" in msg -def test_nonloopback_scp_with_calling_ae_allowlist_ok() -> None: - _scp("0.0.0.0", calling_ae_allowlist=["MOD1"]) # no raise +def test_nonloopback_scp_with_only_calling_ae_allowlist_fails_closed() -> None: + """BACKLOG #316: an AE-title list no longer satisfies the gate alone. + + A Calling AE Title is asserted by the caller in the association request — no key, no signature, + nothing to verify — and AE Titles are published in conformance statements. Before the amendment + this construction succeeded, so an SCP reachable by anyone who knew one string passed a check + named "fail-closed peer controls". The refusal must say specifically why the AE list is not + enough, or the operator just re-reads it as "I already set an allowlist". + """ + with pytest.raises(ValueError) as exc: + _scp("0.0.0.0", calling_ae_allowlist=["MOD1"]) + msg = str(exc.value) + assert "calling_ae_allowlist" in msg + assert "cannot be verified" in msg + assert "filter" in msg # tells them to KEEP it, not delete it + + +def test_nonloopback_scp_with_paired_ae_and_ip_allowlist_ok() -> None: + # The amendment PAIRS rather than removes: the AE list stays useful, alongside a verifiable control. + _scp("0.0.0.0", calling_ae_allowlist=["MOD1"], source_ip_allowlist=["10.0.0.0/8"]) # no raise def test_nonloopback_scp_with_source_ip_allowlist_ok() -> None: