Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

Expand Down
12 changes: 8 additions & 4 deletions docs/PHI.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
52 changes: 48 additions & 4 deletions docs/adr/0049-turnkey-dr-backup-restore-verify.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -386,6 +405,31 @@ 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 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

Expand Down
12 changes: 10 additions & 2 deletions messagefoundry/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4990,8 +4990,10 @@
"""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

Expand Down Expand Up @@ -5021,6 +5023,10 @@
"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:
Expand All @@ -5029,6 +5035,8 @@
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}")

Check notice on line 5039 in messagefoundry/__main__.py

View workflow job for this annotation

GitHub Actions / diff-coverage (advisory)

Missing Coverage

Line 5039 missing coverage
# 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

Expand Down
Loading
Loading