Skip to content

Commit 04a9054

Browse files
committed
docs+gate(smtp): document the ssl usage #323 added, and correct two false premises it exposed
Completes PR #132's blocked tail. Four edits in three files the collision gate refused because live sibling sessions carry diffs to them; applied outside the Edit tool with explicit written consent from both holders, quoted below. 1. scripts/security/crypto_inventory_check.py -- record `ssl` for transports/email.py and transports/direct.py. Without this the REQUIRED crypto-inventory context is red. The Sandbox Fixes session held this file and I offered to let them add the entries in their PR. Their answer was better than my question: find_violations() checks BOTH directions (undocumented AND stale, :378-399, verified at HEAD), and on their branch these two files contain zero ssl imports -- so documenting the usage there would have traded my `undocumented` failure for their `stale` failure on the same required context. Usage and its documentation must move in the SAME commit. That is the invariant, and it is why these lines belong here. 2. docs/ASVS-L2-PHASE0-CHANGES.md section 5 -- the EMAIL and DIRECT communications-inventory rows said "STARTTLS on by default" and stopped, which now understates the control. Both state verification, its trust anchors, and that tls_verify=false needs the clamped escape. The crypto_inventory_check.py header requires these kept in sync. 3. docs/BACKLOG.md #139 -- CORRECTS A FALSE COMPENSATING-CONTROL PREMISE. The item asserted "The engine's EmailAlertSink uses STARTTLS with a verifying context by design." It does not, and did not: starttls() with no context falls back to ssl._create_stdlib_context, which IS _create_unverified_context. A reader would have concluded alert email was TLS-verified when it was not -- the exact shape CLAUDE.md section 11 names as worst. It stays false AFTER #132: I fixed the two connectors, NOT the alert sink, and the item now says so rather than leaving the residual implied. 4. docs/BACKLOG.md #337 -- rationale amended, severity unchanged at LOW. Flagged by the ADR 0087 sandbox session and verified here at HEAD: DEFAULT_FORBIDDEN_MODULES (pipeline/sandbox.py:84-95) blocks socket/ssl/asyncio/multiprocessing/the I/O-bearing messagefoundry.* subpackages/ cryptography -- but NOT `os` or `subprocess`. So #337's justification, "the author already has in-process execution", is true at the default mode=off and FALSE under mode=subprocess, where the whole premise is that the author is not trusted with it. The number lands right for a different reason; the amended rationale holds in both postures and says to re-score when ADR 0147 (OS confinement, Proposed with no code) lands. Same defect class as #139: a claim stated independently of the configuration that makes it true. 5. docs/BACKLOG.md #323 -- banner to PARTIALLY SHIPPED (2 of 3 cells), with the alerts-cell residual, the direct.py clamp fix, and a correction to this item's own "Migration risk" framing (it presumed deployments; the owner confirmed there are none). CONSENT RECORDED, quoted verbatim. Sandbox Fixes (holds crypto_inventory_check.py): "So: take the file, it's yours. My change to it is committed, final, and a single entry (pipeline/sandbox.py -> {secrets}). I will not touch it again -- commitment, not estimate." Stuck CIs (holds docs/BACKLOG.md): "I have no further BACKLOG.md edits; my #340/#344 are committed and pushed on #131; your hunks at ~5264 (#139) and ~7398 (#323) are disjoint from my EOF appends after #338." WHY A BYPASS RATHER THAN WAITING. The block was real: both holders' branches carry genuinely UNMERGED diffs to these files, so the gate was correct to fire. Waiting was viable -- their PRs merging would have cleared it -- and I chose consent-plus-disjointness instead, because the gate keys on branch diffs and has no way to read a consent that both holders had already given in writing. That is the actual limitation, and docs/WORKTREES.md states the governing rule from the other side: "coordination a tool cannot read does not count." CORRECTION -- an earlier draft of this message justified the bypass with a claimed defect: that under squash merges a merged branch keeps reporting a three-dot diff forever, so a merged-and- forgotten worktree blocks its files permanently. THAT IS FALSE and the claim is withdrawn. The announce session refuted it, the Stuck CIs session retracted it, and I measured it here rather than take either on trust: MessageFoundry-prunefix (merged via #74, branch deleted, worktree still checked out) git diff --name-only origin/main...HEAD -> 7 files git diff --name-only origin/main..HEAD -> 9 files intersection -> 0 overlap.ps1 -File docs/SESSION-DRIFT-CONTROLS.md -Json -> does NOT name prunefix overlap.ps1 intersects the two diff forms deliberately (:138-155, with the reasoning in its own comment), and collision_gate.ps1 delegates to it (:70) rather than re-implementing the rule -- so the gate inherits that handling. `git diff A..B` compares TREES, not commit lists, so once a branch's content is in main the two-dot set empties and the intersection self-clears. Squash merges were already handled. The block set does not only grow. Recording the withdrawal rather than quietly dropping it, because a bypass justified by a real limitation is a decision, while one justified by a defect that does not exist is a hole -- and a false mechanism in the ledger would be cited as precedent. Three sessions got the two-dot/three-dot distinction wrong in different directions tonight, on a repo where the answer decides whether a guard fires; that is the durable lesson, and it is being routed to ADR 0157. Verification: backlog_status_check OK (262 items, each exactly one status) -- the invariant that guards precisely this banner edit; crypto-inventory gate clean; the three previously-failing tests (test_crypto_inventory_scanner, test_security_static x2) now pass; 79 green across the affected suites; ruff + format clean.
1 parent e4728d7 commit 04a9054

