Skip to content

Commit e539a4e

Browse files
committed
fix(smtp): the EMAIL and DIRECT TLS hops were encrypted but unauthenticated (#323, layers 1-2)
smtplib takes no context by default and falls back to ssl._create_stdlib_context, which IS ssl._create_unverified_context -- measured on this project's required interpreter (CPython 3.14.6): verify_mode=CERT_NONE, check_hostname=False. So use_tls=true bought encryption without authentication on every SMTP send, and any certificate was accepted. That is worse than a plain gap because three shipped controls asserted the opposite: * transports/email.py registered a RevocationHopGuard on the hop, whose own definition in tls_policy.py says "the caller has already built a verifying context". An enforcing production-PHI instance therefore REFUSED TO START over a possibly-REVOKED certificate, on a hop that never validated a certificate at all. * the same file's comment claimed STARTTLS/SMTP_SSL "verifies the server cert". * the AUTH refusal keyed only on use_tls=false, so with TLS "on" the password went over the unauthenticated hop. WHAT LANDS (2 of the 3 cells): config/tls_policy.py build_smtp_tls_context() -- the shared verifying-context factory, mirroring remotefile.py's _ftps_ssl_context step for step (TLS 1.2 floor, harden_kex_groups, harden_cipher_suites, harden_verify_flags on the verify path). It lives in config/ rather than transports/ because pipeline/alert_sinks.py is the third caller and a transport must not import pipeline/ (ADR 0029's one-way rule). transports/email.py, transports/direct.py a three-arm branch (cleartext / verify-off / verifying) and context= on both smtplib arms. The verify-off arm refuses unless the CLAMPED weakened_tls_escape_permitted_here() allows it, and refuses AUTH outright. config/wiring.py tls_verify / tls_ca_file / tls_check_hostname on Email() and Direct(). Trust config, not verification-off, is the escape: [tls].internal_ca_file is ALREADY threaded onto every Destination and was simply never read here, so an estate that pinned its internal CA for MLLP/FTPS needs no change at all. SEPARABLE FIX, called out rather than folded in silently: direct.py's cleartext arm read the UNCLAMPED insecure_tls_allowed() while its sibling one branch away read the clamped form. It now reads the clamped one -- strictly ADDS refusals (ADR 0092 decision 5). Partially closes #329. VERIFICATION -- the part that matters. The pre-existing tests asserted "STARTTLS was issued", which was true the whole time it was insecure; that assertion could never have caught this. The eight new tests assert the CONTEXT (CERT_REQUIRED, check_hostname, TLS1.2 floor, CERT_NONE only under the escape, the clamp under enforcing PHI, and that a per-connection CA pins to ONLY that CA). Negative control run: with the code change stashed and the tests kept, all eight go RED. ruff + format clean; mypy unchanged at its 21-error pre-existing baseline (missing pynetdicom / webauthn extras, none in touched files); 437 targeted tests green. DELIBERATELY NOT DONE -- the alerts cell (pipeline/alert_sinks.py:384) still calls starttls() bare. It needs an acknowledgment switch rather than the clamp, because the contextvar hop posture is never stamped for that cell. Tracked as the residual on #323. #139's "verifying context by design" claim therefore remains FALSE and is not corrected here. BLOCKED, needs one follow-up commit: adding `ssl` to transports/{email,direct}.py reds the required crypto-inventory gate until scripts/security/crypto_inventory_check.py documents it. That file is checked out live in another session; the collision gate refused the edit and I asked that session for the two lines rather than clobbering their work. docs/BACKLOG.md (#323's banner, #139) is held by two other sessions for the same reason.
1 parent 884036f commit e539a4e

9 files changed

Lines changed: 391 additions & 37 deletions

File tree

messagefoundry/config/tls_policy.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
"TrustAnchorMode",
6363
"TrustAnchorPolicy",
6464
"active_hop_posture",
65+
"build_smtp_tls_context",
6566
"build_verifying_client_context",
6667
"cleartext_acceptance_audit_sink",
6768
"current_hop_posture",
@@ -919,3 +920,61 @@ def build_verifying_client_context(
919920
return ctx
920921
# pinned / per-connection: ONLY this CA (no load_default_certs), matching forward_tls_ca_file.
921922
return ssl.create_default_context(purpose, cafile=anchor.cafile)
923+
924+
925+
def build_smtp_tls_context(
926+
*,
927+
host: str,
928+
cell: str,
929+
verify: bool = True,
930+
ca_file: str | None = None,
931+
check_hostname: bool = True,
932+
trust_anchor_policy: TrustAnchorPolicy | None = None,
933+
) -> ssl.SSLContext:
934+
"""Build the TLS context for an outbound SMTP hop (#323) — STARTTLS or implicit ``SMTP_SSL``.
935+
936+
``smtplib`` accepts no context by default, and its fallback is
937+
:func:`ssl._create_stdlib_context`, which **is** ``ssl._create_unverified_context`` — measured on
938+
CPython 3.14.6: ``verify_mode=CERT_NONE``, ``check_hostname=False``. So every certificate was
939+
accepted and the encrypted session was unauthenticated: an on-path attacker presenting any
940+
certificate read the message body (PHI) and the SMTP AUTH credential. This is the SMTP sibling of
941+
:func:`~messagefoundry.transports.remotefile._ftps_ssl_context` and the MLLP outbound arm, and is
942+
deliberately built here rather than in ``transports/`` because ``pipeline/alert_sinks.py`` is a
943+
third caller and a transport must not import ``pipeline/`` (ADR 0029's one-way rule).
944+
945+
``verify=False`` is NOT gated here — the caller refuses it against the clamped
946+
:func:`~messagefoundry.config.settings.weakened_tls_escape_permitted_here` first, matching how
947+
MLLP/FTPS inline that check. This module cannot read it: ``config.settings`` imports *from* here,
948+
so the dependency runs one way only.
949+
950+
``trust_anchor_policy`` (#190, ADR 0093) supplies the instance ``[tls]`` internal-CA fallback when
951+
the connection names no ``ca_file`` of its own. It only chooses WHICH roots verify the peer — it
952+
never turns verification off.
953+
"""
954+
if verify:
955+
anchor = resolve_trust_anchor(
956+
connection_ca_file=ca_file,
957+
host=host,
958+
policy=trust_anchor_policy if trust_anchor_policy is not None else TrustAnchorPolicy(),
959+
)
960+
ctx = build_verifying_client_context(anchor)
961+
else:
962+
ctx = ssl.create_default_context(ssl.Purpose.SERVER_AUTH, cafile=ca_file)
963+
ctx.minimum_version = ssl.TLSVersion.TLSv1_2
964+
if verify:
965+
ctx.check_hostname = check_hostname
966+
else:
967+
logger.warning(
968+
"%s TLS certificate verification is DISABLED (tls_verify=false) — the SMTP session to %s "
969+
"is encrypted but UNAUTHENTICATED and MITM-able; trusted-network dev/test only.",
970+
cell,
971+
host,
972+
)
973+
# Order is load-bearing: check_hostname must go False BEFORE verify_mode, or ssl raises.
974+
ctx.check_hostname = False
975+
ctx.verify_mode = ssl.CERT_NONE
976+
harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2)
977+
harden_cipher_suites(ctx, connector=cell) # assert forward secrecy (ASVS 12.1.2)
978+
if verify: # nothing to strict-validate on the CERT_NONE path (ASVS 12.1.4)
979+
harden_verify_flags(ctx)
980+
return ctx

messagefoundry/config/wiring.py

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1785,6 +1785,9 @@ def Email(
17851785
username: str | EnvRef | None = None, # optional SMTP AUTH user (use env() for the secret)
17861786
password: str | EnvRef | None = None, # optional SMTP AUTH password (use env() for the secret)
17871787
use_tls: bool = True, # STARTTLS by default; False (dev only) needs MEFOR_ALLOW_INSECURE_TLS
1788+
tls_verify: bool = True, # verify the server cert (#323); False (dev only) needs the escape
1789+
tls_ca_file: str | EnvRef | None = None, # PEM to verify the SMTP server against (not a secret)
1790+
tls_check_hostname: bool = True, # match the cert against `host` (leave on)
17881791
timeout_seconds: float = 30.0,
17891792
encoding: str = "utf-8",
17901793
) -> ConnectionSpec:
@@ -1793,9 +1796,18 @@ def Email(
17931796
text); this delivers it as a plain-text SMTP message to ``host:port`` from ``sender`` to
17941797
``recipients`` with a static ``subject``. STARTTLS by default (``use_tls=True``) on the ``587``
17951798
submission port; port ``465`` is implicit TLS (``SMTP_SSL``). Optional ``username``/``password`` do
1796-
SMTP ``AUTH`` (over TLS only — a cleartext-credential config is refused). Disabling TLS
1797-
(``use_tls=False``) is MITM-able and refused unless ``MEFOR_ALLOW_INSECURE_TLS`` is set (loud
1798-
warning), like LDAPS / SQL Server / MLLP. The egress host is gated by ``[egress].allowed_smtp``. Put
1799+
SMTP ``AUTH`` (over a **verified** TLS session only — a cleartext- or unverified-credential config
1800+
is refused). Disabling TLS (``use_tls=False``) is MITM-able and refused unless
1801+
``MEFOR_ALLOW_INSECURE_TLS`` is set (loud warning), like LDAPS / SQL Server / MLLP.
1802+
1803+
**The server certificate is verified** (``tls_verify=True``, #323) — chain, hostname and strict RFC
1804+
5280 flags, anchored to the OS roots, a per-connection ``tls_ca_file``, or the instance-wide
1805+
``[tls].internal_ca_file`` (ADR 0093). ``smtplib``'s own default context verifies **nothing**
1806+
(``CERT_NONE``/``check_hostname=False``), so before #323 ``use_tls=True`` bought encryption without
1807+
authentication. Point ``tls_ca_file`` at your relay's CA PEM for a private-CA server;
1808+
``tls_verify=False`` is a trusted-network dev/test escape, refused on an enforcing production-PHI
1809+
instance even with ``MEFOR_ALLOW_INSECURE_TLS``, and it also refuses SMTP ``AUTH``.
1810+
The egress host is gated by ``[egress].allowed_smtp``. Put
17991811
secrets in ``env()`` (``username``/``password``), never inline. Delivery is at-least-once, so a retry
18001812
re-sends the email — a mailbox has no idempotency key, so a rare duplicate is possible and accepted
18011813
(a duplicate beats a drop). ADR 0029."""
@@ -1810,6 +1822,9 @@ def Email(
18101822
"username": username,
18111823
"password": password,
18121824
"use_tls": use_tls,
1825+
"tls_verify": tls_verify,
1826+
"tls_ca_file": tls_ca_file,
1827+
"tls_check_hostname": tls_check_hostname,
18131828
"timeout_seconds": timeout_seconds,
18141829
"encoding": encoding,
18151830
},
@@ -1836,6 +1851,9 @@ def Direct(
18361851
username: str | EnvRef | None = None, # optional SMTP AUTH user (use env() for the secret)
18371852
password: str | EnvRef | None = None, # optional SMTP AUTH password (use env() for the secret)
18381853
use_tls: bool = True, # STARTTLS by default; False (dev only) needs MEFOR_ALLOW_INSECURE_TLS
1854+
tls_verify: bool = True, # verify the relay's cert (#323); False (dev only) needs the escape
1855+
tls_ca_file: str | EnvRef | None = None, # PEM to verify the SMTP/HISP relay against
1856+
tls_check_hostname: bool = True, # match the cert against `host` (leave on)
18391857
timeout_seconds: float = 30.0,
18401858
encoding: str = "utf-8",
18411859
) -> ConnectionSpec:
@@ -1844,7 +1862,14 @@ def Direct(
18441862
**body** (content-agnostic — an HL7 string, a CDA/XML document, plain text); this **signs** it with
18451863
``signing_key``/``signing_cert``, **encrypts** the signed blob to the partner's ``recipient_cert``
18461864
(which must chain to ``trust_anchor``), and submits the S/MIME message to ``host:port`` over
1847-
STARTTLS. All cert/key material is loaded + validated at construction (fail loud). The egress host
1865+
STARTTLS. All cert/key material is loaded + validated at construction (fail loud).
1866+
1867+
**The relay's TLS certificate is verified** (``tls_verify=True``, #323 — ``smtplib``'s own default
1868+
verifies nothing). Note the two trust settings are unrelated and easy to confuse: ``trust_anchor``
1869+
is the CA the **partner's S/MIME certificate** must chain to (message-layer), while ``tls_ca_file``
1870+
is the CA the **SMTP relay's TLS certificate** must chain to (transport-layer). The S/MIME body
1871+
protects the clinical payload either way, but the SMTP session still carries envelope metadata and
1872+
any ``AUTH`` credential, which is why the transport hop is verified too. The egress host
18481873
is gated by ``[egress].allowed_direct``. Put secrets in ``env()`` (``signing_key_password``,
18491874
``username``/``password``), never inline. Delivery is at-least-once, so a retry re-sends — a Direct
18501875
mailbox has no idempotency key, so a rare duplicate is possible and accepted (a duplicate beats a
@@ -1865,6 +1890,9 @@ def Direct(
18651890
"username": username,
18661891
"password": password,
18671892
"use_tls": use_tls,
1893+
"tls_verify": tls_verify,
1894+
"tls_ca_file": tls_ca_file,
1895+
"tls_check_hostname": tls_check_hostname,
18681896
"timeout_seconds": timeout_seconds,
18691897
"encoding": encoding,
18701898
},

messagefoundry/transports/direct.py

Lines changed: 60 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import asyncio
4747
import logging
4848
import smtplib
49+
import ssl
4950
from collections.abc import Mapping
5051
from email.message import EmailMessage
5152
from pathlib import Path
@@ -57,7 +58,11 @@
5758
from cryptography.hazmat.primitives.serialization import pkcs7
5859

5960
from messagefoundry.config.models import ConnectorType, Destination
60-
from messagefoundry.config.settings import INSECURE_TLS_ESCAPE_ENV, insecure_tls_allowed
61+
from messagefoundry.config.settings import (
62+
INSECURE_TLS_ESCAPE_ENV,
63+
weakened_tls_escape_permitted_here,
64+
)
65+
from messagefoundry.config.tls_policy import build_smtp_tls_context
6166
from messagefoundry.transports.base import (
6267
DeliveryError,
6368
DeliveryResponse,
@@ -140,6 +145,13 @@ def __init__(self, config: Destination) -> None:
140145
self.username: str | None = str(username) if username else None
141146
self.password: str | None = str(password) if password else None
142147
self.use_tls = bool(s.get("use_tls", True))
148+
# #323: server-certificate verification on the TLS hop, kept byte-identical to
149+
# EmailDestination's spelling (this connector's SMTP core is a deliberate copy, not an import —
150+
# the one-way dependency rule — so the two must not drift).
151+
self.tls_verify = bool(s.get("tls_verify", True))
152+
tls_ca_file = s.get("tls_ca_file")
153+
self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None
154+
self.tls_check_hostname = bool(s.get("tls_check_hostname", True))
143155
self.timeout: float = float(s.get("timeout_seconds", 30.0))
144156
self.encoding: str = str(s.get("encoding", "utf-8"))
145157

@@ -167,11 +179,16 @@ def __init__(self, config: Destination) -> None:
167179
# SMTP is refused unless the project-wide dev escape is set, and credentials are NEVER sent over
168180
# a cleartext channel.
169181
if not self.use_tls:
170-
if not insecure_tls_allowed():
182+
# #323: read through the CLAMPED escape, not the raw insecure_tls_allowed(). This call site
183+
# was the last unclamped one in the file, sitting one branch away from the clamped arm
184+
# below — two different escapes in one connector is how the next bug gets written. The
185+
# change strictly ADDS refusals (ADR 0092 decision 5): an enforcing production-PHI instance
186+
# can no longer silence a cleartext Direct hop with the blunt process-wide env var.
187+
if not weakened_tls_escape_permitted_here():
171188
raise ValueError(
172189
"Direct destination use_tls=false submits over cleartext SMTP; refused unless "
173-
f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only) — use STARTTLS "
174-
"(the default)"
190+
f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only, and refused on a "
191+
"production-PHI instance even with the escape, #200) — use STARTTLS (the default)"
175192
)
176193
if self.username is not None:
177194
raise ValueError(
@@ -183,6 +200,40 @@ def __init__(self, config: Destination) -> None:
183200
"network in CLEARTEXT (dev/trusted-network only)",
184201
self.host,
185202
)
203+
elif not self.tls_verify:
204+
# #323 arm 2 — same shape and wording as EmailDestination's, deliberately.
205+
if not weakened_tls_escape_permitted_here():
206+
raise ValueError(
207+
"Direct destination tls_verify=false disables server-certificate verification on "
208+
f"the SMTP hop to {self.host} — the session is encrypted but UNAUTHENTICATED. The "
209+
"S/MIME body still protects the clinical payload, but envelope metadata and any "
210+
"SMTP AUTH credential are exposed to an on-path attacker presenting any "
211+
"certificate. Use a trusted CA (tls_ca_file, or [tls].internal_ca_file for the "
212+
f"instance), or set {INSECURE_TLS_ESCAPE_ENV}=1 to allow it on a trusted-network "
213+
"bind (refused on a production-PHI instance even with the escape, #200)."
214+
)
215+
if self.username is not None:
216+
raise ValueError(
217+
"Direct destination sends SMTP AUTH credentials over an UNVERIFIED TLS session "
218+
"(tls_verify=false); refused — credentials require a verified TLS session"
219+
)
220+
# Built once at construction (fail-fast), reused by every send. None when TLS is off entirely.
221+
# DIRECT does not take a RevocationHopGuard even though the hop now verifies: adding it would
222+
# make the enumerated count eight and force four "seven verifying hops" docs to change, and the
223+
# clinical payload is S/MIME-protected at the message layer so the PHI argument is materially
224+
# weaker than EMAIL's (ADR 0085). Recorded rather than silently omitted.
225+
self._tls_context: ssl.SSLContext | None = (
226+
build_smtp_tls_context(
227+
host=self.host,
228+
cell="Direct destination",
229+
verify=self.tls_verify,
230+
ca_file=self.tls_ca_file,
231+
check_hostname=self.tls_check_hostname,
232+
trust_anchor_policy=config.trust_anchor_policy,
233+
)
234+
if self.use_tls
235+
else None
236+
)
186237

187238
def _load_private_key(self, value: Any, password: Any) -> Any:
188239
"""Load the sender's signing private key (PEM/DER, optionally passphrase-protected). PHI/secret-
@@ -317,10 +368,13 @@ def _connect(self) -> smtplib.SMTP:
317368
"""Open an SMTP connection, applying STARTTLS / implicit TLS per config (identical posture to
318369
EmailDestination). The caller closes it (``with`` / ``quit``)."""
319370
if self.port == 465 and self.use_tls:
320-
return smtplib.SMTP_SSL(self.host, self.port, timeout=self.timeout)
371+
# context= is REQUIRED (#323) — SMTP_SSL's own default is an unverified stdlib context.
372+
return smtplib.SMTP_SSL(
373+
self.host, self.port, timeout=self.timeout, context=self._tls_context
374+
)
321375
smtp = smtplib.SMTP(self.host, self.port, timeout=self.timeout)
322376
if self.use_tls:
323-
smtp.starttls()
377+
smtp.starttls(context=self._tls_context)
324378
return smtp
325379

326380
def _send(self, payload: str) -> None:

0 commit comments

Comments
 (0)