diff --git a/docs/PHI.md b/docs/PHI.md index db441fae9..4f703ae0f 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -409,10 +409,20 @@ 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 + `header_sha256 ‖ frame_counter(uint64) ‖ final_flag(uint8)` — **not** a per-cell AAD. Because that 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. + vault_transit` **never applies to sealing or unsealing an archive**. And + `[backup].allow_unencrypted = true` writes a **CLEARTEXT `.mfbak.plain`** — a plaintext PHI-body + archive on disk. + - *Scope of that sentence.* It is about the **archive**, not about every step of a backup run. + `[backup].full_restore_verify` opens the *extracted* snapshot and decrypts its cells to prove the + PHI is still readable, and reading a store cell is what the store cipher is for — so that step + **does** go through `build_store_cipher`, and therefore through Transit under `vault_transit`. + Two paths, two key uses, one key source; the split is recorded once in + [ADR 0049](adr/0049-turnkey-dr-backup-restore-verify.md) §"Encryption boundary" and pinned by + `tests/test_phi_at_rest_inventory.py::test_the_mfbak_seal_never_reaches_for_the_store_cipher`. + That pass decrypts into memory and reports a **count** of cells opened; it writes no plaintext, + so it adds no at-rest tier to this inventory. - *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/docs/adr/0049-turnkey-dr-backup-restore-verify.md b/docs/adr/0049-turnkey-dr-backup-restore-verify.md index 0a6d55429..52f9ad246 100644 --- a/docs/adr/0049-turnkey-dr-backup-restore-verify.md +++ b/docs/adr/0049-turnkey-dr-backup-restore-verify.md @@ -202,6 +202,35 @@ below), containing: escape (parallel to `[store].allow_unencrypted_phi`) for a synthetic/non-PHI box. A synthetic instance with no key may back up in the clear; a PHI instance may not, silently. +#### Two key USES, one key SOURCE — which path holds which (BACKLOG #1561) + +`pipeline/dr_backup.py` reaches key material in two places, and they are different operations. Stated here +so the next reader does not re-derive it from the imports, and because a guard was once written on the +premise that there was only one: + +| Operation | Where | Key material | Does `cipher_provider` apply? | +|---|---|---|---| +| **Seal / unseal the `.mfbak`** | `_do_backup` to `_resolve_key` to `_build_archive_blocking` to `encrypt_stream`; `_verify_archive_blocking` to `decrypt_stream` | **Raw DEK bytes** from `resolve_active_key`, handed to `store/backup_codec.py` | **No.** The codec takes bytes and builds its own `AESGCM`. `vault_transit` is never consulted, so a Transit posture does not protect (or reach) the archive frames | +| **Read the extracted snapshot's cells** (`full_restore_verify` only) | `_full_open_check` to `open_store` / `_decrypt_check` | **The store cipher** from `build_store_cipher`, with the store's own per-cell AAD | **Yes.** It is a store read, so it dispatches on `cipher_provider` exactly as a live read does — including Transit | + +Three consequences worth stating rather than inferring: + +- **The snapshot read is not a second at-rest tier.** `_decrypt_check` decrypts into memory and returns a + **count** of cells opened, never a plaintext, and writes nothing. The extracted `store.db` keeps the + store's own column cipher; `docs/PHI.md` §2 already inventories that staging dir. +- **`open_store` builds a store cipher too.** That is why the table lists it beside `build_store_cipher`: + a "does this file touch the store cipher" question answered by searching for one name gets the wrong + answer. The full verify has called `open_store` since this ADR shipped; what #1561 changed is that it + now passes the LIVE settings, so an encrypted snapshot opens under a real key instead of the identity + cipher. +- **The guard is scoped to the seal, and it is an AST call-path check, not a token scan.** + `tests/test_phi_at_rest_inventory.py::test_the_mfbak_seal_never_reaches_for_the_store_cipher` asserts + that every archive-codec call sits in the seal/unseal region, that no store-cipher constructor sits + in it, and that the bytes handed to `encrypt_stream` are the unmodified `resolve_active_key` DEK. + "Sits in" is lexical and deliberate: the guard follows no calls, so a cipher construction moved into + a helper called from the seal lands OUTSIDE the permitted region and reds. Both seams are registered + in `scripts/security/crypto_inventory_check.py` (ASVS 11.1.3). + > **Key-availability consequence for #61's cold seed (addressed by design).** Because the archive is encrypted > under the store DEK, **the DR site must have that DEK available to restore the cold seed.** ADR 0048's cold > path **requires the same `KeyProvider` posture at the DR site** — env var / DPAPI key file / or reachability @@ -239,7 +268,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 +354,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 +434,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 4dc219db9..febdb08eb 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -4953,8 +4953,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 @@ -4981,6 +4983,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: @@ -4989,6 +4995,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/scripts/security/crypto_inventory_check.py b/scripts/security/crypto_inventory_check.py index 73b1b06b0..a87867c18 100644 --- a/scripts/security/crypto_inventory_check.py +++ b/scripts/security/crypto_inventory_check.py @@ -344,8 +344,19 @@ # 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. + # + # BACKLOG #1561 adds the SECOND seam, and the two are different operations under different key + # material — which is the whole reason both are listed rather than one standing in for the other: + # * backup_codec — SEALING the .mfbak, chunked AES-256-GCM over raw DEK bytes from + # resolve_active_key. cipher_provider never reaches it. + # * store.crypto — READING the extracted snapshot's own cells on a FULL restore-verify, through + # the store cipher (build_store_cipher, so vault_transit DOES reach it) with the + # store's own per-cell AAD. Decrypt-and-authenticate only; it reports a COUNT of + # cells opened and writes no plaintext anywhere. + # Registered rather than suppressed: this file now performs at-rest crypto through the store seam, + # and the bidirectional gate should red if that stops being true. "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_phi_at_rest_inventory.py b/tests/test_phi_at_rest_inventory.py index 839277e00..868218461 100644 --- a/tests/test_phi_at_rest_inventory.py +++ b/tests/test_phi_at_rest_inventory.py @@ -743,15 +743,229 @@ 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 through resolve_active_key; §3 says the " + "`.mfbak` DEK comes from there and not from the store cipher — re-derive it." ) assert ".mfbak.plain" in source, ( "the cleartext-archive path is gone; remove the carve-out from §3 in the same change." ) +#: ``dr_backup`` functions that SEAL or UNSEAL the ``.mfbak`` archive itself. §3's carve-out +#: ("the key is resolved by `resolve_active_key` and not `build_store_cipher`, so `vault_transit` +#: never applies") is a claim about THESE functions and no others. +_MFBAK_CODEC_FUNCS = frozenset( + {"_do_backup", "_resolve_key", "_build_archive_blocking", "_verify_archive_blocking"} +) +#: ``dr_backup`` functions that read the EXTRACTED snapshot's own store cells during a full +#: restore-verify. Reading a store cell is what the store cipher is FOR, so these are where +#: ``build_store_cipher`` (and ``open_store``, which builds one internally) belong. +_SNAPSHOT_READ_FUNCS = frozenset({"_full_open_check", "_decrypt_check"}) +#: Names that construct or obtain the STORE cipher. ``open_store`` is in the list because it calls +#: ``build_store_cipher`` itself — the old token scan could not see that, which is half of why it +#: was the wrong instrument. +_STORE_CIPHER_CTORS = frozenset( + {"build_store_cipher", "make_cipher", "build_transit_cipher", "open_store"} +) +#: The archive codec's own entry points (``store/backup_codec.py``), which take RAW DEK BYTES. +_ARCHIVE_CODEC_CALLS = frozenset({"encrypt_stream", "decrypt_stream"}) + + +def _callee_name(call: ast.Call) -> str | None: + if isinstance(call.func, ast.Name): + return call.func.id + if isinstance(call.func, ast.Attribute): + return call.func.attr + return None + + +def _dr_backup_tree() -> ast.Module: + return ast.parse((_PKG / "pipeline" / "dr_backup.py").read_text(encoding="utf-8")) + + +def _named_func(tree: ast.Module, name: str) -> ast.FunctionDef | ast.AsyncFunctionDef: + found = [ + n + for n in ast.walk(tree) + if isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) and n.name == name + ] + assert len(found) == 1, ( + f"pipeline/dr_backup.py declares {len(found)} functions named {name!r}; this guard names the " + "backup's key paths by function, so a rename or a duplicate must be re-derived here, not " + "silently skipped." + ) + return found[0] + + +def _binds(node: ast.AST, name: str) -> bool: + """True when ``node`` assigns to the bare name ``name`` — plain, augmented or walrus.""" + if not isinstance(node, ast.Assign | ast.AugAssign | ast.NamedExpr): + return False + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + return any(isinstance(t, ast.Name) and t.id == name for t in targets) + + +def _sites(calls: list[ast.Call]) -> list[str]: + """Call nodes rendered for a failure message — the callee and the line it sits on.""" + return [f"{_callee_name(c)} at line {c.lineno}" for c in calls] + + +def _split_call_sites( + tree: ast.Module, funcs: frozenset[str], names: frozenset[str] +) -> tuple[list[ast.Call], list[ast.Call]]: + """``(inside, outside)`` — call sites of ``names``, split by whether they sit LEXICALLY in one of + ``funcs``. No call is followed: a helper defined outside ``funcs`` and called from inside one + lands in ``outside``, which errs toward reporting rather than toward a false green. + + Keyed on AST node identity (``ast`` nodes hash by identity), not on a line range, so a nested + helper such as ``_full_open_check._open`` counts as INSIDE its enclosing function and nothing is + double-counted.""" + inside_nodes = { + sub + for name in funcs + for sub in ast.walk(_named_func(tree, name)) + if isinstance(sub, ast.Call) + } + inside: list[ast.Call] = [] + outside: list[ast.Call] = [] + for node in ast.walk(tree): + if isinstance(node, ast.Call) and _callee_name(node) in names: + (inside if node in inside_nodes else outside).append(node) + return inside, outside + + +def test_the_mfbak_seal_never_reaches_for_the_store_cipher() -> None: + """§3's `.mfbak` carve-out, pinned by CALL PATH rather than by a file-wide token. + + RULE: sealing a `.mfbak` under the store cipher would make §3's "`vault_transit` never applies to + a backup" false and would put a per-value string cipher on a multi-GB stream. That is the thing + forbidden, and this asserts it where it happens — the functions that write and read the archive. + + WHAT THIS NO LONGER COVERS, AND WHY THAT IS SAFE. Until BACKLOG #1561 the assertion was + ``"build_store_cipher" not in dr_backup.py`` — the whole file, by token. That scan was wrong in + both directions. It **under-fired**: ``_full_open_check`` has always called ``open_store``, which + builds a store cipher internally, so the token scan could never have caught a seal that obtained + its cipher that way, nor one that aliased the import. And it **over-fired**: a full restore-verify + now decrypts the extracted snapshot's own cells, which is a store read and legitimately needs the + store cipher — a different operation from sealing the archive, in a different function, on a + different key material. A file-wide token cannot tell those two apart. This guard can, so the + narrowing is a strengthening: the permitted region is named and closed, and a seal that reaches + for the store cipher by ANY of the four constructor names now fails. + + SCOPE. ``_split_call_sites`` buckets a call by the function it sits in LEXICALLY; it follows no + calls. A helper extracted out of ``_decrypt_check`` would therefore land outside and red, which + is conservative in the safe direction — the verdict is never falsely green — but it is why the + ADR table and this test say "in" rather than "reached". + """ + # (0) The doc limb. Every sibling in this file pins the §3 prose before it pins the code, and a + # code-only guard would stay green if the narrowed sentence were deleted — pinning a claim the + # document no longer makes, which is the defect this whole file exists to catch. + section3 = _section(3) + for token in ("sealing or unsealing an archive", "resolve_active_key", "build_store_cipher"): + assert token in section3, ( + f"§3's PL-1 encryption rule no longer states {token!r}. The `.mfbak` carve-out is now " + "SCOPED — the store cipher is off the archive seal and on the full restore-verify's " + "snapshot read — and the code assertions below pin only the second half of that claim." + ) + + # (0b) The forbidden-constructor list is hand-named, so pin each name to a real symbol. Without + # this a rename in store/ leaves an entry matching nothing and the arm below passes on a list of + # dead strings — the same shape of defect as the token scan this test replaced. + store_pkg = "".join( + (_PKG / "store" / name).read_text(encoding="utf-8") + for name in ("base.py", "crypto.py", "crypto_transit.py") + ) + unresolved = sorted(n for n in _STORE_CIPHER_CTORS if f"def {n}(" not in store_pkg) + assert not unresolved, ( + f"_STORE_CIPHER_CTORS names {unresolved}, which messagefoundry/store/ no longer defines. " + "Re-derive the store-cipher entry points; a stale name guards nothing." + ) + + tree = _dr_backup_tree() + + # (1) The archive codec is called ONLY from the seal/unseal region. A new sealing site added + # elsewhere lands in `outside` and reds, rather than escaping a region named once and forgotten. + codec_inside, codec_outside = _split_call_sites(tree, _MFBAK_CODEC_FUNCS, _ARCHIVE_CODEC_CALLS) + assert {_callee_name(c) for c in codec_inside} == set(_ARCHIVE_CODEC_CALLS), ( + f"the instrument did not find both archive-codec entry points; it saw {_sites(codec_inside)}. " + "A guard that cannot see the thing it guards proves nothing by passing." + ) + assert not codec_outside, ( + f"pipeline/dr_backup.py seals or unseals a .mfbak outside the named codec region: " + f"{_sites(codec_outside)}. Add the function to _MFBAK_CODEC_FUNCS and re-derive §3's carve-out " + "for it." + ) + + # (2) The store cipher is constructed ONLY on the snapshot-read path. This is the prohibition the + # old token scan was written for, now scoped to where it is true. + cipher_inside, cipher_outside = _split_call_sites( + tree, _SNAPSHOT_READ_FUNCS, _STORE_CIPHER_CTORS + ) + assert cipher_inside, ( + "the instrument found no store-cipher construction anywhere in pipeline/dr_backup.py, so its " + "'none outside the snapshot-read path' result is vacuous. Re-derive which functions open the " + "extracted snapshot." + ) + assert not cipher_outside, ( + f"pipeline/dr_backup.py builds the STORE cipher outside the snapshot-read path: " + f"{_sites(cipher_outside)}. §3 says the `.mfbak` seal is keyed by resolve_active_key and NOT " + "by build_store_cipher, so `cipher_provider = vault_transit` never applies to the archive — " + "sealing with the store cipher makes that sentence false." + ) + + # (3) The key reaching the seal is the resolve_active_key DEK, link by link. Without this a seal + # could swap its key source for anything at all and (2) would still be green. + resolve = _named_func(tree, "_resolve_key") + assert "resolve_active_key" in [ + _callee_name(n) for n in ast.walk(resolve) if isinstance(n, ast.Call) + ], ( + "BackupRunner._resolve_key no longer calls resolve_active_key; §3 names it as the archive DEK." + ) + + do_backup = _named_func(tree, "_do_backup") + assert any( + _binds(n, "key") + and isinstance(n.value, ast.Call) + and _callee_name(n.value) == "_resolve_key" + for n in ast.walk(do_backup) + if isinstance(n, ast.Assign) + ), "BackupRunner._do_backup no longer binds `key` from self._resolve_key()." + # The seal runs off the loop, so the call is `asyncio.to_thread(self._build_archive_blocking, + # ..., key=key, ...)` — _build_archive_blocking is a positional ARGUMENT, not the callee. + assert any( + isinstance(n, ast.Call) + and any( + isinstance(a, ast.Attribute) and a.attr == "_build_archive_blocking" for a in n.args + ) + and any( + kw.arg == "key" and isinstance(kw.value, ast.Name) and kw.value.id == "key" + for kw in n.keywords + ) + for n in ast.walk(do_backup) + ), "BackupRunner._do_backup no longer hands that same `key` to _build_archive_blocking." + + build = _named_func(tree, "_build_archive_blocking") + params = {a.arg for a in (*build.args.posonlyargs, *build.args.args, *build.args.kwonlyargs)} + assert "key" in params, "_build_archive_blocking no longer takes the resolved DEK as `key`." + rebound = [n.lineno for n in ast.walk(build) if _binds(n, "key")] + assert not rebound, ( + f"_build_archive_blocking rebinds `key` at line(s) {rebound}; the DEK the caller resolved is " + "then not the one that seals the archive." + ) + seals = [ + c + for c in ast.walk(build) + if isinstance(c, ast.Call) and _callee_name(c) == "encrypt_stream" + ] + assert len(seals) == 1, f"_build_archive_blocking makes {len(seals)} encrypt_stream calls." + key_arg = seals[0].args[2] if len(seals[0].args) > 2 else None + assert isinstance(key_arg, ast.Name) and key_arg.id == "key", ( + "encrypt_stream is no longer sealed with the unmodified `key` parameter, so the archive's key " + "material is no longer provably the resolve_active_key DEK §3 claims it is." + ) + + def test_pl1_retention_covers_every_tier_it_lists() -> None: """The PL-1 retention/destruction bullet used to cover 8 of the 10 tiers it enumerates.""" section3 = _section(3) 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)