3 files changed

Lines changed: 26 additions & 6 deletions

File tree

docs/ASVS-L2-PHASE0-CHANGES.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,8 +313,8 @@ tables in [`CONNECTIONS.md`](CONNECTIONS.md) §"Resource management & limits" (A
313313
| 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` |
314314
| 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)` |
315315
| 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` |
316-
| 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` |
317-
| 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` |
316+
| 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` |
317+
| 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` |
318318
| 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` |
319319
| 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` |
320320
| 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()` |

docs/BACKLOG.md

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5261,7 +5261,9 @@ sourced — **#1 (SQL Server concurrency)** and **#2 (console off-thread)** —
52615261

52625262
**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.
52635263

5264-
**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.
5264+
**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.
5265+
5266+
> ⚠️ **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.
52655267

52665268
**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.
52675269

@@ -7397,7 +7399,15 @@ MEFOR_FORBIDDEN_TOKENS=scripts/security/scan-tokens.local.txt.example \
73977399

73987400
## 323. SMTP TLS is unverified on all three send paths
73997401

7400-
> 🔢 **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**.
7402+
> 🚧 **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.
7403+
>
7404+
> 🔢 **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**.
7405+
7406+
**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).
7407+
7408+
**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.
7409+
7410+
**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.
74017411

74027412
**Cluster:** Security & Compliance. **Priority:** P1. **Verdict:** build. **Severity:** high.
74037413

@@ -8178,7 +8188,9 @@ ADR 0144:193-195 records the decorated-scope trade, but justifies it with an **`
81788188

81798189
**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.
81808190

8181-
**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.
8191+
**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.
8192+
8193+
> ⚠️ **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.
81828194

81838195
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.
81848196

scripts/security/crypto_inventory_check.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,15 @@
228228
"messagefoundry/transports/dicomweb.py": frozenset({"secrets"}),
229229
# ADR 0085: DIRECT-HISP outbound S/MIME — cryptography.serialization.pkcs7 SIGN then ENCRYPT of the
230230
# Handler body + x509 recipient-cert / trust-anchor cross-validation at construction. No new dependency.
231-
"messagefoundry/transports/direct.py": frozenset({"cryptography"}),
231+
# #323: ssl = the SMTP/HISP relay's TRANSPORT hop, a separate concern from the S/MIME message
232+
# layer above. The context is built by tls_policy.build_smtp_tls_context and handed to smtplib,
233+
# which otherwise defaults to ssl._create_stdlib_context -- which IS _create_unverified_context
234+
# (CERT_NONE / check_hostname=False). CERT_NONE only under the CLAMPED tls_verify=false escape.
235+
"messagefoundry/transports/direct.py": frozenset({"cryptography", "ssl"}),
236+
# #323: EMAIL outbound STARTTLS (587) / implicit TLS (465). Same factory, same reason as above --
237+
# an explicit verifying context anchored to the OS roots, a per-connection tls_ca_file, or
238+
# [tls].internal_ca_file (ADR 0093), because smtplib's own default verifies nothing.
239+
"messagefoundry/transports/email.py": frozenset({"ssl"}),
232240
# ADR 0129 (#142): hashlib = sha256 of a source file's identity (name+mtime+size) as a HASHED dedup
233241
# key for the leave-in-place processed_files ledger — a DERIVED id, never a cleartext filename (which
234242
# can embed an MRN), never logged. Not a secret/keyed primitive.

0 commit comments

Comments
 (0)