From 146611c4dde9c65db0be1715858799f931675fc6 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 10 Aug 2026 23:07:42 -0500 Subject: [PATCH 1/9] fix(approvals): compensate a released-but-unexecuted operation (ASVS 2.3.3) ApprovalGate.approve moves the row to 'approved' BEFORE running the executor. That ordering is load-bearing -- it guards the double-approve race -- and it stays. The gap was what happened when the executor then raised: the row was left asserting an operation that never ran, and because record_audit came AFTER the execute, no approval.approved row was written either. The store therefore carried a released approval with no recorded outcome at all, which is worse than the item describes. The tree already knew about this hazard and had defended exactly one of the three registered operations. _purge's comment states it verbatim -- "a raise would strand the row approved-but-unexecuted" -- and works around it locally by returning a skip result instead of raising. _replay and _config_reload had no such protection, and _config_reload deliberately surfaces ConfigReloadDenied as a raise. So the mitigation existed, in a comment, for one executor out of three. Fixed at the gate rather than per-executor: on a raise the row is rolled 'approved' -> 'failed', an approval.failed row is audited against both identities, and the ORIGINAL error is re-raised so the caller still sees it. Retry-instead-of-compensate was considered and rejected on evidence. The three operations are not uniformly replay-safe: cancel_queued is naturally idempotent, replay_dead would re-requeue, and _config_reload bumps the cluster config version with propagate=True, so a re-drive is not a no-op. Compensation is the only shape that is correct for all three. decide_pending_approval grows a from_status parameter (default 'pending', so every existing caller is unchanged) across the protocol and all three backends. Guarding the compensation on 'approved' means it can never clobber a row another caller rejected or expired, and a re-drive moves zero rows. No CHECK constraint exists on status in any backend, so 'failed' needs no migration. except Exception is deliberate and is not a swallow -- any executor failure must compensate, and the original is re-raised. BaseException is NOT caught, so a cancelled approve is not recorded as a failure. If the compensating transition itself fails it is logged loudly and the original error still wins, because a store that is unreachable here must not mask the error that actually explains the failure. The audit detail records the exception TYPE, never its message: executor text can carry connection names, paths or params, and the audit log is not a PHI sink. Asserted by test. _purge's comment is updated in the same commit. Its premise -- that a raise strands the row -- is false as of this change, and leaving it would be the stale-premise defect the ASVS record exists to catch. The skip behaviour stays, because a non-quiesced outbound is a retryable precondition miss rather than a failed operation. WATCHED FAIL: with the source reverted and the tests present, all three new tests fail, and the first fails on exactly the right assertion -- "assert 'approved' == 'failed'" -- so the defect is demonstrated, not assumed. 12 passed in tests/test_approvals.py with the fix; 113 passed across test_approvals + test_store + test_api. mypy strict is the instrument that checks the three-backend signature change, because the SQL Server and Postgres store legs silently SKIP on this box and would only fail in CI. Clean on all six edited files. The 21 mypy errors in the tree are all import-not-found for optional extras (dicom, fhir, webauthn) in four files this change does not touch. --- messagefoundry/api/app.py | 5 +- messagefoundry/api/approvals.py | 74 ++++++++++++++++++++++++- messagefoundry/store/base.py | 8 ++- messagefoundry/store/postgres.py | 16 ++++-- messagefoundry/store/sqlserver.py | 17 ++++-- messagefoundry/store/store.py | 25 ++++++--- tests/test_approvals.py | 89 ++++++++++++++++++++++++++++++- 7 files changed, 215 insertions(+), 19 deletions(-) diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index fff24937..6ff222f7 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -506,7 +506,10 @@ async def _purge(p: Mapping[str, Any]) -> dict[str, Any]: # Load-bearing dual-control guard (findings #1/#4/#11): ApprovalGate.approve runs THIS executor # directly (purge_connection is NOT re-entered on the release path), and it flips the row to # 'approved' BEFORE executing — so the require-quiesced precondition must be re-checked HERE, and - # a failure must NOT raise (a raise would strand the row approved-but-unexecuted). A non-quiesced + # a failure should NOT raise. (Since ASVS 2.3.3 the gate compensates a raise by rolling the row + # to 'failed' and auditing it, so a raise no longer strands it approved-but-unexecuted; skipping + # is still the better outcome HERE, because a non-quiesced outbound is a retryable precondition + # miss the operator can clear, not a failed operation.) A non-quiesced # (running/stopping) outbound could have an INFLIGHT row cancel_queued cannot cancel, so purging # it would mis-fire; skip fail-closed and record cancelled=0/skipped in the approval audit. The # operator re-Stops (lets it quiesce) and re-requests. diff --git a/messagefoundry/api/approvals.py b/messagefoundry/api/approvals.py index f203d1a8..7f16f1fd 100644 --- a/messagefoundry/api/approvals.py +++ b/messagefoundry/api/approvals.py @@ -16,6 +16,7 @@ from __future__ import annotations import json +import logging import time from collections.abc import Awaitable, Callable, Mapping from dataclasses import dataclass @@ -25,6 +26,8 @@ from messagefoundry.config.settings import ApprovalsSettings from messagefoundry.store.base import Store +log = logging.getLogger(__name__) + #: An executor re-runs a captured operation on approval, returning a small JSON-able result summary. Executor = Callable[[Mapping[str, Any]], Awaitable[dict[str, Any]]] @@ -130,7 +133,28 @@ async def approve( ): raise ApprovalError(409, "request was already decided") params = json.loads(str(row["params"])) - result = await op.execute(params) + try: + result = await op.execute(params) + except Exception as exc: + # ASVS 2.3.3 COMPENSATING TRANSITION. The row moved to 'approved' BEFORE the executor ran + # (that ordering is load-bearing — it guards the double-approve race — and must stay). If + # the executor raises, the row would otherwise be stranded asserting an operation that + # never happened, and no approval.approved row is written either, so the store would carry + # an approval with no outcome at all. Roll it to 'failed' and audit the failure against + # both identities, then re-raise so the caller still sees the error. + # + # `except Exception` is deliberate and is not a swallow: ANY executor failure has to + # compensate, and the original is re-raised below. BaseException (notably CancelledError) + # is intentionally NOT caught — a cancelled approve must not be recorded as a failure. + await self._compensate_failed_execution( + approval_id, + operation=operation, + approver=approver, + requester=str(row["requester"]), + error=exc, + client=client, + ) + raise await self._store.record_audit( "approval.approved", actor=approver, @@ -154,6 +178,54 @@ async def approve( "result": result, } + async def _compensate_failed_execution( + self, + approval_id: str, + *, + operation: str, + approver: str, + requester: str, + error: BaseException, + client: str | None, + ) -> None: + """Roll a released-but-unexecuted request back out of ``approved`` (ASVS 2.3.3). + + Best effort by construction: the caller re-raises the ORIGINAL executor error either way, so + a store that is itself unreachable here must not mask the error that actually explains the + failure. A compensation failure is logged loudly rather than swallowed.""" + try: + # Guarded on 'approved' so this can never clobber a row another caller rejected or + # expired, and so a re-drive of the same failure is idempotent (second call moves 0 rows). + moved = await self._store.decide_pending_approval( + approval_id, + status="failed", + approver=approver, + decided_at=time.time(), + from_status="approved", + ) + await self._store.record_audit( + "approval.failed", + actor=approver, + detail=json.dumps( + { + "approval_id": approval_id, + "operation": operation, + "requester": requester, + # The type, never the message: an executor's exception text can carry + # connection names, paths or params, and the audit log is not a PHI sink. + "error": type(error).__name__, + "compensated": moved, + } + ), + client=client, + ) + except Exception: # noqa: BLE001 - see the docstring; the original error must win + log.exception( + "approval %s: executor failed AND the compensating transition failed; the row may " + "still read 'approved' for an operation that did not run", + approval_id, + ) + async def reject( self, approval_id: str, *, approver: str, client: str | None = None ) -> dict[str, Any]: diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index deb0b92e..baa1a794 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -1486,7 +1486,13 @@ async def get_pending_approval(self, approval_id: str) -> Row | None: ... async def list_pending_approvals(self, *, now: float, limit: int = 100) -> Sequence[Row]: ... async def decide_pending_approval( - self, approval_id: str, *, status: str, approver: str | None, decided_at: float + self, + approval_id: str, + *, + status: str, + approver: str | None, + decided_at: float, + from_status: str = "pending", ) -> bool: ... async def audit_anchor(self) -> tuple[int, str]: ... diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 4d34587f..cdc5c072 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -5985,17 +5985,25 @@ async def list_pending_approvals(self, *, now: float, limit: int = 100) -> Seque ) async def decide_pending_approval( - self, approval_id: str, *, status: str, approver: str | None, decided_at: float + self, + approval_id: str, + *, + status: str, + approver: str | None, + decided_at: float, + from_status: str = "pending", ) -> bool: - """Atomically move a still-``pending`` request to ``status`` (approved/rejected/expired). - Returns ``True`` iff this call made the transition — guards against a double decision.""" + """Atomically move a request in ``from_status`` to ``status``. + Returns ``True`` iff this call made the transition — guards against a double decision. + The SQLite twin documents why the guard is a parameter (ASVS 2.3.3).""" result = await self._pool.execute( "UPDATE pending_approvals SET status = $1, approver = $2, decided_at = $3" - " WHERE id = $4 AND status = 'pending'", + " WHERE id = $4 AND status = $5", status, approver, decided_at, approval_id, + from_status, ) return _rowcount(result) > 0 diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 71c39d66..78e818bc 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -9015,16 +9015,23 @@ async def list_pending_approvals(self, *, now: float, limit: int = 100) -> list[ ) async def decide_pending_approval( - self, approval_id: str, *, status: str, approver: str | None, decided_at: float + self, + approval_id: str, + *, + status: str, + approver: str | None, + decided_at: float, + from_status: str = "pending", ) -> bool: - """Atomically move a still-``pending`` request to ``status`` (approved/rejected/expired). - Returns ``True`` iff this call made the transition — guards against a double decision.""" + """Atomically move a request in ``from_status`` to ``status``. + Returns ``True`` iff this call made the transition — guards against a double decision. + The SQLite twin documents why the guard is a parameter (ASVS 2.3.3).""" async with self._acquire() as conn, self._cursor(conn) as cur: try: await cur.execute( "UPDATE pending_approvals SET status = ?, approver = ?, decided_at = ?" - " WHERE id = ? AND status = 'pending'", - (status, approver, decided_at, approval_id), + " WHERE id = ? AND status = ?", + (status, approver, decided_at, approval_id, from_status), ) count = cur.rowcount await self._commit(conn) diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index d1ead350..38928228 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -1579,7 +1579,9 @@ def _append_channel_scope( params TEXT NOT NULL, -- JSON args captured at request time, replayed on approval requester TEXT NOT NULL, -- who initiated; can never self-approve (dual-control, 2.3.5) requested_at REAL NOT NULL, - status TEXT NOT NULL DEFAULT 'pending', -- pending | approved | rejected | expired + status TEXT NOT NULL DEFAULT 'pending', -- pending | approved | rejected | expired | failed + -- 'failed': the gate released it but the executor raised, so + -- the operation did NOT happen (ASVS 2.3.3 compensation) approver TEXT, -- the distinct second user who released/declined it decided_at REAL, expires_at REAL -- NULL = never; past this a pending request can't be approved @@ -7455,15 +7457,26 @@ async def list_pending_approvals(self, *, now: float, limit: int = 100) -> list[ return list(await cur.fetchall()) async def decide_pending_approval( - self, approval_id: str, *, status: str, approver: str | None, decided_at: float + self, + approval_id: str, + *, + status: str, + approver: str | None, + decided_at: float, + from_status: str = "pending", ) -> bool: - """Atomically move a still-``pending`` request to ``status`` (approved/rejected/expired). - Returns ``True`` iff this call made the transition — guards against a double decision.""" + """Atomically move a request in ``from_status`` to ``status``. + Returns ``True`` iff this call made the transition — guards against a double decision. + + ``from_status`` defaults to ``pending`` (the request/decide path: approved/rejected/expired). + The approval gate also uses it for the ASVS 2.3.3 compensating transition ``approved`` -> + ``failed``, which must NOT be able to move a row some other caller already rejected or + expired — hence the guard is a parameter rather than a hardcoded literal.""" async with self._lock: cur = await self._db.execute( "UPDATE pending_approvals SET status = ?, approver = ?, decided_at = ?" - " WHERE id = ? AND status = 'pending'", - (status, approver, decided_at, approval_id), + " WHERE id = ? AND status = ?", + (status, approver, decided_at, approval_id, from_status), ) await self._commit() return cur.rowcount > 0 diff --git a/tests/test_approvals.py b/tests/test_approvals.py index d8fb1a4f..881ed549 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -9,13 +9,16 @@ from __future__ import annotations import asyncio -from collections.abc import AsyncIterator +import json +from collections.abc import AsyncIterator, Mapping from pathlib import Path +from typing import Any import httpx import pytest from messagefoundry.api import create_app +from messagefoundry.api.approvals import ApprovalGate from messagefoundry.auth import Role from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType @@ -260,3 +263,87 @@ async def test_purge_dual_control_skips_running_outbound(engine: Engine, tmp_pat def test_settings_validator_rejects_unknown_operation() -> None: with pytest.raises(ValueError, match="unknown operation"): ApprovalsSettings(operations=["not_a_real_op"]) + + +# --- ASVS 2.3.3: the released-but-unexecuted compensating transition --------------------------- +# +# approve() moves the row to 'approved' BEFORE running the executor, and that ordering is +# load-bearing (it guards the double-approve race). The gap this closes is what happens when the +# executor then raises: without compensation the row is left asserting an operation that never +# happened, AND no approval.approved row is written either, so the store carries a released +# approval with no recorded outcome at all. + + +async def _gate_with_failing_op(engine: Engine) -> tuple[ApprovalGate, RuntimeError]: + gate = ApprovalGate(engine.store, ON) + boom = RuntimeError("executor exploded") + + async def _raises(_p: Mapping[str, Any]) -> dict[str, Any]: + raise boom + + gate.register("dead_letter_replay", "Replay dead-lettered deliveries", _raises) + return gate, boom + + +async def test_raising_executor_rolls_the_row_out_of_approved(engine: Engine) -> None: + """The row must NOT be left at 'approved' for an operation that did not run.""" + gate, boom = await _gate_with_failing_op(engine) + approval_id = await gate.guard("dead_letter_replay", {}, requester="maker") + assert approval_id is not None + + # The original executor error still reaches the caller -- compensation must not swallow it. + with pytest.raises(RuntimeError) as caught: + await gate.approve(approval_id, approver="checker") + assert caught.value is boom + + row = await engine.store.get_pending_approval(approval_id) + assert row is not None + assert str(row["status"]) == "failed" # pre-fix this read 'approved' + + +async def test_raising_executor_audits_the_failure_against_both_identities( + engine: Engine, +) -> None: + gate, _ = await _gate_with_failing_op(engine) + approval_id = await gate.guard("dead_letter_replay", {}, requester="maker") + assert approval_id is not None + with pytest.raises(RuntimeError): + await gate.approve(approval_id, approver="checker") + + rows = await engine.store.list_audit(limit=50) + audited = {(str(r["action"]), str(r["actor"])) for r in rows} + assert ("approval.requested", "maker") in audited # the maker's half survives + assert ("approval.failed", "checker") in audited # the checker's half records the failure + # A success row must NOT be written for an operation that raised. + assert ("approval.approved", "checker") not in audited + + failed = next(r for r in rows if str(r["action"]) == "approval.failed") + detail = json.loads(str(failed["detail"])) + assert detail["operation"] == "dead_letter_replay" + assert detail["requester"] == "maker" + assert detail["compensated"] is True + # The exception TYPE is recorded, never its message: executor text can carry connection names, + # paths or params, and the audit log is not a PHI sink. + assert detail["error"] == "RuntimeError" + assert "exploded" not in str(failed["detail"]) + + +async def test_compensation_cannot_clobber_an_already_rejected_row(engine: Engine) -> None: + """The compensating transition is guarded on 'approved', so it can only ever move a row this + gate itself released -- never one another caller rejected or expired.""" + gate, _ = await _gate_with_failing_op(engine) + approval_id = await gate.guard("dead_letter_replay", {}, requester="maker") + assert approval_id is not None + await gate.reject(approval_id, approver="checker") + + moved = await engine.store.decide_pending_approval( + approval_id, + status="failed", + approver="checker", + decided_at=0.0, + from_status="approved", + ) + assert moved is False + row = await engine.store.get_pending_approval(approval_id) + assert row is not None + assert str(row["status"]) == "rejected" From 85fe7e85195052d9eff7de8c4150bee65754cdc5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 10 Aug 2026 23:13:12 -0500 Subject: [PATCH 2/9] fix(uploads): the shard budget-multiplication claim is false -- correct it and pin it Two shipped artifacts asserted that engine shards sharing one uploads_dir each get a fresh per-uploader quota: config/settings.py called it "a documented residual, same shape as the summary-rate cap", and uploads.py called the quota "per-process per-uploads_dir". The ASVS 2.3.4 residual inherits the same claim ("N engine shards multiply the budget"), and the backlog item built its worked example on it -- warning that a fix closing only the concurrency limb would leave "the shard-multiplication half untouched". There is no shard-multiplication half. Measured 2026-08-10 by execution, not by reading: two UploadStore instances over ONE directory, quota 3. Shard A refused file 4 (live positive control -- the cap engages). Shard B, at the same dir, ALSO refused. The budget does not multiply. The mechanism is that _scan_metas_sync is an uncached filesystem read: it re-walks the root and decrypts every sidecar on every call, so any process at that dir sees every other process's files. Shards pointed at SEPARATE dirs do get separate budgets, but that is per-directory scoping by construction, not the shared-dir case both comments described. What survives is the check-then-write race, and it is smaller than the multiplication claim: each concurrently in-flight upload can overshoot by at most one file, itself bounded by max_upload_bytes. Shards compound the race (more concurrency) but not the budget. Both comments now say that, with the measurement, instead of the old claim. Pinned by test, so the correction cannot silently regress back into a per-process budget -- which WOULD be the double-booking ASVS 2.3.4 forbids. The test shares one cipher across both stores ON PURPOSE, and says why: real shards run off one unified store and therefore one keyring/DEK. An earlier draft let the fixture mint a key per store, which made shard B skip shard A's sidecars as undecryptable and manufactured a per-process budget that does not exist. That failure was an artifact of the fixture and would have "confirmed" the very claim this commit refutes. This corrects the record only. The check-then-write race itself is the next layer and is not touched here. --- messagefoundry/config/settings.py | 8 ++++++-- messagefoundry/uploads.py | 13 +++++++++---- tests/test_uploads.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 99363a33..ee19bd2e 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -436,8 +436,12 @@ class StoreSettings(_Section): # PHI-at-rest is age-pruned. Enforced in `UploadStore.save` (a would-be over-quota upload is refused # HTTP 409 before any write, audited `upload.reject_quota`) and by an age-based prune sweep (blob+meta # pairs older than `uploads_retention_days` are deleted, opportunistically at save time plus a periodic - # task, each prune audited `upload.prune`). Quotas are per-process per-`uploads_dir` (multiple engine - # shards at one dir multiply the budget — a documented residual, same shape as the summary-rate cap). + # task, each prune audited `upload.prune`). Quotas are enforced per-`uploads_dir`, NOT per-process: + # the check reads the sidecars off disk with no cache, so engine shards sharing one dir see each + # other's files and the budget does NOT multiply (measured 2026-08-10 — two UploadStores over one + # dir, the second refused the same uploader at quota, against a live positive control). What IS + # shared across them is the check-then-write race below, which overshoots by at most one file per + # concurrently in-flight upload. Shards given SEPARATE dirs get separate budgets, by construction. max_upload_files_per_user: int = Field( default=100, ge=1, diff --git a/messagefoundry/uploads.py b/messagefoundry/uploads.py index 00a934c8..6cf6b352 100644 --- a/messagefoundry/uploads.py +++ b/messagefoundry/uploads.py @@ -87,8 +87,11 @@ class UploadQuotaError(UploadError): retain at once (``[store].max_upload_files_per_user`` / ``max_upload_total_bytes_per_user``, both defaults-ON). A would-be over-quota upload is refused at the chokepoint before anything is written; the API maps it to HTTP 409 and a metadata-only ``upload.reject_quota`` audit. Residual: the - check-then-write is not atomic, so concurrent in-flight uploads can overshoot by at most one file - (itself bounded by ``max_upload_bytes``); the quota is per-process per-``uploads_dir``.""" + check-then-write is not atomic, so each concurrently in-flight upload can overshoot by at most one + file (itself bounded by ``max_upload_bytes``). The quota is scoped to the ``uploads_dir``, not to + the process: :meth:`UploadStore._scan_metas_sync` re-reads the sidecars with no cache, so engine + shards sharing one dir enforce ONE budget between them and the race above is the only thing they + compound. Shards pointed at separate dirs get separate budgets, by construction.""" class UploadNotFoundError(UploadError): @@ -391,8 +394,10 @@ async def save( def _build_and_write() -> UploadedFileMeta: # Per-uploader quota (ASVS 5.2.4): scan the uploader's existing sidecars and refuse BEFORE # writing when this file would exceed their file-count or aggregate-byte cap. Runs in the same - # off-loop thread as the write. Residual: the check-then-write is not atomic, so concurrent - # in-flight uploads can overshoot by at most one file (bounded by max_bytes). + # off-loop thread as the write. Residual: the check-then-write is not atomic, so each + # concurrently in-flight upload can overshoot by at most one file (bounded by max_bytes). + # The scan is uncached, so this is the ONLY thing engine shards sharing a dir compound — + # they do not each get a fresh budget. See UploadQuotaError for the measurement. mine = [m for m in self._scan_metas_sync() if m.uploader == uploader] if len(mine) + 1 > self._max_files_per_user: raise UploadQuotaError( diff --git a/tests/test_uploads.py b/tests/test_uploads.py index fbd6ce7e..52c036ed 100644 --- a/tests/test_uploads.py +++ b/tests/test_uploads.py @@ -233,6 +233,37 @@ async def test_quota_is_per_user(tmp_path: Path) -> None: assert {m.uploader for m in await store.list_files()} == {"alice", "bob"} +async def test_quota_is_shared_by_stores_over_one_dir_not_per_process(tmp_path: Path) -> None: + """ASVS 2.3.4 / 5.2.4: the budget is scoped to the uploads_dir, NOT to the process. + + Two UploadStore instances over one directory stand in for two engine shards. The settings + comment and the ASVS 2.3.4 residual both used to assert that shards at one dir "multiply the + budget"; measured 2026-08-10, they do not -- the sidecar scan is uncached, so the second store + sees the first's files. This test pins that, so the corrected claim cannot silently regress + back into a per-process budget (which WOULD be the double-booking 2.3.4 forbids). + """ + # The cipher MUST be shared: real engine shards run off one unified store and therefore one + # keyring/DEK. Giving each store its own key would make shard B skip shard A's sidecars as + # undecryptable and fake a per-process budget -- an artifact of the fixture, not the system. + cipher = make_cipher(generate_key()) + + def _shard() -> UploadStore: + return UploadStore(tmp_path / "uploads", cipher, max_bytes=4096, max_files_per_user=2) + + shard_a, shard_b = _shard(), _shard() # two processes, ONE uploads dir + + for i in range(2): + await shard_a.save(data=f"a{i}\n".encode(), filename=f"a{i}.txt", uploader="alice") + # Positive control: the cap engages at all on the store that wrote the files. + with pytest.raises(UploadQuotaError): + await shard_a.save(data=b"overflow\n", filename="a2.txt", uploader="alice") + + # The question: does the OTHER store grant alice a fresh budget? + with pytest.raises(UploadQuotaError): + await shard_b.save(data=b"from b\n", filename="b.txt", uploader="alice") + assert len(await shard_b.list_files()) == 2 # still exactly the cap, nothing extra written + + async def test_prune_deletes_aged_pairs_and_is_idempotent(tmp_path: Path) -> None: # ASVS 5.2.4: files older than retention_days are deleted (blob AND meta), and a re-run is a no-op. store = _quota_store(tmp_path, retention_days=30) From 61fe28ae2929d7a7575b2a5f10682c53d892f018 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 10 Aug 2026 23:16:08 -0500 Subject: [PATCH 3/9] fix(uploads): make the per-uploader quota check-and-write one critical section (ASVS 2.3.4) The quota scanned the uploader's sidecars and then wrote, with the lock released in between, so two concurrent uploads each read a stale count and both proceeded. That is the double-booking of a limited-quantity resource ASVS 2.3.4 is about, reachable by ordinary concurrency rather than by any special access. Fixed by serialising the whole build-and-write behind an asyncio.Lock, not just the check: releasing between the check and the write IS the race. The throughput cost is acceptable and nowhere near the data plane -- this is the operator diagnostic-upload surface and each pass is already bounded by max_bytes. WATCHED FAIL, made deterministic rather than timing-dependent: the sidecar scan is slowed so both coroutines are guaranteed to overlap. With the lock removed and the test present, a quota of 1 admits TWO files. With the lock, exactly one wins and the other is refused on quota, with only the winner on disk. The residual is now stated precisely instead of vaguely. The critical section is per-process, so N shards sharing one dir can still overshoot by at most N-1 files -- one per shard mid-write while another scans. On the shipped single-process deployment N is 1 and the overshoot is zero. Closing the multi-shard remainder needs a cross-process mechanism (an advisory lock on the dir, or moving the accounting into the unified store); it is NOT closed here and the docstring says so, because a comment implying otherwise would be the compensating-control-on-a-false-premise defect this chapter keeps finding. Deliberately not done: optimistic write-then-verify-then-rollback would also close the cross-process half, but it writes PHI to disk before deciding it is over quota. The existing design refuses BEFORE anything is written and that property is worth more than the remaining N-1. --- messagefoundry/uploads.py | 38 +++++++++++++++++++++++++++----------- tests/test_uploads.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/messagefoundry/uploads.py b/messagefoundry/uploads.py index 6cf6b352..fff83d58 100644 --- a/messagefoundry/uploads.py +++ b/messagefoundry/uploads.py @@ -86,12 +86,20 @@ class UploadQuotaError(UploadError): The uploaded-logs feature caps how many files and how many aggregate bytes a single uploader may retain at once (``[store].max_upload_files_per_user`` / ``max_upload_total_bytes_per_user``, both defaults-ON). A would-be over-quota upload is refused at the chokepoint before anything is written; - the API maps it to HTTP 409 and a metadata-only ``upload.reject_quota`` audit. Residual: the - check-then-write is not atomic, so each concurrently in-flight upload can overshoot by at most one - file (itself bounded by ``max_upload_bytes``). The quota is scoped to the ``uploads_dir``, not to - the process: :meth:`UploadStore._scan_metas_sync` re-reads the sidecars with no cache, so engine - shards sharing one dir enforce ONE budget between them and the race above is the only thing they - compound. Shards pointed at separate dirs get separate budgets, by construction.""" + the API maps it to HTTP 409 and a metadata-only ``upload.reject_quota`` audit. + + The check and the write it authorises run as ONE critical section per process (ASVS 2.3.4), so + concurrent uploads inside an engine cannot double-book the budget. The quota is scoped to the + ``uploads_dir``, not to the process: :meth:`UploadStore._scan_metas_sync` re-reads the sidecars + with no cache, so engine shards sharing one dir enforce ONE budget between them (measured + 2026-08-10). Shards pointed at separate dirs get separate budgets, by construction. + + Residual, stated precisely: the critical section is per-process, so N engine shards sharing one + dir can still overshoot by at most **N-1 files** — one per shard that is mid-write when another + scans, each bounded by ``max_upload_bytes``. On the shipped single-process deployment N is 1 and + the overshoot is zero. Closing the multi-shard remainder needs a cross-process mechanism (an + advisory lock on the dir, or moving the accounting into the unified store); it is not closed + here, and no comment in this module should imply otherwise.""" class UploadNotFoundError(UploadError): @@ -278,6 +286,12 @@ def __init__( self._max_files_per_user = max(1, int(max_files_per_user)) self._max_total_bytes_per_user = max(1, int(max_total_bytes_per_user)) self._retention_days = max(1, int(retention_days)) + # ASVS 2.3.4: the quota check and the write that consumes it must be ONE critical section, or + # concurrent uploads each read a stale count and double-book the budget. Serialising the whole + # build-and-write (not just the check) is what makes it atomic — releasing between them is the + # race. The throughput cost is acceptable here and nowhere near the data plane: this is the + # operator diagnostic-upload surface, and each pass is bounded by max_bytes. + self._quota_lock = asyncio.Lock() @property def max_bytes(self) -> int: @@ -394,10 +408,10 @@ async def save( def _build_and_write() -> UploadedFileMeta: # Per-uploader quota (ASVS 5.2.4): scan the uploader's existing sidecars and refuse BEFORE # writing when this file would exceed their file-count or aggregate-byte cap. Runs in the same - # off-loop thread as the write. Residual: the check-then-write is not atomic, so each - # concurrently in-flight upload can overshoot by at most one file (bounded by max_bytes). - # The scan is uncached, so this is the ONLY thing engine shards sharing a dir compound — - # they do not each get a fresh budget. See UploadQuotaError for the measurement. + # off-loop thread as the write, and the caller holds _quota_lock across BOTH, so no second + # upload in this process can read this count before the write consumes it (ASVS 2.3.4). + # The scan is uncached, so shards sharing a dir enforce one budget rather than one each; + # the residual that survives the lock is per-shard, not per-upload. See UploadQuotaError. mine = [m for m in self._scan_metas_sync() if m.uploader == uploader] if len(mine) + 1 > self._max_files_per_user: raise UploadQuotaError( @@ -430,7 +444,9 @@ def _build_and_write() -> UploadedFileMeta: _atomic_write_text(root, meta_path, meta_ct) return meta - return await asyncio.to_thread(_build_and_write) + # One critical section per process: quota check + write. See _quota_lock in __init__. + async with self._quota_lock: + return await asyncio.to_thread(_build_and_write) async def list_files(self) -> list[UploadedFileMeta]: """List all uploaded files (newest first). Undecryptable/foreign sidecars are skipped with a diff --git a/tests/test_uploads.py b/tests/test_uploads.py index 52c036ed..d90b1816 100644 --- a/tests/test_uploads.py +++ b/tests/test_uploads.py @@ -4,6 +4,7 @@ from __future__ import annotations +import asyncio import dataclasses import time from pathlib import Path @@ -264,6 +265,39 @@ def _shard() -> UploadStore: assert len(await shard_b.list_files()) == 2 # still exactly the cap, nothing extra written +async def test_concurrent_uploads_cannot_double_book_the_quota( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ASVS 2.3.4: the quota check and the write it authorises are ONE critical section. + + Made deterministic rather than timing-dependent: the sidecar scan is slowed so both coroutines + are guaranteed to overlap. Without the lock both read a count of 0 and both write, double-booking + a quota of 1. With it, the second scan sees the first file and refuses. + """ + store = _quota_store(tmp_path, max_files=1) + + real_scan = store._scan_metas_sync + + def _slow_scan() -> list[UploadedFileMeta]: + out = real_scan() + time.sleep(0.05) # widen the window so an unlocked check-then-write WOULD lose the race + return out + + monkeypatch.setattr(store, "_scan_metas_sync", _slow_scan) + + results = await asyncio.gather( + store.save(data=b"first\n", filename="a.txt", uploader="alice"), + store.save(data=b"second\n", filename="b.txt", uploader="alice"), + return_exceptions=True, + ) + + ok = [r for r in results if isinstance(r, UploadedFileMeta)] + refused = [r for r in results if isinstance(r, UploadQuotaError)] + assert len(ok) == 1, f"exactly one upload may win a quota of 1, got {results}" + assert len(refused) == 1, f"the loser must be refused on quota, got {results}" + assert len(await store.list_files()) == 1 # and only the winner is on disk + + async def test_prune_deletes_aged_pairs_and_is_idempotent(tmp_path: Path) -> None: # ASVS 5.2.4: files older than retention_days are deleted (blob AND meta), and a re-run is a no-op. store = _quota_store(tmp_path, retention_days=30) From c9e110f28f40c81ea35bc3a9735eec037442075e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 10 Aug 2026 23:50:56 -0500 Subject: [PATCH 4/9] docs(security): the ECH risk acceptance rested on a premise this repo refutes docs/SECURITY.md called ECH for outbound SNI "infeasible" and said a working client "would require a third-party TLS stack -- violating the no-new-dependency rule for a security-core path". Both halves are false, and the counter-evidence is in this repository. tools/ech-sidecar is a working out-of-process ECH client whose go.mod reads "stdlib-only, no dependencies -- builds offline with GOPROXY=off" and whose every import in main.go is Go stdlib (context, crypto/tls, encoding/*, net, net/http, ...). ADR 0139 records it. That is the compensating-control-on-a-false-premise shape SDS-3.7 forbids, and it is the worst place for it: a reader weighing whether to ACCEPT this residual was being told the control could not be built, while the tree contained one. The paragraph now says the narrower thing that is actually true: the ENGINE PROCESS cannot do ECH, because Python 3.14's stdlib ssl exposes no ECH API and there is no SVCB/HTTPS resolver in scope. The sidecar exists but is not wired into any egress path, not built by CI, and not distributed (pyproject.toml excludes tools/ from sdist and wheel). THE ACCEPTED RESIDUAL IS UNCHANGED -- the destination SNI is visible on the outbound handshake either way. Only the premise is corrected, and the correction is recorded in place rather than silently overwritten. This is true under EITHER branch of the unruled G19 (keep or retire the sidecar), so it does not wait on that ruling. Not touched: ADR 0093 section 3 carries the same framing, but an ADR is a dated decision record and ADR 0139 already post-dates it. Rewriting it would be rewriting history rather than the record; flagged for the owner instead. 285 passed across the nine tests that read docs/SECURITY.md from disk. --- docs/SECURITY.md | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index a4fad12c..61fe21bc 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -1737,15 +1737,25 @@ BACKLOG #190 bundled three integrity residuals; #190 closes with **one built** a ask was a *runbook decision* (does the exposure runbook mandate it), not new engine code. Every PHI-plane surface already carries integrity: outbound bodies (ADR 0018), the audit trail (the HMAC hash-chain), and data at rest (AES-256-GCM AEAD). -- **ECH (Encrypted Client Hello) for outbound SNI — infeasible, documented risk acceptance - (12.1.5).** Hiding the destination hostname in the outbound TLS ClientHello is not buildable here: - Python 3.14's stdlib `ssl` exposes **no ECH API**, there is **no SVCB/HTTPS DNS resolver** (ECHConfig - is DNS-published) and adding one is out of scope, and a working ECH client would require a - **third-party TLS stack** — violating the no-new-dependency rule for a security-core path. The +- **ECH (Encrypted Client Hello) for outbound SNI — not performed by the engine, documented risk + acceptance (12.1.5).** Hiding the destination hostname in the outbound TLS ClientHello is not + reachable **in the engine process**: Python 3.14's stdlib `ssl` exposes **no ECH API**, and there is + **no SVCB/HTTPS DNS resolver** (ECHConfig is DNS-published) — adding one is out of scope. The destination SNI is therefore visible on the outbound handshake. Compensating context: on-prem, a trusted network segment, an operator-configured `[egress]`-allowlisted destination, and TLS still - protects the payload. Re-open when the stdlib gains a first-class ECH API (no new dep) and an - SVCB/HTTPS resolver is in scope. + protects the payload. Re-open when the stdlib gains a first-class ECH API and an SVCB/HTTPS + resolver is in scope. + + > **Correction, 2026-08-10.** This paragraph previously called ECH *infeasible* and said a working + > client "would require a **third-party TLS stack** — violating the no-new-dependency rule". Both + > halves are false, and the counter-evidence is in this repository: `tools/ech-sidecar` is a + > working out-of-process ECH client whose `go.mod` reads "stdlib-only, no dependencies" and whose + > every import is Go stdlib (ADR 0139). A reader weighing this risk acceptance was being told the + > control could not be built while the tree contained one. What is actually true is narrower and + > is what the paragraph now says: the **engine process** cannot do ECH, and the sidecar is **not + > wired into any egress path, not built by CI, and not distributed** (`pyproject.toml` excludes + > `tools/` from both sdist and wheel). **The accepted residual is unchanged** — the SNI is visible + > either way. Only the premise is corrected. ### In-use memory protection — best-effort partial + deployment requirement (13.3.3 / 11.7.1 / 11.7.2, #198) From d5a6cc4d79c9515a96215356ed7cc1c2f94bb36e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 11 Aug 2026 07:18:21 -0500 Subject: [PATCH 5/9] chore(ech): retire tools/ech-sidecar, and reconcile the four records that named it G19 ruled RETIRE. Nothing built, tested, linted or version-pinned the tree. Measured before removing, not asserted: four tracked files (.gitignore, README.md, go.mod, main.go), main.go 312 lines, zero module dependencies, and NO Go toolchain anywhere in CI -- zero matches for setup-go / go-version / go build across .github/, against a live positive control of 14 files matching setup-python. Keeping it would have bought a fourth Dependabot ecosystem, the first COMPILED-language CodeQL leg (both current languages are interpreted, so a shape change rather than a matrix row), a toolchain pin with no hash-lock analogue beside three existing lockfiles, and a fourteenth required context against a set of thirteen -- for a control no measurement has shown reachable: the 2026-07-20 DoH type-65 probe found no healthcare counterparty publishing an ECHConfig, against a working Cloudflare control. ZERO CELLS MOVE. ASVS 12.1.5 was an accepted fail before and after. Custody verified BEFORE deletion rather than trusted from a commit message: all four files are present in the private vault at tools/ech-sidecar/ and that vault is pushed, so the code survives in two places, not just git history. Nothing operational goes with it. The engine-side fail-closed routing (ech_sidecar_url_from_settings, egress_route_from_settings) and tests/test_ech_egress.py stay exactly as they were -- they refuse a non-loopback sidecar, refuse ech_egress without ech_sidecar, and error rather than silently falling back to a SNI-leaking direct hop. An operator supplies their own terminator; samples/ech-sidecar/README.md is unchanged. Four records named the deleted path and are reconciled here: - rest.py's docstring pointed at tools/ech-sidecar/ AND asserted it was "proven to hide the SNI against a real ECH endpoint". Both had to go: the path now resolves to nothing, and #1011 already flagged that the "proven" claim needed whatever evidence actually backs it. - test_ech_egress.py's docstring said the real Go sidecar is "proven separately (tools/ech-sidecar)". Replaced, and the file now states plainly what it does NOT prove: no ECH is originated anywhere in this suite, so no SNI-hiding claim may rest on it. - ADR 0139's status block filed the re-originator under "Deferred" while the tree shipped -- understating what existed. Amended in place with the ruling and the measurement. The DECISION is untouched; Increment 1 still stands. - docs/SECURITY.md carried my own 2026-08-10 correction, which cited the tree as live counter-evidence. That correction now needed its own correction: amended to past tense, and I dropped its claim that pyproject.toml "excludes tools/ from both sdist and wheel" -- verified, sdist is an ALLOW-LIST (only-include) and I found no separate wheel target, so the original wording was both stronger and less accurate than the truth. NOT fixed here, handed over: docs/testing/master-test-plan/16-security-phi- and-supply-chain.md still says the re-originator "now ships" at :217, :253, :323 and :888. Those rows are dense P2 planning text in a document this change does not own, and SEC-71's pass criterion is precisely "a dated owner decision covering all three: keep or retire" -- which this ruling supplies, so the rows want reconciling by whoever owns that plan rather than rewriting in passing. 94 passed on test_ech_egress + test_communications_inventory; 586 passed across the adr/inventory/docs selection. ruff and mypy clean on every touched file (the 21 mypy errors are pre-existing import-not-found for optional extras in four files this does not touch). --- docs/SECURITY.md | 28 +- ...sni-hiding-for-asvs-12-1-5-demand-gated.md | 20 ++ messagefoundry/transports/rest.py | 5 +- tests/test_ech_egress.py | 20 +- tools/ech-sidecar/.gitignore | 1 - tools/ech-sidecar/README.md | 97 ------ tools/ech-sidecar/go.mod | 6 - tools/ech-sidecar/main.go | 312 ------------------ 8 files changed, 54 insertions(+), 435 deletions(-) delete mode 100644 tools/ech-sidecar/.gitignore delete mode 100644 tools/ech-sidecar/README.md delete mode 100644 tools/ech-sidecar/go.mod delete mode 100644 tools/ech-sidecar/main.go diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 61fe21bc..6a2a3b47 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -1746,16 +1746,24 @@ BACKLOG #190 bundled three integrity residuals; #190 closes with **one built** a protects the payload. Re-open when the stdlib gains a first-class ECH API and an SVCB/HTTPS resolver is in scope. - > **Correction, 2026-08-10.** This paragraph previously called ECH *infeasible* and said a working - > client "would require a **third-party TLS stack** — violating the no-new-dependency rule". Both - > halves are false, and the counter-evidence is in this repository: `tools/ech-sidecar` is a - > working out-of-process ECH client whose `go.mod` reads "stdlib-only, no dependencies" and whose - > every import is Go stdlib (ADR 0139). A reader weighing this risk acceptance was being told the - > control could not be built while the tree contained one. What is actually true is narrower and - > is what the paragraph now says: the **engine process** cannot do ECH, and the sidecar is **not - > wired into any egress path, not built by CI, and not distributed** (`pyproject.toml` excludes - > `tools/` from both sdist and wheel). **The accepted residual is unchanged** — the SNI is visible - > either way. Only the premise is corrected. + > **Correction, 2026-08-10, amended 2026-08-11.** This paragraph previously called ECH + > *infeasible* and said a working client "would require a **third-party TLS stack** — violating + > the no-new-dependency rule". Both halves were false **when written**, and the counter-evidence + > was in the tree at the time: `tools/ech-sidecar` was a working out-of-process ECH client whose + > `go.mod` read "stdlib-only, no dependencies" and whose every import was Go stdlib (ADR 0139). A + > reader weighing this risk acceptance was being told the control could not be built while the + > repository contained one. + > + > **That tree was retired on 2026-08-11** under the G19 ruling — nothing built, tested, linted or + > version-pinned it, and there is no Go toolchain in CI. It survives in git history and in the + > private vault, so the refutation above remains checkable; it is simply no longer checkable from + > the working tree. What is true now is narrower and is what the paragraph says: the **engine + > process** cannot do ECH, and the engine ships **no sidecar** — an operator supplying their own + > loopback ECH terminator is the only route, and the engine's fail-closed routing for that case + > stays (`transports/rest.py`, `tests/test_ech_egress.py`). + > + > **The accepted residual is unchanged throughout** — the destination SNI is visible on the + > outbound handshake under every one of these states. Only the premise moved. ### In-use memory protection — best-effort partial + deployment requirement (13.3.3 / 11.7.1 / 11.7.2, #198) diff --git a/docs/adr/0139-ech-egress-sidecar-sni-hiding-for-asvs-12-1-5-demand-gated.md b/docs/adr/0139-ech-egress-sidecar-sni-hiding-for-asvs-12-1-5-demand-gated.md index 22b7a2e7..c358da35 100644 --- a/docs/adr/0139-ech-egress-sidecar-sni-hiding-for-asvs-12-1-5-demand-gated.md +++ b/docs/adr/0139-ech-egress-sidecar-sni-hiding-for-asvs-12-1-5-demand-gated.md @@ -11,6 +11,26 @@ ## Implementation status +> **Amendment 2026-08-11 (G19 ruling) — the sidecar tree is RETIRED.** This block previously filed +> the terminating re-originator under "Deferred (the real ECH work)" while a complete Go +> implementation was tracked at `tools/ech-sidecar/`, so the record contradicted HEAD in the +> direction of understating what shipped. That has now been resolved in the other direction: the +> tree was **removed** on 2026-08-11. Measured at the time of the ruling — four tracked files, +> `main.go` 312 lines, zero module dependencies, and **no Go toolchain anywhere in CI** (zero +> matches for `setup-go`/`go-version`/`go build` across `.github/`, against a live positive control +> of 14 files matching `setup-python`). Keeping it would have bought a fourth Dependabot ecosystem, +> the first compiled-language CodeQL leg, a toolchain pin with no hash-lock analogue, and a +> fourteenth required context — for a control that no measurement has ever shown to be reachable: +> the 2026-07-20 DoH type-65 probe found **no** healthcare counterparty publishing an ECHConfig, +> against a working Cloudflare control. +> +> **The decision below is unchanged and Increment 1 still stands.** The engine-side routing and +> fail-closed plumbing remain shipped and tested. What changed is only that the engine no longer +> carries a reference implementation of the far end — an operator supplies their own loopback ECH +> terminator, per [`samples/ech-sidecar/`](../../samples/ech-sidecar/README.md). The retired code +> survives in git history and in the private vault. **ASVS 12.1.5 remains an accepted `fail` +> either way**; retirement changed no verdict. + **Increment 1 — built + verified (commit `a0c336ce`): the engine-side routing + fail-closed plumbing only.** - `transports/rest.py` — `ech_sidecar_url_from_settings` (an opt-in per-connection route to a **loopback** sidecar, reusing the ADR 0126 opener plumbing) + `egress_route_from_settings`, the single ECH-or-proxy resolver diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index b562d8ff..5400ef11 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -1080,8 +1080,9 @@ def ech_sidecar_url_from_settings(s: Mapping[str, Any]) -> str | None: ECH hides the outbound SNI, but stdlib ``ssl`` (OpenSSL 3.5.x) has no ECH — it is an OpenSSL 4.0 feature (a local ``ctypes`` probe found zero ECH symbols in the bundled ``libssl``). So an - ``ech_egress`` connection routes through a **loopback ECH sidecar** — the TLS-**terminating** - re-originator at ``tools/ech-sidecar/`` (proven to hide the SNI against a real ECH endpoint). It is + ``ech_egress`` connection routes through a **loopback ECH sidecar** — a TLS-**terminating** + re-originator the OPERATOR supplies; the engine ships none (ADR 0139; a reference implementation + was retired from ``tools/`` 2026-08-11 and survives in git history). It is NOT a forward proxy: a proxy would tunnel ``https`` via CONNECT and the engine's own non-ECH ClientHello would still leak the SNI. Instead the REST destination sends its request to the sidecar over **cleartext loopback** with the real destination in the ``Host`` header (see diff --git a/tests/test_ech_egress.py b/tests/test_ech_egress.py index 6dd78417..53d324f3 100644 --- a/tests/test_ech_egress.py +++ b/tests/test_ech_egress.py @@ -2,13 +2,19 @@ # Copyright (C) 2026 MessageFoundry Organization and contributors """Tests for the ECH SNI-hiding send-path (transports/rest.py, ADR 0139, ASVS 12.1.5). -An `ech_egress` REST connection re-addresses each request to a loopback **terminating** sidecar -(`tools/ech-sidecar/`) over cleartext http with the real destination in the `Host` header; the sidecar -re-originates the https + ECH connection (hiding the SNI). These tests cover the resolver -(`ech_sidecar_url_from_settings`), the fail-closed refusal on non-REST connectors -(`egress_route_from_settings`), the connector wiring (`_ech_request` / opener / mutual exclusion), and a -**stub-sidecar `_post` behavioral test** proving the request actually lands on the sidecar naming the -upstream in `Host`. The real Go sidecar + a live ECH endpoint are proven separately (tools/ech-sidecar). +An `ech_egress` REST connection re-addresses each request to a loopback **terminating** sidecar over +cleartext http with the real destination in the `Host` header; the sidecar re-originates the https + +ECH connection (hiding the SNI). **The engine ships no sidecar** — the operator supplies one (recipe: +`samples/ech-sidecar/README.md`). A reference Go implementation lived at `tools/ech-sidecar/` and was +retired 2026-08-11 under the G19 ruling; nothing built, tested or version-pinned it, and it survives +in git history. + +These tests cover the resolver (`ech_sidecar_url_from_settings`), the fail-closed refusal on non-REST +connectors (`egress_route_from_settings`), the connector wiring (`_ech_request` / opener / mutual +exclusion), and a **stub-sidecar `_post` behavioral test** proving the request actually lands on the +sidecar naming the upstream in `Host`. That is the whole of what this file proves: the engine's +ROUTING and its fail-closed behaviour. **It does not prove that any SNI is concealed** — no ECH is +originated anywhere in this suite, and no claim of SNI hiding rests on it. """ from __future__ import annotations diff --git a/tools/ech-sidecar/.gitignore b/tools/ech-sidecar/.gitignore deleted file mode 100644 index b883f1fd..00000000 --- a/tools/ech-sidecar/.gitignore +++ /dev/null @@ -1 +0,0 @@ -*.exe diff --git a/tools/ech-sidecar/README.md b/tools/ech-sidecar/README.md deleted file mode 100644 index db539a59..00000000 --- a/tools/ech-sidecar/README.md +++ /dev/null @@ -1,97 +0,0 @@ -# ech-sidecar — terminating, fail-closed ECH re-originator - -A small **stdlib-only Go** forward proxy that hides the destination **SNI** on -MessageFoundry's outbound HTTPS by re-originating each request with **Encrypted -Client Hello (ECH)**. It exists to satisfy **ASVS 12.1.5** (protect the server name -from a network observer) for egress that CPython's `ssl` cannot yet do (OpenSSL ECH -is not exposed by the stdlib). - -## What it does - -For every request it receives on a loopback HTTP port it: - -1. Determines the upstream host — from the **absolute-form request URI** (classic - forward proxy, e.g. `GET http://host/path` through an HTTP proxy) or, failing - that, the **`Host` header**. -2. Resolves that host's **ECHConfigList** from its DNS **HTTPS record (RR type 65)** - over **DoH** (`https://cloudflare-dns.com/dns-query`, `accept: application/dns-json`), - walking the record's SvcParams for **SvcParamKey 5 (`ech`)** — parsed in Go with - `encoding/hex` + a manual SvcParam walk (`walkSvcParamsForECH`), mirroring the - working ECH proof. -3. Dials the upstream with `crypto/tls` using - `EncryptedClientHelloConfigList = `, so the true `ServerName` is sent - only inside the encrypted **ClientHelloInner**. -4. **Verifies the upstream certificate normally** — `InsecureSkipVerify` is never set. -5. Re-issues the HTTP request over that connection and streams the response back. - -## FAIL-CLOSED (do not weaken) - -There are **two** gates and **no** cleartext-SNI fallback: - -- If the upstream publishes **no ECHConfig**, the request is **refused with `502`** - (`resolveECH` returns an error, never a "no ECH but OK"). -- If the TLS server does **not accept** ECH (`ConnectionState.ECHAccepted == false`), - the connection is **refused** even though a config list was found. - -`CONNECT` tunnels are rejected (`405`): an end-to-end tunnel would leave TLS -origination — and therefore ECH — with the client, defeating the purpose. - -## Build & run (offline) - -Requires **Go >= 1.26** (stdlib ECH). No module dependencies; builds with -`GOPROXY=off`. - -```sh -go build -o ech-sidecar.exe . -./ech-sidecar.exe -addr 127.0.0.1:8123 # loopback-only; refuses non-loopback binds -``` - -Flags: `-addr` (default `127.0.0.1:8123`), `-timeout` (per-request upstream timeout, -default `30s`). - -## Proof — SNI hidden through the sidecar - -```sh -$ curl -s http://127.0.0.1:8123/cdn-cgi/trace -H "Host: crypto.cloudflare.com" -... -sni=encrypted # <-- the destination SNI was encrypted end to end -... -``` - -Equivalent true forward-proxy form (the engine's path): - -```sh -$ curl -s -x http://127.0.0.1:8123 http://crypto.cloudflare.com/cdn-cgi/trace | grep sni= -sni=encrypted -``` - -Fail-closed check (a host with no ECHConfig is refused, not downgraded): - -```sh -$ curl -s -o /dev/null -w "%{http_code}\n" http://127.0.0.1:8123/ -H "Host: example.com" -502 -``` - -## Engine integration (not wired here) - -⛔ **Superseded — do NOT pair this sidecar with the forward proxy.** This section predates the wiring -and prescribed reusing the ADR 0126 forward-proxy handler; the engine **refuses** that pairing. - -ADR 0139 "Increment 1" has since landed in `messagefoundry/transports/rest.py`, and it did *not* reuse -the proxy handler: the REST destination resolves a **dedicated** loopback sidecar URL via -`ech_sidecar_url_from_settings` (`rest.py:1077`), sets no proxy at all on that path, and raises at -construction on the pairing — *"ech_egress and proxy_url are mutually exclusive — the ECH sidecar IS -this connection's egress path (ADR 0139)"* (`rest.py:1189-1193`). - -Point the engine at this sidecar with the per-connection `ech_egress` / `ech_sidecar` settings instead — -see [`samples/ech-sidecar/`](../../samples/ech-sidecar/README.md) for the one authoring form that works. -The sidecar terminates the loopback hop, upgrades the scheme to `https` and originates ECH; the engine -keeps its own verifying opener for every other connection. - -## Scope / limitations - -- Cloudflare DoH JSON returns type-65 RDATA in RFC 3597 generic form - (`\# `); a presentation-form `ech=""` fallback is also handled. -- No DoH result caching (one HTTPS lookup per request) — add a short TTL cache before - high-volume use. -- HTTP/1.1 and HTTP/2 upstream via `net/http` defaults; no WebSocket upgrade. diff --git a/tools/ech-sidecar/go.mod b/tools/ech-sidecar/go.mod deleted file mode 100644 index 3cae7d73..00000000 --- a/tools/ech-sidecar/go.mod +++ /dev/null @@ -1,6 +0,0 @@ -module messagefoundry.dev/ech-sidecar - -// stdlib-only, no dependencies — builds offline with GOPROXY=off. -// Requires Go >= 1.26 for crypto/tls Encrypted Client Hello -// (tls.Config.EncryptedClientHelloConfigList + ConnectionState.ECHAccepted). -go 1.26 diff --git a/tools/ech-sidecar/main.go b/tools/ech-sidecar/main.go deleted file mode 100644 index 474547ab..00000000 --- a/tools/ech-sidecar/main.go +++ /dev/null @@ -1,312 +0,0 @@ -// Command ech-sidecar is a terminating, fail-closed Encrypted Client Hello (ECH) -// re-originator for MessageFoundry (ASVS 12.1.5 — protect against SNI leakage). -// -// It listens on a loopback HTTP port as a forward proxy. For each request it: -// - determines the upstream host (absolute-form request URI, else the Host header), -// - resolves that host's ECHConfigList from its DNS HTTPS record (RR type 65) -// over DoH (application/dns-json), extracting SvcParamKey 5 (ech) by walking -// the SvcParams in the record's RDATA, -// - dials the upstream with crypto/tls using EncryptedClientHelloConfigList so the -// real SNI travels only inside the encrypted ClientHelloInner, -// - verifies the upstream certificate normally (verification is NEVER disabled), -// - re-issues the HTTP request over that connection and streams the response back. -// -// FAIL-CLOSED: if the upstream publishes no ECHConfig, or the TLS server does not -// accept ECH (ConnectionState.ECHAccepted == false), the request is REFUSED with a -// 502 — the sidecar never silently falls back to a cleartext-SNI connection. -// -// stdlib-only; no module dependencies; builds offline (GOPROXY=off). -package main - -import ( - "context" - "crypto/tls" - "encoding/base64" - "encoding/hex" - "encoding/json" - "errors" - "flag" - "fmt" - "io" - "log" - "net" - "net/http" - "net/url" - "strings" - "time" -) - -const dohURL = "https://cloudflare-dns.com/dns-query" - -// hop-by-hop headers that must not be forwarded (RFC 7230 §6.1). -var hopByHop = map[string]bool{ - "connection": true, - "proxy-connection": true, - "keep-alive": true, - "proxy-authenticate": true, - "proxy-authorization": true, - "te": true, - "trailer": true, - "transfer-encoding": true, - "upgrade": true, -} - -func main() { - addr := flag.String("addr", "127.0.0.1:8123", "loopback listen address (host:port)") - timeout := flag.Duration("timeout", 30*time.Second, "per-request upstream timeout") - flag.Parse() - - host, _, err := net.SplitHostPort(*addr) - if err != nil { - log.Fatalf("bad -addr %q: %v", *addr, err) - } - if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() { - log.Fatalf("refusing to bind non-loopback address %q (ECH sidecar is loopback-only)", *addr) - } - - h := &handler{timeout: *timeout} - srv := &http.Server{ - Addr: *addr, - Handler: h, - ReadHeaderTimeout: 15 * time.Second, - } - log.Printf("ech-sidecar: fail-closed ECH re-originator listening on http://%s", *addr) - log.Fatal(srv.ListenAndServe()) -} - -type handler struct { - timeout time.Duration -} - -func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - // CONNECT tunnels would give end-to-end TLS the sidecar cannot originate ECH - // into — reject explicitly rather than proxy blindly. - if r.Method == http.MethodConnect { - http.Error(w, "ech-sidecar does not support CONNECT tunnels; send an absolute-form request or set the Host header", http.StatusMethodNotAllowed) - return - } - - // Upstream host: prefer the absolute-form request URI (classic forward proxy), - // fall back to the Host header (plain http://sidecar/path + Host: upstream). - hostport := r.URL.Host - if hostport == "" { - hostport = r.Host - } - if hostport == "" { - http.Error(w, "no upstream host (absolute-form URI or Host header required)", http.StatusBadRequest) - return - } - hostname, port := hostport, "443" - if hh, pp, err := net.SplitHostPort(hostport); err == nil { - hostname, port = hh, pp - } - - ctx, cancel := context.WithTimeout(r.Context(), h.timeout) - defer cancel() - - echList, err := resolveECH(ctx, hostname) - if err != nil { - // FAIL-CLOSED: no ECHConfig -> refuse, do not connect in the clear. - http.Error(w, "ech-sidecar refusing (fail-closed): "+err.Error(), http.StatusBadGateway) - log.Printf("REFUSE %s: %v", hostname, err) - return - } - - transport := &http.Transport{ - Proxy: nil, - ForceAttemptHTTP2: true, - MaxIdleConns: 10, - IdleConnTimeout: 30 * time.Second, - TLSHandshakeTimeout: 15 * time.Second, - ExpectContinueTimeout: 1 * time.Second, - DialTLSContext: func(ctx context.Context, network, addr string) (net.Conn, error) { - d := &net.Dialer{Timeout: 15 * time.Second} - raw, err := d.DialContext(ctx, network, addr) - if err != nil { - return nil, err - } - cfg := &tls.Config{ - ServerName: hostname, // encrypted inside ClientHelloInner - EncryptedClientHelloConfigList: echList, - MinVersion: tls.VersionTLS13, - // InsecureSkipVerify stays false — the upstream cert is verified. - } - tc := tls.Client(raw, cfg) - if err := tc.HandshakeContext(ctx); err != nil { - raw.Close() - return nil, fmt.Errorf("tls handshake to %s: %w", hostname, err) - } - // FAIL-CLOSED second gate: even with a config list, if the server did - // not accept ECH the real SNI may have leaked — refuse the connection. - if !tc.ConnectionState().ECHAccepted { - tc.Close() - return nil, fmt.Errorf("ECH not accepted by %s — refusing (fail-closed)", hostname) - } - return tc, nil - }, - } - defer transport.CloseIdleConnections() - - outURL := &url.URL{ - Scheme: "https", - Host: net.JoinHostPort(hostname, port), - Path: r.URL.Path, - RawQuery: r.URL.RawQuery, - } - outReq, err := http.NewRequestWithContext(ctx, r.Method, outURL.String(), r.Body) - if err != nil { - http.Error(w, "bad upstream request: "+err.Error(), http.StatusBadGateway) - return - } - copyHeaders(outReq.Header, r.Header) - outReq.Host = hostname - - resp, err := transport.RoundTrip(outReq) - if err != nil { - http.Error(w, "ech-sidecar upstream error: "+err.Error(), http.StatusBadGateway) - log.Printf("UPSTREAM %s: %v", hostname, err) - return - } - defer resp.Body.Close() - - copyHeaders(w.Header(), resp.Header) - w.Header().Set("X-ECH-Sidecar", "ech-accepted") - w.WriteHeader(resp.StatusCode) - io.Copy(w, resp.Body) - log.Printf("OK %s %s -> %d (ECH accepted)", r.Method, hostname, resp.StatusCode) -} - -func copyHeaders(dst, src http.Header) { - for k, vv := range src { - if hopByHop[strings.ToLower(k)] { - continue - } - for _, v := range vv { - dst.Add(k, v) - } - } -} - -// resolveECH queries the DNS HTTPS record (type 65) for host over DoH and returns -// the ECHConfigList bytes from SvcParamKey 5. It returns an error (never a nil, -// nil "no ECH but ok") so callers fail closed. -func resolveECH(ctx context.Context, host string) ([]byte, error) { - q := dohURL + "?name=" + url.QueryEscape(host) + "&type=65" - req, err := http.NewRequestWithContext(ctx, http.MethodGet, q, nil) - if err != nil { - return nil, err - } - req.Header.Set("accept", "application/dns-json") - - resp, err := http.DefaultClient.Do(req) - if err != nil { - return nil, fmt.Errorf("DoH query for %s: %w", host, err) - } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("DoH query for %s: status %d", host, resp.StatusCode) - } - var out struct { - Answer []struct { - Type int `json:"type"` - Data string `json:"data"` - } `json:"Answer"` - } - if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { - return nil, fmt.Errorf("decode DoH JSON for %s: %w", host, err) - } - for _, a := range out.Answer { - if a.Type != 65 { // HTTPS RR - continue - } - ech, err := extractECH(a.Data) - if err != nil { - continue - } - if len(ech) > 0 { - return ech, nil - } - } - return nil, fmt.Errorf("no ECHConfig (HTTPS/SvcParamKey 5) published for %s", host) -} - -// extractECH pulls the ech SvcParamValue (SvcParamKey 5) out of an HTTPS RR's -// RDATA as returned by DoH JSON. Cloudflare returns type-65 RDATA in RFC 3597 -// generic form: `\# `. We hex-decode the RDATA and walk the -// SvcParams. As a fallback we also accept presentation form containing an -// `ech=""` token. -func extractECH(data string) ([]byte, error) { - data = strings.TrimSpace(data) - if strings.HasPrefix(data, `\#`) { - fields := strings.Fields(data) // ["\#", "", "", ...] - if len(fields) < 2 { - return nil, errors.New("malformed generic RDATA") - } - rdata, err := hex.DecodeString(strings.Join(fields[2:], "")) - if err != nil { - return nil, fmt.Errorf("hex RDATA: %w", err) - } - return walkSvcParamsForECH(rdata) - } - // Presentation-form fallback: find ech="..." (or ech=...). - if i := strings.Index(data, "ech="); i >= 0 { - rest := data[i+len("ech="):] - rest = strings.TrimSpace(rest) - if strings.HasPrefix(rest, `"`) { - if j := strings.Index(rest[1:], `"`); j >= 0 { - rest = rest[1 : 1+j] - } - } else if sp := strings.IndexAny(rest, " \t"); sp >= 0 { - rest = rest[:sp] - } - b, err := base64.StdEncoding.DecodeString(rest) - if err != nil { - return nil, fmt.Errorf("base64 ech value: %w", err) - } - return b, nil - } - return nil, errors.New("no ech SvcParam in RDATA") -} - -// walkSvcParamsForECH parses SVCB/HTTPS RDATA: SvcPriority(2) + TargetName(domain) -// + SvcParams(key(2) len(2) value(len))*, returning the value for key 5 (ech). -func walkSvcParamsForECH(rdata []byte) ([]byte, error) { - pos := 0 - if len(rdata) < 2 { - return nil, errors.New("RDATA too short for SvcPriority") - } - pos += 2 // SvcPriority - - // TargetName: length-prefixed labels ending in a zero-length root label. - for { - if pos >= len(rdata) { - return nil, errors.New("RDATA truncated in TargetName") - } - l := int(rdata[pos]) - pos++ - if l == 0 { - break // root label - } - if l&0xC0 != 0 { // no compression pointers in HTTPS RDATA - return nil, errors.New("unexpected compression in TargetName") - } - pos += l - } - - for pos+4 <= len(rdata) { - key := int(rdata[pos])<<8 | int(rdata[pos+1]) - plen := int(rdata[pos+2])<<8 | int(rdata[pos+3]) - pos += 4 - if pos+plen > len(rdata) { - return nil, errors.New("RDATA truncated in SvcParam value") - } - val := rdata[pos : pos+plen] - pos += plen - if key == 5 { // ech - out := make([]byte, len(val)) - copy(out, val) - return out, nil - } - } - return nil, errors.New("no ech SvcParam (key 5)") -} From 936c9ad1dd951597c02e76560ebe2db315214701 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 11 Aug 2026 07:34:34 -0500 Subject: [PATCH 6/9] feat(asvs): a structured blocker record for externally-blocked fails (G19 part b) A bare `fail` cannot distinguish two very different states: one nobody has got to, and one no amount of correct code can move. Prose in `residual` can say which, but prose carries no date -- so the second kind rots invisibly, because the blocking condition is external and can lift without anyone noticing. That is not hypothetical. The G19 retirement decision landed today on a DoH probe finding no counterparty publishing an ECHConfig, and at ruling time that measurement was three weeks old with NO recorded cadence for re-running it. The single fact that would have reversed the decision was going stale unwatched. This makes that computable instead. Seven fields, all mandatory: blocked_by, reason, evidence, unblock_signal, unblock_probe, checked_on, recheck_days. The unblock half is the whole point -- a signal nobody can probe, or a probe with no date, cannot go stale visibly. A PARTIAL record is refused at load, because a reason with no probe reads as diligence and carries none: the compensating-control-on-a-false- premise shape arriving through the fix. Modelled on the decision_closed precedent -- on the Cell rather than loose TOML, so the renderer can surface it. A blocker nobody can see is one a pass walks straight past. DELIBERATELY NOT a seventh verdict value: that would change the denominator and every renderer, and every existing count in every document would silently mean something else. DELIBERATELY not `na` either -- ASVS 5.0 dropped 4.0's clause letting a documented exclusion preserve a compliance claim, so `na` buys nothing and misdescribes what was assessed. The requirement applies, it was assessed, and it failed. HONEST LIMIT, stated in the docstring so it is not overclaimed: overdue blockers are REPORTED, not enforced. Making a stale probe red the gate would block unrelated pull requests on a calendar date -- attention bought at a cost nobody agreed to. --status prints the count where humans and CI already look, including when it is ZERO, so "no blockers" and "the section was dropped from the renderer" cannot look alike. Promoting it to a hard failure is a one-line change and a deliberate decision. Scoped to verdict `fail` on purpose. A blocked PARTIAL is a real thing (the V10 relying-party cells are the obvious candidates), but admitting one is a ruling, not a default -- so the loader refuses it and says so. WATCHED FAIL, 6/6 negative controls before the tests were written: a missing field, a blocker on a non-fail verdict, a non-ISO checked_on, and recheck_days = 0 are each REFUSED; the real file loads; and a well-formed but 400-day-stale record LOADS and reports "RE-PROBE OVERDUE: 12.1.5 by 310d". Now pinned by 12 tests, including one parametrised over all seven fields. 138 passed in tests/test_asvs_scorecard.py. --- scripts/asvs/scorecard.py | 128 +++++++++++++++++++++++++++++++++++ tests/test_asvs_scorecard.py | 92 +++++++++++++++++++++++++ 2 files changed, 220 insertions(+) diff --git a/scripts/asvs/scorecard.py b/scripts/asvs/scorecard.py index 9d5b8ff5..c73edafe 100644 --- a/scripts/asvs/scorecard.py +++ b/scripts/asvs/scorecard.py @@ -207,6 +207,59 @@ class Absence: observable: str = "" +@dataclass(frozen=True) +class Blocker: + """Why a ``fail`` cannot be closed by our own work, and WHEN that was last actually true. + + A bare ``fail`` cannot distinguish two very different states: one we have not got to, and one no + amount of correct code can move. Prose in ``residual`` can say which, but prose carries no date, + so the second kind rots invisibly — the blocking condition is external and can lift without + anyone noticing. That is not hypothetical here: the ASVS 12.1.5 retirement decision (G19, + 2026-08-11) rested on a DoH probe finding no counterparty publishing an ECHConfig, and at ruling + time that measurement was three weeks old with **no recorded cadence for re-running it**. The one + fact that would reverse the decision was going stale with nobody watching. + + So the unblock half is mandatory, not optional. ``unblock_signal`` states what would change in + the world; ``unblock_probe`` is the executable procedure that detects it; ``checked_on`` is when + that probe last ran; ``recheck_days`` is how long that answer stays good. Together they make + staleness a computable property rather than something a reader has to notice. + + **Deliberately NOT a seventh verdict value.** A new verdict would change the denominator and + every renderer, and every existing count in every document would silently mean something else. + **Deliberately not ``na`` either:** ASVS 5.0 dropped 4.0's clause that let a documented exclusion + preserve a compliance claim, so ``na`` buys nothing here and misdescribes what was assessed — the + requirement applies, it was assessed, and it failed. + + **Honest limit, stated so it is not overclaimed:** overdue blockers are REPORTED, not enforced. + Making a stale probe red the gate would block unrelated pull requests on a calendar date, which + buys attention at a cost nobody agreed to. The count is printed by ``--status`` so it is visible + where humans and CI already look. Promoting it to a hard failure is a one-line change in + :func:`verify` and a deliberate decision, not an oversight. + """ + + #: Short slug for the external thing that blocks it, e.g. ``cpython-stdlib``, ``no-counterparty``. + blocked_by: str + #: Prose: why our own work cannot close it. + reason: str + #: The measurement or citation backing ``reason`` — not an assertion. + evidence: str + #: What would have to change in the world for this to become closable. + unblock_signal: str + #: The executable procedure that detects ``unblock_signal``. A signal nobody can test is a wish. + unblock_probe: str + #: ISO date ``unblock_probe`` last ran. This is the field that makes staleness computable. + checked_on: str + #: How many days ``checked_on`` stays good before the probe owes a re-run. + recheck_days: int + + def days_overdue(self, today: datetime.date) -> int: + """Days past the re-probe deadline; 0 when still current. Never negative.""" + due = datetime.date.fromisoformat(self.checked_on) + datetime.timedelta( + days=self.recheck_days + ) + return max(0, (today - due).days) + + @dataclass(frozen=True) class Cell: id: str @@ -224,6 +277,9 @@ class Cell: decision_closed: bool = False decision_closed_on: str = "" decision_closed_by: str = "" + #: Set when this cell's verdict is held down by something outside the project's control. See + #: :class:`Blocker` — the point of it is the re-probe cadence, not the excuse. + blocker: Blocker | None = None evidence: tuple[Anchor, ...] = () absence: tuple[Absence, ...] = () @@ -382,11 +438,65 @@ def load_scorecard(path: Path) -> list[Cell]: "the thing came back; do NOT derive it from the pattern, which makes the check " "vacuous. See the Absence docstring for a worked example" ) + # A blocker record is admissible ONLY as a complete set. A partial one -- a reason with no + # probe, or a probe with no date -- reads as diligence and carries none: it is exactly the + # "compensating control resting on a false premise" shape, arriving through the fix. So every + # field is required, and the two computable ones are type-checked here rather than at the + # point of use, where a bad value would surface as a traceback in a renderer. + blocker: Blocker | None = None + if (rb := raw.get("blocker")) is not None: + if verdict != "fail": + raise ScorecardError( + f"cell {raw.get('id')!r}: a `blocker` is only meaningful on verdict 'fail' (got " + f"{verdict!r}). A blocked PARTIAL is a real thing, but admitting one here needs " + "its own ruling -- widening this is a decision, not a default" + ) + missing = [ + k + for k in ( + "blocked_by", + "reason", + "evidence", + "unblock_signal", + "unblock_probe", + "checked_on", + "recheck_days", + ) + if not str(rb.get(k, "")).strip() + ] + if missing: + raise ScorecardError( + f"cell {raw.get('id')!r}: `blocker` is missing {missing} -- a partial blocker " + "record is a comment, not a control. The unblock half is the whole point: a " + "signal nobody can probe, or a probe with no date, cannot go stale visibly" + ) + try: + datetime.date.fromisoformat(str(rb["checked_on"])) + except ValueError as exc: + raise ScorecardError( + f"cell {raw.get('id')!r}: `blocker.checked_on` must be an ISO date " + f"(YYYY-MM-DD), got {rb['checked_on']!r}" + ) from exc + if int(rb["recheck_days"]) < 1: + raise ScorecardError( + f"cell {raw.get('id')!r}: `blocker.recheck_days` must be >= 1; a cadence of " + "zero or less never comes due, which is the same as having none" + ) + blocker = Blocker( + blocked_by=str(rb["blocked_by"]), + reason=str(rb["reason"]), + evidence=str(rb["evidence"]), + unblock_signal=str(rb["unblock_signal"]), + unblock_probe=str(rb["unblock_probe"]), + checked_on=str(rb["checked_on"]), + recheck_days=int(rb["recheck_days"]), + ) cells.append( Cell( id=str(raw["id"]), level=int(raw["level"]), verdict=verdict, # type: ignore[arg-type] + blocker=blocker, residual=str(raw.get("residual", "")), posture=str(raw.get("posture", "single")), decision_closed=raw.get("decision_closed") is True, @@ -1720,8 +1830,26 @@ def status_lines(cells: list[Cell]) -> list[str]: 1 for c in cells if c.verdict in DECIDED_VERDICTS and not c.evidence and not c.absence ) pct = (100.0 * examined / total) if total else 0.0 + # Externally-blocked fails, and -- the part that matters -- whether anyone has re-probed the + # thing blocking them lately. Printed even when the count is zero, so "no blockers" and "the + # blocker section was dropped from the renderer" cannot look alike. + blocked = [c for c in cells if c.blocker is not None] + today = datetime.date.today() + overdue = [ + (c.id, c.blocker.days_overdue(today)) + for c in blocked + if c.blocker is not None and c.blocker.days_overdue(today) > 0 + ] + blocker_line = f"blocked {len(blocked)} fail cell(s) held by an external condition" + if overdue: + blocker_line += "; RE-PROBE OVERDUE: " + ", ".join( + f"{cid} by {n}d" for cid, n in sorted(overdue, key=lambda x: -x[1]) + ) + elif blocked: + blocker_line += "; all re-probes current" return [ f"cells {total}: " + ", ".join(f"{c} {v}" for v, c in parts), + blocker_line, f"examined {examined} of {total} ({pct:.1f}%) against the pinned text; " f"{inherited} decided with no last_verified; {closed} closed by owner decision", f"evidence {anchors} anchors in {anchored_cells} cells over {len(paths)} paths; " diff --git a/tests/test_asvs_scorecard.py b/tests/test_asvs_scorecard.py index b5c9423f..3810aa41 100644 --- a/tests/test_asvs_scorecard.py +++ b/tests/test_asvs_scorecard.py @@ -2557,3 +2557,95 @@ def test_descent_and_transparency_tables_do_not_drift_apart() -> None: lacks. A node in `_DESCEND_ONLY` but not `_TRANSPARENT` would start emitting a chain element nobody authored; the reverse would make a transparent node undescendable.""" assert frozenset(_DESCEND_ONLY) == _TRANSPARENT + + +# --- the structured blocker record (owner ruling, G19 part b, 2026-08-11) ------------------------ +# +# A `fail` cannot, on its own, distinguish "we have not got to it" from "no correct code can move +# it". The blocker record says which, and -- the part that matters -- carries a re-probe cadence so +# the external condition cannot rot invisibly. These tests exist because the ruling was made on a +# probe that was already three weeks stale with no cadence recorded. + +_BLOCKED_CELL = """ +[[cell]] +id = "1.1.1" +level = 1 +verdict = "fail" +residual = "held down by something outside our control" + [cell.blocker] + blocked_by = "upstream-stdlib" + reason = "the interpreter exposes no API for it" + evidence = "a ctypes probe found zero symbols, against a live control" + unblock_signal = "the stdlib ships the API" + unblock_probe = "python -c 'import ssl; print(dir(ssl))'" + checked_on = "2026-07-20" + recheck_days = 90 + [[cell.evidence]] + path = "messagefoundry/m.py" + line = 1 + expect = "x" +""" + + +def test_a_well_formed_blocker_loads_and_attaches(tmp_path: Path) -> None: + cell = load_scorecard(_scorecard_file(tmp_path, _BLOCKED_CELL))[0] + assert cell.blocker is not None + assert cell.blocker.blocked_by == "upstream-stdlib" + assert cell.blocker.recheck_days == 90 + + +@pytest.mark.parametrize( + "field", + [ + "blocked_by", + "reason", + "evidence", + "unblock_signal", + "unblock_probe", + "checked_on", + "recheck_days", + ], +) +def test_a_partial_blocker_record_is_refused(tmp_path: Path, field: str) -> None: + """Every field is load-bearing. A reason with no probe, or a probe with no date, reads as + diligence and carries none -- the compensating-control-on-a-false-premise shape arriving + through the fix.""" + body = _BLOCKED_CELL.replace(f" {field} = ", f" removed_{field} = ", 1) + with pytest.raises(ScorecardError, match="partial blocker record is a comment"): + load_scorecard(_scorecard_file(tmp_path, body)) + + +def test_a_blocker_on_a_non_fail_verdict_is_refused(tmp_path: Path) -> None: + """A blocked PARTIAL is a real thing; admitting one is a ruling, not a default.""" + body = _BLOCKED_CELL.replace('verdict = "fail"', 'verdict = "partial"', 1) + with pytest.raises(ScorecardError, match="only meaningful on verdict 'fail'"): + load_scorecard(_scorecard_file(tmp_path, body)) + + +def test_a_non_iso_checked_on_is_refused(tmp_path: Path) -> None: + body = _BLOCKED_CELL.replace('checked_on = "2026-07-20"', 'checked_on = "last July"', 1) + with pytest.raises(ScorecardError, match="must be an ISO date"): + load_scorecard(_scorecard_file(tmp_path, body)) + + +def test_a_cadence_that_never_comes_due_is_refused(tmp_path: Path) -> None: + """recheck_days = 0 is indistinguishable from having no cadence, which is the defect.""" + body = _BLOCKED_CELL.replace("recheck_days = 90", "recheck_days = 0", 1) + with pytest.raises(ScorecardError, match="never comes due"): + load_scorecard(_scorecard_file(tmp_path, body)) + + +def test_overdue_is_computed_not_asserted(tmp_path: Path) -> None: + """Overdue is a REPORTING state, not a load error -- a stale probe must not red the gate and + block unrelated pull requests on a calendar date. So it loads, and the staleness is a number.""" + import datetime + + stale = (datetime.date.today() - datetime.timedelta(days=400)).isoformat() + body = _BLOCKED_CELL.replace('checked_on = "2026-07-20"', f'checked_on = "{stale}"', 1) + cell = load_scorecard(_scorecard_file(tmp_path, body))[0] + assert cell.blocker is not None + assert cell.blocker.days_overdue(datetime.date.today()) == 400 - 90 + # And a current one reports zero rather than a negative number. + fresh = load_scorecard(_scorecard_file(tmp_path, _BLOCKED_CELL))[0] + assert fresh.blocker is not None + assert fresh.blocker.days_overdue(datetime.date.fromisoformat("2026-07-21")) == 0 From 0ace6bb6499c74104883a07a61510c7afc1d6e79 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 11 Aug 2026 07:47:46 -0500 Subject: [PATCH 7/9] feat(auth)!: retire SHA-1 from TOTP in favour of SHA-256 (G18) BREAKING for any enrolled authenticator. Ruled under G18 on 2026-08-11, on the argument that this is the one moment the cutover is free: there is no per-user TOTP algorithm column, so it is a cutover rather than a migration, and at zero deployments there are zero enrolled users to re-enrol. That argument expires on first deployment and never returns. THE DANGEROUS PROPERTY, and why this is not a one-line change. The engine computes with its digest and the authenticator computes with whatever otpauth_uri advertised. If those two ever disagree, NOTHING RAISES -- codes simply never match, for every user, with no diagnostic. So the advertised name is DERIVED from the digest (_TOTP_ALGORITHM = _TOTP_DIGEST().name.upper()) rather than written beside it, which makes editing one without the other impossible instead of merely discouraged. hashlib's .name is exactly the otpauth spelling, and the derivation is correct for all three RFC 6238 permitted digests. A test pins that it stays derived. KNOWN COST, documented at the call site rather than discovered later: Google Authenticator historically IGNORES the otpauth `algorithm` parameter and computes SHA-1 regardless, so its codes will never match this engine. Most modern authenticators honour it (1Password, Bitwarden, Aegis, FreeOTP, Authy). That is a support burden, not a security one, and the fix for an operator hitting it is an app that honours the parameter. The RFC 6238 conformance test moves to the SHA-256 rows of the same Appendix B table rather than being deleted -- and is now asserted at the RFC's own EIGHT digits instead of truncated to six, which tests strictly more of the truncation math than before. The vectors were derived independently (a hand-rolled HMAC-SHA256 HOTP, no engine code) and match the published table on all six rows. Note the seeds differ per digest: SHA-1 uses the 20-byte ASCII seed, SHA-256 the 32-byte one, and pairing the SHA-1 seed with the SHA-256 rows silently produces non-matching codes -- called out in the test so the next reader does not have to rediscover it. _SECRET_BYTES stays at 20 DELIBERATELY, and its comment no longer cites a retired algorithm as its rationale. RFC 6238 R6 says the key SHOULD match the HMAC output length (32 bytes for SHA-256), but that clause is about interop convention rather than strength; 160 bits is ample against HMAC-SHA256, and 32 bytes would lengthen manual entry from 32 to 52 base32 characters on a screen an operator types from. Swept the tree for other TOTP algorithm claims: none in docs/, packaging/ or the web console. The only remaining SHA-1 mention was the secret-length comment, corrected here. 543 passed across the totp/mfa/auth selection; ruff and mypy clean. --- messagefoundry/auth/totp.py | 41 +++++++++++++++++++++++---- tests/test_totp.py | 55 ++++++++++++++++++++++++++----------- 2 files changed, 75 insertions(+), 21 deletions(-) diff --git a/messagefoundry/auth/totp.py b/messagefoundry/auth/totp.py index b59d1c1b..eb8440e4 100644 --- a/messagefoundry/auth/totp.py +++ b/messagefoundry/auth/totp.py @@ -46,7 +46,13 @@ #: ± steps of clock skew tolerated at verify time (one step each side ≈ 30 s). DEFAULT_WINDOW = 1 -_SECRET_BYTES = 20 # 160 bits — RFC 4226 recommends ≥ 128 bits, 160 for HMAC-SHA1 +# 160 bits. RFC 4226 requires >= 128 and recommends 160. RFC 6238 R6 additionally says the key +# SHOULD match the HMAC output length -- 32 bytes now that the digest is SHA-256 (see _TOTP_DIGEST). +# Kept at 20 DELIBERATELY: that clause is about interoperability convention rather than strength, 160 +# bits is ample against HMAC-SHA256, and 32 bytes would lengthen manual entry from 32 to 52 base32 +# characters on a screen an operator types from. Revisit only with a real interop failure, not on the +# SHOULD alone. +_SECRET_BYTES = 20 # Recovery codes: human-legible groups from an unambiguous alphabet (no 0/O/1/I/L confusion). _RECOVERY_ALPHABET = "ABCDEFGHJKMNPQRSTUVWXYZ23456789" @@ -66,9 +72,25 @@ def _decode_secret(secret: str) -> bytes: return base64.b32decode(cleaned + padding, casefold=True) +#: The HOTP MAC. RFC 6238 permits SHA-1, SHA-256 and SHA-512; SHA-1 is the RFC default and was the +#: shipped choice until 2026-08-11, when it was retired under the G18 ruling. +#: +#: THIS CONSTANT AND THE ADVERTISED ALGORITHM MUST NEVER DIVERGE. The authenticator computes with +#: whatever ``otpauth_uri`` told it and the engine computes with this — so a change to one alone does +#: not fail loudly, it silently produces codes that never match, for every user, with no diagnostic. +#: That is why :data:`_TOTP_ALGORITHM` is DERIVED from this rather than written beside it: the two +#: cannot be edited apart. ``hashlib.sha256().name.upper()`` is exactly the otpauth spelling, and the +#: same derivation is correct for all three permitted digests. +_TOTP_DIGEST = hashlib.sha256 +_TOTP_ALGORITHM = _TOTP_DIGEST().name.upper() + + def _hotp(key: bytes, counter: int, digits: int) -> str: - """RFC 4226 HOTP: HMAC-SHA1 over the 8-byte counter, dynamically truncated to ``digits`` decimals.""" - mac = hmac.new(key, counter.to_bytes(8, "big"), hashlib.sha1).digest() + """RFC 4226 HOTP over the 8-byte counter, dynamically truncated to ``digits`` decimals. + + The MAC is :data:`_TOTP_DIGEST` (SHA-256 since 2026-08-11), not RFC 4226's SHA-1. + """ + mac = hmac.new(key, counter.to_bytes(8, "big"), _TOTP_DIGEST).digest() offset = mac[-1] & 0x0F truncated = int.from_bytes(mac[offset : offset + 4], "big") & 0x7FFFFFFF return str(truncated % (10**digits)).zfill(digits) @@ -170,14 +192,23 @@ def otpauth_uri( period: int = DEFAULT_PERIOD, digits: int = DEFAULT_DIGITS, ) -> str: - """Build the ``otpauth://totp/…`` URI an authenticator app scans (the UI renders it as a QR code).""" + """Build the ``otpauth://totp/…`` URI an authenticator app scans (the UI renders it as a QR code). + + **Advertises SHA-256, and enrolling apps must honour it.** Most modern authenticators do (1Password, + Bitwarden, Aegis, FreeOTP, Authy). **Google Authenticator historically IGNORES the ``algorithm`` + parameter and computes SHA-1 regardless** — against which this engine's codes will simply never + match, with no error to explain why. That is the known cost of the 2026-08-11 SHA-1 retirement + (G18), accepted while there are zero enrolled users; it is a support burden, not a security one. + An operator hitting it needs an app that honours the parameter, not a re-enrolment. + """ # The "issuer:account" colon is the conventional literal label separator (keep it; encode the rest). label = quote(f"{issuer}:{account}", safe=":") params = urlencode( { "secret": secret, "issuer": issuer, - "algorithm": "SHA1", + # DERIVED, never a literal — see _TOTP_DIGEST for why these two cannot be edited apart. + "algorithm": _TOTP_ALGORITHM, "digits": digits, "period": period, } diff --git a/tests/test_totp.py b/tests/test_totp.py index 9bdbc9c2..a073b70c 100644 --- a/tests/test_totp.py +++ b/tests/test_totp.py @@ -2,9 +2,10 @@ # Copyright (C) 2026 MessageFoundry Organization and contributors """Unit tests for the RFC 6238 TOTP second factor (auth/totp.py, WP-14). -The vector tests pin the implementation against the official RFC 6238 Appendix B test values (the -SHA-1, 8-digit set) reduced to the 6-digit codes authenticator apps emit, so a regression in the -HMAC/truncation math is caught in CI. +The vector tests pin the implementation against the official RFC 6238 Appendix B test values. The +engine retired SHA-1 on 2026-08-11 (G18), so these are the **SHA-256** rows of that same table, and +they are asserted at the published **8 digits** rather than truncated to 6 — the RFC prints 8, so +asserting 8 tests strictly more of the truncation math than reducing them first. """ from __future__ import annotations @@ -15,25 +16,47 @@ from messagefoundry.auth import totp -# RFC 6238 Appendix B uses the ASCII seed "12345678901234567890" (20 bytes) for HMAC-SHA1; the -# engine API takes base32, so encode it the way an authenticator app stores it. -_RFC_SECRET = base64.b32encode(b"12345678901234567890").decode("ascii") +# RFC 6238 Appendix B seeds each digest differently: SHA-1 uses the 20-byte ASCII +# "12345678901234567890", SHA-256 uses the 32-byte "12345678901234567890123456789012". Using the +# SHA-1 seed against the SHA-256 rows silently produces non-matching codes, so the pairing matters. +# The engine API takes base32, so encode it the way an authenticator app stores it. +_RFC_SECRET = base64.b32encode(b"12345678901234567890123456789012").decode("ascii") @pytest.mark.parametrize( ("moment", "expected"), [ - (59, "287082"), - (1111111109, "081804"), - (1111111111, "050471"), - (1234567890, "005924"), - (2000000000, "279037"), - (20000000000, "353130"), + (59, "46119246"), + (1111111109, "68084774"), + (1111111111, "67062674"), + (1234567890, "91819424"), + (2000000000, "90698825"), + (20000000000, "77737706"), ], ) -def test_rfc6238_sha1_vectors_6digit(moment: int, expected: str) -> None: - # Each is the last 6 digits of the published 8-digit RFC 6238 SHA-1 vector at that timestamp. - assert totp.totp(_RFC_SECRET, now=moment) == expected +def test_rfc6238_sha256_vectors_8digit(moment: int, expected: str) -> None: + """The published SHA-256 rows of RFC 6238 Appendix B, at the RFC's own 8 digits. + + Falsified on purpose while writing: against the SHA-1 seed, or against the pre-2026-08-11 SHA-1 + digest, every one of these six goes red. + """ + assert totp.totp(_RFC_SECRET, now=moment, digits=8) == expected + + +def test_the_digest_and_the_advertised_algorithm_cannot_diverge() -> None: + """The single most dangerous edit in this module is changing one without the other. + + The authenticator computes with whatever `otpauth_uri` advertised; the engine computes with + `_TOTP_DIGEST`. If they disagree nothing raises -- codes simply never match, for every user, with + no diagnostic. So the advertised name is DERIVED from the digest, and this pins that it stays + derived rather than drifting back to a literal. + """ + assert totp._TOTP_DIGEST().name.upper() == totp._TOTP_ALGORITHM + assert totp._TOTP_ALGORITHM == "SHA256" + assert f"algorithm={totp._TOTP_ALGORITHM}" in totp.otpauth_uri(_RFC_SECRET, "u@example.test") + # And the engine's own codes verify under the algorithm it advertises -- the round trip the + # divergence would break. + assert totp.verify_totp(_RFC_SECRET, totp.totp(_RFC_SECRET, now=59), now=59) def test_generate_secret_is_decodable_160_bit_and_unique() -> None: @@ -76,7 +99,7 @@ def test_otpauth_uri_carries_secret_and_metadata() -> None: assert uri.startswith("otpauth://totp/MessageFoundry:alice?") assert f"secret={secret}" in uri assert "issuer=MessageFoundry" in uri - assert "algorithm=SHA1" in uri + assert "algorithm=SHA256" in uri # SHA-1 retired 2026-08-11 (G18) assert "digits=6" in uri assert "period=30" in uri From e0e979d51d6a556688c8b7d5a753f7265e9fe9d9 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 11 Aug 2026 08:33:45 -0500 Subject: [PATCH 8/9] feat(mllp): pace inbound message rate per connection, default off (ASVS 2.4.1 / 15.2.2) The engine had no bound on messages per second from an accepted peer in ANY configuration -- verified by grep against a live positive control (zero matches in transports/mllp.py, ten in transports/http_listener.py on the identical pattern), with docs/SECURITY.md stating the absence itself. A sender able to reach the NIC-bound data plane could submit unbounded messages, each durably persisted before its ACK. IT HAD TO BE A PACER, NOT A LIMITER, and that is the whole design. The count-and-log invariant forbids accept-and-drop, so discarding was never available. NAKing would mean refusing clinical messages the engine can process. Closing the connection moves the loss outside our boundary where we cannot count it. A store-side quota bounds retention, not intake. Pacing the READ is the only option that satisfies the invariant BY CONSTRUCTION: the excess is never framed, so it never becomes a received message the invariant would then oblige us to account for, and TCP applies the back-pressure itself. The wait sits BEFORE the read and never around the handler. Delaying anything after decode would pace a message already counted -- the same control with none of the property. A bounded delay rather than a hard stop, deliberately: refusing to read at all holds the connection open and keeps consuming a max_connections slot, so a flood of paced peers could exhaust the slot budget and BECOME the denial of service this exists to prevent. The delay is exactly the bucket's deficit, so it is bounded by messages/rate. Scoped per connection, not per peer IP. MLLP peers are unauthenticated and identified only by address, so a per-IP budget collapses under NAT or a shared integration host and would throttle unrelated feeds sharing an egress address. A peer opening more connections is bounded by max_connections instead. SHIPS OFF, and that is a ruled exception to this module's "key absent -> secure default" convention rather than an oversight -- stated at the constant. A rate limit on a clinical interface is only safe at a number derived from a real feed profile, and this project has none; a guessed default would throttle real traffic, which is worse than the unbounded intake it guards. So ASVS 2.4.1 stays `partial` on the shipped default and the record will say why. That is the honest outcome of the ruling, not a shortfall in this change. THE LOAD-BEARING TEST IS NOT THAT PACING HAPPENS -- it is test_pacing_never_drops_a_message: 12 messages at 20/s with burst 2 all arrive, all ACKed, in order. Note it still PASSES with the pacer neutered, and that is correct: it asserts a SAFETY property that must hold either way, while test_pacing_actually_delays_the_reads asserts the liveness half. Watched fail: neutering charge() to return 0.0 reds three tests including the end-to-end one. The timing assertion is a LOWER bound only. An upper bound would pin scheduler timing and make this the flaky test someone deletes. 638 passed across the mllp/transport selection; 198 across the docs selection after documenting both settings in docs/CONNECTIONS.md. ruff and mypy clean. --- docs/CONNECTIONS.md | 2 + messagefoundry/transports/mllp.py | 89 +++++++++++++++++ tests/test_mllp_message_pacing.py | 156 ++++++++++++++++++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 tests/test_mllp_message_pacing.py diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 7d15bd3d..8b306ed4 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -286,6 +286,8 @@ duplicate name (across **any** of these files) and an inbound that binds a route | `max_connections` | in | `256` | cap on concurrent client connections (connection-flood guard). `None`/`0` = unlimited. | | `receive_timeout` | in | `60.0` | close a client idle this many seconds (slowloris guard). `None`/`0` = no timeout. | | `max_frame_bytes` | both | `16 MiB` | reject a single MLLP frame larger than this before buffering it whole (OOM guard); applies to inbound frames and outbound ACKs. `None`/`0` = unlimited. | +| `max_messages_per_second` | in | **off** | sustained message-rate ceiling per **connection** (ASVS 2.4.1 / 15.2.2). Over budget the listener **pauses reading**, so TCP back-pressures the sender — **no message is ever dropped, refused or NAK'd**, and none is reordered. Unset = no bound, which is a deliberate exception to this table's usual secure-default rule: a guessed rate on a clinical interface throttles real traffic, so the number has to come from your own feed profile. | +| `message_burst` | in | = the rate | tokens the bucket holds, i.e. how large a burst passes unpaced before the sustained rate applies. Only meaningful with `max_messages_per_second` set. Floor of 1 so a connection can always make progress. | | `connect_timeout` | out | `10.0` | TCP connect timeout (s) | | `timeout_seconds` | out | `30.0` | wait this long for the ACK | | `no_ack` | out | `false` | **(BACKLOG #117, ADR 0124) fire-and-forward (MLLP outbound only):** when `true`, deliver on the successful TCP **write** and read **no** ACK — delivery is confirmed on write, **not** on a positive MSA-1 ACK, so there is **no NAK- or timeout-driven retry** (*at-most-once-confirmation*). A connect/drain failure is still charged and retried (at-least-once for the write; a retry may duplicate — receivers stay idempotent). Composes with `persistent=true` (no handshake **and** no ACK wait — the max-throughput non-acking posture). **Incompatible with `capture_response`/`reingress_to`** (nothing to capture) and MLLP-only — both rejected at `check`. `false` (default) = **byte-identical** (read + validate one ACK). | diff --git a/messagefoundry/transports/mllp.py b/messagefoundry/transports/mllp.py index c08a02fb..e589a504 100644 --- a/messagefoundry/transports/mllp.py +++ b/messagefoundry/transports/mllp.py @@ -105,6 +105,15 @@ DEFAULT_MAX_FRAME_BYTES = 16 * 1024 * 1024 # 16 MiB — fits embedded base64 docs, bounds OOM DEFAULT_MAX_CONNECTIONS = 256 # bound concurrent inbound clients (connection-flood guard) DEFAULT_RECEIVE_TIMEOUT = 60.0 # seconds — close inbound sockets idle this long (slowloris guard) + +#: Message-rate pacing ships OFF, and that is a DELIBERATE DEVIATION from this module's +#: "key absent -> secure default" convention, ruled 2026-08-11 (ASVS 2.4.1 / 15.2.2). A rate limit +#: on a clinical interface is only safe at a number derived from a real feed profile, and this +#: project has no site data to derive one from — shipping a guessed default would throttle real +#: traffic, which is a worse failure than the unbounded intake it would be guarding. So the +#: mechanism exists and an operator opts in with their own number. The cell stays `partial` on the +#: shipped default and the record says why; that is the honest outcome, not a disappointing one. +DEFAULT_MAX_MESSAGES_PER_SECOND: float | None = None # On stop()/reload, established clients are closed and their handlers given this long to finish an # in-flight commit before the connection tasks are cancelled — bounds shutdown so a peer holding a # connection open can't hang it (review H-2). @@ -1296,6 +1305,55 @@ def _peer_host(writer: asyncio.StreamWriter) -> str | None: return None +class _MessagePacer: + """Per-connection message-rate pacing for the MLLP data plane (ASVS 2.4.1, and the availability + bound of 15.2.2 — one control, because building them apart yields two halves that interact). + + Charges one token per DECODED message and answers ONE question: how long to wait before + consuming more from the socket. + + **It never drops, never NAKs and never refuses, and that is the whole design.** The count-and-log + invariant forbids accept-and-drop, so a limiter that discards is not available. NAKing would mean + refusing clinical messages the engine is able to process. Closing the connection moves the loss + outside our boundary, where we cannot count it. Pacing the READ is the only option that satisfies + the invariant *by construction*: the excess is never framed, so it never becomes a received + message that the invariant would then oblige us to account for. Back-pressure is applied by TCP + itself once we stop consuming. + + **Why a bounded delay rather than a hard stop.** Refusing to read at all holds the connection open + and keeps consuming a ``max_connections`` slot, so a flood of paced peers could exhaust the slot + budget and *become* the denial of service this exists to prevent. A delay bounded by the deficit + paces the peer without pinning capacity. + + Scoped per connection, deliberately. MLLP peers are unauthenticated and identified only by + address, so a per-IP budget collapses under NAT or a shared integration host — it would throttle + unrelated feeds that happen to share an egress address. A per-connection budget is honest about + what it bounds; a peer opening more connections is bounded by ``max_connections`` instead. + """ + + __slots__ = ("_capacity", "_last", "_rate", "_tokens", "pending_wait") + + def __init__(self, rate: float, burst: float, *, now: float) -> None: + self._rate = rate + self._capacity = max(burst, 1.0) + self._tokens = self._capacity + self._last = now + #: Debt owed before the next read, in seconds. Carried on the pacer rather than in a local so + #: the read loop has exactly one place to consult and one place to clear. + self.pending_wait = 0.0 + + def charge(self, messages: int, *, now: float) -> float: + """Charge ``messages`` and return the seconds to wait before reading again (0.0 if none). + + The returned delay is exactly the time for the bucket to return to non-negative, so it is + bounded by ``messages / rate`` and cannot grow without limit. + """ + self._tokens = min(self._capacity, self._tokens + (now - self._last) * self._rate) + self._last = now + self._tokens -= messages + return 0.0 if self._tokens >= 0 else -self._tokens / self._rate + + class MLLPSource(SourceConnector): """Listen for inbound MLLP connections, hand each message to the pipeline handler, and frame whatever the handler returns back to the sender as the ACK.""" @@ -1315,6 +1373,14 @@ def __init__(self, config: Source) -> None: self.receive_timeout: float | None = float(rt) if rt else None mf = s.get("max_frame_bytes", DEFAULT_MAX_FRAME_BYTES) self.max_frame_bytes: int | None = int(mf) if mf else None + # Message-rate pacing. Absent -> OFF, unlike the caps above; see + # DEFAULT_MAX_MESSAGES_PER_SECOND for why that deviation is deliberate and ruled. + mps = s.get("max_messages_per_second", DEFAULT_MAX_MESSAGES_PER_SECOND) + self.max_messages_per_second: float | None = float(mps) if mps else None + # Burst defaults to one second's worth, so a peer that sends in bursts is not paced until it + # exceeds the SUSTAINED rate. Meaningless when pacing is off. + mb = s.get("message_burst") or self.max_messages_per_second or 0.0 + self.message_burst: float = float(mb) # Per-connection peer-IP allowlist (Tier 4 operability): when set, a connecting peer whose IP # is not listed is refused at accept time. Absent/empty = no restriction. sa = s.get("source_ip_allowlist") @@ -1430,7 +1496,24 @@ async def _on_client(self, reader: asyncio.StreamReader, writer: asyncio.StreamW await self._emit_event("established", peer_host=peer_host) try: decoder = MLLPDecoder(max_frame_bytes=self.max_frame_bytes) + pacer = ( + _MessagePacer( + self.max_messages_per_second, + self.message_burst, + now=time.monotonic(), + ) + if self.max_messages_per_second + else None + ) while True: + # ASVS 2.4.1 / 15.2.2. The wait happens BEFORE the read and never around the + # handler: stopping consumption lets TCP back-pressure the sender, so the excess + # is never framed and never becomes a received message. Delaying anything AFTER + # decode would pace a message the count-and-log invariant has already obliged us + # to account for -- which is the same control with none of the property. + if pacer is not None and (wait := pacer.pending_wait) > 0.0: + pacer.pending_wait = 0.0 + await asyncio.sleep(wait) if self.receive_timeout: try: chunk = await asyncio.wait_for(reader.read(4096), self.receive_timeout) @@ -1442,11 +1525,17 @@ async def _on_client(self, reader: asyncio.StreamReader, writer: asyncio.StreamW if not chunk: break try: + decoded = 0 for message in decoder.feed(chunk): + decoded += 1 reply = await self._handler(message) if reply is not None: writer.write(frame(reply, self.encoding)) await writer.drain() + # Charge AFTER the messages in this chunk are fully handled and ACKed. The + # debt is settled before the NEXT read, never by withholding an ACK. + if pacer is not None and decoded: + pacer.pending_wait = pacer.charge(decoded, now=time.monotonic()) except MLLPFrameError as exc: peer = writer.get_extra_info("peername") logger.warning( diff --git a/tests/test_mllp_message_pacing.py b/tests/test_mllp_message_pacing.py new file mode 100644 index 00000000..58639dff --- /dev/null +++ b/tests/test_mllp_message_pacing.py @@ -0,0 +1,156 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""ASVS 2.4.1 / 15.2.2 — message-rate pacing on the inbound MLLP data plane. + +The engine had no bound on messages per second from an accepted peer in any configuration, so a +sender able to reach the NIC-bound data plane could submit unbounded messages, each durably +persisted before its ACK. + +**The control had to be a pacer rather than a limiter, and that is what these tests pin.** The +count-and-log invariant forbids accept-and-drop, so discarding was never available; NAKing would +mean refusing clinical messages the engine can process; closing the connection moves the loss +outside the boundary where it cannot be counted. Pacing the READ satisfies the invariant by +construction — the excess is never framed, so it never becomes a received message. + +The load-bearing test here is therefore NOT that pacing happens. It is +:func:`test_pacing_never_drops_a_message` — every message a paced sender sends still arrives. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +from messagefoundry.config.models import ConnectorType, Source +from messagefoundry.transports.mllp import ( + DEFAULT_MAX_MESSAGES_PER_SECOND, + MLLPSource, + _MessagePacer, + frame, +) + +_ADT = "MSH|^~\\&|A|B|C|D|202601011200||ADT^A01|{mid}|P|2.5\rPID|1||MRN1||DOE^JOHN\r" + + +def _source(**settings: object) -> MLLPSource: + return MLLPSource( + Source(name="IB_TEST", type=ConnectorType.MLLP, settings={"port": 0, **settings}) + ) + + +# --- the pacer itself, pure and clock-injected ------------------------------------------------- + + +def test_a_pacer_under_budget_asks_for_no_wait() -> None: + pacer = _MessagePacer(10.0, 10.0, now=0.0) + assert pacer.charge(5, now=0.0) == 0.0 + + +def test_a_pacer_over_budget_asks_for_exactly_the_deficit() -> None: + """The wait is the time for the bucket to return to non-negative -- so it is bounded by + messages/rate and cannot grow without limit, which is what stops a pacer becoming a stall.""" + pacer = _MessagePacer(10.0, 10.0, now=0.0) + assert pacer.charge(10, now=0.0) == 0.0 # burst absorbed + # 5 more with an empty bucket at 10/s -> 0.5s of debt. + assert pacer.charge(5, now=0.0) == pytest.approx(0.5) + + +def test_tokens_refill_with_elapsed_time_and_never_exceed_burst() -> None: + pacer = _MessagePacer(10.0, 10.0, now=0.0) + pacer.charge(10, now=0.0) + # A full second later the bucket has refilled to its cap, not beyond it. + assert pacer.charge(10, now=1.0) == 0.0 + assert pacer.charge(1, now=100.0) == 0.0 # long idle does not bank unlimited credit + assert pacer.charge(10, now=100.0) == pytest.approx(0.1) + + +def test_burst_floor_is_one_so_a_pacer_can_always_make_progress() -> None: + """A zero/negative burst would leave the bucket permanently empty and stall the connection.""" + pacer = _MessagePacer(1.0, 0.0, now=0.0) + assert pacer.charge(1, now=0.0) == 0.0 + + +# --- the shipped default ------------------------------------------------------------------------ + + +def test_pacing_ships_off() -> None: + """Ruled 2026-08-11: absent -> OFF, deliberately against this module's usual + "key absent -> secure default" convention, because a guessed rate on a clinical interface + throttles real traffic. Pinned so the deviation cannot be 'tidied' into the convention.""" + assert DEFAULT_MAX_MESSAGES_PER_SECOND is None + assert _source().max_messages_per_second is None + + +def test_burst_defaults_to_one_seconds_worth() -> None: + src = _source(max_messages_per_second=25) + assert src.max_messages_per_second == 25.0 + assert src.message_burst == 25.0 + + +def test_an_explicit_burst_is_honoured() -> None: + assert _source(max_messages_per_second=25, message_burst=100).message_burst == 100.0 + + +# --- end to end, on a real socket --------------------------------------------------------------- + + +async def _run_against(src: MLLPSource, count: int) -> list[str]: + """Send ``count`` framed messages down ONE connection and return what the handler received.""" + seen: list[str] = [] + + async def handler(message: str) -> str | None: + seen.append(message) + return "MSA|AA|x" + + await src.start(handler) + assert src._server is not None + port = src._server.sockets[0].getsockname()[1] + reader, writer = await asyncio.open_connection("127.0.0.1", port) + try: + for i in range(count): + writer.write(frame(_ADT.format(mid=i), "utf-8")) + await writer.drain() + # Read one ACK per message: the sender is told AA for every one, paced or not. + for _ in range(count): + await asyncio.wait_for(reader.readuntil(b"\x1c\r"), timeout=10.0) + finally: + writer.close() + await asyncio.gather(writer.wait_closed(), return_exceptions=True) + await src.stop() + return seen + + +async def test_pacing_never_drops_a_message() -> None: + """THE test for this control. A paced sender is SLOWED, never truncated. + + Rate 20/s with burst 2 against 12 messages guarantees the pacer engages several times. Every + message must still reach the handler and every one must still be ACKed -- accept-and-drop is + what the count-and-log invariant forbids, and a limiter that discarded would pass a + 'rate is bounded' test while breaking the thing that actually matters. + """ + seen = await _run_against(_source(max_messages_per_second=20, message_burst=2), 12) + assert len(seen) == 12 + # And in order: pacing must not reorder either, since FIFO is the project's ordering model. + ids = [(m.decode() if isinstance(m, bytes) else m).split("|")[9] for m in seen] + assert ids == [str(i) for i in range(12)] + + +async def test_pacing_off_delivers_everything_unchanged() -> None: + seen = await _run_against(_source(), 12) + assert len(seen) == 12 + + +async def test_pacing_actually_delays_the_reads() -> None: + """Watched fail: with the pacer removed this elapsed time collapses to near zero. + + Deliberately a LOWER bound only. Asserting an upper bound would pin scheduler timing and make + this the flaky test that gets deleted; the claim under test is that a wait occurs at all. + """ + loop = asyncio.get_running_loop() + start = loop.time() + seen = await _run_against(_source(max_messages_per_second=20, message_burst=2), 12) + elapsed = loop.time() - start + assert len(seen) == 12 + # 12 messages, burst 2, 20/s -> at least (12-2)/20 = 0.5s of debt must be paid somewhere. + assert elapsed >= 0.3 From b3b278e681869eb82264445babf04732cb121442 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 11 Aug 2026 10:05:41 -0500 Subject: [PATCH 9/9] docs(security): the ingest-plane row said no rate limit exists -- my own build falsified it The MLLP pacing control (e0e979d5) made three things false at once, and all three had to move together or the record contradicts the code: 1. docs/SECURITY.md's ingest-plane row asserted "no message-rate or volume limit exists ... and nothing in transports/, config/ or pipeline/ exposes a messages-per-second control". transports/mllp.py now exposes exactly that. 2. tests/test_security_doc_rate_limits.py PINNED that sentence, so the guard was enforcing a stale fact. It is updated rather than deleted -- the row must now state BOTH that a control exists and that it ships OFF, because either half alone misleads: "exists" implies the shipped default is bounded, and "none" is simply false. 3. The ASVS 2.4.1 cell's absence claim asserted MLLPSource carries no rate limiting. Re-scored vault-side in the same act. The Scope column reads in-process rather than a new token, and that is the accurate word: one bucket per connection, coordinating across neither engine shards nor peers. The row keeps naming the honest gaps -- an off default bounds nothing, and the raw-TCP inbound never got this control. This is the shape the ASVS programme keeps finding in other people's work, so it is worth being plain that it was mine this time: I shipped a control and left three artifacts asserting it did not exist. 238 passed across the four suites that parse this document. --- docs/SECURITY.md | 2 +- tests/test_security_doc_rate_limits.py | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index b26bfce1..361944f9 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -1592,7 +1592,7 @@ multi-host deployment must additionally front the API with a proxy/WAF limiter a | Request body | `[store].max_upload_bytes` (the `/uploads` routes only) | 1 MiB elsewhere | per request | no | no | no | **stateless** — every route, in ASGI middleware | **413** over the cap, **400** on ambiguous CL+TE framing or an invalid `Content-Length`, **411** on a chunked body | | OIDC pending flows | `[auth].oidc_flow_cache_max` (global), `DEFAULT_PER_IP_CAP` (per-IP, no knob), `oidc_flow_ttl_seconds` | 512 / 16 / 300 s | 300 s TTL | no | **yes** (512) | **yes** (16) | **in-process** — `GET /ui/oidc/start` — reject-when-full, never evict | 303 → `/ui/login?e=rate_limited`, WARNING-logged, **never** audited | | WebAuthn pending ceremonies | `GLOBAL_PENDING_CAP`, `PER_USER_PENDING_CAP`, `CHALLENGE_TTL_SECONDS` (module constants, no knobs) | 4096 / 16 / 120 s | 120 s TTL | **yes** (16) | **yes** (4096) | no | **in-process** — every passkey registration + assertion ceremony | per-user: evicts that user's **own** oldest pending ceremony (silent); global: `ChallengeCacheFullError` naming the cause + the `admin_reset_mfa` recovery path | -| **Ingest plane** | *(none)* | — | — | — | — | — | **n/a** — inbound connections | **no message-rate or volume limit exists.** Inbound carries resource caps only — `max_connections` (256), `receive_timeout` (60.0 s), `max_frame_bytes` (16 MiB), per-connection `max_message_bytes`, `source_ip_allowlist` — and nothing in `transports/`, `config/` or `pipeline/` exposes a messages-per-second control | +| **Ingest plane** | `max_messages_per_second`, `message_burst` (MLLP inbound) | **off** | per message | no | no | no | **in-process** — one bucket per MLLP connection, so it neither coordinates across engine shards nor aggregates per peer | **exists but ships OFF, so unset there is still no volume bound.** When set, the listener **pauses reading** over budget so TCP back-pressures the sender: no message is dropped, refused, NAK'd or reordered (the count-and-log invariant forbids accept-and-drop, so a discarding limiter was never available). Bounded by the bucket deficit. The off default is **ruled, not accidental** — a rate on a clinical interface is only safe at a number from a real feed profile. **Not covered:** the raw-TCP inbound, and any per-peer bound (MLLP peers are unauthenticated, so the only key would be source IP, which NAT collapses). Other inbound caps are resource-only — `max_connections` (256), `receive_timeout` (60.0 s), `max_frame_bytes` (16 MiB), per-connection `max_message_bytes`, `source_ip_allowlist` | **What these limits defend, and what they do not.** The full inventory of resource-demanding functionality — including the surfaces that remain **unbounded** at this release — is diff --git a/tests/test_security_doc_rate_limits.py b/tests/test_security_doc_rate_limits.py index 1d5758a9..a3fddf70 100644 --- a/tests/test_security_doc_rate_limits.py +++ b/tests/test_security_doc_rate_limits.py @@ -530,11 +530,24 @@ def test_scope_guard_detects_a_planted_rescoping() -> None: ) -def test_ingest_plane_is_documented_as_having_no_rate_limit() -> None: - """A silent omission reads as coverage. The data plane has resource caps only.""" +def test_ingest_plane_rate_limit_is_documented_as_existing_but_off() -> None: + """A silent omission reads as coverage, and so does an overstatement in the other direction. + + Until 2026-08-11 this guard asserted the doc said "no message-rate or volume limit exists", which + was true and load-bearing. The MLLP pacing build (ASVS 2.4.1 / 15.2.2) falsified it, so the guard + had to move with the code and the doc rather than be deleted -- the row must now state BOTH that + a control exists and that it ships OFF, because either half alone misleads: "exists" implies the + shipped default is bounded, and "none" is now simply false. + """ block = _section(_H_LIMITS) assert "Ingest plane" in block - assert "no message-rate or volume limit exists" in block + assert "max_messages_per_second" in block, ( + "the ingest row must name the control that now exists" + ) + assert "ships OFF" in block, "and must say it is not on by default, or 'exists' overstates it" + # The honest gaps stay named: an off default bounds nothing, and the raw-TCP intake never got it. + assert "still no volume bound" in block + assert "raw-TCP inbound" in block for cap in ("max_connections", "receive_timeout", "max_frame_bytes", "max_message_bytes"): assert cap in block, f"the ingest row must name the {cap} resource cap it DOES have"