diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 55cc19edd..811c58530 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -10130,6 +10130,18 @@ Proof: four mutations, each red, zero vacuous -- per-character floor disabled, v **Still not an honest pass:** re-scoring on the writer evidence while leaving the read passthrough. That is more tempting now than when this item was filed, because the writer evidence got better -- `[store].aad_bind` ships true (`config/settings.py:410`, threaded at `store/base.py:1879`) and relocation detection was measured refusing, so three of four limbs read verified with clean measurements. The downgrade limb is not a weaker version of the substitution limb: substitution is a VERIFICATION with a tag to check, downgrade has no tag and only a REFUSAL can protect it. The second trap is nearly reached by anyone who measures the migration: arguing the passthrough is unreachable by any legitimate value and therefore harmless. It is false on the server backends for the attachment table, false on the uploads surface entirely, and grades the wrong thing even where true -- "no legitimate value has this shape" is an argument about inputs, not a control. Proposed work, unallocated and by subject: the strict-ciphertext read at both cipher implementations, defaulting on and registered in `security_loosenings()`; server-backend migration parity for the attachment table plus a mechanical reconciliation so a future covered table cannot ship without a pass; uploaded-file store crypto parity with a distinct logged and alerted refusal event; a review of the queue-claim `CipherError` containment sites, where a stripped payload would be absorbed as an ordinary undecryptable row at warning level; the DIRECT integrity option, costed and interop-measured; the re-put ruling packet; and a re-verification that re-anchors the stale evidence, takes delivery of the enveloped surface, adds the SSH transit hop, and sets the reviewer field this record lacks. +**PRECONDITIONS BUILT 2026-09-04. The refusal is still NOT built and still needs the re-put owner ruling — this item stays open.** Everything below was re-measured by execution against `a2eef0f37` before any code was written; every line number in the 2026-08-20 research had moved. The core claim survives verbatim: `crypto.py:756-757` returned a forged plaintext unchanged from a keyed cipher, with both controls firing in the same run (a mutated ciphertext and a wrong-cell AAD each raised `CipherError`). On the uploads surface a hand-written plaintext sidecar was accepted by a KEYED store and returned filename, uploader and uploader id all attacker-chosen, while a relocated SEALED sidecar raised — the positive control separating the passthrough from a broken test. + +**A SECOND precondition as reported is FALSE, and this one is the item's own.** `attachment_chunk` was reported "unmigrated on all three backends" on the strength of its three `INSERT INTO` sites. Those sites answer *where it is written*, not *where it is sealed* — an SDS-3.8 instrument mismatch. Measured: **SQLite has had an on-open pass all along** (`store/store.py:2765-2798`), and **rotation covers the table on all three** backends. The real gap was one pass on two backends: `_encrypt_existing_rows` omitted it on `postgres.py` and `sqlserver.py` only. Both now have it, with a batch of 16 rather than the sibling passes' 500 because one row is a whole 1 MiB `DETACH_CHUNK_BYTES` slice and 500 would hold ~650 MiB resident in one transaction at store open. The `shared_body` correction **holds** and needed no new evidence: `tests/test_phi_at_rest_inventory.py` already pinned it, with the same instrument and the same SQLite-only reasoning, *before this item was filed* — so the record contained its own refutation. Not restated in the new guard (SDS-3.5). + +**Precondition 2 was worse than reported and is a standing data-loss defect, not only a blocker.** The uploaded-file store had no migration pass *and* no rotation pass. Measured: after a rotation the on-disk blob was byte-identical, and once the prior key was dropped — the step `rotate-key`'s own docstring tells the operator to take — the upload raised `CipherError` permanently. `UploadStore.reseal_to_active()` now closes both transitions and `rotate-key` calls it on the store's live cipher instance (ASVS 11.3.4). **One asymmetry is deliberate and named rather than papered over:** the store seals legacy plaintext automatically at every keyed open, whereas this pass has one caller, so a first key-enable is closed only when an operator runs the command. A whole-directory crypto sweep at API startup is unbounded boot-time work and a separate decision. + +**Precondition 3 closed.** `_scan_metas_sync`'s blanket `except Exception` folded a cipher refusal, a damaged file and a malformed sidecar into one identical warning — measured, three causes, three identical lines. Each class is now caught and logged separately, so the refusal this item contemplates would be visible on the surface where planting is easiest instead of reading like a routine post-rotation skip. + +**A mechanical reconciliation ships with it** (`tests/test_store_cipher_sweep_parity.py`): every `cell_aad` cell whose table a backend INSERTs into must be reached by BOTH sweeps, so a future covered table cannot ship half-added the way this one did. The `INSERT INTO` scoping is load-bearing — without it the check falsely accuses both server backends over `shared_body`. **Recorded because it nearly shipped broken:** the first version matched raw source text and passed the real Postgres omission, because a comment above the missing call said the word `attachment_chunk`. Prose satisfied a check about code. It now matches only executed string constants, and both backend mutations were measured red before the fix and green after. + +**Named, measured and NOT done.** The refusal, pending the ruling. The three store `_reencrypt_value` staticmethods and the uploads one are four spellings of a single seam; the leaf-module rule blocks importing the store copies, so the new one carries the same name to stay greppable. Each backend still hand-maintains its own list of composite passes — roughly 26 sites, and SQL Server has no `_CIPHER_COLUMNS` at all, so `postgres.py:856`'s "mirrors `MessageStore._CIPHER_COLUMNS`" is untrue of one backend. Driving all passes off one declaration would retire this guard and ~1,300 lines of sibling AST scaffolding, but it is ~1,000 lines in the one path **no CI leg executes** (every SQL Server and Postgres leg is keyless and returns before the sweep body), so it is a separate item. `UploadStore` is also constructed from settings in three places with a repeated `uploads_dir` gate. + ## 1170. research an honest pass for ASVS 11.3.5 -- asserting encrypt-then-MAC on hops that deliberately keep CBC-SHA2 for hospital peers > 🔢 **Re-scored 2026-08-20 -> P2.** Value **5/10** · Difficulty **3/10** · _fill-in_. The at-rest half is AEAD-only (store/crypto.py:109-111 registers AES-256-GCM alone) and the transport half is unchanged, with the deliberate six-CBC-SHA2 retention recorded at tls_policy.py:375-376 and no encrypt_then_mac reference anywhere under messagefoundry/, so nothing asserts, logs or refuses on the RFC 7366 state. Value 5 because a deploying site has a clean workaround in the shipped tls_ciphers setting (settings.py:766, forward-secrecy-validated at :935-940), leaving the default's breadth rather than an unfixable property; difficulty 3 because the item's named concrete unknown is now answered -- the stdlib exposes no negotiation state at all on this interpreter -- so the remainder is a documented finding plus the V11.3-versus-V12 scope ruling. _(was 4/10 · 7/10.)_ diff --git a/messagefoundry/__main__.py b/messagefoundry/__main__.py index a19cb1e81..f8c2711e6 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -4464,6 +4464,7 @@ def _rotate_key(args: argparse.Namespace) -> int: from messagefoundry.store.base import open_store, resolve_active_key from messagefoundry.store.crypto import CipherError from messagefoundry.store.keyprovider import KeyProviderError + from messagefoundry.uploads import ResealResult, UploadStore cli: dict[str, dict[str, object]] = {} if args.db is not None: @@ -4495,7 +4496,7 @@ def _rotate_key(args: argparse.Namespace) -> int: ) return 2 - async def run() -> int: + async def run() -> tuple[int, ResealResult]: import datetime from messagefoundry.store.store import SecretRotationMetaStore @@ -4503,6 +4504,21 @@ async def run() -> int: store = await open_store(settings.store) try: count = await store.reencrypt_to_active() + # BACKLOG #1169: the uploaded-file store is the OTHER surface this cipher covers, and it + # had no rotation of any kind — a rotation re-sealed every database cell and left every + # uploaded file under the retired key, so the operator's next step (dropping that key) + # silently destroyed them. Re-seal it in the SAME command, on the store's own live cipher + # instance so the AES-GCM invocation bound (ASVS 11.3.4) charges to the new key exactly + # as the store's own pass does. A second cipher over the same DEK would charge nothing. + uploads = ( + await UploadStore( + settings.store.uploads_dir, + store.cipher(), + max_bytes=settings.store.max_upload_bytes, + ).reseal_to_active() + if settings.store.uploads_dir + else ResealResult() + ) # ASVS 13.3.4: stamp the DEK rotation so the watcher's clock resets automatically (rotation # auto-detected). The store is open under the NEW active key, so its key-id is the new # fingerprint; preserve the tracked-since floor. NON-SECRET (key-id + dates only). @@ -4518,21 +4534,37 @@ async def run() -> int: tracked_since=prior.tracked_since if prior is not None else today, last_rotated=today, ) - return count + return count, uploads finally: await store.close() try: - count = asyncio.run(run()) + count, uploads = asyncio.run(run()) except CipherError as exc: - # A value couldn't be decrypted by any supplied key — the prior key is missing. Nothing was - # corrupted (a batch is all-or-nothing); supply the key and re-run. + # A value couldn't be decrypted by any supplied key — the prior key is missing. Nothing is + # corrupted: every pass is all-or-nothing per batch AND idempotent, so re-running with the + # key supplied finishes the job. Note the command now spans TWO surfaces (the store, then + # the uploaded-file store), so a failure in the second leaves the FIRST already committed + # and the ASVS 13.3.4 rotation stamp unwritten. That is safe precisely because both passes + # skip what is already under the active key — it is a resumable rotation, not a rollback. print(f"error: rotation aborted — {exc}", file=sys.stderr) return 1 except NotImplementedError as exc: print(f"error: {exc}", file=sys.stderr) return 2 - print(f"OK: re-encrypted {count} value(s) under the active key") + print( + f"OK: re-encrypted {count} value(s) under the active key" + f" (+{uploads.resealed} uploaded-file value(s) re-sealed)" + ) + if uploads.skipped: + # Say it plainly and on stderr: a skipped file is STILL under the old key, so retiring that + # key now destroys it. This is the one outcome where "OK" alone would mislead. + print( + f"warning: {uploads.skipped} uploaded-file value(s) could not be read and were NOT " + "re-sealed — they are still under the prior key. Fix the cause and re-run rotate-key " + "BEFORE removing MEFOR_STORE_ENCRYPTION_KEYS_RETIRED.", + file=sys.stderr, + ) return 0 diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 6c670fbe0..eaea06c6e 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -163,6 +163,12 @@ _FIFO_HEADS_LANE_CHUNK = 500 # ADR 0066 §3.1: release_claimed id-chunk bound (ids per UPDATE statement). _RELEASE_CHUNK = 500 +# BACKLOG #1169: rows per batch for the `attachment_chunk` at-rest migration. Every OTHER cipher pass +# batches 500 because its rows are kilobyte-shaped; one attachment_chunk row is a whole +# DETACH_CHUNK_BYTES (1 MiB) slice, ~1.33 MiB once base64+GCM sealed. 16 keeps a batch near 21 MiB, +# which is the memory budget the 500-row passes actually spend; 500 here would be ~650 MiB held in a +# single transaction while the store is still opening. +_ATTACHMENT_CHUNK_BATCH = 16 # Advisory-lock keys passed to the TWO-key pg_advisory_xact_lock(classid, hashtext($key)). They # serialize the audit-chain append (H-7) and schema init across concurrent opens; the finalize lock is @@ -1745,6 +1751,21 @@ async def _encrypt_existing_rows(self) -> None: encrypt=True, value_col=col, ) + # The `attachment_chunk` table (#149, ADR 0105) is cipher-covered (`ciphertext`) with the + # composite PK (attachment_id, seq), so it can't ride the id-keyed loop either. Its ROTATION + # pass already existed; this ON-OPEN pass did not, so a keyless→keyed transition left legacy + # plaintext chunks unsealed on this backend while SQLite sealed them (BACKLOG #1169). + # A SMALL batch, unlike every sibling above: each row is one DETACH_CHUNK_BYTES (1 MiB) slice + # rather than a kilobyte-shaped value, so 500 would hold ~650 MiB resident in one transaction + # while the store is still opening. + total += await self._encrypt_existing_composite( + "attachment_chunk", + ("attachment_id", "seq"), + like, + encrypt=True, + value_col="ciphertext", + limit=_ATTACHMENT_CHUNK_BATCH, + ) # BIGSERIAL-id tables bind to insert-time-known natural columns (id_keyed=True; see # _CIPHER_COLUMNS) — their own composite migration passes (ASVS 11.3.3). total += await self._encrypt_existing_composite( @@ -1783,6 +1804,7 @@ async def _encrypt_existing_composite( encrypt: bool, value_col: str = "value", id_keyed: bool = False, + limit: int = 500, ) -> int: """Encrypt the ``value_col`` of a non-id-keyed table in place — the migration loop for tables that can't ride the id-keyed loop. Each value binds to ``cell_aad(table, value_col, *aad_cols)`` (ASVS @@ -1791,7 +1813,11 @@ async def _encrypt_existing_composite( ``response`` passes ``body``/``detail``. ``aad_cols`` are the composite PK for state/reference/ response; for the BIGSERIAL-id tables (``message_events``/``connection_event``/``alert_instance``) set ``id_keyed=True`` — the AAD then comes from ``aad_cols`` (insert-time-known natural columns) - while the UPDATE targets ``id`` (so a natural-column collision can never re-write the wrong row).""" + while the UPDATE targets ``id`` (so a natural-column collision can never re-write the wrong row). + + ``limit`` is the rows held in memory per batch. 500 suits the KILOBYTE-shaped columns this + started with (state/reference/response); ``attachment_chunk`` holds one 1 MiB slice per row, + where 500 would be ~650 MiB resident inside a single transaction at store open.""" rotated = 0 select_cols = ("id", *aad_cols) if id_keyed else aad_cols pk_select = ", ".join(select_cols) @@ -1801,7 +1827,7 @@ async def _encrypt_existing_composite( while True: rows = await self._fetchall( f"SELECT {pk_select}, {value_col} AS v FROM {table}" - f" WHERE {value_col} NOT LIKE $1 AND {value_col} <> '' LIMIT 500", + f" WHERE {value_col} NOT LIKE $1 AND {value_col} <> '' LIMIT {int(limit)}", like, ) if not rows: diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index c6bf45ea9..8a1cfff72 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -152,6 +152,12 @@ _FIFO_HEADS_LANE_CHUNK = 500 # ADR 0066 §3.1: release_claimed id-chunk bound (ids per UPDATE statement). _RELEASE_CHUNK = 500 +# BACKLOG #1169: rows per batch for the `attachment_chunk` at-rest migration. Every OTHER cipher pass +# batches 500 because its rows are kilobyte-shaped; one attachment_chunk row is a whole +# DETACH_CHUNK_BYTES (1 MiB) slice, ~1.33 MiB once base64+GCM sealed. 16 keeps a batch near 21 MiB, +# which is the memory budget the 500-row passes actually spend; 500 here would be ~650 MiB held in a +# single transaction while the store is still opening. +_ATTACHMENT_CHUNK_BATCH = 16 # ADR 0073: ownership-scoped reset lane-chunk bound (lane names per UPDATE's IN list) — well under # pyodbc's ~2,100-parameter bound with the fixed parameters; chunks run inside the reset's single # transaction, so the all-or-nothing recovery pass is unchanged. @@ -2805,9 +2811,53 @@ async def _encrypt_existing_rows(self) -> None: raise await self._charge_bound_batch() total += len(rows) + # `attachment_chunk` detached-document slices (#149, ADR 0105, composite PK + # (attachment_id, seq)) — a separate pass (can't ride the id-keyed loop). Its ROTATION pass + # already existed here; this ON-OPEN pass did not, so a no-key -> key transition left legacy + # plaintext chunks unsealed on SQL Server and Postgres while SQLite sealed them + # (BACKLOG #1169). `ciphertext` is NOT NULL, so the `<> ''` guard alone keeps a blank from + # becoming ciphertext-of-empty. A SMALL batch, unlike every sibling above: each row is one + # DETACH_CHUNK_BYTES (1 MiB) slice, so the usual 500 would hold ~650 MiB resident in one + # transaction while the store is still opening. It is still a whole SLICE at a time, not a + # whole document — a large attachment spans many rows and is never reassembled here. + while True: + rows = await self._fetchall( + f"SELECT TOP ({_ATTACHMENT_CHUNK_BATCH}) attachment_id, seq, ciphertext" + " FROM attachment_chunk WHERE ciphertext NOT LIKE ? AND ciphertext <> ''", + (like,), + ) + if not rows: + break + async with self._acquire() as conn, self._cursor(conn) as cur: + try: + for r in rows: + await cur.execute( + "UPDATE attachment_chunk SET ciphertext=?" + " WHERE attachment_id=? AND seq=?", + ( + self._cipher.encrypt( + r["ciphertext"], + aad=cell_aad( + "attachment_chunk", + "ciphertext", + r["attachment_id"], + r["seq"], + ), + ), + r["attachment_id"], + r["seq"], + ), + ) + await self._commit(conn) + except Exception: + await conn.rollback() + raise + await self._charge_bound_batch() + total += len(rows) if total: log.info( - "encrypted %d existing message/outbox/response/reference/state row(s) at rest", + "encrypted %d existing message/outbox/response/reference/state/attachment row(s) " + "at rest", total, ) diff --git a/messagefoundry/uploads.py b/messagefoundry/uploads.py index d79f2d188..71c35a182 100644 --- a/messagefoundry/uploads.py +++ b/messagefoundry/uploads.py @@ -35,7 +35,7 @@ import re import secrets import time -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Iterator from dataclasses import asdict, dataclass from pathlib import Path from typing import Protocol @@ -44,7 +44,7 @@ from messagefoundry.parsing.sniff import _looks_like_hl7, _lstrip_bom_ws from messagefoundry.parsing.split import split_batch from messagefoundry.store.content_search import SearchSpec, row_matches -from messagefoundry.store.crypto import Cipher, cell_aad +from messagefoundry.store.crypto import AesGcmCipher, Cipher, CipherError, cell_aad _log = logging.getLogger(__name__) @@ -299,6 +299,19 @@ def browse_messages( ) +@dataclass(frozen=True) +class ResealResult: + """What one :meth:`UploadStore.reseal_to_active` pass did. + + ``skipped`` is load-bearing, not decoration: an operator reads it to decide whether it is safe to + drop the retired key. A file this pass could not read is a file still sealed under the OLD key, + and dropping that key makes it permanently unreadable — so a non-zero ``skipped`` means "run it + again before you retire anything".""" + + resealed: int = 0 + skipped: int = 0 + + class UploadQuotaLedger(Protocol): """The ONE thing :class:`UploadStore` needs from the message store: an atomic, cross-process reservation of an uploader's in-flight upload budget (ASVS 2.3.4). @@ -437,25 +450,62 @@ def _decrypt_meta(self, stored: str, file_id: str) -> UploadedFileMeta: message_count=int(d.get("message_count", 0)), ) - def _scan_metas_sync(self) -> list[UploadedFileMeta]: - """Walk the uploads root and decrypt every well-formed ``.meta`` sidecar (UNSORTED). A - bad/foreign/undecryptable sidecar is skipped with a warning (never a body in the log), so a - rotated-away key can neither sink the listing nor silently drop a quota/retention pass. Pure - filesystem read — the caller runs it off the event loop.""" + def _iter_sidecars(self) -> Iterator[tuple[str, Path]]: + """Every ``(file_id, sidecar path)`` pair in the uploads root — the ONE definition of "a file + of ours". Both the listing scan and the re-seal pass consume it, so the id shape and the + sidecar suffix cannot drift apart between them. Anything else in the directory (an operator's + stray file, a temp file, an id that fails the strict 32-hex shape) is not ours and is skipped + without being opened.""" root = self._root if not root.is_dir(): - return [] - out: list[UploadedFileMeta] = [] + return for entry in root.iterdir(): if not entry.name.endswith(_META_SUFFIX): continue fid = entry.name[: -len(_META_SUFFIX)] - if not _FILE_ID_RE.match(fid): - continue + if _FILE_ID_RE.match(fid): + yield fid, entry + + def _scan_metas_sync(self) -> list[UploadedFileMeta]: + """Walk the uploads root and decrypt every well-formed ``.meta`` sidecar (UNSORTED). A + bad/foreign/undecryptable sidecar is skipped **with its cause named** (never a body in the + log), so a rotated-away key can neither sink the listing nor silently drop a quota/retention + pass. Pure filesystem read — the caller runs it off the event loop. + + **Each failure class is caught on its own and logged distinctly (BACKLOG #1169, CLAUDE.md + §6).** One blanket ``except Exception`` used to fold all three into a single + "skipping unreadable sidecar" line, which made three unrelated conditions indistinguishable + in the log: a key that was rotated away, a sidecar whose bytes are damaged, and the cipher + REFUSING a value. The third is the one that matters — it is the class a strict-ciphertext + read would raise in, on exactly the surface where planting a file is easiest (a pair of + plain files in a directory, no database write needed). A refusal folded into the same line + as a routine post-rotation skip is a refusal nobody can see, so the strict read cannot + honestly be built on top of this handler until the classes are separated. Separating them + does not itself refuse anything: an unmarked sidecar is still accepted today by the cipher's + read passthrough (``store/crypto.py`` ``decrypt``), which awaits an owner ruling. + + The cipher's own message is safe to log — every ``CipherError`` carries only key ids, + marker versions and algorithm names, never a decrypted value. The malformed-shape branch + logs only the exception TYPE, because a ``ValueError`` from coercing a metadata field can + echo that field's value back into the message.""" + out: list[UploadedFileMeta] = [] + for fid, entry in self._iter_sidecars(): try: out.append(self._decrypt_meta(entry.read_text(encoding="utf-8"), fid)) - except Exception: # noqa: BLE001 — a bad/foreign sidecar must not sink the scan - _log.warning("skipping unreadable uploaded-file sidecar %s", fid) + except CipherError as exc: + # The cipher declined the value. Today that is a wrong/rotated-away key or a blob + # relocated into another cell; once a strict-ciphertext read ships it is ALSO the + # refusal of an unmarked (planted or downgraded) sidecar. Named on its own so the + # two can be told apart in a log. + _log.warning("uploaded-file sidecar %s: cipher declined it: %s", fid, exc) + except (OSError, UnicodeDecodeError) as exc: + # The bytes never reached the cipher — unreadable file, or not valid UTF-8. + _log.warning("uploaded-file sidecar %s: unreadable on disk: %s", fid, exc) + except (ValueError, TypeError, KeyError) as exc: + # Decrypted, but the JSON is malformed or a field will not coerce. Type only. + _log.warning( + "uploaded-file sidecar %s: malformed metadata (%s)", fid, type(exc).__name__ + ) return out # --- public API (all disk/crypto/split work off the event loop) -------------------------------- @@ -704,6 +754,108 @@ def _delete() -> UploadedFileMeta: return await asyncio.to_thread(_delete) + async def reseal_to_active(self) -> ResealResult: + """Re-seal every uploaded ``(blob, meta)`` pair under the **active** key — the uploads half of + ``messagefoundry rotate-key`` (BACKLOG #1169, ASVS 11.2.2/11.3.3). + + The store's :meth:`~messagefoundry.store.base.Store.reencrypt_to_active` has covered its + cipher columns since WP-5; this surface had **no pass of any kind**, so an uploaded file was + left behind by both transitions the store handles. A first key-enable left it plaintext on + disk forever, and a key rotation left it under the retired key — which is a data-loss defect + on its own, because the operator's next step is to drop that key and every upload written + before the rotation then stops decrypting. + + **The two transitions are not closed the same way, and the difference is deliberate.** The + store seals legacy plaintext AUTOMATICALLY, in ``_encrypt_existing_rows`` at every keyed open. + This pass has one caller, ``rotate-key``, so it closes the rotation half automatically and + the first-key-enable half only when an operator runs that command. Wiring a whole-directory + crypto sweep into API startup is unbounded boot-time work over a directory with no batching + seam, which is a different risk and a separate decision — so it is named here rather than + quietly assumed. + + Same contract as the store's pass: rewrites values that are plaintext **or** under a retired + key, skips values already under the active key (so it is idempotent), and lets a + :class:`CipherError` propagate rather than dropping data — a value no configured key opens + means the prior key was not supplied, and the CLI says so. Also mirrors its **limitation**: + a non-``AesGcmCipher`` (the identity cipher, or a ``vault_transit`` cipher whose DEK never + enters the heap) rotates nothing and returns zeros, exactly as ``reencrypt_to_active`` does. + + Re-sealing happens at the **cipher** layer — ``encrypt(decrypt(stored))`` over the stored + string — so a body's base64 envelope is never decoded and the plaintext bytes are never + reassembled here. Files are processed one at a time and each is rewritten through the same + atomic temp-and-replace the write path uses. Peak memory is a SMALL MULTIPLE of ``max_bytes``, + not ``max_bytes`` itself: base64 and the AES-GCM round trip each hold their own copy, so a + 25 MiB upload transiently costs a few hundred MiB. A file already under the active key is + detected from its first bytes and never read whole. + + **This pass LAUNDERS an unmarked value, and that is not an oversight to overlook.** Handed a + plaintext file, the cipher's read passthrough returns it unchanged and this method then seals + it into a genuine, AAD-bound ciphertext — after which nothing distinguishes it from a file + the engine wrote itself. The store's rotation has the identical property. That is precisely + why #1169 wants a strict-ciphertext read, and this method does not substitute for one: it is + a **precondition** for it. A refusal built before this pass existed would fire on legitimate + pre-rotation uploads, because until now nothing could ever seal them. The refusal itself + awaits an owner ruling and is deliberately not built here. + + Runs entirely off the event loop.""" + return await asyncio.to_thread(self._reseal_to_active_sync) + + def _reseal_to_active_sync(self) -> ResealResult: + """The blocking half of :meth:`reseal_to_active` (disk + cipher). Caller runs it off-loop.""" + cipher = self._cipher + if not isinstance(cipher, AesGcmCipher): + return ResealResult() # identity / transit cipher — nothing this process can rotate + root = self._root + if not root.is_dir(): + return ResealResult() + active = cipher.active_marker_prefix + resealed = skipped = 0 + for fid, _sidecar in self._iter_sidecars(): + try: + blob_path, meta_path = self._paths(fid) + except UploadPathError: + # Unreachable for an id that passed _FILE_ID_RE — the guard re-checks the same shape. + # Kept anyway, because `prune_expired` guards its unlink the same way: a path guard + # this pass skipped would be the one place a bad id reaches the filesystem. + skipped += 1 + _log.warning("uploaded file %s: skipped, its id fails the path guard", fid) + continue + # The sidecar carries the AAD kind "meta"; the body carries "body" (see _encrypt_meta / + # _encrypt_blob). Re-binding the SAME cell AAD is what keeps a re-sealed value readable. + for path, kind in ((meta_path, "meta"), (blob_path, "body")): + try: + # Read the MARKER first, not the file. The idempotency test is a prefix compare, + # and a sealed 25 MiB upload is ~44 MiB on disk — reading it whole only to skip + # it made a re-run cost a full pass over every already-current file. The store's + # own passes push this filter into SQL (`WHERE col NOT LIKE ...`) for the same + # reason; on a filesystem the equivalent is a bounded read. + with path.open(encoding="utf-8") as handle: + if handle.read(len(active)) == active: + continue # already under the active key in the active format + stored = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + # A half-deleted pair or an unreadable file. Counted and named, never silent: + # this file is still under the OLD key and the operator must not retire it yet. + skipped += 1 + _log.warning("uploaded file %s (%s): skipped, unreadable: %s", fid, kind, exc) + continue + aad = cell_aad("uploaded_file", kind, fid) + # A CipherError here means a prior key was not supplied. It PROPAGATES, before any + # write, so the operator is told to supply it rather than losing the file. + _atomic_write_text(root, path, _reencrypt_value(cipher, stored, aad)) + # Drop the file-sized buffer before the next iteration allocates its own. Without + # this the previous file's plaintext stays reachable from the frame while the next + # one is read, roughly doubling peak memory for no reason. + del stored + resealed += 1 + if resealed or skipped: + _log.info( + "re-sealed %d uploaded-file value(s) under the active key (%d skipped)", + resealed, + skipped, + ) + return ResealResult(resealed=resealed, skipped=skipped) + async def prune_expired( self, *, now: float | None = None, retention_days: int | None = None ) -> list[UploadedFileMeta]: @@ -818,6 +970,19 @@ async def run_once(self, now: float | None = None) -> list[UploadedFileMeta]: return pruned +def _reencrypt_value(cipher: AesGcmCipher, stored: str, aad: bytes) -> str: + """Decrypt (keyring — any configured key) then re-encrypt under the active key, rebinding the + SAME cell AAD (ASVS 11.3.3). + + Deliberately named to match ``MessageStore._reencrypt_value`` and its Postgres and SQL Server + twins, which are the identical computation for the database half. They are four spellings of one + seam and folding them into ``store/crypto.py`` is worth doing — but those three are staticmethods + on store classes this LEAF module may not import (see the module docstring), so the shared name + is what keeps a grep for ``_reencrypt_value`` from missing this one. Pairing a decrypt and an + encrypt with different AADs is the mistake the single-expression form exists to prevent.""" + return cipher.encrypt(cipher.decrypt(stored, aad=aad), aad=aad) + + def _atomic_write_text(root: Path, path: Path, text: str) -> None: """Write ``text`` to ``path`` via a temp file + ``os.replace`` (atomic on the same dir), owner-only.""" tmp = root / f".{path.name}.{secrets.token_hex(4)}.tmp" diff --git a/tests/test_cli.py b/tests/test_cli.py index 7f7770d96..9e202d3f3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -260,6 +260,97 @@ async def read_with_b_only() -> int: assert asyncio.run(read_with_b_only()) == 1 # readable under the new key alone +def _seed_store_and_upload(db: Path, uploads: Path, key: str) -> str: + """Write one queued message and one uploaded file under ``key``; return the upload's file id. + + The upload store is built on the STORE's own cipher instance, which is what `api/app.py` does + and what the ASVS 11.3.4 invocation bound requires — a second cipher over the same DEK charges + nothing to the key's persisted count. + """ + import asyncio + + from messagefoundry.store.crypto import make_cipher + from messagefoundry.store.store import MessageStore + from messagefoundry.uploads import UploadStore + + async def run() -> str: + store = await MessageStore.open(db, cipher=make_cipher(key)) + try: + await store.enqueue_message(channel_id="ch", raw=ADT_A01, deliveries=[("d", ADT_A01)]) + meta = await UploadStore(uploads, store.cipher(), max_bytes=1 << 20).save( + data=ADT_A01.encode(), filename="partner.hl7", uploader="op", uploader_id="u-op" + ) + return meta.file_id + finally: + await store.close() + + return asyncio.run(run()) + + +def _rotate_env( + monkeypatch: pytest.MonkeyPatch, *, active: str, retired: str, uploads: Path +) -> None: + monkeypatch.setenv("MEFOR_STORE_ENCRYPTION_KEY", active) + monkeypatch.setenv("MEFOR_STORE_ENCRYPTION_KEYS_RETIRED", retired) + monkeypatch.setenv("MEFOR_STORE_UPLOADS_DIR", str(uploads)) + + +def test_rotate_key_also_reseals_the_uploaded_file_store( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """BACKLOG #1169: uploaded files ride the SAME cipher, and rotation used to skip them entirely. + + The store's columns were re-encrypted and every uploaded file was left under the retired key — + so the operator's documented next step, dropping that key, silently destroyed them. Rotate, then + read back with the new key ALONE, which is the assertion the store's own rotation test makes. + """ + import asyncio + + from messagefoundry.store.crypto import generate_key, make_cipher + from messagefoundry.store.store import MessageStore + from messagefoundry.uploads import UploadStore + + monkeypatch.chdir(tmp_path) + db, uploads = tmp_path / "rot-uploads.db", tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + file_id = _seed_store_and_upload(db, uploads, key_a) + + _rotate_env(monkeypatch, active=key_b, retired=key_a, uploads=uploads) + assert main(["rotate-key", "--db", str(db)]) == 0 + captured = capsys.readouterr() + assert "uploaded-file value(s) re-sealed" in captured.out + assert captured.err == "" # nothing was skipped, so no retire-the-key warning + + async def read_with_b_only() -> bytes: + store = await MessageStore.open(db, cipher=make_cipher(key_b)) # key A is gone + try: + return await UploadStore(uploads, store.cipher(), max_bytes=1 << 20).read_bytes(file_id) + finally: + await store.close() + + assert asyncio.run(read_with_b_only()).decode() == ADT_A01 + + +def test_rotate_key_warns_when_an_uploaded_file_could_not_be_resealed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """A skipped file is STILL under the prior key, so "OK" alone would mislead the operator into + dropping a key that is still load-bearing. The warning names that consequence.""" + from messagefoundry.store.crypto import generate_key + + monkeypatch.chdir(tmp_path) + db, uploads = tmp_path / "rot-skip.db", tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + file_id = _seed_store_and_upload(db, uploads, key_a) + (uploads / f"{file_id}.blob").unlink() # a half-deleted pair + + _rotate_env(monkeypatch, active=key_b, retired=key_a, uploads=uploads) + assert main(["rotate-key", "--db", str(db)]) == 0 + err = capsys.readouterr().err + assert "could not be read and were NOT re-sealed" in err + assert "MEFOR_STORE_ENCRYPTION_KEYS_RETIRED" in err + + def test_rotate_key_requires_a_key( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: diff --git a/tests/test_store_cipher_sweep_parity.py b/tests/test_store_cipher_sweep_parity.py new file mode 100644 index 000000000..b95018147 --- /dev/null +++ b/tests/test_store_cipher_sweep_parity.py @@ -0,0 +1,324 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Every cipher-covered cell a backend WRITES must be named by BOTH of that backend's sweeps. + +**The gap this closes, and why the existing guard could not see it.** +``tests/test_sqlserver_encrypt_pass_tables.py`` checks the sweeps in the *other* direction: a table a +sweep names must be a table the module creates. That catches a sweep pointing at a dropped table. It +cannot catch the opposite and more likely drift — a covered table that **no sweep names at all** — +because an omission has no literal to inspect. That is exactly what shipped: ``attachment_chunk`` +(#149, ADR 0105) was added to all three backends with a rotation pass but, on Postgres and SQL +Server, no ON-OPEN migration pass. SQLite had one. Nothing anywhere compared them. + +**Why both sweeps and not just one.** They handle the two different transitions and are not +interchangeable: ``_encrypt_existing_rows`` runs at every keyed open and seals legacy plaintext; +``reencrypt_to_active`` runs offline under ``rotate-key`` and moves values onto the active key. A +cell covered by only the second stays plaintext at rest until someone rotates a key. + +**Scoped to tables the module INSERTs into, which is the part that keeps this honest.** A naive +"every ``cell_aad`` cell must be swept" rule falsely accuses both server backends over +``shared_body``: they declare the table for schema parity but never write a row into it, so there is +nothing for a migration to seal. ``INSERT INTO`` is the instrument that separates a real omission +from a table a backend merely declares — and it is the same instrument that corrected #1169's +originally-reported ``shared_body`` precondition. That fact is pinned in +``tests/test_phi_at_rest_inventory.py`` and is deliberately not restated here. + +BACKLOG #1169, ASVS 11.3.3. Reads engine source; needs no database, driver or key, so it runs on the +plain leg — the SQL Server and Postgres CI legs are all KEYLESS and return before either sweep body. +""" + +from __future__ import annotations + +import ast +import functools +import pathlib +import re + +import pytest + +import messagefoundry + +_STORE_DIR = pathlib.Path(messagefoundry.__file__).resolve().parent / "store" +_BACKENDS = ("store.py", "postgres.py", "sqlserver.py") + +#: The two passes. Both are reached only on a KEYED handle, which is why CI never executes either. +_SWEEPS = ("_encrypt_existing_rows", "reencrypt_to_active") + +_INSERT_INTO = re.compile(r"INSERT INTO\s+(\w+)", re.IGNORECASE) + + +def _covered_cells(tree: ast.AST) -> set[tuple[str, str]]: + """Every ``cell_aad("", "", ...)`` literal in the module. + + This is the module's OWN definition of a cipher-covered cell, not a second list that can drift + from it: the same call builds the AAD on the write path and on the read path, so a cell that + appears here is a cell whose value is sealed and must therefore be migrated and rotated. + """ + found: set[tuple[str, str]] = set() + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or len(node.args) < 2: + continue + fn = node.func + name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None) + if name != "cell_aad": + continue + table, column = node.args[0], node.args[1] + if ( + isinstance(table, ast.Constant) + and isinstance(column, ast.Constant) + and isinstance(table.value, str) + and isinstance(column.value, str) + ): + found.add((table.value, column.value)) + return found + + +def _executable_strings(node: ast.AST) -> set[str]: + """Every string constant under ``node`` that the code actually USES. + + Docstrings and bare string statements are excluded, and comments never enter the AST at all. + **That exclusion is the whole point.** The first version of this check matched table names + against raw source text and reported the shipped Postgres omission as CLEAN — because a comment + two lines above the missing call said the word ``attachment_chunk``. Prose describing coverage + satisfied a check about coverage: the instrument was answering "is this table mentioned here", + not "is this table swept here" (CLAUDE.md section 11, SDS-3.8). A table name only counts when it + reaches a SQL string or a sweep tuple that runs. + """ + docstrings = { + id(child.value) + for child in ast.walk(node) + if isinstance(child, ast.Expr) and isinstance(child.value, ast.Constant) + } + return { + child.value + for child in ast.walk(node) + if isinstance(child, ast.Constant) + and isinstance(child.value, str) + and id(child) not in docstrings + } + + +def _declarations(tree: ast.AST) -> dict[str, ast.Assign]: + """MODULE- and CLASS-level assignments only — the sweep tuples like ``_CIPHER_COLUMNS``. + + Deliberately not ``ast.walk``: that also collects every local inside every function (~250 names + per backend, last-wins), so an incidental local called ``value`` or ``total`` would drag an + unrelated assignment's SQL into the reach set. Since a bigger reach set makes :func:`_is_swept` + EASIER to satisfy, over-collecting here silently weakens the very detection this file exists for. + """ + out: dict[str, ast.Assign] = {} + bodies = [getattr(tree, "body", [])] + bodies += [node.body for node in ast.walk(tree) if isinstance(node, ast.ClassDef)] + for body in bodies: + for node in body: + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name): + out[target.id] = node + return out + + +def _sweep_strings(tree: ast.AST, entry: str) -> set[str]: + """The executable strings a sweep can reach: its own body, the same-module helpers it CALLS + DIRECTLY, and the module/class-level declarations it names. + + All three matter. ``_encrypt_existing_rows`` delegates most tables to helpers + (``_encrypt_existing_composite``, ``_encrypt_message_events``), and the id-keyed tables are not + literals in any function at all — they live in the ``_CIPHER_COLUMNS`` class attribute the loop + iterates. Reading only a function body reports four tables missing on every backend, which is + how a guard ends up crying wolf and getting switched off. + + **One level of delegation, not transitive closure.** Following calls all the way down reaches + ``close``, ``checkpoint_cipher_invocations`` and the connection-pool machinery — 13 to 14 + functions on ``sqlserver.py`` — and every SQL string in them then counts as "the sweep names + this table". Every real pass is a direct callee, so the extra depth adds only dilution. + """ + functions = { + node.name: node + for node in ast.walk(tree) + if isinstance(node, ast.AsyncFunctionDef | ast.FunctionDef) + } + entry_fn = functions.get(entry) + if entry_fn is None: + return set() + declarations = _declarations(tree) + strings: set[str] = set() + reached: list[ast.AsyncFunctionDef | ast.FunctionDef] = [entry_fn] + for node in ast.walk(entry_fn): + if not isinstance(node, ast.Call): + continue + fn = node.func + called = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None) + if called and called in functions and called != entry: + reached.append(functions[called]) + for function in reached: + strings |= _executable_strings(function) + named = { + child.id if isinstance(child, ast.Name) else child.attr + for child in ast.walk(function) + if isinstance(child, ast.Name | ast.Attribute) + } + for name in named & declarations.keys(): + strings |= _executable_strings(declarations[name]) + return strings + + +def _written_cells(source: str, tree: ast.AST) -> set[tuple[str, str]]: + """Covered cells whose table this module actually INSERTs into — the ones it must sweep.""" + written = set(_INSERT_INTO.findall(source)) + return {cell for cell in _covered_cells(tree) if cell[0] in written} + + +def _is_swept(cell: tuple[str, str], reach: set[str]) -> bool: + """A cell is swept when both its table and its column reach an executable string — either as an + exact sweep-tuple entry (``("messages", "raw")``) or inside a SQL statement. + + **The table and the column are matched INDEPENDENTLY, and that looseness is forced — do not + "fix" it to require co-occurrence in one string.** The backends express a pass in two different + shapes: the id-keyed tables arrive as separate constants from the ``_CIPHER_COLUMNS`` tuple, + where ``"messages"`` and ``"raw"`` never appear in the same string, while the composite passes + arrive as f-string SQL where they do. Requiring one string to hold both immediately reds every + id-keyed cell on all three backends. So this proves a table and its column are both reachable + from the sweep, not that a specific statement exists — which is exactly enough to catch the + omission this file was written for, and no more. Tightening it needs the source to be data + first (see the module docstring). + """ + table, column = cell + return any(table in s for s in reach) and any(column in s for s in reach) + + +@functools.cache +def _parsed(backend: str) -> tuple[str, ast.Module]: + """Source + AST for one backend, parsed ONCE. The two parametrize axes cross (3 backends x 2 + sweeps), and these modules are 4,700 to 8,300 lines — re-parsing per case costs about half a + second for nothing. The trees are only ever read here, so sharing them is safe.""" + source = (_STORE_DIR / backend).read_text(encoding="utf-8") + return source, ast.parse(source) + + +@pytest.mark.parametrize("backend", _BACKENDS) +@pytest.mark.parametrize("sweep", _SWEEPS) +def test_every_written_cipher_cell_is_swept(backend: str, sweep: str) -> None: + """A covered cell this backend writes must be reachable from this sweep. + + Mutation receipt: deleting the ``attachment_chunk`` pass from ``postgres.py``'s + ``_encrypt_existing_rows`` reds this, on the plain leg, with no database — which is the state + that shipped before BACKLOG #1169. + """ + source, tree = _parsed(backend) + cells = _written_cells(source, tree) + reach = _sweep_strings(tree, sweep) + + # Liveness receipts. Either of these silently empty makes the assertion below vacuous, which is + # the exact failure this file exists to prevent elsewhere. + assert cells, f"{backend}: no cell_aad cells found in INSERTed tables — the walk is broken" + assert reach, f"{backend}: {sweep} was not found — renamed, or the AST walk is broken" + + unswept = sorted(c for c in cells if not _is_swept(c, reach)) + assert not unswept, ( + f"{backend}: {sweep} never names these cipher-covered cells, so a value written to them " + "is left behind by that transition (plaintext at rest, or stranded under a retired key). " + "No CI leg can catch this at runtime: every SQL Server and Postgres leg is keyless and " + f"returns before the sweep body.\n unswept: {unswept}" + ) + + +def test_the_guard_can_actually_see_an_unswept_cell() -> None: + """Prove the detector fires rather than trusting that it would. + + The parametrized test passing tells you nothing on its own — it passes identically if + ``_covered_cells`` returns nothing or ``_sweep_reach`` returns the whole module. Drive the same + functions over synthetic source with one swept cell and one omitted cell. + """ + # Faithful to the real module shape: a covered cell is declared by a LITERAL cell_aad call on a + # read/write path, while the sweep reaches it through the `_CIPHER_COLUMNS` tuple, whose loop + # passes variables. Both halves have to work or the guard mis-reports. + synthetic = """ +class S: + _CIPHER_COLUMNS = (("messages", "raw"),) + + async def put_message(self) -> None: + self._cipher.encrypt(raw, aad=cell_aad("messages", "raw", mid)) + await self._db.execute("INSERT INTO messages (raw) VALUES (?)") + + async def _encrypt_existing_rows(self) -> None: + for table, column in self._CIPHER_COLUMNS: + self._cipher.encrypt(v, aad=cell_aad(table, column, r["id"])) + + async def put_chunk(self) -> None: + self._cipher.encrypt(c, aad=cell_aad("attachment_chunk", "ciphertext", ref, seq)) + await self._db.execute("INSERT INTO attachment_chunk (ciphertext) VALUES (?)") +""" + tree = ast.parse(synthetic) + cells = _written_cells(synthetic, tree) + assert cells == {("messages", "raw"), ("attachment_chunk", "ciphertext")}, cells + + reach = _sweep_strings(tree, "_encrypt_existing_rows") + unswept = sorted(c for c in cells if not _is_swept(c, reach)) + assert unswept == [("attachment_chunk", "ciphertext")], unswept + + # And the id-keyed cell IS seen, only because the class attribute was pulled in — the failure + # mode that would make this guard accuse every backend of omitting `messages`. + assert _is_swept(("messages", "raw"), reach) + + +def test_a_comment_naming_the_table_does_not_count_as_sweeping_it() -> None: + """The false-NEGATIVE guard, and it is here because this check failed it once. + + Matching table names against raw source text reported the real shipped Postgres omission as + clean: a comment above the missing call said ``attachment_chunk``, and the substring search could + not tell prose from code. A guard that green-lights the exact defect it was written for is worse + than no guard, because it also certifies the absence. + """ + synthetic = ''' +class S: + async def put_chunk(self) -> None: + self._cipher.encrypt(c, aad=cell_aad("attachment_chunk", "ciphertext", ref, seq)) + await self._db.execute("INSERT INTO attachment_chunk (ciphertext) VALUES (?)") + + async def _encrypt_existing_rows(self) -> None: + """Seals attachment_chunk.ciphertext among others.""" + # The `attachment_chunk` table rides its own pass below. + return None +''' + tree = ast.parse(synthetic) + cells = _written_cells(synthetic, tree) + assert ("attachment_chunk", "ciphertext") in cells + + reach = _sweep_strings(tree, "_encrypt_existing_rows") + assert not _is_swept(("attachment_chunk", "ciphertext"), reach), ( + "a comment and a docstring naming the table were accepted as sweeping it" + ) + assert reach == set(), reach # the sweep executes no strings at all + + +def test_a_declared_but_never_written_table_is_not_demanded() -> None: + """The false-accusation guard: a table a backend declares but never INSERTs into is out of + scope, because there is no value of its to seal. + + Without this rule the check reds on Postgres and SQL Server over ``shared_body`` — a real + finding-shaped result that is not a finding, and the kind that gets a guard switched off. + """ + synthetic = """ +_SCHEMA = ["CREATE TABLE shared_body (hash TEXT PRIMARY KEY, body TEXT)"] + + +class S: + async def read_body(self) -> None: + self._cipher.decrypt(row, aad=cell_aad("shared_body", "body", h)) + + async def _encrypt_existing_rows(self) -> None: + return None +""" + tree = ast.parse(synthetic) + assert ("shared_body", "body") in _covered_cells(tree) # it IS a covered cell + assert _written_cells(synthetic, tree) == set() # but nothing here writes one + # And the CREATE TABLE text must not be what rescues it: declaring is not writing. + assert not _is_swept(("shared_body", "body"), _sweep_strings(tree, "_encrypt_existing_rows")) + + # NOTE: "only SQLite writes shared_body" is NOT re-asserted here. It is already pinned, with the + # same `INSERT INTO shared_body` instrument and the same reasoning, by + # `tests/test_phi_at_rest_inventory.py::_per_backend_cipher_counts` — which predates BACKLOG + # #1169 and therefore already contradicted that item's originally-reported precondition. Stating + # a load-bearing fact once and linking to it is the rule (CLAUDE.md section 11, SDS-3.5); two + # copies drift, and the copy a reader finds first wins. diff --git a/tests/test_uploads_reseal.py b/tests/test_uploads_reseal.py new file mode 100644 index 000000000..ee8b1512f --- /dev/null +++ b/tests/test_uploads_reseal.py @@ -0,0 +1,285 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The uploaded-file store's key migration + rotation pass, and its scan's per-cause logging. + +Both are **preconditions** for the strict-ciphertext read BACKLOG #1169 researches (ASVS 11.3.3), and +both were missing: + +* The store's cipher columns have ridden ``reencrypt_to_active`` since WP-5. This surface had no pass + of any kind, so ``rotate-key`` re-sealed every database cell and left every uploaded file under the + retired key. That is a **data-loss defect standing on its own merits** — the operator's documented + next step is to drop the retired key, and every upload written before the rotation then stops + decrypting. It is also a precondition, because a refusal that fires on unmarked values would fire on + legitimate pre-key uploads for as long as nothing could seal them. +* ``_scan_metas_sync`` folded every failure into one blanket ``except Exception`` and one generic + warning, so a cipher REFUSAL was indistinguishable in the log from a routine post-rotation skip — + on the surface where planting a file is easiest (two plain files in a directory, no database write). + +The refusal itself is deliberately NOT built here: it awaits an owner ruling, and +``test_a_planted_plaintext_sidecar_is_still_accepted`` pins the standing behaviour so the builder who +ships the refusal is told to convert it rather than discovering it in CI. +""" + +from __future__ import annotations + +import base64 +import json +import logging +from pathlib import Path + +import pytest + +from messagefoundry.store.crypto import CipherError, cell_aad, generate_key, make_cipher +from messagefoundry.uploads import ResealResult, UploadStore + +_ADT = "MSH|^~\\&|A|B|C|D|202601011200||ADT^A01|MSGID1|P|2.5\rPID|1||MRN123^^^HOSP||DOE^JOHN\r" + + +def _keyed_store(root: Path, key: str | None, retired: tuple[str, ...] = ()) -> UploadStore: + # write_v2=True is the shipped posture ([store].aad_bind defaults on), so the cell AAD is live. + return UploadStore(root, make_cipher(key, list(retired), write_v2=True), max_bytes=1 << 20) + + +def _write_sealed_meta(root: Path, fid: str, key: str, payload: str) -> None: + """Hand-write one sealed sidecar. The cell AAD lives in ONE place here, so a test that means to + vary the KEY cannot accidentally vary the binding too.""" + (root / f"{fid}.meta").write_text( + make_cipher(key, write_v2=True).encrypt( + payload, aad=cell_aad("uploaded_file", "meta", fid) + ), + encoding="utf-8", + ) + + +async def _seed(store: UploadStore) -> str: + meta = await store.save( + data=_ADT.encode(), filename="acme.hl7", uploader="op", uploader_id="u-op" + ) + return meta.file_id + + +# --- the rotation half: without this pass, rotate-key destroys uploads ------------------------ + + +async def test_rotation_reseals_so_the_prior_key_can_be_dropped(tmp_path: Path) -> None: + """The defect this pass closes, stated as the operator's own sequence. + + Rotate to B with A retired, then drop A — which is what the ``rotate-key`` docstring tells the + operator to do once it finishes. Before this pass existed the upload was still sealed under A at + that point and became permanently unreadable. + """ + root = tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + fid = await _seed(_keyed_store(root, key_a)) + + result = await _keyed_store(root, key_b, (key_a,)).reseal_to_active() + assert result == ResealResult(resealed=2, skipped=0) # the body and the sidecar + + dropped = _keyed_store(root, key_b) # key A is gone, exactly as the runbook says it may be + assert (await dropped.read_bytes(fid)).decode() == _ADT + assert (await dropped.get_meta(fid)).filename == "acme.hl7" + + +async def test_without_the_reseal_dropping_the_prior_key_loses_the_file(tmp_path: Path) -> None: + """The negative control for the test above: prove the loss is real, not assumed. + + A guard that only ever runs the fixed path cannot tell you the defect existed. This skips the + reseal and asserts the file is unreadable — so if some other change ever makes uploads survive a + rotation on their own, this test reds and says the pass above is no longer load-bearing. + """ + root = tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + fid = await _seed(_keyed_store(root, key_a)) + + with pytest.raises(CipherError): + await _keyed_store(root, key_b).read_bytes(fid) + + +async def test_reseal_is_idempotent(tmp_path: Path) -> None: + """A second pass re-seals nothing: values already under the active key are skipped, so an + interrupted rotation can simply be re-run.""" + root = tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + await _seed(_keyed_store(root, key_a)) + + rotator = _keyed_store(root, key_b, (key_a,)) + assert (await rotator.reseal_to_active()).resealed == 2 + assert await rotator.reseal_to_active() == ResealResult(resealed=0, skipped=0) + + +# --- the migration half: keyless -> keyed ------------------------------------------------------ + + +async def test_reseal_seals_a_keyless_upload_once_a_key_is_configured(tmp_path: Path) -> None: + """The transition the store handles at open (``_encrypt_existing_rows``) and this surface did + not handle anywhere: a file written before encryption was enabled stayed plaintext on disk.""" + root = tmp_path / "uploads" + fid = await _seed(_keyed_store(root, None)) + blob = root / f"{fid}.blob" + assert not blob.read_text(encoding="utf-8").startswith("mfenc:") # plaintext base64 + + key = generate_key() + keyed = _keyed_store(root, key) + assert (await keyed.reseal_to_active()).resealed == 2 + assert blob.read_text(encoding="utf-8").startswith("mfenc:") + assert (await keyed.read_bytes(fid)).decode() == _ADT + + +# --- contracts mirrored from the store's own rotation ------------------------------------------ + + +async def test_reseal_returns_zeros_for_a_cipher_it_cannot_rotate(tmp_path: Path) -> None: + """Identity (no key) and a Vault-Transit cipher whose DEK never enters the heap both rotate + nothing in-process — the same limitation ``reencrypt_to_active`` documents.""" + root = tmp_path / "uploads" + await _seed(_keyed_store(root, generate_key())) + assert await _keyed_store(root, None).reseal_to_active() == ResealResult() + + +async def test_reseal_raises_rather_than_dropping_a_file_it_cannot_open(tmp_path: Path) -> None: + """A missing prior key aborts BEFORE any write, so the operator is told to supply it instead of + losing data. Asserting the file still opens under the original key is the half that matters: a + pass that raised *after* rewriting would leave a shredded file behind.""" + root = tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + fid = await _seed(_keyed_store(root, key_a)) + + with pytest.raises(CipherError): + await _keyed_store(root, key_b).reseal_to_active() # key A never supplied + + assert (await _keyed_store(root, key_a).read_bytes(fid)).decode() == _ADT + + +async def test_an_unreadable_file_is_counted_as_skipped_not_as_success(tmp_path: Path) -> None: + """A half-deleted pair must not be reported as re-sealed. + + ``skipped`` is what tells the operator NOT to retire the prior key yet, so it has to be non-zero + here — counting the pass as clean is the failure that loses the file two commands later. + """ + root = tmp_path / "uploads" + key_a, key_b = generate_key(), generate_key() + fid = await _seed(_keyed_store(root, key_a)) + (root / f"{fid}.blob").unlink() # sidecar without a body + + result = await _keyed_store(root, key_b, (key_a,)).reseal_to_active() + assert result == ResealResult(resealed=1, skipped=1) + + +async def test_reseal_ignores_a_foreign_file_in_the_uploads_root(tmp_path: Path) -> None: + """Anything that is not a well-formed ``<32-hex>.meta`` is not ours and is left untouched.""" + root = tmp_path / "uploads" + key = generate_key() + await _seed(_keyed_store(root, key)) # already under the active key, so it is skipped + stray = root / "notes.txt" + stray.write_text("operator scratch", encoding="utf-8") + bad_id = root / "zzz.meta" + bad_id.write_text("not an upload", encoding="utf-8") + + result = await _keyed_store(root, key).reseal_to_active() + assert result == ResealResult() # nothing matched, so nothing was opened or rewritten + assert stray.read_text(encoding="utf-8") == "operator scratch" + assert bad_id.read_text(encoding="utf-8") == "not an upload" + + +# --- the scan's per-cause logging --------------------------------------------------------------- + + +async def test_the_scan_names_each_failure_cause_distinctly( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Three unrelated conditions must produce three distinguishable log lines. + + The blanket handler emitted the identical "skipping unreadable uploaded-file sidecar " for + all three, which is what would have made a future strict-read REFUSAL invisible: it would have + read exactly like a routine post-rotation skip. + """ + root = tmp_path / "uploads" + root.mkdir(parents=True) + key, other = generate_key(), generate_key() + store = _keyed_store(root, key) + + # 1. decrypts, but the plaintext is not the JSON object the sidecar promises. + _write_sealed_meta(root, "1" * 32, key, "not json") + # 2. the cipher declines it — a rotated-away key today; a strict-read refusal tomorrow. + _write_sealed_meta(root, "2" * 32, other, "{}") + # 3. the bytes never reach the cipher at all. + (root / f"{'3' * 32}.meta").write_bytes(b"\xff\xfe\x00 not utf-8") + + with caplog.at_level(logging.WARNING, logger="messagefoundry.uploads"): + assert await store.list_files() == [] + assert len(caplog.messages) == 3, caplog.messages + by_id = {m.split()[2].rstrip(":"): m for m in caplog.messages} + assert "malformed metadata (JSONDecodeError)" in by_id["1" * 32] + assert "cipher declined it" in by_id["2" * 32] + assert "unreadable on disk" in by_id["3" * 32] + + +async def test_the_scan_never_logs_a_decrypted_body( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """PHI rule (CLAUDE.md section 9): naming the cause must not start printing content. + + The malformed branch logs the exception TYPE only, because a ``ValueError`` raised coercing a + metadata field embeds that field's value in its message. + """ + root = tmp_path / "uploads" + root.mkdir(parents=True) + key = generate_key() + store = _keyed_store(root, key) + fid = "4" * 32 + _write_sealed_meta(root, fid, key, json.dumps({"file_id": fid, "size": "MRN123-NOT-AN-INT"})) + + with caplog.at_level(logging.WARNING, logger="messagefoundry.uploads"): + assert await store.list_files() == [] + joined = " ".join(caplog.messages) + assert "MRN123" not in joined, joined + assert "malformed metadata (ValueError)" in joined + + +# --- the standing defect, pinned so the refusal's builder converts it --------------------------- + + +async def test_a_planted_plaintext_sidecar_is_still_accepted(tmp_path: Path) -> None: + """TRIGGER, not an endorsement. BACKLOG #1169's refusal is NOT built and awaits an owner ruling. + + The cipher's read passthrough (``store/crypto.py`` ``decrypt``: an unmarked value is returned + unchanged) means a hand-written sidecar is accepted by a KEYED store, with every field + attacker-chosen. This test records that as the behaviour on this branch. **When the strict read + ships, this test fails — that is the intended signal.** Convert it to assert the refusal; do not + delete it, because the assertion below about laundering is what makes the refusal worth having. + """ + root = tmp_path / "uploads" + root.mkdir(parents=True) + key = generate_key() + store = _keyed_store(root, key) + fid = "f" * 32 + (root / f"{fid}.meta").write_text( + json.dumps( + { + "file_id": fid, + "filename": "planted.hl7", + "uploader": "attacker", + "uploader_id": "attacker-id", + "size": 3, + "uploaded_at": 1.0, + } + ), + encoding="utf-8", + ) + (root / f"{fid}.blob").write_text(base64.b64encode(b"PWN").decode("ascii"), encoding="utf-8") + + got = await store.get_meta(fid) + assert (got.filename, got.uploader, got.uploader_id) == ( + "planted.hl7", + "attacker", + "attacker-id", + ) + assert await store.read_bytes(fid) == b"PWN" + + # And the reseal pass LAUNDERS it into a genuine AAD-bound ciphertext, after which nothing + # distinguishes it from a file the engine wrote. The store's own rotation has this property too; + # it is the reason #1169 wants a refusal at the read, and the reason this pass is a precondition + # for one rather than a substitute. + assert (await store.reseal_to_active()).resealed == 2 + assert (root / f"{fid}.meta").read_text(encoding="utf-8").startswith("mfenc:") + assert (await store.get_meta(fid)).uploader == "attacker"