diff --git a/docs/adr/0021-inbound-ack-nak-capture-response-sent.md b/docs/adr/0021-inbound-ack-nak-capture-response-sent.md index 22366f70..dbce8bf7 100644 --- a/docs/adr/0021-inbound-ack-nak-capture-response-sent.md +++ b/docs/adr/0021-inbound-ack-nak-capture-response-sent.md @@ -81,7 +81,7 @@ CREATE TABLE IF NOT EXISTS connection_event ( transport TEXT NOT NULL, -- 'mllp' (ConnectorType.value) — which listener kind TEXT NOT NULL, -- bounded enum: see §7.3 peer_host TEXT, -- peer IP from getsockname/peername (socket metadata; nullable) - reason TEXT -- safe_text-scrubbed, bounded; plaintext metadata, not PHI + reason TEXT -- safe_text-scrubbed, bounded; metadata, not PHI (encrypted at rest) ); CREATE INDEX IF NOT EXISTS ix_connection_event_conn ON connection_event(connection, ts); ``` @@ -125,7 +125,7 @@ A first-class field on `InboundConnection` ([wiring.py:2483](../../messagefoundr #### §7.5 PHI: encryption-at-rest, retention-purge, audit -- **Encryption at rest.** `reason` is the only column that could in theory carry residual free text. On SQLite/Postgres, add `('connection_event', 'reason')` to `_CIPHER_COLUMNS` ([store.py:838-850](../../messagefoundry/store/store.py)) so it rides the existing `_encrypt_existing_rows`/`reencrypt_to_active` id-keyed loops (the table has an `id` PK, so unlike `response` it needs **no** bespoke cipher pass). **SQL Server: flagged plaintext.** Just like `message_events.detail`, `messages.error`, and `queue.last_error`, this backend keeps the detail column plaintext at rest ([sqlserver.py:196-202](../../messagefoundry/store/sqlserver.py)); `connection_event.reason` joins that documented set. This is acceptable because `reason` is metadata-only and `safe_text`-scrubbed at the chokepoint — the **same** control the existing plaintext SQL-Server detail columns rely on — and the at-rest encryption of that class on SQL Server is the same tracked defense-in-depth follow-up (docs/PHI.md). The other four columns (`connection`/`transport`/`kind`/`peer_host`) are non-PHI metadata and stay plaintext on every backend by design. +- **Encryption at rest.** `reason` is the only column that could in theory carry residual free text. On SQLite/Postgres, add `('connection_event', 'reason')` to `_CIPHER_COLUMNS` ([store.py:838-850](../../messagefoundry/store/store.py)) so it rides the existing `_encrypt_existing_rows`/`reencrypt_to_active` id-keyed loops (the table has an `id` PK, so unlike `response` it needs **no** bespoke cipher pass). **SQL Server: this section is SUPERSEDED and the direction it gave was not followed.** It said to flag `connection_event.reason` as plaintext at rest on SQL Server, joining the `message_events.detail` / `messages.error` / `queue.last_error` set. **That is no longer true and the direction should not be carried forward:** the plaintext residual was retired and `reason` is encrypted on SQL Server too, on a composite-AAD pass rather than the id-keyed `_CIPHER_COLUMNS` loop — the table's `IDENTITY` id is unknown at INSERT, so the cell AAD binds insert-time-known natural columns instead. The store's own code comments name this section as stale. **Derive the current at-rest treatment from the backend rather than from this ADR**, and note that a search for `_CIPHER_COLUMNS` alone will not find it: that is a different mechanism from the one SQL Server uses here. The other four columns (`connection`/`transport`/`kind`/`peer_host`) are non-PHI metadata and stay plaintext on every backend by design. - **Retention purge.** Two options, and the ADR picks **age-based delete** over body-null: unlike `messages`/`response`, a `connection_event` row has **no body to null and no FK to keep** (it does not reference `messages`), so it is purged by a plain `DELETE FROM connection_event WHERE ts < :older_than` added inside `purge_message_bodies`' existing transaction ([store.py:3515](../../messagefoundry/store/store.py) + PG/SQL-Server twins), keyed on the same `older_than` cutoff. (A body-null UPDATE would leave an ever-growing metadata table; a delete bounds it.) The row is *purged*, not retained-with-nulled-body, because none of its surviving columns are an audit obligation tied to a `messages` row. These rows are purged by **event age against the same retention cutoff as message bodies** — an intentional reuse of the body-retention window for a metadata clock (not a per-message-eligibility delete), mirroring the existing non-message-keyed `state` purge already run in that transaction on SQL Server ([sqlserver.py:1590](../../messagefoundry/store/sqlserver.py)); connection-error rows are not expected to outlive message bodies. - **Audit / read surface.** Exposure is **deferred with its consumer** (same posture as §5's deferred fleet ack analytics and ADR 0013's read-surface gating): when a console "Connection Errors" view is built, it reads through a new engine API route gated `require_phi_read(Permission.MESSAGES_READ)` and emitting an audit event, reusing the field-authz/`record_view` machinery. v1 ships the **capture + store + purge** only (engine-side); no new endpoint or permission lands until the view does. This keeps §7 strictly engine-side (no `api/`/`console/` import, CLAUDE.md §4). @@ -148,7 +148,7 @@ A burst of `peer_not_allowlisted` / `framing_error` is operationally interesting - **Capture is single-shot at receipt, never replayed.** `_handle_inbound` runs exactly once per physical receipt; `reset_stale_inflight` recovers routed/outbound rows but never re-invokes the listener path. This is correct (the ACK is single-shot on the wire), but it means there is no re-run path that re-derives the ack row — a swallowed store-write failure is the only loss window. (ADR 0013's "on a re-run the reply re-derives" framing does NOT apply here; struck.) - The seq/PK rule must be keyed on `kind` and ack rows use the `\x1fack:` sentinel `destination_name`, or an inbound name colliding with an outbound destination name could corrupt per-destination authoritative-reply ordering — a deliberate, tested guard. - Captures the ACK we **GENERATED**, NOT a wire-confirmed drain (the socket write is at [mllp.py:605](../../messagefoundry/transports/mllp.py) after the handler returns) — matches ADR 0013's "what we produced" semantics; a reconciliation user must know captured ≠ delivered-on-wire. -- **§7** adds a second optional `SourceConnector.start` callback (`on_connection_event`) + a new store method + a `connection_event` table on all three backends; on SQL Server its `reason` column is **plaintext at rest** (joining the documented `message_events.detail`/`messages.error` set) — acceptable because it is `safe_text`-scrubbed metadata, never a body or field value. +- **§7** adds a second optional `SourceConnector.start` callback (`on_connection_event`) + a new store method + a `connection_event` table on all three backends; on SQL Server its `reason` column was originally specified as **plaintext at rest**, joining the documented `message_events.detail`/`messages.error` set. **That was superseded before it shipped: `reason` is encrypted at rest on SQL Server as well** (see §7.5, which records the correction). It remains `safe_text`-scrubbed metadata, never a body or field value. ## Alternatives considered diff --git a/docs/adr/0030-anonymization-test-harness-tee.md b/docs/adr/0030-anonymization-test-harness-tee.md index 013dafee..3b98f912 100644 --- a/docs/adr/0030-anonymization-test-harness-tee.md +++ b/docs/adr/0030-anonymization-test-harness-tee.md @@ -219,15 +219,25 @@ original→surrogate pair is ever emitted together with `dataset_key`. **`dataset_key` is a per-run secret salt.** It is drawn from `secrets.token_bytes`/`os.urandom` with **≥128 bits** of entropy, held **only in process memory**, classified **PHI-equivalent** (it is itself a -re-identification key for the seeded PRNG), **never written / logged / committed**, and **discarded at run -end**. Keying is **one-way** (seeded PRNG, no inverse), so no surrogate can be inverted to its original. - -**Irreversibility is salt-dependent, not cryptographic.** `random.Random` seeded from a string is **not** a -keyed hash; the brute-force resistance here rests entirely on the salt never being persisted/logged and on -**no `(original, surrogate)` pair ever leaking alongside `dataset_key`** — within a run, with a small -surrogate pool, reversal is feasible if the salt or a plaintext pair leaks. If true cryptographic -brute-force resistance is later required, switch the seed derivation to an HMAC/BLAKE2 keyed hash; that is a -*To resolve on acceptance* question, called out below. +re-identification key for the keying construction), **never written / logged / committed**, and +**discarded at run end**. Keying is **one-way** (a keyed hash, no inverse), so no surrogate can be +inverted to its original. *(This ADR was written against a seeded-PRNG construction; the shipped one is +a keyed BLAKE2b -- see the correction below and `messagefoundry/anon/keying.py`.)* + +**Irreversibility was salt-dependent rather than cryptographic WHEN THIS ADR WAS WRITTEN. THAT IS NO +LONGER THE IMPLEMENTATION.** The paragraph below described a `random.Random` seed derived from a +string, which is not a keyed hash, so brute-force resistance rested entirely on the salt never being +persisted or logged and on **no `(original, surrogate)` pair ever leaking alongside `dataset_key`** -- +within a run, with a small surrogate pool, reversal was feasible if the salt or a plaintext pair +leaked. It closed by saying that if true cryptographic brute-force resistance were later required, the +seed derivation should switch to an HMAC or BLAKE2 keyed hash. + +**That switch was made.** The shipped keying is a **keyed BLAKE2b** under the per-dataset salt +(`messagefoundry/anon/keying.py`), so the "not a keyed hash" premise no longer holds and the *To +resolve on acceptance* question it raised is answered. **Read the construction from `anon/keying.py` +rather than from this paragraph.** The operational rules it states are unchanged and still binding: +the salt is never persisted or logged, and an `(original, surrogate)` pair must never leak alongside +`dataset_key`. **Reproducibility model.** The per-run random salt means the **same real message anonymized twice yields a *different* fixture** — good for one-shot exports, but it makes a regenerated "committable anonymized diff --git a/docs/adr/0047-cloud-kubernetes-ha-deployment-packaging.md b/docs/adr/0047-cloud-kubernetes-ha-deployment-packaging.md index 27e11d5e..8919f9b0 100644 --- a/docs/adr/0047-cloud-kubernetes-ha-deployment-packaging.md +++ b/docs/adr/0047-cloud-kubernetes-ha-deployment-packaging.md @@ -171,11 +171,15 @@ The six deliverables: halves of each block: drop *"those listeners have NO startup transport guard"* **and** correct the contrasting clause *"MLLP and the DICOM C-STORE SCP are guarded"* to the **complete set** (MLLP, DICOM C-STORE SCP, HTTP, **raw-TCP, and X12** are all exposed-gated — a non-loopback bind without TLS is - refused at start), plus fix the same gap in the research note. **Off-box log forwarding is a separate - open item, not flipped on here:** the built `[logging]` syslog forwarder is **plaintext** (UDP/TCP only; - `SyslogProtocol` has **no TLS variant** — `config/settings.py`), so enabling it from a cloud/ephemeral-pod - posture would put PHI-adjacent log metadata on the wire in cleartext. Any prod-HA enablement is gated on - pairing it with a TLS-forwarding sidecar / TLS collector (tracked in *To resolve*). Note for operators: + refused at start), plus fix the same gap in the research note. **Off-box log forwarding was a separate + open item, not flipped on here. THAT IS NO LONGER THE POSTURE and the text below is superseded:** + it said the built `[logging]` syslog forwarder was **plaintext** with `SyslogProtocol` carrying **no + TLS variant**, so a cloud/ephemeral-pod enablement would put PHI-adjacent log metadata on the wire in + cleartext, and gated any prod-HA enablement on pairing it with a TLS-forwarding sidecar or TLS + collector. **A native TLS transport has since shipped** (ADR 0080), so the sidecar is no longer the + only path. **Derive the available transports from `SyslogProtocol` in `config/settings.py` rather + than from this ADR** -- an enumeration written here is one that goes stale, which is exactly what + happened to this paragraph. Note for operators: **API TLS cert rotation needs a pod restart** (uvicorn builds the TLS context once; only MLLP certs hot-reload on `/config/reload`) — fold it into the rolling-renewal runbook. @@ -269,8 +273,8 @@ mid-run) — verify current NextGen Helm/Docker specifics before leaning on the failover** (the passive primary-only-health-check LB is the chosen mechanism; an engine that manipulates a VIP is a separate reserved item); active-active / a second concurrent writer (#396, deleted); an inbound **DICOMweb / HTTP web-service receiver** as a public cloud ingress (a distinct not-yet-built auth/TLS -surface, ADR 0023 territory); **TLS off-box log forwarding** (the built forwarder is plaintext — a TLS -syslog variant or a TLS sidecar/collector is a separate item); DB-tier write-scaling (L5 / ADR 0039) and +surface, ADR 0023 territory); **TLS off-box log forwarding** (out of scope for this ADR; +the native TLS syslog transport has since shipped under ADR 0080); DB-tier write-scaling (L5 / ADR 0039) and loss-of-site DR (ADR 0048) — those are separate levers this ADR only references. This ADR ships **no new engine reliability code**; the raw-TCP/X12 guard is ratified-as-built, not re-implemented. @@ -300,9 +304,10 @@ engine reliability code**; the raw-TCP/X12 guard is ratified-as-built, not re-im encryption/segmentation/inventory floor — confirm we publish against the **current** Security Rule and flag the NPRM as anticipated-not-final (it was unfinalized as of 2026-06), so the doc doesn't assert an unenacted requirement. -- [ ] **Off-box log forwarding hop:** the built `[logging]` syslog forward is **plaintext** today - (`SyslogProtocol` is UDP/TCP only — no TLS variant). Decide the prod-posture HA path: pair it with a - TLS-forwarding sidecar / TLS collector before any enablement, **or** keep off-box forwarding out of the +- [x] **Off-box log forwarding hop: RESOLVED by ADR 0080, which shipped a native TLS syslog + transport.** This item was written when the built `[logging]` forward had no TLS option and the only + answers were a sidecar or omission. It read: decide the prod-posture HA path, pair it with a + TLS-forwarding sidecar or TLS collector before any enablement, **or** keep off-box forwarding out of the cloud manifest entirely — but do **not** instruct operators to "flip it on" as-is (cleartext PHI-adjacent metadata on the wire). - [ ] **Manifest-lint CI leg:** confirm the new `kubeconform`/policy-lint job (the verifier named by @@ -321,5 +326,5 @@ Owner delegated the open items ("you sort it out / do what is best"); resolved a - **`terminationGracePeriodSeconds` ≥ `leader_lease_ttl` + serial-drain + margin** (reconcile with the existing single-node `40`). - **Edge-relay = the same engine image** (reuse the outbound MLLP/TCP connectors) — **no new code**. - **Cloud PHI doc** is published against the **current** HIPAA Security Rule; the 2025 NPRM floor is flagged as anticipated-not-final. -- **Off-box syslog stays OUT of the cloud manifest as-is** — `[logging]` syslog forwarding is plaintext; pair it with a TLS sidecar/collector before any enablement. Operators are **not** told to "flip it on" (no cleartext PHI-adjacent metadata on the wire). +- **Off-box syslog stayed OUT of the cloud manifest as-is** — at the time `[logging]` syslog forwarding had no TLS transport, so the decision was to pair it with a TLS sidecar/collector before any enablement and never tell operators to "flip it on" (no cleartext PHI-adjacent metadata on the wire). **A native TLS syslog transport has since shipped under ADR 0080**, so the premise for the sidecar-only answer no longer holds; the manifest decision itself is unrevisited here. - **A kubeconform / policy-lint CI leg ships with the build lane** (the verifier AC-4/AC-5 name). diff --git a/docs/adr/0105-streaming-very-large-hl7-attachments-detach-the-opaque-document-from-the-transformable-skeleton.md b/docs/adr/0105-streaming-very-large-hl7-attachments-detach-the-opaque-document-from-the-transformable-skeleton.md index 09b1b3fb..5dd49197 100644 --- a/docs/adr/0105-streaming-very-large-hl7-attachments-detach-the-opaque-document-from-the-transformable-skeleton.md +++ b/docs/adr/0105-streaming-very-large-hl7-attachments-detach-the-opaque-document-from-the-transformable-skeleton.md @@ -279,19 +279,23 @@ and (B) a Handler that picks up a PDF, base64-encodes it, and builds a large MDM #### Known gap carried to Phase 3 — retention/purge does NOT yet decref an attachment — **CLOSED by Phase 3a (2026-07-13)** -`purge_message_bodies` (and `strip_embedded_documents`) null a message's `raw` body — which holds the -skeleton + its `mfdoc:v1:ref:` handle — but do **not** decref the referenced attachment, and the -`messages` table does **not** persist a message's `attachment_refs` (the ingress incref happens in -`enqueue_ingress` but no column records which refs a message holds for a later release). **Consequence:** -when a detached message's body is purged, its attachment's refcount is never decremented, so the -attachment + its chunks are **retained past their last referrer** (a PHI-at-rest over-retention, not a -loss — the ADR "refcount over-count keeps PHI past its last referrer" hazard). This is **not** a -Phase-1b delivery concern (delivery must never decref — see above); it belongs to **Phase 3** (the -read-surface + retention migration), which must persist per-message attachment refs and decref them in the -same transaction as the body purge (mirroring `_release_outbound_body_refs` for `shared_body`). Flagged -here rather than left as a silent leak. Until Phase 3 lands, an operator reclaiming a streaming feed's -storage relies on the startup `sweep_orphan_attachments` (refcount-0 only) — a purged-but-still-referenced -attachment is **not** yet reclaimed. +**This section describes the gap as it stood BEFORE Phase 3a, and is kept for the record. Every +sentence below is past tense deliberately: the heading above and the line below both state that +Phase 3a closed it, and a present-tense body under a closed heading reads as a live defect.** + +`purge_message_bodies` (and `strip_embedded_documents`) nulled a message's `raw` body — which holds the +skeleton + its `mfdoc:v1:ref:` handle — but did **not** decref the referenced attachment, and the +`messages` table did **not** persist a message's `attachment_refs` (the ingress incref happened in +`enqueue_ingress` but no column recorded which refs a message held for a later release). **Consequence +at the time:** when a detached message's body was purged, its attachment's refcount was never +decremented, so the attachment + its chunks were **retained past their last referrer** (a PHI-at-rest +over-retention, not a loss — the ADR "refcount over-count keeps PHI past its last referrer" hazard). +That was **not** a Phase-1b delivery concern (delivery must never decref — see above); it belonged to +**Phase 3** (the read-surface + retention migration), which had to persist per-message attachment refs +and decref them in the same transaction as the body purge (mirroring `_release_outbound_body_refs` for +`shared_body`). Flagged at the time rather than left as a silent leak. Until Phase 3a landed, an +operator reclaiming a streaming feed's storage relied on the startup `sweep_orphan_attachments` +(refcount-0 only) — a purged-but-still-referenced attachment was **not** reclaimed. **→ This gap is CLOSED by Phase 3a (below):** retention now persists the linkage and decrefs on purge. diff --git a/docs/adr/0111-not-deployed-connections.md b/docs/adr/0111-not-deployed-connections.md index ebd09c8a..b32ad0b2 100644 --- a/docs/adr/0111-not-deployed-connections.md +++ b/docs/adr/0111-not-deployed-connections.md @@ -260,12 +260,18 @@ SQLite/Postgres/SQL Server with no `CHECK` constraint anywhere. No migration rev Steps-palette work in the same tree. The flag is fully functional without them: hand-edited `connections.toml` and code-first `deployed=False` both work, and the engine, CLI, API and web console all honor it. The GUI simply will not *offer* the checkbox yet. -- **BACKLOG #234 — `connections.toml` GUI save silently strips per-connection fields.** Uncovered while - adding `deployed` to the TOML **write** schema: `config/connections_edit.py:31-44` `_SCALAR_FIELDS` is a - **12-key whitelist with no passthrough**, so any GUI/CLI `connection upsert` silently drops `priority`, - `schedule`, `shard`, `metadata`, `batch`, `stall` and `dead_letter_days` from an existing connection's - table. That is a **pre-existing data-loss bug**, filed as **#234** and deliberately **not** fixed here — - this change only adds `deployed` (and `auto_start`) to the whitelist. Fixing the whitelist properly is a - passthrough-preservation change with its own blast radius. +- **BACKLOG #234 — `connections.toml` GUI save silently stripped per-connection fields. CLOSED; the + description below is kept as the record of what was true when this ADR was written.** Uncovered while + adding `deployed` to the TOML **write** schema, `_SCALAR_FIELDS` in `config/connections_edit.py` was + then a short whitelist with no passthrough, so a GUI/CLI `connection upsert` dropped fields it did not + name from an existing connection's table. That was a **pre-existing data-loss bug**, filed as **#234** + and deliberately **not** fixed by this ADR, which only added `deployed` and `auto_start` to the + whitelist. + + **#234 has since landed, and both halves of the old description are now wrong** — the count and the + mechanism. The whitelist is no longer short, and an unrecognised key is no longer dropped: it is + **rejected fail-loud with a `WiringError`**, mirroring the loader's `_reject_unknown` message shape. + Derive the current field set from `_SCALAR_FIELDS` in `config/connections_edit.py` rather than trusting + any count written here — a number in this ADR is a number that goes stale. - `ack_after=delivered`, per-connection scheduling of a *go-live date*, and any notion of a "planned" deployment window. `deployed` is a boolean; a date is a different feature.