diff --git a/CHANGELOG.md b/CHANGELOG.md index 8db0ab9..a4dcb69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ replacing a module has to be a decision with visible consequences. ## Unreleased +- **Backup verification reads each archive once instead of twice.** `GET /backups` hashed every + snapshot file and then read every archive member's payload to the end, doubling the disk reads + of large model archives on each request. The hash already proves the bytes match the manifest, + and the tar header walk rejects the same truncated archives, so members are now checked by + their headers, plus the first 16 bytes of each required SQLite store. + - **`/processes` no longer returns command lines, and wrong keys no longer cost PBKDF2.** `/processes` listed each host process's command line, which can carry a password or a key; it now returns the same metadata as `/runtime` (pid, name, user, CPU, memory). diff --git a/docs/backups.md b/docs/backups.md index 91839a1..2b19ed5 100644 --- a/docs/backups.md +++ b/docs/backups.md @@ -15,9 +15,10 @@ never extracted. A snapshot changing during verification is rejected. `unavailable`, an empty root reports `empty`, and rejected candidates make the response `degraded`. An incomplete directory does not become a recovery point merely because it has a recent name. -Verification reads the snapshot files in full on each request; large model archives can make -this endpoint slow. Poll it deliberately rather than at dashboard animation frequency. Responses -include `checked_at` and per-snapshot `verified_at`; there is no stale success cache. Hashes establish +Verification hashes each snapshot file in full once per request; archive members are checked by +their headers, plus the first 16 bytes of each required SQLite store. Large model archives can +still make this endpoint slow. Poll it deliberately rather than at dashboard animation frequency. +Responses include `checked_at` and per-snapshot `verified_at`; there is no stale success cache. Hashes establish consistency with the manifest, not authenticity against an attacker who can rewrite both. `restore_status: not_tested` is explicit: this telemetry does not test decryption, restore a stack, replay later deletions or reconcile actions. Manifest deletion-ledger and execution-journal states diff --git a/systemgate/backups.py b/systemgate/backups.py index caa11b9..9490200 100644 --- a/systemgate/backups.py +++ b/systemgate/backups.py @@ -76,12 +76,13 @@ def _archive(path: Path, required: str | None) -> None: seen.add(member.name) if member.name == required and not member.isfile(): _invalid("Required SQLite store is not a file; obtain an intact snapshot.") - if member.isfile(): + # The manifest hash already proved these bytes, and walking to the + # next header rejects archives cut inside a member's data, so only + # the required store's SQLite magic needs a payload read. + if member.isfile() and member.name == required: with archive.extractfile(member) as stream: - if member.name == required and stream.read(16) != b"SQLite format 3\x00": + if stream.read(16) != b"SQLite format 3\x00": _invalid("Required SQLite store is invalid; obtain an intact snapshot.") - while stream.read(1024 * 1024): - pass if required and required not in seen: _invalid("Required SQLite store is missing; obtain an intact snapshot.") diff --git a/tests/test_backups.py b/tests/test_backups.py index d291f8f..b945036 100644 --- a/tests/test_backups.py +++ b/tests/test_backups.py @@ -131,6 +131,54 @@ def test_even_hash_matching_unsafe_archives_fail(tmp_path, member_name, kind): assert backups.backup_status(tmp_path)["latest"] is None +def test_a_truncated_archive_is_never_a_recovery_point(tmp_path): + directory = snapshot(tmp_path) + buffer = io.BytesIO() + with tarfile.open(fileobj=buffer, mode="w") as archive: + payload = b"SQLite format 3\x00" + b"x" * 5000 + member = tarfile.TarInfo("pi.db") + member.size = len(payload) + archive.addfile(member, io.BytesIO(payload)) + tail = tarfile.TarInfo("data") + tail.size = 1024 + archive.addfile(tail, io.BytesIO(b"y" * 1024)) + data = buffer.getvalue() + # pi.db's 5016 bytes occupy 5120 padded, so the cut lands 100 bytes into the + # second member's payload. + (directory / "pi.tar").write_bytes(data[:512 + 5120 + 512 + 100]) + save_manifest(directory) + result = backups.backup_status(tmp_path) + assert result["latest"] is None + assert [row["name"] for row in result["rejected"]] == ["snapshot-a"] + + +def test_a_required_store_that_is_not_sqlite_is_never_a_recovery_point(tmp_path): + directory = snapshot(tmp_path) + with tarfile.open(directory / "pi.tar", "w") as archive: + member = tarfile.TarInfo("pi.db") + member.size = 16 + archive.addfile(member, io.BytesIO(b"not a database!!")) + save_manifest(directory) + result = backups.backup_status(tmp_path) + assert result["latest"] is None + assert [row["name"] for row in result["rejected"]] == ["snapshot-a"] + + +def test_only_the_required_store_payload_is_opened(tmp_path, monkeypatch): + snapshot(tmp_path) + opened = [] + original = tarfile.TarFile.extractfile + + def record(archive, member): + opened.append(member.name) + return original(archive, member) + + monkeypatch.setattr(tarfile.TarFile, "extractfile", record) + result = backups.backup_status(tmp_path) + assert result["latest"] is not None and result["latest"]["status"] == "verified" + assert sorted(opened) == ["pi.db", "toolgate.db"] + + def test_read_errors_are_unverifiable_without_leaking_payload(tmp_path, monkeypatch): snapshot(tmp_path) monkeypatch.setattr(backups, "_hash", lambda path: (_ for _ in ()).throw(