fix(store,uploads): close the three preconditions for a strict-ciphertext read (BACKLOG #1169)

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.)_

messagefoundry/__main__.py

@@ -4436,7 +4436,7 @@
     return 0 if ok else 1
 
 
 def _rotate_key(args: argparse.Namespace) -> int:
     """Re-encrypt every cipher-covered value under the active key (WP-5 key rotation, ASVS 11.2.2).
 
     Run **offline** (engine stopped): set ``MEFOR_STORE_ENCRYPTION_KEY`` to the NEW active key and keep
@@ -4464,6 +4464,7 @@
     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,14 +4496,29 @@
         )
         return 2
 
-    async def run() -> int:
+    async def run() -> tuple[int, ResealResult]:
         import datetime
 
         from messagefoundry.store.store import SecretRotationMetaStore
 
         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 @@
                         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
 
 

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 @@
                 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 @@
         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 @@
         ``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 @@
         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:

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.
@@ -2591,7 +2597,7 @@
         self._audit_keyed_from = watermark
         return True, f"audit chain keyed from id={watermark}"
 
     async def _encrypt_existing_rows(self) -> None:
         """Re-encrypt legacy plaintext bodies in place when encryption is enabled (STORE-1).
 
         Idempotent + batched: skips rows already carrying the ciphertext prefix."""
@@ -2805,9 +2811,53 @@
                     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,
             )