From dc2f5f8e2a67af078adfb61fc4fb66d3488c615b Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 14 Sep 2026 16:11:47 -0500 Subject: [PATCH 1/3] fix(dr): open the snapshot under the real store settings, and read its PHI Full restore verification built a bare StoreSettings to open the extracted snapshot, so an encrypted snapshot opened under the identity cipher: no key, no keyring, no provider. PRAGMA quick_check passed anyway, and the verify reported PASS having proved nothing about whether a single PHI cell was readable. Adding the decrypt pass without the settings fix turns that into the opposite and worse failure -- a perfectly good encrypted archive reported FAIL, which would teach a deploying operator that their backup is bad when it is fine, and a disaster-recovery check that cries wolf gets switched off. Four changes: 1. Thread the live StoreSettings into _verify_archive_blocking and on into _full_open_check, substituting only the path (and the backend, which the archive member is by construction). model_copy, not a rebuilt object, so a field added to StoreSettings later rides along instead of being dropped. 2. Add _decrypt_check: decrypt AND authenticate the snapshot's cipher-covered cells under the store's own cipher, with the same cell-bound AAD the store writes. A bit-flipped AEAD cell passes quick_check and every row count, so this is the half that makes the check worth running. VerifyResult now carries decrypted_cells -- a count, never a plaintext. 3. Bind the store before the try in _full_open_check, so an open that raises surfaces its own cause rather than a cleanup error over the top of it. 4. ADR 0049: correct the full_restore_verify prose, which claimed the open ran through the cipher, and add AC-13. Scope of the decrypt pass, stated rather than implied: it covers the store's own id-keyed _CIPHER_COLUMNS declaration. The cipher-covered tables whose AAD binds to a composite or natural key are enumerated only as code inside the store's bespoke passes, so widening this means giving the store one declaration to publish -- a separate change. --- .../0049-turnkey-dr-backup-restore-verify.md | 37 +++- messagefoundry/__main__.py | 12 +- messagefoundry/pipeline/dr_backup.py | 157 +++++++++++++++-- tests/test_restore_verify.py | 159 +++++++++++++++++- 4 files changed, 340 insertions(+), 25 deletions(-) diff --git a/docs/adr/0049-turnkey-dr-backup-restore-verify.md b/docs/adr/0049-turnkey-dr-backup-restore-verify.md index 0a6d55429..2b3a253e7 100644 --- a/docs/adr/0049-turnkey-dr-backup-restore-verify.md +++ b/docs/adr/0049-turnkey-dr-backup-restore-verify.md @@ -239,7 +239,7 @@ retention_keep = 7 # keep-N: prune the oldest archives beyond N af snapshot_method = "vacuum_into" # "vacuum_into" (default, writer-lock under off-peak schedule) | "online_backup" (low-contention) include_config = true # bundle the loaded --config dir into the archive verify_after_backup = true # run the lightweight restore-verify after each backup (default ON) -full_restore_verify = false # the heavier restore-to-temp through open_store; on-demand / opt-in extra +full_restore_verify = false # the heavier open through open_store + a decrypt pass; on-demand / opt-in extra config_only_on_server_db = true # on postgres/sqlserver, back up config only; the DB is DBA-delegated (#52) allow_unencrypted = false # audited escape: permit a clear archive ONLY for a no-key synthetic instance ``` @@ -325,9 +325,28 @@ The owner-locked posture is **lightweight verification after each backup**, with the keep-N prune does not count it as the latest good backup** (so a failing backup never silently evicts the last *good* one). -`full_restore_verify` (opt-in / on-demand) additionally restores the snapshot to a throwaway temp DB and opens -it through the real `open_store` path (cipher + migrations) to prove an end-to-end restore — heavier, so not -the per-backup default. +### Full restore-verify (opt-in / on-demand) + +`full_restore_verify` additionally opens the extracted snapshot through the real `open_store` path — **under +this instance's LIVE `[store]` settings, with only the path and the backend substituted** — and then decrypts +**and authenticates** every cipher-covered cell it holds. Heavier, so not the per-backup default. + +Two properties of that sentence are load-bearing, and the shipped code got both wrong until they were named +here: + +- **The settings must be the live ones.** A bare `StoreSettings(path=…)` resolves no key, so an encrypted + snapshot opens under the identity cipher. Substituting *only* the path is what carries `cipher_provider`, + `key_provider`, the active + retired keyring and `aad_bind` into the verify, and it is why this is a + `model_copy` rather than a rebuilt object: a field added to `StoreSettings` later rides along instead of + being silently dropped. `backend` is the one other substitution — the archive member is a SQLite file by + construction, so an instance that has since moved to a server DB still verifies its older SQLite archive + against SQLite. +- **Opening the store is not reading the PHI.** `PRAGMA quick_check` and the row counts are blind to a + bit-flipped AEAD cell, so without the decrypt pass a full verify would report `PASS` on an archive whose + bodies no longer decrypt. Each cell is opened with the same cell-bound AAD the store writes (ASVS 11.3.3), + and the result reports a **count** of cells opened — never a plaintext — so a `PASS` states which claim it + is making. The covered cells are the store's own `_CIPHER_COLUMNS` declaration; its cipher-covered tables + whose AAD binds to a composite/natural key are out of scope until the store publishes them as data too. ## Acceptance Criteria @@ -386,6 +405,16 @@ the per-backup default. - **AC-12** — WHILE clustered (active-passive HA) AND not the leader, THE SYSTEM SHALL NOT take a backup or prune the shared destination; WHILE single-node (`NullCoordinator`), THE SYSTEM SHALL always run. → `tests/test_backup_runner.py::test_backup_is_leader_gated` +- **AC-13** — WHEN `full_restore_verify` runs, THE SYSTEM SHALL open the extracted snapshot under this + instance's live `[store]` settings (only the path and the backend substituted) AND decrypt **and + authenticate** every cipher-covered cell in it, reporting the number of cells opened; IF a cell fails its + AEAD tag, OR the settings resolve no key for a snapshot that holds sealed cells, OR no live settings are + supplied, THEN THE SYSTEM SHALL return `FAIL` naming that cause. A good encrypted archive SHALL verify + `PASS`, and a good unencrypted archive SHALL verify `PASS` with zero cells opened. + → `tests/test_restore_verify.py::test_full_verify_passes_on_a_good_encrypted_archive` + → `tests/test_restore_verify.py::test_full_verify_fails_on_a_corrupted_aead_cell` + → `tests/test_restore_verify.py::test_full_verify_fails_when_the_snapshot_opens_without_its_key` + → `tests/test_restore_verify.py::test_full_verify_passes_on_a_good_unencrypted_archive` ## Options considered diff --git a/messagefoundry/__main__.py b/messagefoundry/__main__.py index 05977389b..427e27795 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -4950,8 +4950,10 @@ def _restore_verify(args: argparse.Namespace) -> int: """Verify an existing ``.mfbak`` archive WITHOUT activating it (ADR 0049, #60 — 0049's owned primitive that ADR 0048's cold-seed activation calls): key-fingerprint precheck (a clean ``KEY_MISMATCH`` before any decrypt) -> decrypt -> open the embedded store read-only -> - ``integrity_check`` + per-table row-count vs the manifest. Reports ``PASS``/``FAIL``/ - ``KEY_MISMATCH``; PHI-safe (counts + a reason only, never a body).""" + ``integrity_check`` + per-table row-count vs the manifest. ``--full`` additionally re-opens the + snapshot under THIS instance's real store settings (cipher, keyring, key provider) and decrypts + + authenticates its cipher-covered cells. Reports ``PASS``/``FAIL``/``KEY_MISMATCH``; PHI-safe (counts + + a reason only, never a body).""" import asyncio from pathlib import Path @@ -4978,6 +4980,10 @@ def _restore_verify(args: argparse.Namespace) -> int: "integrity_ok": result.integrity_ok, "row_counts": result.row_counts, "manifest_counts": result.manifest_counts, + # A count of the cipher-covered cells --full decrypted AND authenticated (0 on a lightweight + # verify, and on an unencrypted store). It is what separates "the snapshot opened" from "its + # PHI was readable", so an operator can see which claim a PASS is making. + "decrypted_cells": result.decrypted_cells, "reason": result.reason, } if args.json: @@ -4986,6 +4992,8 @@ def _restore_verify(args: argparse.Namespace) -> int: print(f"{result.status}: {result.reason or 'archive verified'}") if result.row_counts: print(f" row_counts={result.row_counts}") + if result.decrypted_cells: + print(f" decrypted_cells={result.decrypted_cells}") # exit 0 only on PASS; FAIL/KEY_MISMATCH are non-zero so a script/cold-seed activation can gate on it. return 0 if result.ok else 1 diff --git a/messagefoundry/pipeline/dr_backup.py b/messagefoundry/pipeline/dr_backup.py index 48ac435b1..ff56f1263 100644 --- a/messagefoundry/pipeline/dr_backup.py +++ b/messagefoundry/pipeline/dr_backup.py @@ -40,11 +40,11 @@ from dataclasses import dataclass, field from pathlib import Path -from messagefoundry.config.settings import BackupSettings, StoreBackend +from messagefoundry.config.settings import BackupSettings, StoreBackend, StoreSettings from messagefoundry.pipeline.alerts import AlertSink, LoggingAlertSink from messagefoundry.pipeline.cluster import ClusterCoordinator, NullCoordinator from messagefoundry.redaction import safe_exc -from messagefoundry.store import Store +from messagefoundry.store import MessageStore, Store from messagefoundry.store.backup_codec import ( FORMAT_VERSION, BackupCodecError, @@ -56,9 +56,11 @@ ) from messagefoundry.store.base import ( DbaDelegatedError, + build_store_cipher, resolve_active_key, resolve_decrypt_keys, ) +from messagefoundry.store.crypto import MARKER_PREFIX, CipherError, cell_aad __all__ = [ "BackupRunner", @@ -108,6 +110,11 @@ class VerifyResult: row_counts: dict[str, int] = field(default_factory=dict) manifest_counts: dict[str, int] = field(default_factory=dict) reason: str | None = None + #: How many cipher-covered cells the FULL verify decrypted AND authenticated in the snapshot. A + #: count, never a plaintext. ``0`` on a lightweight verify (it does not run the pass) and on an + #: unencrypted store (there is nothing sealed to open) — so it distinguishes "read the PHI" from + #: "opened the file", which is the whole difference a full verify is meant to prove. + decrypted_cells: int = 0 @property def ok(self) -> bool: @@ -353,6 +360,9 @@ async def _do_backup(self, now: float, *, force_config_only: bool = False) -> Ba keys=[key] if key is not None else [], full=s.full_restore_verify, allow_unencrypted=s.allow_unencrypted, + # The LIVE store settings, so a full verify opens the snapshot under this instance's + # real cipher/keyring/provider rather than a bare default (see _full_open_check). + store_settings=self._store_settings, ) if not verify.ok: # A verify FAIL means the archive is unusable — but it must NOT be counted as the latest @@ -534,6 +544,9 @@ async def _record_success(self, result: BackupResult, now: float) -> None: "row_counts": result.row_counts, "verify": verify.status if verify is not None else "skipped", "verify_integrity_ok": verify.integrity_ok if verify is not None else None, + # A COUNT of cipher-covered cells the full verify opened — PHI-free, and the one field that + # distinguishes "the snapshot opened" from "its PHI was readable". + "verify_decrypted_cells": verify.decrypted_cells if verify is not None else None, "pruned": result.pruned, } await self._store.record_audit( @@ -644,8 +657,23 @@ def _select_decrypt_key(keys: list[bytes], header_key_id: str) -> bytes | None: return None +def _as_store_settings(settings: object) -> StoreSettings | None: + """Narrow the loosely-typed ``store_settings`` seam to the real model, or ``None``. + + The public entry points (:class:`BackupRunner`, :func:`run_restore_verify`) type it as ``object``, + and that annotation is left alone so no caller has to change. The FULL verify needs the real model + to reach the cipher/keyring/provider, and anything else must fail the verify rather than quietly + fall back to defaults.""" + return settings if isinstance(settings, StoreSettings) else None + + def _verify_archive_blocking( - *, archive_path: str, keys: list[bytes], full: bool, allow_unencrypted: bool = False + *, + archive_path: str, + keys: list[bytes], + full: bool, + allow_unencrypted: bool = False, + store_settings: object | None = None, ) -> VerifyResult: """Lightweight (or full) restore-verify of a ``.mfbak`` archive — runs OFF the event loop. @@ -659,10 +687,15 @@ def _verify_archive_blocking( archive for an unauthenticated one) and is refused here as ``KEY_MISMATCH`` — unless ``allow_unencrypted`` is set for a synthetic/no-PHI box. + ``store_settings`` is this instance's live :class:`StoreSettings`. It is what a ``full`` verify opens + the snapshot with, so the snapshot is read under the real cipher, keyring and key provider; without + it a ``full`` verify FAILs rather than opening keyless (see :func:`_full_open_check`). + Steps (ADR 0049): (1) key-fingerprint precheck — a mismatch is a clean ``KEY_MISMATCH`` BEFORE any decrypt; (2) decrypt the archive; (3) extract + open ``store.db`` read-only, run ``PRAGMA integrity_check``; (4) compare per-table row counts to the manifest. ``full`` additionally - re-opens the snapshot through the real ``open_store`` path (cipher + migrations) — heavier.""" + re-opens the snapshot through the real ``open_store`` path (cipher + migrations) and decrypts + + authenticates every cipher-covered cell in it — heavier.""" try: # (1) Pre-decryption key check (only meaningful for an encrypted archive). For a plaintext # archive (no codec header) there is no key to mismatch. @@ -745,10 +778,14 @@ def _verify_archive_blocking( manifest_counts=manifest_counts, reason=f"row-count mismatch: snapshot={row_counts} manifest={manifest_counts}", ) + decrypted_cells = 0 if full: # The heavier end-to-end restore: open the snapshot through the real open_store path - # (cipher + migrations) to prove it restores, then discard it. - full_ok, full_msg = _full_open_check(snap) + # (cipher + migrations) to prove it restores, decrypt + authenticate its PHI, then + # discard it. + full_ok, full_msg, decrypted_cells = _full_open_check( + snap, _as_store_settings(store_settings) + ) if not full_ok: return VerifyResult( "FAIL", @@ -762,6 +799,7 @@ def _verify_archive_blocking( integrity_ok=True, row_counts=row_counts, manifest_counts=manifest_counts, + decrypted_cells=decrypted_cells, ) except BackupKeyMismatch as exc: return VerifyResult("KEY_MISMATCH", reason=safe_exc(exc)) @@ -802,6 +840,9 @@ async def run_restore_verify( keys=keys, full=full, allow_unencrypted=allow_unencrypted, + # Threaded through so a full verify opens the snapshot under the SAME cipher/keyring/provider + # the keyring above was resolved from, instead of a bare default (see _full_open_check). + store_settings=store_settings, ) @@ -863,24 +904,110 @@ def _integrity_check(db_path: Path) -> tuple[bool, str]: return ok, "ok" if ok else "; ".join(results)[:500] -def _full_open_check(snap: Path) -> tuple[bool, str]: - """Open the snapshot through the real ``open_store`` path (cipher + migrations) on a copy, to prove - an end-to-end restore. Heavier; only run for ``full_restore_verify``.""" - from messagefoundry.config.settings import StoreSettings +def _full_open_check(snap: Path, settings: StoreSettings | None) -> tuple[bool, str, int]: + """Open the snapshot through the real ``open_store`` path, then decrypt + authenticate its PHI. + Returns ``(ok, message, decrypted_cells)``. Heavier; only run for ``full_restore_verify``. + + ``settings`` must be the LIVE store settings, with **only** the path (and the backend, below) + substituted. A bare ``StoreSettings(path=...)`` resolves no key, so an ENCRYPTED snapshot opens under + the identity cipher: the open succeeds, ``quick_check`` passes, and the verify reports PASS having + proved nothing about whether a single PHI cell is readable. Substituting only the path is what keeps + every field that governs HOW the bytes are read — ``cipher_provider``, ``key_provider``, the active + + retired keyring, ``aad_bind`` — and it is why this is a ``model_copy`` rather than a rebuilt object: + a field added to ``StoreSettings`` later rides along instead of being silently dropped. + + ``backend`` is the one other substitution. The extracted member is a SQLite file by construction + (``Store.snapshot_to`` writes one, and the integrity/row-count steps above already read it with + ``sqlite3``), so an instance that has since moved to a server DB must still verify its older SQLite + archive against SQLite rather than dialling Postgres/SQL Server with a file path.""" from messagefoundry.store.base import open_store + if settings is None: + # Fail-closed. The only thing available without the live settings is a keyless open, and a PASS + # from one says nothing about an encrypted archive — which is the archive worth verifying. + return False, "no live store settings were supplied for the full restore-verify", 0 + snap_settings = settings.model_copy(update={"path": str(snap), "backend": StoreBackend.SQLITE}) + async def _open() -> tuple[bool, str]: - store = await open_store(StoreSettings(path=str(snap))) + # Bind the store BEFORE the try. An open that raises — a keyless or wrong-key open, an + # unreachable key provider — must surface ITS OWN cause, not a NameError from a finally closing + # a store that was never created, and not the temp-directory cleanup error a leaked handle + # raises over the top of it on Windows one frame up. + store = await open_store(snap_settings) try: - ok, msg = await store.integrity_check() - return ok, msg + return await store.integrity_check() finally: await store.close() try: - return asyncio.run(_open()) + ok, msg = asyncio.run(_open()) except Exception as exc: # a restore that won't even open is the thing we're trying to catch - return False, safe_exc(exc) + return False, safe_exc(exc), 0 + if not ok: + return False, msg, 0 + return _decrypt_check(snap, snap_settings) + + +def _decrypt_check(snap: Path, settings: StoreSettings) -> tuple[bool, str, int]: + """Decrypt AND authenticate every cipher-covered cell retained in the snapshot, under the store's own + cipher. Returns ``(ok, message, cells)`` — a COUNT and a PHI-free reason, never a plaintext. + + Opening the store proves the file is a readable SQLite database. It does not prove the PHI inside it + is readable, and those are the two different claims a disaster-recovery check gets confused about: a + bit-flipped AEAD cell passes ``PRAGMA quick_check`` and every row count, so without this pass a full + verify would report PASS on an archive whose bodies no longer decrypt. Each value is opened with the + same cell-bound AAD the store writes (ASVS 11.3.3), so a ciphertext moved between cells fails its tag + here exactly as it would at a live read. + + The cell list is the store's own ``MessageStore._CIPHER_COLUMNS`` — what the store declares + encrypted-at-rest — rather than a list invented here, so a column added to the cipher's coverage is + covered by this pass without a second edit. Its cipher-covered tables whose AAD binds to a + composite/natural key (``response``, ``state``, ``reference``, ``shared_body``, ``attachment_chunk``, + ``message_events``, ``connection_event``, ``alert_instance``) are NOT in that tuple and are therefore + out of scope here: each is a bespoke pass inside the store rather than data any caller can read. + Widening this means giving the store one declaration to publish, not copying its private passes into + this module.""" + import sqlite3 + + cipher = build_store_cipher(settings) + cells = 0 + conn = sqlite3.connect(f"file:{snap}?mode=ro", uri=True) + try: + tables = {r[0] for r in conn.execute("SELECT name FROM sqlite_master WHERE type='table'")} + for table, column in MessageStore._CIPHER_COLUMNS: + if table not in tables: + continue # an older snapshot predating the table — not a verify failure + names = {r[1] for r in conn.execute(f"PRAGMA table_info({table})")} # constant table + if column not in names or "id" not in names: + continue + # table/column are constants from _CIPHER_COLUMNS; only the marker prefix is a parameter. + rows = conn.execute( + f"SELECT id, {column} FROM {table} WHERE {column} LIKE ?", (f"{MARKER_PREFIX}%",) + ) + for row_id, stored in rows: + try: + plain = cipher.decrypt(str(stored), aad=cell_aad(table, column, row_id)) + except CipherError as exc: + return ( + False, + f"{table}.{column} id={row_id} did not decrypt: {safe_exc(exc)}", + cells, + ) + if cipher.is_encrypted(plain): + # The identity cipher hands an mfenc: value straight back (the #241 F2 keyless-open + # trap): the snapshot holds sealed cells and this open resolved no key for them. + return ( + False, + f"{table}.{column} is encrypted at rest but the store settings resolved no key " + "to open it (keyless open of an encrypted snapshot)", + cells, + ) + cells += 1 + except sqlite3.Error as exc: + return False, safe_exc(exc), cells + finally: + conn.close() + return True, "ok", cells def _read_manifest_from_tar(tar_path: Path) -> dict[str, object]: diff --git a/tests/test_restore_verify.py b/tests/test_restore_verify.py index 16009fa76..172637f3d 100644 --- a/tests/test_restore_verify.py +++ b/tests/test_restore_verify.py @@ -3,19 +3,31 @@ """restore-verify (ADR 0049 AC-5): the key-fingerprint precheck returns a clean KEY_MISMATCH BEFORE any decrypt; a matching key (active OR a retired key still in the keyring after a rotation) decrypts + opens the embedded store read-only + integrity_check + row-count compare (PASS); a corrupted archive is FAIL. -(The at-least-once-across-restore case, AC-11, lives in ``test_backup_restore_atleastonce.py``.)""" +(The at-least-once-across-restore case, AC-11, lives in ``test_backup_restore_atleastonce.py``.) + +The FULL verify (AC-13) opens the snapshot under the instance's LIVE store settings and decrypts + +authenticates its cipher-covered cells, so a PASS means the PHI was readable, not merely that a SQLite +file opened.""" from __future__ import annotations +import base64 +import sqlite3 from pathlib import Path from messagefoundry.config.settings import BackupSettings, StoreSettings -from messagefoundry.pipeline.dr_backup import BackupRunner, run_restore_verify +from messagefoundry.pipeline.dr_backup import ( + BackupRunner, + _verify_archive_blocking, + run_restore_verify, +) from messagefoundry.store import MessageStore from messagefoundry.store.crypto import generate_key, make_cipher -async def _backup(tmp_path: Path, key_b64: str | None) -> tuple[MessageStore, str, StoreSettings]: +async def _backup( + tmp_path: Path, key_b64: str | None, *, allow_unencrypted: bool = False +) -> tuple[MessageStore, str, StoreSettings]: cipher = make_cipher(key_b64) if key_b64 else None store = await MessageStore.open(tmp_path / "msg.db", cipher=cipher) await store.enqueue_message( @@ -28,7 +40,11 @@ async def _backup(tmp_path: Path, key_b64: str | None) -> tuple[MessageStore, st ss = StoreSettings(path=str(tmp_path / "msg.db"), encryption_key=key_b64) runner = BackupRunner( store, - BackupSettings(enabled=True, destination=str(tmp_path / "b")), + BackupSettings( + enabled=True, + destination=str(tmp_path / "b"), + allow_unencrypted=allow_unencrypted, + ), store_settings=ss, config_dir=None, ) @@ -37,6 +53,29 @@ async def _backup(tmp_path: Path, key_b64: str | None) -> tuple[MessageStore, st return store, result.archive_path, ss +def _flip_one_aead_byte(db: Path, table: str, column: str) -> None: + """Flip one bit inside a stored AEAD blob so the cell still PARSES as an ``mfenc:`` value but fails + its GCM tag — the shape bit rot takes on an encrypted PHI cell. SQLite's own integrity check and + every row count are blind to it, which is exactly why the verify has to open the cell itself.""" + conn = sqlite3.connect(db) + try: + row = conn.execute( + f"SELECT id, {column} FROM {table} WHERE {column} LIKE 'mfenc:%'" # constants + ).fetchone() + assert row is not None, f"no encrypted cell in {table}.{column} to corrupt" + row_id, stored = row + head, _, payload = str(stored).rpartition(":") + blob = bytearray(base64.b64decode(payload)) + blob[-1] ^= 0x01 # the last byte is inside the GCM tag + conn.execute( + f"UPDATE {table} SET {column} = ? WHERE id = ?", # constants + (f"{head}:{base64.b64encode(bytes(blob)).decode()}", row_id), + ) + conn.commit() + finally: + conn.close() + + async def test_verify_pass_failclosed_and_key_mismatch(tmp_path) -> None: key_b64 = generate_key() store, archive, ss = await _backup(tmp_path, key_b64) @@ -96,6 +135,118 @@ async def test_full_restore_verify_opens_through_open_store(tmp_path) -> None: await store.close() +# --- AC-13: the FULL verify opens the snapshot under the LIVE store settings -------------------- + + +async def test_full_verify_passes_on_a_good_encrypted_archive(tmp_path) -> None: + """The regression this fix exists for. A good ENCRYPTED archive must verify PASS under ``full``, and + the PASS must be the strong claim: the snapshot's cipher-covered cells were opened and authenticated + under the live keyring. Building a bare ``StoreSettings`` for the open cannot satisfy both halves — + it opens keyless, so either the decrypt pass fails the archive or there is no decrypt pass at all.""" + key_b64 = generate_key() + store, archive, ss = await _backup(tmp_path, key_b64) + + res = await run_restore_verify(archive, store_settings=ss, full=True) + assert res.status == "PASS", res.reason + assert res.integrity_ok is True + assert res.decrypted_cells >= 1, "a full verify that decrypted nothing proves nothing" + await store.close() + + +async def test_full_verify_fails_on_a_corrupted_aead_cell(tmp_path) -> None: + """A bit-flipped AEAD cell in the snapshot must be FAIL. It survives ``PRAGMA quick_check`` and the + manifest row counts untouched, so only decrypting the cell catches it.""" + key_b64 = generate_key() + db = tmp_path / "msg.db" + store = await MessageStore.open(db, cipher=make_cipher(key_b64)) + await store.enqueue_message( + channel_id="c1", + raw="MSH|^~\\&|x", + deliveries=[("d1", "OUT|y")], + control_id="CID-1", + now=1.0, + ) + await store.close() + _flip_one_aead_byte(db, "messages", "raw") + + store = await MessageStore.open(db, cipher=make_cipher(key_b64)) + ss = StoreSettings(path=str(db), encryption_key=key_b64) + runner = BackupRunner( + store, + BackupSettings(enabled=True, destination=str(tmp_path / "b")), + store_settings=ss, + config_dir=None, + ) + result = await runner.run_once(now=1.0) + assert result is not None + await store.close() + + # The lightweight verify is blind to it by design — it never opens a cell. + light = await run_restore_verify(result.archive_path, store_settings=ss) + assert light.status == "PASS" + + res = await run_restore_verify(result.archive_path, store_settings=ss, full=True) + assert res.status == "FAIL" + assert "messages.raw" in (res.reason or "") and "did not decrypt" in (res.reason or "") + + +def test_full_verify_fails_when_the_snapshot_opens_without_its_key(tmp_path) -> None: + """A keyless or wrong-key open of an ENCRYPTED snapshot is FAIL, with the real cause named. + + This calls the blocking verify directly because that is the only way to reach the shipped defect's + shape: the codec key is in hand (the archive itself decrypts fine), while the settings threaded into + the full open resolve no key — which is what a bare ``StoreSettings(path=...)`` did. The snapshot + then opens under the identity cipher, its PHI cells stay sealed, and a PASS there would be false. + + Synchronous on purpose: ``_verify_archive_blocking`` runs ``asyncio.run`` for the full open, so it + needs a thread with no loop of its own — which is how the engine calls it (``asyncio.to_thread``).""" + import asyncio + + key_b64 = generate_key() + + async def _setup() -> str: + store, archive, _ = await _backup(tmp_path, key_b64) + await store.close() + return archive + + archive = asyncio.run(_setup()) + codec_key = base64.b64decode(key_b64) + + keyless = _verify_archive_blocking( + archive_path=archive, + keys=[codec_key], + full=True, + store_settings=StoreSettings(path="unused"), + ) + assert keyless.status == "FAIL" + assert "keyless open" in (keyless.reason or ""), keyless.reason + + wrong = _verify_archive_blocking( + archive_path=archive, + keys=[codec_key], + full=True, + store_settings=StoreSettings(path="unused", encryption_key=generate_key()), + ) + assert wrong.status == "FAIL" + assert "did not decrypt" in (wrong.reason or ""), wrong.reason + + # And with no settings at all the full verify refuses rather than falling back to a keyless open. + absent = _verify_archive_blocking(archive_path=archive, keys=[codec_key], full=True) + assert absent.status == "FAIL" + assert "no live store settings" in (absent.reason or ""), absent.reason + + +async def test_full_verify_passes_on_a_good_unencrypted_archive(tmp_path) -> None: + """A no-key (synthetic / no-PHI) instance still verifies PASS under ``full``. Nothing is sealed, so + the decrypt pass has nothing to open and reports zero cells — a PASS that claims exactly that.""" + store, archive, ss = await _backup(tmp_path, None, allow_unencrypted=True) + res = await run_restore_verify(archive, store_settings=ss, full=True, allow_unencrypted=True) + assert res.status == "PASS", res.reason + assert res.integrity_ok is True + assert res.decrypted_cells == 0 + await store.close() + + async def test_verify_missing_archive_is_reported(tmp_path) -> None: ss = StoreSettings(path=str(tmp_path / "msg.db"), encryption_key=generate_key()) res = await run_restore_verify(str(tmp_path / "nope.mfbak"), store_settings=ss) From d34676f557682325eee9eb9d3363d325e538df84 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 14 Sep 2026 19:20:58 -0500 Subject: [PATCH 2/3] fix(dr): a keyless full restore-verify is KEY_MISMATCH, and a failed open closes its handle (BACKLOG #1718) Builds on the commit beneath it, which threaded the live store settings into the full verify and added the decrypt pass. Three things it left open. 1. The status. A snapshot holding sealed cells that the settings resolve no key for was FAIL, which sends an operator looking for a bad archive when the archive is fine and the key configuration is not. It is now KEY_MISMATCH, raised from both places the condition surfaces: the decrypt pass, and the StoreKeylessError the store's own eager state/reference warm-ups raise first. A failed AEAD tag stays FAIL -- CipherError cannot separate bit rot from a key that was never supplied, and bit rot is the reading that must not be softened. 2. The leaked handle. MessageStore.open left its aiosqlite connection open when a warm-up raised, so the verify's temp-directory cleanup was refused and its PermissionError replaced the real cause. Measured here: without the guard the new test reports WinError 32 on the extracted snapshot instead of the missing key. 3. The case the item was filed for. No test held a state or reference row -- the rows the store decrypts eagerly at open, and so the ones that turned every scheduled backup of a keyed store into a failing verify. ADR 0049 AC-13 is amended for the status split and AC-14 added for the close; the CONFIGURATION.md row for full_restore_verify now says what the leg proves. The crypto-inventory gate (ASVS 11.1.3) also had to be told that dr_backup.py now imports the store-cipher seam. That import arrives with the commit below, so this branch reds that required leg without the row. --- docs/CONFIGURATION.md | 2 +- .../0049-turnkey-dr-backup-restore-verify.md | 21 +++- messagefoundry/pipeline/dr_backup.py | 62 +++++++--- messagefoundry/store/store.py | 93 ++++++++++----- scripts/security/crypto_inventory_check.py | 8 +- tests/test_restore_verify.py | 111 +++++++++++++++++- 6 files changed, 242 insertions(+), 55 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index a8a281002..a98104023 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1471,7 +1471,7 @@ DBA-delegated (#52): config-only, or skipped, per `config_only_on_server_db`. | `snapshot_method` | str | `vacuum_into` | `vacuum_into` (default; takes a writer lock, sized for the off-peak schedule) or `online_backup` (low-contention, page-batched) | | `include_config` | bool | `true` | bundle the loaded `--config` dir into the archive, so the cold seed is self-sufficient (store **plus** the config that interprets it) without assuming the DR box can reach the org's git repo | | `verify_after_backup` | bool | `true` | run the lightweight restore-verify after every backup (open + `integrity_check` + row-count). On by default — a backup nobody has opened is a backup that silently doesn't restore | -| `full_restore_verify` | bool | `false` | the heavier verify: restore the snapshot to a throwaway temp DB and open it through the real `open_store` path. On-demand / opt-in extra, deliberately **not** the per-backup default | +| `full_restore_verify` | bool | `false` | the heavier verify: restore the snapshot to a throwaway temp DB, open it through the real `open_store` path **under this instance's live `[store]` settings** (only the path and the backend substituted), then decrypt and authenticate its cipher-covered cells and report how many were opened. A snapshot holding sealed cells that these settings resolve no key for is reported `KEY_MISMATCH`, not `FAIL` — the archive is fine, the key configuration is not. On-demand / opt-in extra, deliberately **not** the per-backup default | | `config_only_on_server_db` | bool | `true` | on a Postgres/SQL Server store the DB backup is DBA-delegated (#52), so back up the **config bundle only**. `false` = skip the backup entirely on a server-DB store (not even a config-only archive) | | `allow_unencrypted` | bool | `false` | audited escape permitting a **cleartext** archive on a **no-key** instance (the parallel of `[security].allow_unencrypted_phi`). Left `false`, a keyless instance **refuses** to write the archive rather than putting message bodies on disk in the clear. **This row used to say a PHI instance refuses regardless of the flag. That was never true of the code** — `BackupRunner` reads the key and this flag and nothing else, so on a keyless instance setting it would write a plaintext archive. Every instance carries patient data now ([ADR 0186](adr/0186-retire-the-synthetic-data-declaration-every-instance-carries-patient-data.md)), so configure `MEFOR_STORE_ENCRYPTION_KEY` instead of reaching for this | diff --git a/docs/adr/0049-turnkey-dr-backup-restore-verify.md b/docs/adr/0049-turnkey-dr-backup-restore-verify.md index 2b3a253e7..73b38571c 100644 --- a/docs/adr/0049-turnkey-dr-backup-restore-verify.md +++ b/docs/adr/0049-turnkey-dr-backup-restore-verify.md @@ -408,14 +408,29 @@ here: - **AC-13** — WHEN `full_restore_verify` runs, THE SYSTEM SHALL open the extracted snapshot under this instance's live `[store]` settings (only the path and the backend substituted) AND decrypt **and authenticate** every cipher-covered cell in it, reporting the number of cells opened; IF a cell fails its - AEAD tag, OR the settings resolve no key for a snapshot that holds sealed cells, OR no live settings are - supplied, THEN THE SYSTEM SHALL return `FAIL` naming that cause. A good encrypted archive SHALL verify - `PASS`, and a good unencrypted archive SHALL verify `PASS` with zero cells opened. + AEAD tag, OR no live settings are supplied, THEN THE SYSTEM SHALL return `FAIL` naming that cause; IF the + settings resolve **no** key for a snapshot that holds sealed cells, THEN THE SYSTEM SHALL return + `KEY_MISMATCH` naming that cause. A good encrypted archive SHALL verify `PASS` — including one holding + `state` or `reference` rows, which the store decrypts eagerly at open — and a good unencrypted archive + SHALL verify `PASS` with zero cells opened. → `tests/test_restore_verify.py::test_full_verify_passes_on_a_good_encrypted_archive` + → `tests/test_restore_verify.py::test_full_verify_passes_on_a_snapshot_holding_state_and_reference_rows` → `tests/test_restore_verify.py::test_full_verify_fails_on_a_corrupted_aead_cell` → `tests/test_restore_verify.py::test_full_verify_fails_when_the_snapshot_opens_without_its_key` → `tests/test_restore_verify.py::test_full_verify_passes_on_a_good_unencrypted_archive` + **Why the keyless case is `KEY_MISMATCH` and a failed tag is not.** With no key resolved, nothing could + have opened those cells: the archive is fine and the operator's key configuration is not, so `FAIL` would + send them looking for a bad backup. A keyring that *does* hold keys and still cannot open a cell is a + different matter — `CipherError` cannot separate a corrupted ciphertext from a key that was never + supplied, and bit rot on a PHI cell is the reading that must not be talked down. + +- **AC-14** — IF the full open FAILS, THEN THE SYSTEM SHALL report the cause of the failed open. The + snapshot is opened inside a temp directory the verify unwinds on the way out, and `MessageStore.open` + closes its connection when a warm-up raises, so the open's own error is what reaches the operator rather + than a Windows `PermissionError` from the cleanup of a file a leaked handle still held. + → `tests/test_restore_verify.py::test_full_verify_on_a_failed_open_reports_the_open_error_not_a_cleanup_error` + ## Options considered 1. **Engine-managed consistent SQLite snapshot + config bundle, encrypted with a chunked-AEAD codec keyed by diff --git a/messagefoundry/pipeline/dr_backup.py b/messagefoundry/pipeline/dr_backup.py index ff56f1263..52b2d08f4 100644 --- a/messagefoundry/pipeline/dr_backup.py +++ b/messagefoundry/pipeline/dr_backup.py @@ -60,7 +60,12 @@ resolve_active_key, resolve_decrypt_keys, ) -from messagefoundry.store.crypto import MARKER_PREFIX, CipherError, cell_aad +from messagefoundry.store.crypto import ( + MARKER_PREFIX, + CipherError, + StoreKeylessError, + cell_aad, +) __all__ = [ "BackupRunner", @@ -695,7 +700,9 @@ def _verify_archive_blocking( decrypt; (2) decrypt the archive; (3) extract + open ``store.db`` read-only, run ``PRAGMA integrity_check``; (4) compare per-table row counts to the manifest. ``full`` additionally re-opens the snapshot through the real ``open_store`` path (cipher + migrations) and decrypts + - authenticates every cipher-covered cell in it — heavier.""" + authenticates every cipher-covered cell in it — heavier. That leg returns its own + ``KEY_MISMATCH`` when the settings resolve no key for a snapshot that holds sealed cells, so an + archive that is fine and a key configuration that is not are not both reported as ``FAIL``.""" try: # (1) Pre-decryption key check (only meaningful for an encrypted archive). For a plaintext # archive (no codec header) there is no key to mismatch. @@ -783,16 +790,18 @@ def _verify_archive_blocking( # The heavier end-to-end restore: open the snapshot through the real open_store path # (cipher + migrations) to prove it restores, decrypt + authenticate its PHI, then # discard it. - full_ok, full_msg, decrypted_cells = _full_open_check( + full_status, full_msg, decrypted_cells = _full_open_check( snap, _as_store_settings(store_settings) ) - if not full_ok: + if full_status != "PASS": return VerifyResult( - "FAIL", + full_status, integrity_ok=True, row_counts=row_counts, manifest_counts=manifest_counts, - reason=f"full restore-open failed: {full_msg}", + # No status word in the prefix: the caller that turns this into a BackupError + # already prints `verify.status`, and repeating it reads as two verdicts. + reason=f"full restore-verify: {full_msg}", ) return VerifyResult( "PASS", @@ -904,9 +913,18 @@ def _integrity_check(db_path: Path) -> tuple[bool, str]: return ok, "ok" if ok else "; ".join(results)[:500] -def _full_open_check(snap: Path, settings: StoreSettings | None) -> tuple[bool, str, int]: +def _full_open_check(snap: Path, settings: StoreSettings | None) -> tuple[str, str, int]: """Open the snapshot through the real ``open_store`` path, then decrypt + authenticate its PHI. - Returns ``(ok, message, decrypted_cells)``. Heavier; only run for ``full_restore_verify``. + Returns ``(status, message, decrypted_cells)`` — the ``VerifyResult`` status this leg earned, so the + caller can hand it straight on. Heavier; only run for ``full_restore_verify``. + + ``KEY_MISMATCH`` is reserved for the one cause the keyring is unambiguously to blame for: the + snapshot holds sealed (``mfenc:``) cells and these settings resolve **no** key for them, so nothing + could have opened them. An operator reading that fixes their key configuration; reading ``FAIL`` + they would go looking for a bad archive, and the archive is fine. A cell that will not decrypt under + a keyring that DOES hold keys stays ``FAIL``, because :class:`CipherError` cannot tell a corrupted + ciphertext from a key that was never supplied (see its own docstring) and corruption is the reading + that must not be talked down. ``settings`` must be the LIVE store settings, with **only** the path (and the backend, below) substituted. A bare ``StoreSettings(path=...)`` resolves no key, so an ENCRYPTED snapshot opens under @@ -925,7 +943,7 @@ def _full_open_check(snap: Path, settings: StoreSettings | None) -> tuple[bool, if settings is None: # Fail-closed. The only thing available without the live settings is a keyless open, and a PASS # from one says nothing about an encrypted archive — which is the archive worth verifying. - return False, "no live store settings were supplied for the full restore-verify", 0 + return "FAIL", "no live store settings were supplied for the full restore-verify", 0 snap_settings = settings.model_copy(update={"path": str(snap), "backend": StoreBackend.SQLITE}) async def _open() -> tuple[bool, str]: @@ -941,16 +959,21 @@ async def _open() -> tuple[bool, str]: try: ok, msg = asyncio.run(_open()) + except StoreKeylessError as exc: + # The store's own eager `state`/`reference` warm-ups fail closed on a keyless open of an + # encrypted store, and they reach this before the decrypt pass below ever runs. Same cause, + # same verdict — an absent keyring, not a bad archive. + return "KEY_MISMATCH", safe_exc(exc), 0 except Exception as exc: # a restore that won't even open is the thing we're trying to catch - return False, safe_exc(exc), 0 + return "FAIL", safe_exc(exc), 0 if not ok: - return False, msg, 0 + return "FAIL", msg, 0 return _decrypt_check(snap, snap_settings) -def _decrypt_check(snap: Path, settings: StoreSettings) -> tuple[bool, str, int]: +def _decrypt_check(snap: Path, settings: StoreSettings) -> tuple[str, str, int]: """Decrypt AND authenticate every cipher-covered cell retained in the snapshot, under the store's own - cipher. Returns ``(ok, message, cells)`` — a COUNT and a PHI-free reason, never a plaintext. + cipher. Returns ``(status, message, cells)`` — a COUNT and a PHI-free reason, never a plaintext. Opening the store proves the file is a readable SQLite database. It does not prove the PHI inside it is readable, and those are the two different claims a disaster-recovery check gets confused about: a @@ -988,26 +1011,31 @@ def _decrypt_check(snap: Path, settings: StoreSettings) -> tuple[bool, str, int] try: plain = cipher.decrypt(str(stored), aad=cell_aad(table, column, row_id)) except CipherError as exc: + # FAIL, not KEY_MISMATCH: CipherError cannot separate a corrupted ciphertext from a + # key that was never supplied, and the keyring here is not empty (the keyless case + # is the branch below). Reporting the softer verdict on a bit-flipped PHI cell is + # the reading that must not be talked down. return ( - False, + "FAIL", f"{table}.{column} id={row_id} did not decrypt: {safe_exc(exc)}", cells, ) if cipher.is_encrypted(plain): # The identity cipher hands an mfenc: value straight back (the #241 F2 keyless-open # trap): the snapshot holds sealed cells and this open resolved no key for them. + # Nothing could have opened them, so the keyring is unambiguously the cause. return ( - False, + "KEY_MISMATCH", f"{table}.{column} is encrypted at rest but the store settings resolved no key " "to open it (keyless open of an encrypted snapshot)", cells, ) cells += 1 except sqlite3.Error as exc: - return False, safe_exc(exc), cells + return "FAIL", safe_exc(exc), cells finally: conn.close() - return True, "ok", cells + return "PASS", "ok", cells def _read_manifest_from_tar(tar_path: Path) -> dict[str, object]: diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 92e552c81..e6ff6d44d 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -2276,24 +2276,42 @@ async def open( f"invalid synchronous mode {synchronous!r}; expected 'NORMAL' or 'FULL'" ) db = await aiosqlite.connect(str(path)) - db.row_factory = aiosqlite.Row - await db.execute("PRAGMA journal_mode=WAL") - # NORMAL is crash-safe under WAL (only risk is losing the last txn on OS crash/power loss, - # never corruption) and avoids an fsync per commit — a large write-throughput win vs FULL. - # `sync` is validated above, so this f-string can't inject. FULL is available for the - # paranoid (every commit fsynced) via [store] synchronous = "full". - await db.execute(f"PRAGMA synchronous={sync}") - await db.execute("PRAGMA foreign_keys=ON") - await db.execute("PRAGMA busy_timeout=5000") - await db.executescript(_SCHEMA) - await cls._migrate(db) - await db.commit() - # Tighten permissions now that the file (and its WAL siblings) exist — they hold PHI. - if str(path) != ":memory:": - main = Path(path) - for f in (main, main.with_name(main.name + "-wal"), main.with_name(main.name + "-shm")): - if f.exists(): - _secure_file(f) + # A FAILED open must never strand this handle. An open aiosqlite connection keeps the DB file + # (and its -wal/-shm siblings) locked, so a caller unwinding a temp directory around the failure + # — the DR restore-verify is the live case — hits a Windows PermissionError from the cleanup + # that REPLACES the real reason the open failed. Both guards below close what they own and + # re-raise, so the caller sees the original error. + try: + db.row_factory = aiosqlite.Row + await db.execute("PRAGMA journal_mode=WAL") + # NORMAL is crash-safe under WAL (only risk is losing the last txn on OS crash/power loss, + # never corruption) and avoids an fsync per commit — a large write-throughput win vs FULL. + # `sync` is validated above, so this f-string can't inject. FULL is available for the + # paranoid (every commit fsynced) via [store] synchronous = "full". + await db.execute(f"PRAGMA synchronous={sync}") + await db.execute("PRAGMA foreign_keys=ON") + await db.execute("PRAGMA busy_timeout=5000") + await db.executescript(_SCHEMA) + await cls._migrate(db) + await db.commit() + # Tighten permissions now that the file (and its WAL siblings) exist — they hold PHI. + if str(path) != ":memory:": + main = Path(path) + for f in ( + main, + main.with_name(main.name + "-wal"), + main.with_name(main.name + "-shm"), + ): + if f.exists(): + _secure_file(f) + except BaseException: + try: + await db.close() + except Exception: # noqa: BLE001 — cleanup must never mask the open's own error + log.warning( + "could not close the connection after a failed store open", exc_info=True + ) + raise store = cls( db, path=path, @@ -2305,18 +2323,33 @@ async def open( audit_mac_fn=audit_mac_fn, message_events=message_events, ) - # ASVS 11.3.4: enable the PERSISTED per-key AES-GCM invocation bound and reserve the first block - # BEFORE anything on this handle encrypts — the at-rest migration below included, since on a - # store that is having a key enabled for the first time it is itself a large burst. A no-op when - # the cipher carries no bound (keyless / `vault_transit`). - await store.checkpoint_cipher_invocations() - await store._encrypt_existing_rows() # one-time PHI-at-rest migration when a key is set - await store._load_audit_chain_meta() # load/auto-init the #190 keying watermark - await ( - store._load_state_cache() - ) # populate the in-memory state read-through cache (ADR 0005) - await store._load_reference_cache() # populate the reference-snapshot read cache (ADR 0006) - await store._open_read_pool(str(path)) # dedicated read-only WAL pool (lockfree-reads) + try: + # ASVS 11.3.4: enable the PERSISTED per-key AES-GCM invocation bound and reserve the first + # block BEFORE anything on this handle encrypts — the at-rest migration below included, + # since on a store that is having a key enabled for the first time it is itself a large + # burst. A no-op when the cipher carries no bound (keyless / `vault_transit`). + await store.checkpoint_cipher_invocations() + await store._encrypt_existing_rows() # one-time PHI-at-rest migration when a key is set + await store._load_audit_chain_meta() # load/auto-init the #190 keying watermark + await ( + store._load_state_cache() + ) # populate the in-memory state read-through cache (ADR 0005) + await ( + store._load_reference_cache() + ) # populate the reference-snapshot read cache (ADR 0006) + await store._open_read_pool(str(path)) # dedicated read-only WAL pool (lockfree-reads) + except BaseException: + # The eager warm-ups above are the ones that FAIL CLOSED on a keyless/undecryptable open + # (`_load_state_cache` / `_load_reference_cache` raise StoreKeylessError or CipherError), + # and `_open_read_pool` opens further handles. Closing the half-built store here is what + # keeps that fail-closed error the one the caller sees: leaving the handles open locks the + # DB file on Windows, so a caller unwinding a temp directory around the failure — the DR + # restore-verify — reports a PermissionError from the cleanup instead of the missing key. + try: + await store.close() + except Exception: # noqa: BLE001 — cleanup must never mask the open's own error + log.warning("could not close the store after a failed open", exc_info=True) + raise if store._group_commit is not None: store._group_commit.start() # spin the committer coroutine (needs the running loop) return store diff --git a/scripts/security/crypto_inventory_check.py b/scripts/security/crypto_inventory_check.py index 73b1b06b0..d10fe52c8 100644 --- a/scripts/security/crypto_inventory_check.py +++ b/scripts/security/crypto_inventory_check.py @@ -344,8 +344,14 @@ # manifest + the dr_backup audit row as a PHI-free integrity fingerprint) and re-derives the key_id # fingerprint via the backup codec; the AEAD itself is delegated to store/backup_codec.py — a # CRYPTO_SEAM_MODULES import, so that delegation is now a first-class inventory token. + # ADR 0049 AC-13 adds the store-cipher seam (store/crypto.py): the FULL restore-verify opens the + # snapshot's cipher-covered cells through the store's own cipher, under the same cell-bound AAD the + # store writes (cell_aad, ASVS 11.3.3), to prove the PHI is readable and not merely that a SQLite + # file opened. No primitive is implemented here — the cipher is built by build_store_cipher and the + # AEAD runs inside it; this module holds only the marker prefix, the AAD constructor and the + # fail-closed CipherError/StoreKeylessError verdicts. "messagefoundry/pipeline/dr_backup.py": frozenset( - {"hashlib", "messagefoundry.store.backup_codec"} + {"hashlib", "messagefoundry.store.backup_codec", "messagefoundry.store.crypto"} ), # ADR 0073: rendezvous (HRW) outbound-lane ownership for engine shards — sha256 as a STABLE, # process-independent hash (the salted builtin hash() differs per process, which would let two diff --git a/tests/test_restore_verify.py b/tests/test_restore_verify.py index 172637f3d..d74250fe8 100644 --- a/tests/test_restore_verify.py +++ b/tests/test_restore_verify.py @@ -21,8 +21,9 @@ _verify_archive_blocking, run_restore_verify, ) -from messagefoundry.store import MessageStore +from messagefoundry.store import MessageStatus, MessageStore from messagefoundry.store.crypto import generate_key, make_cipher +from messagefoundry.store.store import Stage async def _backup( @@ -191,7 +192,10 @@ async def test_full_verify_fails_on_a_corrupted_aead_cell(tmp_path) -> None: def test_full_verify_fails_when_the_snapshot_opens_without_its_key(tmp_path) -> None: - """A keyless or wrong-key open of an ENCRYPTED snapshot is FAIL, with the real cause named. + """A keyless open of an ENCRYPTED snapshot is KEY_MISMATCH; a wrong-key one is FAIL. Both name the + real cause, and the split is the point: with no key resolved nothing could have opened the cells, so + the operator's key configuration is at fault and the archive is fine. A keyring that DOES hold keys + and still cannot open a cell is indistinguishable from bit rot, so it keeps the harder verdict. This calls the blocking verify directly because that is the only way to reach the shipped defect's shape: the codec key is in hand (the archive itself decrypts fine), while the settings threaded into @@ -218,7 +222,7 @@ async def _setup() -> str: full=True, store_settings=StoreSettings(path="unused"), ) - assert keyless.status == "FAIL" + assert keyless.status == "KEY_MISMATCH" assert "keyless open" in (keyless.reason or ""), keyless.reason wrong = _verify_archive_blocking( @@ -236,6 +240,107 @@ async def _setup() -> str: assert "no live store settings" in (absent.reason or ""), absent.reason +async def _state_and_reference_store(db: Path, key_b64: str) -> MessageStore: + """A keyed store carrying a transform-``state`` row and a ``reference`` snapshot as well as a + message. Both tables are warmed EAGERLY by ``MessageStore.open`` and both decrypt through the + fail-closed helper, so they are what turns a keyless open from a false PASS into a hard raise.""" + store = await MessageStore.open(db, cipher=make_cipher(key_b64)) + mid = await store.enqueue_ingress(channel_id="IB", raw="MSH|^~\\&|x") + ingress = await store.claim_next_fifo("IB", stage=Stage.INGRESS.value) + assert ingress is not None + await store.route_handoff( + ingress_id=ingress.id, + message_id=mid, + channel_id="IB", + handlers=[("H", "MSH|^~\\&|x")], + disposition=MessageStatus.ROUTED, + ) + routed = await store.claim_next_fifo("IB", stage=Stage.ROUTED.value) + assert routed is not None + await store.transform_handoff( + routed_id=routed.id, + message_id=mid, + channel_id="IB", + deliveries=[("d1", "OUT|y")], + state_ops=[("ns", "k", {"seq": 7})], + ) + await store.write_reference_snapshot(name="prov", version="1", rows={"NPI1": "Dr Who"}) + return store + + +async def test_full_verify_passes_on_a_snapshot_holding_state_and_reference_rows(tmp_path) -> None: + """A good encrypted archive that holds transform state must verify PASS — the half of the shipped + defect that failed in the opposite direction. + + ``MessageStore.open`` warms the ``state`` and ``reference`` caches eagerly and both decrypt through + the fail-closed ``decrypt_json_cell`` helper, so opening this snapshot keyless does not merely prove + too little: it RAISES ``StoreKeylessError``. Under the shipped code every scheduled backup of a keyed + store that had ever written state failed its own verify, with the missing key reported as a bad + archive.""" + key_b64 = generate_key() + db = tmp_path / "msg.db" + store = await _state_and_reference_store(db, key_b64) + ss = StoreSettings(path=str(db), encryption_key=key_b64) + runner = BackupRunner( + store, + BackupSettings(enabled=True, destination=str(tmp_path / "b")), + store_settings=ss, + config_dir=None, + ) + result = await runner.run_once(now=1.0) + assert result is not None + await store.close() + + res = await run_restore_verify(result.archive_path, store_settings=ss, full=True) + assert res.status == "PASS", res.reason + assert res.decrypted_cells >= 1 + + +def test_full_verify_on_a_failed_open_reports_the_open_error_not_a_cleanup_error(tmp_path) -> None: + """A full open that FAILS must report why it failed. The snapshot lives in a temp directory the + verify unwinds on the way out, and ``MessageStore.open`` used to leave its aiosqlite handle open + when a warm-up raised — on Windows that handle holds the file, the unlink is refused, and the + ``PermissionError`` from the cleanup REPLACES the missing-key error one frame up. The operator then + reads a file-locking complaint about a temp path that no longer exists. + + A ``state`` row is what makes this reachable: it is the eager warm-up that raises. Synchronous for + the same reason as the keyless test above — the full open runs its own loop.""" + import asyncio + + key_b64 = generate_key() + db = tmp_path / "msg.db" + + async def _setup() -> str: + store = await _state_and_reference_store(db, key_b64) + ss = StoreSettings(path=str(db), encryption_key=key_b64) + runner = BackupRunner( + store, + BackupSettings(enabled=True, destination=str(tmp_path / "b")), + store_settings=ss, + config_dir=None, + ) + result = await runner.run_once(now=1.0) + assert result is not None + await store.close() + return result.archive_path + + archive = asyncio.run(_setup()) + + res = _verify_archive_blocking( + archive_path=archive, + keys=[ + base64.b64decode(key_b64) + ], # the ARCHIVE decrypts; only the store settings are keyless + full=True, + store_settings=StoreSettings(path="unused"), + ) + assert res.status == "KEY_MISMATCH", res.reason + reason = res.reason or "" + assert "encryption key" in reason, reason + # The negative half, and the point of the test: no leaked handle, so no cleanup error over the top. + assert "another process" not in reason and "WinError" not in reason, reason + + async def test_full_verify_passes_on_a_good_unencrypted_archive(tmp_path) -> None: """A no-key (synthetic / no-PHI) instance still verifies PASS under ``full``. Nothing is sealed, so the decrypt pass has nothing to open and reports zero cells — a PASS that claims exactly that.""" From 460d2194800d0de10b5ef3bc358b27bafedb58b1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 14 Sep 2026 19:43:53 -0500 Subject: [PATCH 3/3] fix(docs): PHI.md said vault_transit never applies to a backup, and now it half does (BACKLOG #1718) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The full restore-verify opens the extracted snapshot's cipher-covered cells through the STORE cipher (build_store_cipher, ADR 0049 AC-13). That is the same cipher that wrote them, so under cipher_provider = vault_transit the read runs in Transit. PHI.md's PL-1 rule said vault_transit never applies to a backup, full stop, and that sentence is now a false premise -- the kind §11 forbids a control from resting on. The archive's own seal is unchanged: backup_codec, keyed by resolve_active_key, so vault_transit still never applies to sealing or unsealing a .mfbak. The distinction is the archive versus the cells inside it, and the doc now states it. The guard that caught this asserted "build_store_cipher not in dr_backup.py", which pinned "one cipher in this module" rather than the claim itself. It now pins the distinction: if the module builds the store cipher, §3 has to say which cipher governs which read. Its other assertions are untouched. This is a required CI leg, red on both the ubuntu and windows-2022 test rows of PR 1126 before this commit. --- docs/PHI.md | 12 ++++++++---- tests/test_phi_at_rest_inventory.py | 18 +++++++++++++++--- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/docs/PHI.md b/docs/PHI.md index db441fae9..9f66ff81f 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -409,10 +409,14 @@ application log files (`[logging].log_dir`). ([store/backup_codec.py](../messagefoundry/store/backup_codec.py), whose own docstring says the cipher *mechanism* is net-new): chunked AES-256-GCM under the store DEK resolved directly by `resolve_active_key`, with a per-chunk AAD of - `header_sha256 ‖ frame_counter(uint64) ‖ final_flag(uint8)` — **not** a per-cell AAD. Because the - key is resolved by `resolve_active_key` and not `build_store_cipher`, `cipher_provider = - vault_transit` **never applies to a backup**. And `[backup].allow_unencrypted = true` writes a - **CLEARTEXT `.mfbak.plain`** — a plaintext PHI-body archive on disk. + `header_sha256 ‖ frame_counter(uint64) ‖ final_flag(uint8)` — **not** a per-cell AAD. The archive's + own seal is keyed by `resolve_active_key` and not `build_store_cipher`, so `cipher_provider = + vault_transit` **never applies to sealing or unsealing a `.mfbak`**. It DOES apply one frame + further in, and the distinction is the archive versus the cells inside it: `full_restore_verify` + opens the extracted snapshot's cipher-covered cells through the **store** cipher + (`build_store_cipher`, ADR 0049 AC-13) — the same cipher that wrote them — so under `vault_transit` + that read runs in Transit exactly as a live cell read would. And `[backup].allow_unencrypted = true` + writes a **CLEARTEXT `.mfbak.plain`** — a plaintext PHI-body archive on disk. - *File-connector spill dirs* — **plaintext on disk**; there is no cipher on that path, only volume/share encryption and the directory ACL. - **Integrity.** The per-value GCM tag is the tamper-evidence. For `attachment_chunk.ciphertext` each diff --git a/tests/test_phi_at_rest_inventory.py b/tests/test_phi_at_rest_inventory.py index 839277e00..7654c859b 100644 --- a/tests/test_phi_at_rest_inventory.py +++ b/tests/test_phi_at_rest_inventory.py @@ -743,10 +743,22 @@ def test_pl1_encryption_rule_carves_out_the_backup_codec() -> None: "so vault_transit never applies), and allow_unencrypted writes a CLEARTEXT archive." ) source = (_PKG / "pipeline" / "dr_backup.py").read_text(encoding="utf-8") - assert "resolve_active_key" in source and "build_store_cipher" not in source, ( - "dr_backup now uses build_store_cipher; the doc says vault_transit never applies to a " - "backup — re-derive it." + assert "resolve_active_key" in source, ( + "dr_backup no longer resolves the archive key directly; the doc's `.mfbak` seal sentence " + "rests on that — re-derive it." ) + # ADR 0049 AC-13 put a SECOND cipher in this module: the full restore-verify opens the extracted + # snapshot's CELLS through build_store_cipher, where cipher_provider does apply. The archive's own + # seal still does not. This used to assert `build_store_cipher not in source`, which read as "one + # cipher here" and would now be a false premise under §11's compensating-control rule — so it pins + # the distinction instead: if the module builds the store cipher, §3 must say which governs which. + if "build_store_cipher" in source: + assert "build_store_cipher" in section3, ( + "dr_backup builds the STORE cipher (the full restore-verify reads the snapshot's cells " + "back through it), and §3's PL-1 rule does not name it. Left unstated, the `.mfbak` " + "bullet's 'vault_transit never applies to a backup' reads as covering that read too, and " + "it does not — under vault_transit the cell read runs in Transit." + ) assert ".mfbak.plain" in source, ( "the cleartext-archive path is gone; remove the carve-out from §3 in the same change." )