diff --git a/docs/ASVS-L2-PHASE0-CHANGES.md b/docs/ASVS-L2-PHASE0-CHANGES.md index 474cfaac..aa40278c 100644 --- a/docs/ASVS-L2-PHASE0-CHANGES.md +++ b/docs/ASVS-L2-PHASE0-CHANGES.md @@ -314,8 +314,8 @@ tables in [`CONNECTIONS.md`](CONNECTIONS.md) §"Resource management & limits" (A | DICOMweb STOW-RS destination (`dicomweb`) | outbound | HTTPS; port from the base URL (ADR 0025 Phase 2) | `verify_tls` default true (false needs `MEFOR_ALLOW_INSECURE_TLS`) | static bearer or HTTP Basic | **yes** — `url` per Connection | `DICOMweb(url, study_uid, timeout_seconds, verify_tls)`, `[egress].allowed_http` | | DICOM C-STORE SCP (`dimse`) | inbound | DICOM upper layer over TCP, **default port 104**, bound to `[inbound].bind_host` | opt-in TLS with opt-in mTLS via `tls_ca_file`; a **non-loopback cleartext SCP is refused at start** unless `serve --allow-insecure-bind` | `calling_ae_allowlist` (AE-Title gate) + `require_called_ae_title` (default true) + the `[inbound].source_ip_allowlist` IP gate | bind host is `[inbound].bind_host`; the modality dials in | `DICOM(ae_title, port, calling_ae_allowlist, require_called_ae_title, max_associations, max_pdu_size, max_object_bytes, timeout_seconds)` | | DICOM C-STORE SCU + C-ECHO destination (`dimse`) | outbound | DICOM upper layer over TCP, **default port 104** | opt-in TLS (`tls`, `tls_allow_expired`) | the association's calling / `called_ae_title` AE Titles | **yes** — `host`/`port`/`called_ae_title` per Connection | `DICOM(host, port, called_ae_title, timeout_seconds, connect_timeout)`, `[egress].allowed_tcp` | -| SMTP email destination (`email`) | outbound | SMTP, **default port 587** (STARTTLS submission); port 465 selects implicit TLS (ADR 0029) | `use_tls` default true (STARTTLS); false needs `MEFOR_ALLOW_INSECURE_TLS` | optional SMTP AUTH `username` / `password` (`env()`) | **yes** — `host`/`port` per Connection | `Email(host, port, sender, recipients, use_tls, username, password, timeout_seconds)`, `[egress].allowed_smtp` | -| Direct-Project S/MIME destination (`direct`) | outbound | SMTP to a HISP relay, **default port 587** (465 = implicit TLS), ADR 0085 | STARTTLS on by default; the sign-then-encrypt S/MIME body protects PHI **independently of** session TLS | the sender's S/MIME `signing_key` + `signing_cert` (+ optional `signing_key_password`), the partner `recipient_cert` chaining to `trust_anchor`, plus optional SMTP AUTH | **yes** — `host` / `recipient_cert` / `trust_anchor` per Connection | `Direct(host, port, signing_key, signing_cert, recipient_cert, trust_anchor, use_tls, timeout_seconds)`, `[egress].allowed_direct` | +| SMTP email destination (`email`) | outbound | SMTP, **default port 587** (STARTTLS submission); port 465 selects implicit TLS (ADR 0029) | `use_tls` default true (STARTTLS); false needs `MEFOR_ALLOW_INSECURE_TLS`. The server certificate **is verified** (`tls_verify` default true, #323) — chain + hostname + strict RFC 5280, anchored to the OS roots, a per-connection `tls_ca_file`, or `[tls].internal_ca_file`; `tls_verify=false` needs the **clamped** escape and also refuses SMTP AUTH | optional SMTP AUTH `username` / `password` (`env()`) | **yes** — `host`/`port` per Connection | `Email(host, port, sender, recipients, use_tls, username, password, timeout_seconds)`, `[egress].allowed_smtp` | +| Direct-Project S/MIME destination (`direct`) | outbound | SMTP to a HISP relay, **default port 587** (465 = implicit TLS), ADR 0085 | STARTTLS on by default **and the relay certificate is verified** (`tls_verify` default true, #323; `tls_ca_file` is the TLS-hop CA, distinct from `trust_anchor`, which is the partner's S/MIME CA); the sign-then-encrypt S/MIME body protects PHI **independently of** session TLS | the sender's S/MIME `signing_key` + `signing_cert` (+ optional `signing_key_password`), the partner `recipient_cert` chaining to `trust_anchor`, plus optional SMTP AUTH | **yes** — `host` / `recipient_cert` / `trust_anchor` per Connection | `Direct(host, port, signing_key, signing_cert, recipient_cert, trust_anchor, use_tls, timeout_seconds)`, `[egress].allowed_direct` | | DATABASE destination, poll source, and `db_lookup` read connection (`database`) | outbound (the poll source also dials out, for inbound data) | ODBC — TDS for the `sqlserver` dialect, **default port 1433** | the `sqlserver` dialect enforces Driver-18 TLS (`encrypt` default true, `trust_server_certificate` default false); the `generic` dialect delegates TLS to the operator's `odbc_params` | SQL / Integrated / Entra; statements are parameterized | **yes** — `server` / `database` / statement per Connection | `Database(...)`, `DatabasePoll(...)`, `DatabaseLookup(...)` with `connect_timeout`, `pool_max`, `acquire_timeout`; `[egress].allowed_db` | | Reference-set sync dial-out — `DatabaseRef` (`database`) | outbound (periodic) | ODBC, **default port 1433** (ADR 0006) | as above | as above | **yes** — `server` / `statement` per reference set | `DatabaseRef(...)` + `Reference(name, refresh_seconds)`; `[egress].allowed_db` | | Internal sources that **open no socket**: `timer`, `loopback`, `passthrough` | inbound (internal only) | none — clock-driven (ADR 0011), re-ingress-only (ADR 0013), and Handler-fed pass-through respectively | n/a | n/a — they reach no external system | no | `Timer(...)`, `Loopback()`, `PassThrough()` | diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 43d8a665..c228972d 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5261,7 +5261,9 @@ sourced — **#1 (SQL Server concurrency)** and **#2 (console off-thread)** — **Why:** Real gap. The alert SMTP sink (EmailTransport in pipeline/alert_sinks.py) calls starttls() with the default SSL context and exposes only email_use_tls plus the global MEFOR_ALLOW_INSECURE_TLS cleartext escape, so there is no per-mail-server option to keep TLS on yet unconditionally trust a self-signed/mismatched certificate. -**Why it is an anti-feature:** unconditionally trusting an SMTP server's certificate defeats TLS. The engine's `EmailAlertSink` uses STARTTLS with a verifying context by design; the only escape (`MEFOR_ALLOW_INSECURE_TLS`) is global and deliberately loud. Recorded for parity completeness, not as a want. +**Why it is an anti-feature:** unconditionally trusting an SMTP server's certificate defeats TLS. The only escape (`MEFOR_ALLOW_INSECURE_TLS`) is global and deliberately loud. Recorded for parity completeness, not as a want. + +> ⚠️ **CORRECTED 2026-08-01 — this item previously asserted "The engine's `EmailAlertSink` uses STARTTLS with a verifying context by design." That was FALSE**, and it is the shape [`CLAUDE.md`](../CLAUDE.md) §11 names as worst: *a compensating control resting on a false premise*. `smtplib.starttls()` with no context falls back to `ssl._create_stdlib_context`, which **is** `ssl._create_unverified_context` (`CERT_NONE`, `check_hostname=False`) — so the alert sink was encrypting without authenticating, and a reader of this item would have concluded alert email was TLS-verified when it was not. #323 fixed the two **connectors** (`EmailDestination` / `DirectDestination`); it did **not** fix the alert sink, so the **Why:** paragraph above remains accurate and this item's premise stays false until #323's alerts-cell residual lands. Do not re-assert verification here before then. **Nearest existing mechanism:** EmailTransport / send_plain_email in pipeline/alert_sinks.py (SMTP alert sink, built by notifier_from_settings from AlertsSettings in config/settings.py); its only TLS knob is email_use_tls (STARTTLS on/off) plus the global MEFOR_ALLOW_INSECURE_TLS / insecure_tls_allowed() escape, which permits CLEARTEXT SMTP — not a keep-TLS-but-trust-any-cert override. @@ -7397,7 +7399,15 @@ MEFOR_FORBIDDEN_TOKENS=scripts/security/scan-tokens.local.txt.example \ ## 323. SMTP TLS is unverified on all three send paths -> 🔢 **Filed 2026-08-01 — not started.** Value **8/10** · Difficulty **4/10** · _fill-in_. All three SMTP send paths call `starttls()` / `SMTP_SSL()` with **no** SSL context, so Python 3.14's stdlib default applies (`CERT_NONE`, `check_hostname=False`) — the EMAIL destination puts Handler PHI *and* the SMTP AUTH password over an encrypted-but-unauthenticated hop, while the #201 revocation guard, the cleartext-credential rule and `DEPLOYMENT.md` all describe that same hop as **verified**. +> 🚧 **Status 2026-08-01 — PARTIALLY SHIPPED, 2 of 3 cells** (PR #132). The two **connectors** verify: `EmailDestination` and `DirectDestination` build an explicit verifying context via the new `tls_policy.build_smtp_tls_context()` and pass it on both `smtplib` arms, with per-connection `tls_verify` / `tls_ca_file` / `tls_check_hostname` on the `Email()` and `Direct()` factories. Proven by **negative control**: with the code change stashed and the tests kept, all eight new assertions in `tests/test_email_destination.py` go **red** — the pre-existing "STARTTLS was issued" assertions stayed green throughout the insecure period and could never have caught this. ⚠️ **The alerts cell is NOT fixed**: `pipeline/alert_sinks.py:384` still calls `smtp.starttls()` bare, so alert + security-notify email remains unverified. That is the residual below, and it is why [#139](#139)'s premise is still false. +> +> 🔢 **Filed 2026-08-01.** Value **8/10** · Difficulty **4/10** · _fill-in_. All SMTP send paths called `starttls()` / `SMTP_SSL()` with **no** SSL context, so Python 3.14's stdlib default applied (`ssl._create_stdlib_context` **is** `ssl._create_unverified_context` — `CERT_NONE`, `check_hostname=False`) — the EMAIL destination put Handler PHI *and* the SMTP AUTH password over an encrypted-but-unauthenticated hop, while the #201 revocation guard, the cleartext-credential rule and `DEPLOYMENT.md` all described that same hop as **verified**. + +**Residual — the alerts cell (~1 layer).** `pipeline/alert_sinks.py` `send_plain_email` and `pipeline/security_notify.py` need the same context, plumbed from new `[alerts].email_tls_verify` / `email_tls_ca_file`, plus a `[security].allow_unverified_alert_smtp_tls` acknowledgment switch at the serve gate. It needs an **acknowledgment switch rather than the clamp** because the contextvar hop posture is never stamped for that cell — which is why it was deferred rather than folded in. The shared `refuse_unverified_smtp_tls()` helper belongs in `config/settings.py` at that point; the connectors currently **inline** the refusal, matching the `mllp.py` / `remotefile.py` house style. Then: register the deviation in `security_loosenings()` (see [#333](#333)), add a `checks.py` advisory, and correct `docs/PHI.md` and [#139](#139). + +**Also landed, called out rather than folded in silently.** `transports/direct.py`'s cleartext arm read the **unclamped** `insecure_tls_allowed()` while its sibling arm one branch away read the clamped `weakened_tls_escape_permitted_here()`. Two different escapes in one connector is how the next bug gets written, so it now reads the clamped one. Strictly **adds** refusals (ADR 0092 decision 5). Partially closes the [#329](#329) concern. + +**Correction to this item's own text.** The "Migration risk, stated plainly" framing filed with this item presumed existing deployments. The owner confirmed on 2026-08-01 that **there are none**, so secure-by-default was simply correct and no phased rollout, CHANGELOG breaking entry or migration guide was warranted. Do not resurrect that framing from this item's history. **Cluster:** Security & Compliance. **Priority:** P1. **Verdict:** build. **Severity:** high. @@ -7759,6 +7769,12 @@ What it *is*: the realistic failure is a dev/CI environment variable riding into The AI-broker cell has an additional argument: `transports/smart.py:126-149` moved the *same* question — a credential on a cleartext token endpoint — off the raw escape and onto `refuse_cleartext_credential_hop` in commit `a3015196`, with a comment describing exactly this defect (*"It used to read the raw, UNCLAMPED `MEFOR_ALLOW_INSECURE_TLS`"*). `ai_broker.py:140` is the un-migrated twin of a cell fixed days ago. +**Additionally — converting all five is what makes the property *checkable*, not just true.** While these five remain, "no unclamped escape survives on an enforcing PHI posture" is five separate per-site facts, each verifiable only by opening the site and reading it, and each silently falsified by a sixth cell added later. Convert them all and it collapses into **one repo-wide invariant**: the raw `insecure_tls_allowed()` becomes unreachable outside `config/settings.py`'s own clamp, so the absence of the raw predicate is checkable everywhere at once, with `weakened_tls_escape_permitted_here` as the thing that must still be present. Today the property is a convention enforced by review; afterwards it is an invariant enforced by a grep — and a *new* unclamped cell fails immediately instead of waiting for the next audit to enumerate it. + +That distinction matters concretely for the ASVS record. The scorecard's absence-claim mechanism runs regexes over the whole `*.py` corpus and **cannot scope a grep to one file**, so a per-connector claim ("`direct.py`'s escape is clamped") is not expressible and has to be carried as stated-but-unchecked prose. A repo-wide claim is expressible and machine-verified on every commit. So this item is not only five leaks to plug: it is the difference between a security property that must be re-audited by hand and one that a gate can hold. *(Framing contributed by the ADR 0156 ASVS-sweep session, 2026-08-02.)* + +**Scope note, because the count is moving and two censuses will disagree.** #323 routes `transports/direct.py` and `transports/email.py` through the clamp, taking the remaining set to four once it lands — so a census taken on that branch disagrees with one taken on `main`, and neither is wrong. Measured at `main` by counting **`ast.Call` nodes**, not matching lines: six real call sites outside `config/settings.py` — `auth/ldap.py`, `pipeline/alert_sinks.py`, `transports/ai_broker.py`, `transports/database.py`, `transports/direct.py`, `transports/remotefile.py`. `transports/database.py` is the documented unstamped fallback, excluded above; `transports/mllp.py` matches a naive grep for the raw name but its occurrence is **prose inside a docstring, not a call at all**. A line-based census reports it as a further site; an AST-based one does not — which is the instrument distinction, not a detail about this item. + **Proposed:** convert all five, but note that a blanket swap to `weakened_tls_escape_permitted_here()` would silently fix only two of them. 1. **In-gate cells — a one-line swap each.** `remotefile.py:375` and `direct.py:170` are built inside `build_check_registry`/`wiring_runner`'s `active_hop_posture` scope (`config/tls_policy.py:587-603`; the stamping sites are all in `pipeline/wiring_runner.py`), so `weakened_tls_escape_permitted_here()` reads a real posture there — byte-identical to how `remotefile.py:176`/`:577` and `email.py:134` already behave. @@ -8178,7 +8194,9 @@ ADR 0144:193-195 records the decorated-scope trade, but justifies it with an **` **The third gap the audit named is narrower than described.** The non-recursive `base.glob("*.py")` at `checks.py:893`/`:898` (ADR 0144:196) is **not** an unscanned execution path. `load_config` globs `directory.glob("*.py")` non-recursively too (`config/wiring.py:3969`, and `:4392` for `validate_config`), and `_SiblingHelperFinder.find_spec` returns `None` for any dotted name and serves only `_`-prefixed top-level helpers from the config dir (`wiring.py:3902`, `:3908-3912`). A `.py` in a config subdirectory is therefore neither executed by the loader nor importable by a sibling — and `_assert_safe_config_source` is non-recursive for the same reason (`wiring.py:4194`, `:4324`). The lint's file set already equals the executable set. A recursive walk here would make the lint report on files the safe-source ownership gate never vets — an asymmetry in the other direction. #226 (`docs/BACKLOG.md:6917`) already parks recursion as a *loader* question; it belongs there, not here. -**Why:** Bounded, and bounded hard. The lint is advisory by default (`checks.py:953`, `ok=not strict, required=strict`), so a finding blocks nobody unless an adopter opts into `--strict-handler-security` on their own CI. It governs code the adopter's own administrator authors, inside a directory whose write access is already the trust boundary (`_assert_safe_config_source`, `wiring.py:4194`/`:4324`) — anyone who can drop a `.py` there already has arbitrary in-process execution under the engine account, so this is **not** a privilege boundary and evading it buys an attacker nothing they did not already have. ADR 0144:171-174 and the `_check_handler_security` docstring (`checks.py:878`) both say so: "a filter, not a fix." There is no PHI-exposure path and no runtime behaviour change of any kind. +**Why:** Bounded, and bounded hard. The lint is advisory by default (`checks.py:953`, `ok=not strict, required=strict`), so a finding blocks nobody unless an adopter opts into `--strict-handler-security` on their own CI. It governs code the adopter's own administrator authors, inside a directory whose write access is already the trust boundary (`_assert_safe_config_source`, `wiring.py:4194`/`:4324`) — anyone who can drop a `.py` there already has arbitrary in-process execution under the engine account, so this is **not** a privilege boundary and evading it buys an attacker nothing they did not already have. + +> ⚠️ **Rationale amended 2026-08-01 (ADR 0087 sandbox session) — the severity is right, the reason was not.** "The author already has in-process execution" is true at the **default** `[sandbox].mode=off`, and **false** under `mode=subprocess`, where the entire premise is that the author is *not* trusted with it. A severity floor resting on a posture-specific claim reads as settled and misleads the next reader. The rationale that holds in **both** postures: the lint is advisory and pre-deployment; under `mode=off` the author already has in-process execution, and under `mode=subprocess` an evasion still only reaches **host** actions the sandbox does not confine — `DEFAULT_FORBIDDEN_MODULES` (`pipeline/sandbox.py:84-95`) blocks `socket`, `ssl`, `asyncio`, `multiprocessing`, the I/O-bearing `messagefoundry.*` subpackages and `cryptography`, but **not `os` or `subprocess`** (verified at HEAD). ADR 0087 confines the **address space** (the child cannot reach the parent's DEK, audit chain or sockets), not the **host**; OS-level default-deny is ADR 0147, *Proposed with no code*. So an evasion reaches neither the DEK nor the audit chain in either posture. **Re-score upward when ADR 0147 lands**, at which point the lint becomes load-bearing for exactly the class OS confinement is meant to close. ADR 0144:171-174 and the `_check_handler_security` docstring (`checks.py:878`) both say so: "a filter, not a fix." There is no PHI-exposure path and no runtime behaviour change of any kind. What it *is*: an adopter who turns on the strict gate gets a **green build** on a Handler containing `getattr(os, "system")`, and gets a green build on a transforms helper logging `msg.raw` at INFO. Gap (2) is the one that actually costs something, because the miss is not a malicious bypass — it is the ordinary fallible-author case ADR 0144 exists for, landing in the exact file the project's own layout guidance created. Gap (1) is mostly a claim-hygiene problem: the ADR asserts the false negative in prose and no test proves it, so nobody notices if a future change silently widens or narrows it. diff --git a/docs/PHI.md b/docs/PHI.md index 974e20d8..09c50abc 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -913,7 +913,7 @@ with materially different PHI profiles, so they get their own rows; stream 4 is | **8. `alert_instance` table — default on wherever an `[alerts]` notifier exists** | resolvable operator alerts: `connection_stopped`, `queue_buildup`, `lane_stuck`, `message_stall`, `saturation`, `connection_error`, `content_match`, `storage_threshold`, `cert_expiry`, `secret_rotation`, `bootstrap_admin_expiring` (the UNCLAIMED first-run bootstrap admin nearing its auto-disable deadline — ASVS 6.4.5; its payload carries only the ISO deadline plus whole hours remaining, never the password or any secret), `integrity_drift`, `update_available`, `backup_failed`, `rcsi_off_degraded`, `leadership_acquired`, `dr_activated`, `gcm_invocations` (the per-key AES-GCM invocation bound crossing its 2^31 soft warn — ASVS 11.3.4; its payload carries a one-way `key_id` fingerprint plus counters, never key bytes) The three reachable **inverse** signals — `connection_restored`, `leadership_lost`, `dr_released` — are never rows here: `_record_state` routes an inverse through `_AUTO_RESOLVE` to `resolve_alert_instances_for`, never to `upsert_alert_instance`. (A fourth mapped key, `connection_started`, is emitted by no code path today.) | rows: `event_type`, `connection`, `severity`, `status`, `first_seen`, `last_seen`, `count`, `reason`, `acked_by`, `acked_at`, `resolved_at`, `suspended_until`, `escalation_tier` | the store database, **all three backends** | the operator alert list — acknowledge / resolve / suspend. Durable state is recorded **before** any suppression or throttle return, so a muted alert still leaves a record | `GET /alerts/active` under **`monitoring:diagnose`** (**not** a PHI permission) with the same per-channel scope; ack/resolve/suspend/**resume** are POSTs on the same tier, and the separate read-only `GET /alerts/rules` view sits on its own gate | shares the connection-event window; **only RESOLVED instances are DELETEd**, by `resolved_at` — an open or acknowledged condition is never aged out from under an operator | **`reason` is free text** taken from the event's `detail`/`reason`/`label`: `safe_text(reason)[:200]` then cipher-encrypted (AAD `("alert_instance","reason",event_type,connection)` — the de-dup grain, so one AAD covers both the INSERT and the re-fire UPDATE). `content_match` is **PHI-free by contract**: the sink method takes no value parameter, only the connection, an operator label and an optional rule id | | **9. `response` rows with `kind='ack_sent'` — DEFAULT ON** (`[diagnostics].response_sent = true`) | the ACK/NAK the engine returned to an inbound sender, under a sentinel destination `\x1fack:` | rows: `ack_code` (`AA`/`AE`/`AR`/`CA`/`CE`/`CR`), `ack_phase` (`decode`/`parse`/`strict`/`ingest`), `outcome`, `body`, `detail` | the store database | "what did we actually reply, and why" — the operator's answer to a sender disputing an ACK | `GET /messages/{id}/responses` under `messages:read` + `require_phi_read`; the `body` only for a caller who also holds `messages:view_raw`; every read writes a `response.read` audit row | `body`, `detail` and `resp_headers` are set to `NULL` in place by `purge_message_bodies` on the message-body window, on all three backends | **PHI fail-safe:** the ACK **body** is stored **only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext — and every NAK passes no body at all, so the offending field value is never persisted. The disposition metadata (`ack_code`/`ack_phase`/`outcome`) is non-PHI and always captured; `detail` is `safe_text`-scrubbed, 200-char bounded and encrypted | | **10. `[alerts]` webhook transport** (off by default — `webhook_url` unset) | one HTTPS POST per alert, carrying every non-underscore event key as JSON | JSON | the operator's webhook endpoint (Slack/Teams/PagerDuty/custom) | operator notification | **`https` only** — a plaintext `http://` webhook URL is refused at construction unless the raw `MEFOR_ALLOW_INSECURE_TLS` escape is set (and then a warning is logged); note this path reads the **unclamped** escape, unlike the connectors. Redirects are refused; an optional `webhook_allowed_hosts` egress allowlist gates the host | the endpoint's | **carries the alert's `detail`/`reason` free text** (`safe_exc()`-scrubbed at the emit sites, but **not** re-run through `safe_text` on this path). Internal `_`-prefixed keys (per-rule recipients, rule id, cooldown) are stripped before send, so recipient addresses never cross the wire | -| **11. `[alerts]` SMTP transport — operator alert list** (off unless `email_smtp_host` + `email_from` + ≥1 `email_to`) | one email per alert; default subject `[MessageFoundry] `, default body every non-underscore event key as `k: v` | plain text (always kept — never HTML-only); optional HTML alternative | the operators' mailboxes | operator notification | `smtp_allowed_hosts` egress allowlist; the SMTP password comes from `MEFOR_ALERTS_EMAIL_PASSWORD` or a `[secrets]` provider, never the config file; per-send timeout `email_timeout` | the mail system's | carries the same `detail`/`reason` free text as the webhook. #138 operator templates are constrained to a **closed non-PHI variable allowlist** validated fail-closed at config load. **Transport caveat, stated plainly:** `send_plain_email` calls `smtp.starttls()` with **no SSL context**, so Python's stdlib default applies — `check_hostname = False`, `verify_mode = CERT_NONE`. The hop is therefore **encrypted but unauthenticated (MITM-able)** when `email_use_tls` is true (the default), and cleartext when it is false. There is **no hop gradient or attestation on this path**: PR #1163 hardened the EMAIL *message destination* connector, not the `[alerts]` SMTP path | +| **11. `[alerts]` SMTP transport — operator alert list** (off unless `email_smtp_host` + `email_from` + ≥1 `email_to`) | one email per alert; default subject `[MessageFoundry] `, default body every non-underscore event key as `k: v` | plain text (always kept — never HTML-only); optional HTML alternative | the operators' mailboxes | operator notification | `smtp_allowed_hosts` egress allowlist; the SMTP password comes from `MEFOR_ALERTS_EMAIL_PASSWORD` or a `[secrets]` provider, never the config file; per-send timeout `email_timeout` | the mail system's | carries the same `detail`/`reason` free text as the webhook. #138 operator templates are constrained to a **closed non-PHI variable allowlist** validated fail-closed at config load. **Transport caveat, stated plainly — and this path is now the ONLY one left:** `send_plain_email` calls `smtp.starttls()` with **no SSL context**, so Python's stdlib default applies (`ssl._create_stdlib_context` **is** `ssl._create_unverified_context` — `check_hostname = False`, `verify_mode = CERT_NONE`). The hop is therefore **encrypted but unauthenticated (MITM-able)** when `email_use_tls` is true (the default), and cleartext when it is false. There is **no hop gradient or attestation on this path**. ⚠️ **Do not generalise this row to the message connectors.** [#323](BACKLOG.md) fixed the **EMAIL** and **DIRECT** *message destinations* — both now build an explicit verifying context (chain + hostname + strict RFC 5280, anchored to the OS roots, a per-connection `tls_ca_file`, or `[tls].internal_ca_file`), with `tls_verify=false` refused unless the **clamped** escape permits it. The `[alerts]` cell was deliberately deferred because it needs an acknowledgment switch rather than that clamp (the contextvar hop posture is never stamped here), so it is the residual on #323 — not an oversight, and not evidence that SMTP is unverified engine-wide | | **12. Per-user security-event SMTP notifier** — **posture-mandatory on a PHI instance** | `account_locked`, `login_after_failures`, `password_changed`, `password_reset`, `email_changed`, `roles_changed`, `account_disabled`, `mfa_enabled`, `mfa_disabled`, `admin_action_new_ip` | plain-text email | the **affected user's own** mailbox | ASVS 6.3.5 / 6.3.7 out-of-band notification of security-relevant account changes | shares stream 11's SMTP transport and therefore its caveat. On a PHI instance with auth enabled `serve` **refuses to start (exit 2) under `[security].enforcement = enforce`** when no effective channel exists; the explicit, **audited** opt-out is `[alerts].security_notifications_required = false` | the mail system's | the body carries the account username, a fixed description, optionally the failed-attempt count or the new email on file, and the source IP — **no message data, no secrets**. Dispatch is a bounded background queue; a failed send is logged, never raised (the event is still in `audit_log`) | | **13. `LoggingAlertSink` fallback** (when no `[alerts]` transport is configured) | every alert **this state-less sink implements**, at `WARNING` — `leadership_lost` / `dr_released` at `INFO`, and `connection_restored` is a **deliberate no-op** (a recovery needs no page and there is no instance to auto-resolve), so a lane recovery produces no record on this stream at all. `content_match` exists only on `NotifierAlertSink` and has no fallback-path record | — | folds into stream 1 | so alerts are never silent | inherits stream 1's | inherits stream 1's | includes the `detail`/`reason` free text, and therefore inherits stream 1's filters, ACL, forwarder and retention | diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index 66ab3b16..ffbe826c 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -62,6 +62,7 @@ "TrustAnchorMode", "TrustAnchorPolicy", "active_hop_posture", + "build_smtp_tls_context", "build_verifying_client_context", "cleartext_acceptance_audit_sink", "current_hop_posture", @@ -919,3 +920,61 @@ def build_verifying_client_context( return ctx # pinned / per-connection: ONLY this CA (no load_default_certs), matching forward_tls_ca_file. return ssl.create_default_context(purpose, cafile=anchor.cafile) + + +def build_smtp_tls_context( + *, + host: str, + cell: str, + verify: bool = True, + ca_file: str | None = None, + check_hostname: bool = True, + trust_anchor_policy: TrustAnchorPolicy | None = None, +) -> ssl.SSLContext: + """Build the TLS context for an outbound SMTP hop (#323) — STARTTLS or implicit ``SMTP_SSL``. + + ``smtplib`` accepts no context by default, and its fallback is + :func:`ssl._create_stdlib_context`, which **is** ``ssl._create_unverified_context`` — measured on + CPython 3.14.6: ``verify_mode=CERT_NONE``, ``check_hostname=False``. So every certificate was + accepted and the encrypted session was unauthenticated: an on-path attacker presenting any + certificate read the message body (PHI) and the SMTP AUTH credential. This is the SMTP sibling of + :func:`~messagefoundry.transports.remotefile._ftps_ssl_context` and the MLLP outbound arm, and is + deliberately built here rather than in ``transports/`` because ``pipeline/alert_sinks.py`` is a + third caller and a transport must not import ``pipeline/`` (ADR 0029's one-way rule). + + ``verify=False`` is NOT gated here — the caller refuses it against the clamped + :func:`~messagefoundry.config.settings.weakened_tls_escape_permitted_here` first, matching how + MLLP/FTPS inline that check. This module cannot read it: ``config.settings`` imports *from* here, + so the dependency runs one way only. + + ``trust_anchor_policy`` (#190, ADR 0093) supplies the instance ``[tls]`` internal-CA fallback when + the connection names no ``ca_file`` of its own. It only chooses WHICH roots verify the peer — it + never turns verification off. + """ + if verify: + anchor = resolve_trust_anchor( + connection_ca_file=ca_file, + host=host, + policy=trust_anchor_policy if trust_anchor_policy is not None else TrustAnchorPolicy(), + ) + ctx = build_verifying_client_context(anchor) + else: + ctx = ssl.create_default_context(ssl.Purpose.SERVER_AUTH, cafile=ca_file) + ctx.minimum_version = ssl.TLSVersion.TLSv1_2 + if verify: + ctx.check_hostname = check_hostname + else: + logger.warning( + "%s TLS certificate verification is DISABLED (tls_verify=false) — the SMTP session to %s " + "is encrypted but UNAUTHENTICATED and MITM-able; trusted-network dev/test only.", + cell, + host, + ) + # Order is load-bearing: check_hostname must go False BEFORE verify_mode, or ssl raises. + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + harden_cipher_suites(ctx, connector=cell) # assert forward secrecy (ASVS 12.1.2) + if verify: # nothing to strict-validate on the CERT_NONE path (ASVS 12.1.4) + harden_verify_flags(ctx) + return ctx diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 11a6496f..87ce5cbe 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -1785,6 +1785,9 @@ def Email( username: str | EnvRef | None = None, # optional SMTP AUTH user (use env() for the secret) password: str | EnvRef | None = None, # optional SMTP AUTH password (use env() for the secret) use_tls: bool = True, # STARTTLS by default; False (dev only) needs MEFOR_ALLOW_INSECURE_TLS + tls_verify: bool = True, # verify the server cert (#323); False (dev only) needs the escape + tls_ca_file: str | EnvRef | None = None, # PEM to verify the SMTP server against (not a secret) + tls_check_hostname: bool = True, # match the cert against `host` (leave on) timeout_seconds: float = 30.0, encoding: str = "utf-8", ) -> ConnectionSpec: @@ -1793,9 +1796,18 @@ def Email( text); this delivers it as a plain-text SMTP message to ``host:port`` from ``sender`` to ``recipients`` with a static ``subject``. STARTTLS by default (``use_tls=True``) on the ``587`` submission port; port ``465`` is implicit TLS (``SMTP_SSL``). Optional ``username``/``password`` do - SMTP ``AUTH`` (over TLS only — a cleartext-credential config is refused). Disabling TLS - (``use_tls=False``) is MITM-able and refused unless ``MEFOR_ALLOW_INSECURE_TLS`` is set (loud - warning), like LDAPS / SQL Server / MLLP. The egress host is gated by ``[egress].allowed_smtp``. Put + SMTP ``AUTH`` (over a **verified** TLS session only — a cleartext- or unverified-credential config + is refused). Disabling TLS (``use_tls=False``) is MITM-able and refused unless + ``MEFOR_ALLOW_INSECURE_TLS`` is set (loud warning), like LDAPS / SQL Server / MLLP. + + **The server certificate is verified** (``tls_verify=True``, #323) — chain, hostname and strict RFC + 5280 flags, anchored to the OS roots, a per-connection ``tls_ca_file``, or the instance-wide + ``[tls].internal_ca_file`` (ADR 0093). ``smtplib``'s own default context verifies **nothing** + (``CERT_NONE``/``check_hostname=False``), so before #323 ``use_tls=True`` bought encryption without + authentication. Point ``tls_ca_file`` at your relay's CA PEM for a private-CA server; + ``tls_verify=False`` is a trusted-network dev/test escape, refused on an enforcing production-PHI + instance even with ``MEFOR_ALLOW_INSECURE_TLS``, and it also refuses SMTP ``AUTH``. + The egress host is gated by ``[egress].allowed_smtp``. Put secrets in ``env()`` (``username``/``password``), never inline. Delivery is at-least-once, so a retry re-sends the email — a mailbox has no idempotency key, so a rare duplicate is possible and accepted (a duplicate beats a drop). ADR 0029.""" @@ -1810,6 +1822,9 @@ def Email( "username": username, "password": password, "use_tls": use_tls, + "tls_verify": tls_verify, + "tls_ca_file": tls_ca_file, + "tls_check_hostname": tls_check_hostname, "timeout_seconds": timeout_seconds, "encoding": encoding, }, @@ -1836,6 +1851,9 @@ def Direct( username: str | EnvRef | None = None, # optional SMTP AUTH user (use env() for the secret) password: str | EnvRef | None = None, # optional SMTP AUTH password (use env() for the secret) use_tls: bool = True, # STARTTLS by default; False (dev only) needs MEFOR_ALLOW_INSECURE_TLS + tls_verify: bool = True, # verify the relay's cert (#323); False (dev only) needs the escape + tls_ca_file: str | EnvRef | None = None, # PEM to verify the SMTP/HISP relay against + tls_check_hostname: bool = True, # match the cert against `host` (leave on) timeout_seconds: float = 30.0, encoding: str = "utf-8", ) -> ConnectionSpec: @@ -1844,7 +1862,14 @@ def Direct( **body** (content-agnostic — an HL7 string, a CDA/XML document, plain text); this **signs** it with ``signing_key``/``signing_cert``, **encrypts** the signed blob to the partner's ``recipient_cert`` (which must chain to ``trust_anchor``), and submits the S/MIME message to ``host:port`` over - STARTTLS. All cert/key material is loaded + validated at construction (fail loud). The egress host + STARTTLS. All cert/key material is loaded + validated at construction (fail loud). + + **The relay's TLS certificate is verified** (``tls_verify=True``, #323 — ``smtplib``'s own default + verifies nothing). Note the two trust settings are unrelated and easy to confuse: ``trust_anchor`` + is the CA the **partner's S/MIME certificate** must chain to (message-layer), while ``tls_ca_file`` + is the CA the **SMTP relay's TLS certificate** must chain to (transport-layer). The S/MIME body + protects the clinical payload either way, but the SMTP session still carries envelope metadata and + any ``AUTH`` credential, which is why the transport hop is verified too. The egress host is gated by ``[egress].allowed_direct``. Put secrets in ``env()`` (``signing_key_password``, ``username``/``password``), never inline. Delivery is at-least-once, so a retry re-sends — a Direct mailbox has no idempotency key, so a rare duplicate is possible and accepted (a duplicate beats a @@ -1865,6 +1890,9 @@ def Direct( "username": username, "password": password, "use_tls": use_tls, + "tls_verify": tls_verify, + "tls_ca_file": tls_ca_file, + "tls_check_hostname": tls_check_hostname, "timeout_seconds": timeout_seconds, "encoding": encoding, }, diff --git a/messagefoundry/transports/direct.py b/messagefoundry/transports/direct.py index 1209f5a0..59663a7d 100644 --- a/messagefoundry/transports/direct.py +++ b/messagefoundry/transports/direct.py @@ -46,6 +46,7 @@ import asyncio import logging import smtplib +import ssl from collections.abc import Mapping from email.message import EmailMessage from pathlib import Path @@ -57,7 +58,11 @@ from cryptography.hazmat.primitives.serialization import pkcs7 from messagefoundry.config.models import ConnectorType, Destination -from messagefoundry.config.settings import INSECURE_TLS_ESCAPE_ENV, insecure_tls_allowed +from messagefoundry.config.settings import ( + INSECURE_TLS_ESCAPE_ENV, + weakened_tls_escape_permitted_here, +) +from messagefoundry.config.tls_policy import build_smtp_tls_context from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -140,6 +145,13 @@ def __init__(self, config: Destination) -> None: self.username: str | None = str(username) if username else None self.password: str | None = str(password) if password else None self.use_tls = bool(s.get("use_tls", True)) + # #323: server-certificate verification on the TLS hop, kept byte-identical to + # EmailDestination's spelling (this connector's SMTP core is a deliberate copy, not an import — + # the one-way dependency rule — so the two must not drift). + self.tls_verify = bool(s.get("tls_verify", True)) + tls_ca_file = s.get("tls_ca_file") + self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None + self.tls_check_hostname = bool(s.get("tls_check_hostname", True)) self.timeout: float = float(s.get("timeout_seconds", 30.0)) self.encoding: str = str(s.get("encoding", "utf-8")) @@ -167,11 +179,26 @@ def __init__(self, config: Destination) -> None: # SMTP is refused unless the project-wide dev escape is set, and credentials are NEVER sent over # a cleartext channel. if not self.use_tls: - if not insecure_tls_allowed(): + # #323: read through the CLAMPED escape, not the raw insecure_tls_allowed(). This call site + # was the last unclamped one in the file, sitting one branch away from the clamped arm + # below — two different escapes in one connector is how the next bug gets written. The + # change strictly ADDS refusals (ADR 0092 decision 5): an enforcing production-PHI instance + # can no longer silence a cleartext Direct hop with the blunt process-wide env var. + # + # THE ESCAPE STILL EXISTS — it is CLAMPED, not removed. Stated because this file now has + # NO CALL to `insecure_tls_allowed()` (only these comments mention it), and reading that + # as "this connector has no escape" would be a FALSE ABSENCE claim. + # MEFOR_ALLOW_INSECURE_TLS still governs this hop: weakened_tls_escape_permitted_here() is + # that same env var plus the production-PHI clamp. Any absence claim about the raw call is + # scoped to THIS FILE and never repo-wide — `insecure_tls_allowed()` remains live at call + # sites in auth/ldap.py, pipeline/alert_sinks.py, transports/{ai_broker,database,mllp}.py + # and config/settings.py (docs/DEPLOYMENT.md enumerates the ones the clamp does not yet + # cover — see #329). Verified by grep at the time of writing, not assumed. + if not weakened_tls_escape_permitted_here(): raise ValueError( "Direct destination use_tls=false submits over cleartext SMTP; refused unless " - f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only) — use STARTTLS " - "(the default)" + f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only, and refused on a " + "production-PHI instance even with the escape, #200) — use STARTTLS (the default)" ) if self.username is not None: raise ValueError( @@ -183,6 +210,40 @@ def __init__(self, config: Destination) -> None: "network in CLEARTEXT (dev/trusted-network only)", self.host, ) + elif not self.tls_verify: + # #323 arm 2 — same shape and wording as EmailDestination's, deliberately. + if not weakened_tls_escape_permitted_here(): + raise ValueError( + "Direct destination tls_verify=false disables server-certificate verification on " + f"the SMTP hop to {self.host} — the session is encrypted but UNAUTHENTICATED. The " + "S/MIME body still protects the clinical payload, but envelope metadata and any " + "SMTP AUTH credential are exposed to an on-path attacker presenting any " + "certificate. Use a trusted CA (tls_ca_file, or [tls].internal_ca_file for the " + f"instance), or set {INSECURE_TLS_ESCAPE_ENV}=1 to allow it on a trusted-network " + "bind (refused on a production-PHI instance even with the escape, #200)." + ) + if self.username is not None: + raise ValueError( + "Direct destination sends SMTP AUTH credentials over an UNVERIFIED TLS session " + "(tls_verify=false); refused — credentials require a verified TLS session" + ) + # Built once at construction (fail-fast), reused by every send. None when TLS is off entirely. + # DIRECT does not take a RevocationHopGuard even though the hop now verifies: adding it would + # make the enumerated count eight and force four "seven verifying hops" docs to change, and the + # clinical payload is S/MIME-protected at the message layer so the PHI argument is materially + # weaker than EMAIL's (ADR 0085). Recorded rather than silently omitted. + self._tls_context: ssl.SSLContext | None = ( + build_smtp_tls_context( + host=self.host, + cell="Direct destination", + verify=self.tls_verify, + ca_file=self.tls_ca_file, + check_hostname=self.tls_check_hostname, + trust_anchor_policy=config.trust_anchor_policy, + ) + if self.use_tls + else None + ) def _load_private_key(self, value: Any, password: Any) -> Any: """Load the sender's signing private key (PEM/DER, optionally passphrase-protected). PHI/secret- @@ -317,10 +378,13 @@ def _connect(self) -> smtplib.SMTP: """Open an SMTP connection, applying STARTTLS / implicit TLS per config (identical posture to EmailDestination). The caller closes it (``with`` / ``quit``).""" if self.port == 465 and self.use_tls: - return smtplib.SMTP_SSL(self.host, self.port, timeout=self.timeout) + # context= is REQUIRED (#323) — SMTP_SSL's own default is an unverified stdlib context. + return smtplib.SMTP_SSL( + self.host, self.port, timeout=self.timeout, context=self._tls_context + ) smtp = smtplib.SMTP(self.host, self.port, timeout=self.timeout) if self.use_tls: - smtp.starttls() + smtp.starttls(context=self._tls_context) return smtp def _send(self, payload: str) -> None: diff --git a/messagefoundry/transports/email.py b/messagefoundry/transports/email.py index 0a47cff7..81749a44 100644 --- a/messagefoundry/transports/email.py +++ b/messagefoundry/transports/email.py @@ -44,6 +44,7 @@ import asyncio import logging import smtplib +import ssl from collections.abc import Mapping from email.message import EmailMessage from typing import Any @@ -53,7 +54,7 @@ INSECURE_TLS_ESCAPE_ENV, weakened_tls_escape_permitted_here, ) -from messagefoundry.config.tls_policy import RevocationHopGuard +from messagefoundry.config.tls_policy import RevocationHopGuard, build_smtp_tls_context from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -108,6 +109,12 @@ def __init__(self, config: Destination) -> None: self.username: str | None = str(username) if username else None self.password: str | None = str(password) if password else None self.use_tls = bool(s.get("use_tls", True)) + # #323: server-certificate verification on the TLS hop. Defaults ON — smtplib's own default is + # an UNVERIFIED context, so before this the encrypted session was unauthenticated. + self.tls_verify = bool(s.get("tls_verify", True)) + tls_ca_file = s.get("tls_ca_file") + self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None + self.tls_check_hostname = bool(s.get("tls_check_hostname", True)) self.timeout: float = float(s.get("timeout_seconds", 30.0)) self.encoding: str = str(s.get("encoding", "utf-8")) @@ -169,22 +176,66 @@ def __init__(self, config: Destination) -> None: connection=config.name, ) self._hop_guard.enforce_construction() + elif not self.tls_verify: + # #323 arm 2: TLS is on but verification is off. Refused unless the CLAMPED escape permits + # it (#200, ADR 0092 decision 2) — inlined here exactly as the MLLP and FTPS verify-off arms + # inline it (remotefile.py:176, mllp.py:547), not routed through a shared helper, because + # the sibling cells do it this way and a third spelling is how the next bug gets written. + # NOTE the raw insecure_tls_allowed() is deliberately NOT used: the blunt process-wide + # escape must not silence a verify-off PHI hop on an enforcing instance. + if not weakened_tls_escape_permitted_here(): + raise ValueError( + "Email destination tls_verify=false disables server-certificate verification on " + f"the SMTP hop to {self.host} — the session is encrypted but UNAUTHENTICATED, so " + "an on-path attacker presenting any certificate reads the message body (PHI) and " + "any SMTP AUTH credential. Use a trusted CA (tls_ca_file, or [tls].internal_ca_file " + f"for the instance), or set {INSECURE_TLS_ESCAPE_ENV}=1 to allow it on a " + "trusted-network bind (refused on a production-PHI instance even with the escape, " + "#200)." + ) + # Credentials over an unauthenticated channel are refused for the same reason they are + # refused over cleartext: an on-path attacker who can present any cert can capture the + # AUTH exchange. The use_tls=false arm above states the cleartext half of this rule. + if self.username is not None: + raise ValueError( + "Email destination sends SMTP AUTH credentials over an UNVERIFIED TLS session " + "(tls_verify=false); refused — credentials require a verified TLS session" + ) + # No RevocationHopGuard here: a hop that does not verify the certificate at all has nothing + # whose revocation status could matter, and the composition rule forbids two gates deciding + # the same hop (docs/DEPLOYMENT.md §"composition"). The refusal above is that hop's gate. else: - # #201 (ADR 0078 amendment): STARTTLS (587) / implicit-TLS SMTP_SSL (465) verifies the server - # cert, but smtplib's default SSL context does NO OCSP/CRL revocation (stdlib ssl has none) — - # so a revoked-but-unexpired SMTP-server cert is still accepted. The message body carries PHI - # over this verified hop, so refuse an off-loopback production-PHI hop unless revocation is - # attested (loopback / synthetic / non-prod / attested byte-identical). Same posture-keyed - # RevocationHopGuard as the REST/SOAP/FHIR/DICOMweb https paths; SMTP is a different construction - # seam (smtplib, not the urllib/ssl scheme), so it calls the guard directly rather than the - # https-scheme-keyed refuse_unrevoked_verified_hop wrapper. Composes with the cleartext refusal - # above (that gate keys on use_tls=false; this one only on the verifying use_tls=true path). + # #201 (ADR 0078 amendment): the hop now genuinely verifies the server cert (#323 — it did + # not before; smtplib's default context is ssl._create_unverified_context), but stdlib ssl + # does NO OCSP/CRL revocation, so a revoked-but-unexpired SMTP-server cert is still + # accepted. The message body carries PHI over this verified hop, so refuse an off-loopback + # production-PHI hop unless revocation is attested (loopback / synthetic / non-prod / + # attested byte-identical). Same posture-keyed RevocationHopGuard as the REST/SOAP/FHIR/ + # DICOMweb https paths; SMTP is a different construction seam (smtplib, not the urllib/ssl + # scheme), so it calls the guard directly rather than the https-scheme-keyed + # refuse_unrevoked_verified_hop wrapper. Composes with the two refusals above: this arm is + # reached only when use_tls=true AND tls_verify=true, which is the guard's documented + # precondition (tls_policy.py: "the caller has already built a verifying context") — that + # precondition was VIOLATED by this call site before #323. RevocationHopGuard.capture( host=self.host, cell="Email destination (verified SMTP TLS, no revocation check)", description="delivers over verified SMTP TLS but performs no certificate revocation checking", attested=config.tls_revocation_attested, ).enforce_construction() + # Built once at construction (fail-fast), reused by every send. None when TLS is off entirely. + self._tls_context: ssl.SSLContext | None = ( + build_smtp_tls_context( + host=self.host, + cell="Email destination", + verify=self.tls_verify, + ca_file=self.tls_ca_file, + check_hostname=self.tls_check_hostname, + trust_anchor_policy=config.trust_anchor_policy, + ) + if self.use_tls + else None + ) async def send( self, payload: str, *, metadata: Mapping[str, str] | None = None @@ -210,10 +261,13 @@ def _connect(self) -> smtplib.SMTP: if self.port == 465 and self.use_tls: # Port 465 is implicit TLS — the whole session is wrapped from connect, so SMTP_SSL rather # than SMTP+STARTTLS (ADR 0029 §D3). The submission port (587) keeps STARTTLS below. - return smtplib.SMTP_SSL(self.host, self.port, timeout=self.timeout) + # context= is REQUIRED (#323): SMTP_SSL's own default is an unverified stdlib context. + return smtplib.SMTP_SSL( + self.host, self.port, timeout=self.timeout, context=self._tls_context + ) smtp = smtplib.SMTP(self.host, self.port, timeout=self.timeout) if self.use_tls: - smtp.starttls() + smtp.starttls(context=self._tls_context) return smtp def _send(self, payload: str) -> None: diff --git a/scripts/security/crypto_inventory_check.py b/scripts/security/crypto_inventory_check.py index 590626e1..55ff4e07 100644 --- a/scripts/security/crypto_inventory_check.py +++ b/scripts/security/crypto_inventory_check.py @@ -228,7 +228,15 @@ "messagefoundry/transports/dicomweb.py": frozenset({"secrets"}), # ADR 0085: DIRECT-HISP outbound S/MIME — cryptography.serialization.pkcs7 SIGN then ENCRYPT of the # Handler body + x509 recipient-cert / trust-anchor cross-validation at construction. No new dependency. - "messagefoundry/transports/direct.py": frozenset({"cryptography"}), + # #323: ssl = the SMTP/HISP relay's TRANSPORT hop, a separate concern from the S/MIME message + # layer above. The context is built by tls_policy.build_smtp_tls_context and handed to smtplib, + # which otherwise defaults to ssl._create_stdlib_context -- which IS _create_unverified_context + # (CERT_NONE / check_hostname=False). CERT_NONE only under the CLAMPED tls_verify=false escape. + "messagefoundry/transports/direct.py": frozenset({"cryptography", "ssl"}), + # #323: EMAIL outbound STARTTLS (587) / implicit TLS (465). Same factory, same reason as above -- + # an explicit verifying context anchored to the OS roots, a per-connection tls_ca_file, or + # [tls].internal_ca_file (ADR 0093), because smtplib's own default verifies nothing. + "messagefoundry/transports/email.py": frozenset({"ssl"}), # ADR 0129 (#142): hashlib = sha256 of a source file's identity (name+mtime+size) as a HASHED dedup # key for the leave-in-place processed_files ledger — a DERIVED id, never a cleartext filename (which # can embed an MRN), never logged. Not a secret/keyed primitive. diff --git a/tests/test_alert_recipients.py b/tests/test_alert_recipients.py index e0615cf5..a4b23486 100644 --- a/tests/test_alert_recipients.py +++ b/tests/test_alert_recipients.py @@ -7,6 +7,7 @@ from __future__ import annotations import asyncio +import ssl from typing import Any import pytest @@ -128,7 +129,7 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *a: object) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: pass def send_message(self, msg: Any) -> None: diff --git a/tests/test_alert_sinks.py b/tests/test_alert_sinks.py index ca297758..74025703 100644 --- a/tests/test_alert_sinks.py +++ b/tests/test_alert_sinks.py @@ -5,6 +5,7 @@ from __future__ import annotations import asyncio +import ssl import time from typing import Any @@ -244,7 +245,7 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *a: object) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: sent["tls"] = True def login(self, user: str, password: str) -> None: diff --git a/tests/test_alert_templates.py b/tests/test_alert_templates.py index 56f6d15e..60d7491d 100644 --- a/tests/test_alert_templates.py +++ b/tests/test_alert_templates.py @@ -6,6 +6,7 @@ from __future__ import annotations +import ssl from typing import Any import pytest @@ -99,7 +100,7 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *a: object) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: pass def send_message(self, msg: Any) -> None: diff --git a/tests/test_direct_transport.py b/tests/test_direct_transport.py index 6dfab1a4..a2ad7822 100644 --- a/tests/test_direct_transport.py +++ b/tests/test_direct_transport.py @@ -22,6 +22,7 @@ import datetime import smtplib +import ssl from email.message import EmailMessage from pathlib import Path from typing import Any @@ -158,12 +159,18 @@ class _FakeSMTP: instances: list[_FakeSMTP] = [] def __init__( - self, host: str, port: int, timeout: float = 0.0, fail_at: str | None = None + self, + host: str, + port: int, + timeout: float = 0.0, + fail_at: str | None = None, + context: ssl.SSLContext | None = None, ) -> None: self.host = host self.port = port self.timeout = timeout self.fail_at = fail_at + self.tls_context = context # #323 — see test_email_destination._FakeSMTP self.started_tls = False self.logged_in: tuple[str, str] | None = None self.sent: list[EmailMessage] = [] @@ -179,9 +186,10 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *exc: Any) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: if self.fail_at == "starttls": raise smtplib.SMTPException("STARTTLS not supported") + self.tls_context = context self.started_tls = True def ehlo_or_helo_if_needed(self) -> None: @@ -208,8 +216,10 @@ def _install_fake( ) -> type[_FakeSMTP]: _FakeSMTP.instances = [] - def factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: - return _FakeSMTP(host, port, timeout, fail_at=fail_at) + def factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: + return _FakeSMTP(host, port, timeout, fail_at=fail_at, context=context) monkeypatch.setattr(direct_mod.smtplib, "SMTP", factory) monkeypatch.setattr(direct_mod.smtplib, "SMTP_SSL", factory) diff --git a/tests/test_email_destination.py b/tests/test_email_destination.py index 45e1f9e3..75bd4a55 100644 --- a/tests/test_email_destination.py +++ b/tests/test_email_destination.py @@ -7,6 +7,7 @@ from __future__ import annotations import smtplib +import ssl from email.message import EmailMessage from typing import Any @@ -33,12 +34,22 @@ class _FakeSMTP: instances: list[_FakeSMTP] = [] def __init__( - self, host: str, port: int, timeout: float = 0.0, fail_at: str | None = None + self, + host: str, + port: int, + timeout: float = 0.0, + fail_at: str | None = None, + context: ssl.SSLContext | None = None, ) -> None: self.host = host self.port = port self.timeout = timeout self.fail_at = fail_at + # #323: the context the connector handed us — on 465 via SMTP_SSL(context=), on 587 via + # starttls(context=). Recorded (not ignored) so a test can assert it actually verifies: + # smtplib's own default is ssl._create_unverified_context, so "a context was passed" is the + # whole point and a fake that silently swallowed it would hide a regression. + self.tls_context = context self.started_tls = False self.logged_in: tuple[str, str] | None = None self.sent: list[EmailMessage] = [] @@ -54,9 +65,10 @@ def __enter__(self) -> _FakeSMTP: def __exit__(self, *exc: Any) -> None: return None - def starttls(self) -> None: + def starttls(self, context: ssl.SSLContext | None = None) -> None: if self.fail_at == "starttls": raise smtplib.SMTPException("STARTTLS not supported") + self.tls_context = context self.started_tls = True def ehlo_or_helo_if_needed(self) -> None: @@ -83,8 +95,10 @@ def _install_fake( ) -> type[_FakeSMTP]: _FakeSMTP.instances = [] - def factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: - return _FakeSMTP(host, port, timeout, fail_at=fail_at) + def factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: + return _FakeSMTP(host, port, timeout, fail_at=fail_at, context=context) monkeypatch.setattr(email_mod.smtplib, "SMTP", factory) monkeypatch.setattr(email_mod.smtplib, "SMTP_SSL", factory) @@ -158,13 +172,17 @@ async def test_port_465_uses_implicit_tls_not_starttls(monkeypatch: pytest.Monke # On 465 the whole session is wrapped (SMTP_SSL), so no explicit STARTTLS is issued. captured: dict[str, str] = {} - def ssl_factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: + def ssl_factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: captured["which"] = "SMTP_SSL" - return _FakeSMTP(host, port, timeout) + return _FakeSMTP(host, port, timeout, context=context) - def plain_factory(host: str, port: int, timeout: float = 0.0) -> _FakeSMTP: + def plain_factory( + host: str, port: int, timeout: float = 0.0, context: ssl.SSLContext | None = None + ) -> _FakeSMTP: captured["which"] = "SMTP" - return _FakeSMTP(host, port, timeout) + return _FakeSMTP(host, port, timeout, context=context) _FakeSMTP.instances = [] monkeypatch.setattr(email_mod.smtplib, "SMTP_SSL", ssl_factory) @@ -450,3 +468,131 @@ def test_email_and_smtp_factories_exported() -> None: assert spec.settings["host"] == "smtp.partner.org" assert spec.settings["port"] == 587 # STARTTLS submission default assert "Email" in mf.__all__ and "SMTP" in mf.__all__ + + +# --- #323: the TLS hop actually VERIFIES ------------------------------------------------------------ +# +# These are the regression fence for #323. Before it, both arms called smtplib with NO context, and +# smtplib's fallback is ssl._create_stdlib_context — which IS ssl._create_unverified_context +# (CERT_NONE / check_hostname=False). So `use_tls=True` bought encryption without authentication and +# every certificate was accepted. Asserting "STARTTLS was issued" (the pre-existing tests) could never +# catch that: it was issued, over an unverified session. These assert the CONTEXT, which is the only +# thing that distinguishes the fixed state from the broken one. Run them against the pre-fix connector +# and they go red on `tls_context is None`. + + +async def test_starttls_gets_a_verifying_context(monkeypatch: pytest.MonkeyPatch) -> None: + _install_fake(monkeypatch) + await EmailDestination(_dest()).send("body") + [smtp] = _FakeSMTP.instances + assert smtp.started_tls is True + ctx = smtp.tls_context + assert ctx is not None, "starttls() was called with no context — smtplib would not verify" + assert ctx.verify_mode is ssl.CERT_REQUIRED + assert ctx.check_hostname is True + assert ctx.minimum_version is ssl.TLSVersion.TLSv1_2 + + +async def test_implicit_tls_465_gets_a_verifying_context(monkeypatch: pytest.MonkeyPatch) -> None: + # The 465 arm builds SMTP_SSL, a different smtplib entry point with its own unverified default. + _install_fake(monkeypatch) + await EmailDestination(_dest(port=465)).send("body") + [smtp] = _FakeSMTP.instances + assert smtp.started_tls is False # implicit TLS — no explicit STARTTLS + ctx = smtp.tls_context + assert ctx is not None, "SMTP_SSL was constructed with no context — it would not verify" + assert ctx.verify_mode is ssl.CERT_REQUIRED + assert ctx.check_hostname is True + + +def test_tls_verify_false_refused_without_escape(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv(INSECURE_TLS_ESCAPE_ENV, raising=False) + with pytest.raises(ValueError, match="tls_verify=false"): + EmailDestination(_dest(tls_verify=False)) + + +def test_tls_verify_false_refuses_credentials_even_with_escape( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # An unverified session is as bad as cleartext for an AUTH exchange: an on-path attacker + # presenting any certificate captures it. Mirrors the use_tls=false credential refusal. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with pytest.raises(ValueError, match="UNVERIFIED"): + EmailDestination(_dest(tls_verify=False, username="svc", password="pw")) + + +async def test_tls_verify_false_with_escape_builds_an_unverified_context( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + _install_fake(monkeypatch) + await EmailDestination(_dest(tls_verify=False)).send("body") + [smtp] = _FakeSMTP.instances + ctx = smtp.tls_context + assert ctx is not None # still a context — just a deliberately non-verifying one + assert ctx.verify_mode is ssl.CERT_NONE + assert ctx.check_hostname is False + + +def test_tls_verify_false_refused_on_enforcing_phi_even_with_escape( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The clamp (#200, ADR 0092 decision 2): the blunt process-wide escape must NOT silence a + # verify-off PHI hop on an enforcing instance. This is why the arm reads + # weakened_tls_escape_permitted_here() and not the raw insecure_tls_allowed(). + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with ( + active_hop_posture(HopPosture(enforcing=True, is_phi=True)), + pytest.raises(ValueError, match="tls_verify=false"), + ): + EmailDestination(_dest(tls_verify=False)) + + +async def test_tls_ca_file_pins_that_ca_only( + monkeypatch: pytest.MonkeyPatch, tmp_path: Any +) -> None: + # A per-connection CA wins verbatim, and pins to ONLY that CA — no system bundle (ADR 0093 + # precedence #1). This is the supported route for a private-CA relay, and the reason + # tls_verify=false should almost never be needed. + import datetime + + from cryptography import x509 + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import rsa + from cryptography.x509.oid import NameOID + + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "Test Relay CA")]) + now = datetime.datetime.now(datetime.UTC) + cert = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - datetime.timedelta(days=1)) + .not_valid_after(now + datetime.timedelta(days=3650)) + .add_extension(x509.BasicConstraints(ca=True, path_length=None), critical=True) + .sign(key, hashes.SHA256()) + ) + ca = tmp_path / "relay-ca.pem" + ca.write_bytes(cert.public_bytes(serialization.Encoding.PEM)) + + _install_fake(monkeypatch) + await EmailDestination(_dest(tls_ca_file=str(ca))).send("body") + [smtp] = _FakeSMTP.instances + ctx = smtp.tls_context + assert ctx is not None + assert ctx.verify_mode is ssl.CERT_REQUIRED + loaded = ctx.get_ca_certs() + assert len(loaded) == 1, ( + "a per-connection CA must pin to ONLY that CA, not augment system roots" + ) + assert dict(x[0] for x in loaded[0]["subject"])["commonName"] == "Test Relay CA" + + +def test_tls_ca_file_that_does_not_exist_fails_loudly(tmp_path: Any) -> None: + # Silently falling back to system roots on an unreadable CA would be the worst outcome: the + # operator believes they pinned, and did not. + with pytest.raises((FileNotFoundError, ssl.SSLError, OSError)): + EmailDestination(_dest(tls_ca_file=str(tmp_path / "nope.pem"))) diff --git a/tests/test_tls_policy.py b/tests/test_tls_policy.py index 014639af..c55864d9 100644 --- a/tests/test_tls_policy.py +++ b/tests/test_tls_policy.py @@ -4,6 +4,7 @@ from __future__ import annotations +import contextlib import inspect import ssl import types @@ -21,6 +22,7 @@ InsecureHopRefused, _is_forward_secret, active_hop_posture, + build_smtp_tls_context, current_hop_posture, enforce_insecure_hop, fips_attestation, @@ -611,3 +613,138 @@ def test_the_call_site_scan_examined_real_files() -> None: assert sites >= 5, ( f"expected the known TLS context sites, found {sites} — the scan is not landing" ) + + +# --- #323: build_smtp_tls_context REFUSES an untrusted peer (behavioural, not attribute) ------------ +# +# The connector tests assert this context's ATTRIBUTES (verify_mode, check_hostname). That is not the +# same claim as "it refuses a bad certificate", and an attribute check cannot establish it — the whole +# defect being fixed was a context whose attributes were never inspected by anyone. These drive a REAL +# TLS handshake against a locally-minted self-signed server so the refusal is OBSERVED. Run against the +# pre-#323 code path (smtplib's default context) every REFUSED case below returns HANDSHAKE OK, which +# is precisely the bug. +# +# No network: binds 127.0.0.1 on an ephemeral port, and the server thread only completes a handshake +# and closes — it speaks no SMTP, because the property under test is the TLS layer, not the protocol. + + +@pytest.fixture(scope="module") +def _tls_peer(tmp_path_factory: pytest.TempPathFactory) -> tuple[int, str]: + """A local TLS server with a self-signed 'localhost' cert. Yields (port, ca_pem_path).""" + import datetime + import socket + import threading + + from cryptography import x509 + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import rsa + from cryptography.x509.oid import NameOID + + tmp = tmp_path_factory.mktemp("smtp_tls") + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "localhost")]) + now = datetime.datetime.now(datetime.UTC) + cert = ( + x509.CertificateBuilder() + .subject_name(name) + .issuer_name(name) + .public_key(key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - datetime.timedelta(days=1)) + .not_valid_after(now + datetime.timedelta(days=365)) + .add_extension(x509.SubjectAlternativeName([x509.DNSName("localhost")]), critical=False) + .sign(key, hashes.SHA256()) + ) + cert_p, key_p = tmp / "server.pem", tmp / "server.key" + cert_p.write_bytes(cert.public_bytes(serialization.Encoding.PEM)) + key_p.write_bytes( + key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + ) + + srv = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + srv.load_cert_chain(cert_p, key_p) + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + listener.listen(8) + + def _accept_loop() -> None: + while True: + try: + conn, _ = listener.accept() + except OSError: + return # listener closed at teardown + threading.Thread(target=_handshake, args=(conn,), daemon=True).start() + + def _handshake(conn: socket.socket) -> None: + # A client that (correctly) refuses us aborts mid-handshake, so OSError here is the EXPECTED + # path for the refusal tests, not an error -- suppress on both legs. + with contextlib.suppress(OSError): + srv.wrap_socket(conn, server_side=True).close() + with contextlib.suppress(OSError): + conn.close() + + threading.Thread(target=_accept_loop, daemon=True).start() + yield listener.getsockname()[1], str(cert_p) + listener.close() + + +def _handshake_result(ctx: ssl.SSLContext, port: int, server_hostname: str) -> str: + import socket + + try: + with ( + socket.create_connection(("127.0.0.1", port), timeout=10) as sock, + ctx.wrap_socket(sock, server_hostname=server_hostname), + ): + return "ok" + except ssl.SSLCertVerificationError: + return "refused" + except OSError: # a reset mid-refusal still means the client did not accept the peer + return "refused" + + +def test_smtp_context_refuses_an_untrusted_peer(_tls_peer: tuple[int, str]) -> None: + """THE regression fence for #323. Pre-fix this returned 'ok' — any certificate was accepted.""" + port, _ = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination") + assert _handshake_result(ctx, port, "localhost") == "refused" + + +def test_smtp_context_accepts_a_peer_signed_by_the_connection_ca( + _tls_peer: tuple[int, str], +) -> None: + """tls_ca_file is the supported route for a private-CA relay — it must actually work, or the + only remaining escape would be turning verification off.""" + port, ca = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination", ca_file=ca) + assert _handshake_result(ctx, port, "localhost") == "ok" + + +def test_smtp_context_enforces_hostname(_tls_peer: tuple[int, str]) -> None: + """A trusted chain is not enough: the cert must match the host we meant to reach.""" + port, ca = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination", ca_file=ca) + assert _handshake_result(ctx, port, "wrong.example") == "refused" + + +def test_smtp_context_check_hostname_false_relaxes_only_the_name( + _tls_peer: tuple[int, str], +) -> None: + """tls_check_hostname=False drops the name check while KEEPING chain validation.""" + port, ca = _tls_peer + ctx = build_smtp_tls_context( + host="localhost", cell="Email destination", ca_file=ca, check_hostname=False + ) + assert _handshake_result(ctx, port, "wrong.example") == "ok" + + +def test_smtp_context_verify_false_accepts_anything(_tls_peer: tuple[int, str]) -> None: + """The escape genuinely disables verification — asserted so nobody 'hardens' it into a + silently-broken state where the escape no longer connects and operators cannot tell why.""" + port, _ = _tls_peer + ctx = build_smtp_tls_context(host="localhost", cell="Email destination", verify=False) + assert _handshake_result(ctx, port, "localhost") == "ok"