compact: authenticate a gap object before dropping its bytes, #10093

[mr-raj12]

Fixes #10093.

superseded_gap_ranges computed the byte ranges compact drops from unverified object headers. A wrong data_size in a header whose chunk id is indexed elsewhere extended the dropped range over the gap bytes after the object, which can hold the only copy of a chunk (for example from a backup that crashed before writing its index).

Changes

• A gap object is dropped only if validate (repoobj.object_validator) accepts its header and metadata slot. That verifies its chunk id, meta_size and data_size, so the dropped range is exactly the object. The stored size of the indexed copy may differ (other compression, obfuscation padding) and is not compared.
• The walk over a gap ends at a header that does not parse or that reaches past the gap. The rest of that gap, and a superseded object that does not validate, are kept and logged as a warning with the pack id and the offset.
• validate is a required argument of delete(), compact_pack(), transform_pack() and superseded_gap_ranges(); None drops no gap bytes. compact, repo-compress and check --repair pass the repository's object validator.
• crypto.key.key_from_repository loads the key from the first stored object that identifies the key type: the manifest, then up to 999 indexed objects, skipping damaged objects and unsupported key types. ArchiveChecker.make_key, debug dump-repo-objs, debug search-repo-objs and debug delete-obj use it. check now skips a damaged object and tries the next one; dump-repo-objs and search-repo-objs read the manifest first and raise IntegrityError if no object identifies the key type.
• borg debug delete-obj validates with the repository key, so it asks for the passphrase of an encrypted repository. A wrong or missing passphrase stops the command before anything is deleted. If no key is found, the key is invalid, or no stored object identifies the key type, it logs a warning and deletes without dropping gap bytes. The key is set up only if at least one given id is valid.
• docs/internals/packs.rst: new "Gap bytes" section.

Left as TODO

A wrong obj_size in a gap header usually ends the walk over that gap, so the superseded duplicates after it are kept on every rewrite. Resyncing with PackReader._find_header can resume at a copy of an object inside another object's unencrypted data (none-* and authenticated-* modes), whose range can cover the gap objects after it.

Full suite: 3039 passed, 1125 skipped. ruff and black clean.

[codecov]

Codecov Report

❌ Patch coverage is 97.53086% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.05%. Comparing base (11d7d10) to head (8c6ea89).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/archive.py	85.71%	0 Missing and 1 partial ⚠️
src/borg/archiver/debug_cmd.py	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10167      +/-   ##
==========================================
+ Coverage   87.93%   88.05%   +0.11%     
==========================================
  Files         103      103              
  Lines       18893    18913      +20     
  Branches     2917     2919       +2     
==========================================
+ Hits        16613    16653      +40     
+ Misses       1583     1567      -16     
+ Partials      697      693       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[ThomasWaldmann]

@mr-raj12 please rebase onto current master. and then ask claude for feedback.

[ThomasWaldmann]

@mr-raj12 ping

[ThomasWaldmann]

review by claude fable 5.1 max

Verdict. The authentication part is right and I found no correctness bug in it. I would still ask for one change before merging: drop the obj_size equality check. It turns a class of superseded duplicates into bytes nothing can ever reclaim, which master reclaims today.

1. Blocking: the size-equality check regresses reclaim. Equal chunk ids mean equal plaintext, not equal stored size. I ran superseded_gap_ranges from this branch with a real validator (object_validator over a chacha20-poly1305 key) on the same chunk stored twice, the gap copy in one pack and the indexed copy at another location:

gap copy	indexed copy	dropped
lz4, 310 B	lz4, 310 B	yes
lz4, 310 B	zstd,3, 297 B	no
none, 6450 B	lz4, 310 B	no
obfuscate,110,lz4, 847 B	obfuscate,110,lz4, 632 B	no
obfuscate,250,lz4, 329 B	obfuscate,250,lz4, 329 B	yes

Sizes differ whenever two clients use different compression, when borg repo-compress ran between the crash and the compact, and on every single store with obfuscate levels 1 to 6 and 110 to 123, which pad randomly. Only Padmé, level 250, is deterministic. Master drops all of these. Worse, they get stuck: the index rebuild in cache.py keeps one copy per id, so after borg check --repair the others are superseded gaps again, and every later rewrite copies them verbatim. The docs' "for borg check --repair to re-index" does not hold for them.

The check buys nothing that the validator does not already provide. The PR text itself notes that data_size is pinned to the authenticated csize, and meta_size is covered by the tag. In the none-* modes the tag is an unkeyed checksum, so a deliberate forgery passes, but whoever can plant a forged object in a pack can also rewrite the index fragments or delete packs, so the index is no independent source there either. Against accidental corruption the checksum suffices. Suggest requiring only the validator. The "second source" bullet in packs.rst and the docstrings go with it, test_superseded_gap_ranges_rejects_a_size_the_index_contradicts reduces to its object_validator assertion, and a new test should show that a differently sized authoritative copy is still reported.

2. Minor: the new read-ahead constant duplicates an existing one. META_READ_SIZE = 1024 in repository.py already serves the same purpose for the repair walk in iter_headers. The comment on GAP_META_READAHEAD claims a metadata slot is at most 112 bytes over all key modes. I measured 113 for the AEAD modes with obfuscate,250, and it is not a bound by construction. Reuse the existing constant and drop the claim.

3. Minor: docs wording. "payloads are user content stored as it is" is only true with --compression none. "stored unencrypted" is what the argument needs.

4. Test nit. DATA_SIZE_OFFSET and META_SIZE_OFFSET are already imported into repository_test.py from repoobj_test.py. The new tests recompute them by hand (hdr_size - 4, len(OBJ_MAGIC) + 1).

Verified fine:

• validate is threaded from the three callers that hold a key. borg debug delete-obj opens with manifest=False and loses gap reclaim, as documented.
• borg compact always has a manifest, since with_repository defaults to manifest=True. Every manifest=None test site was converted.
• ChunkIndex has get() through MutableMapping.
• _parse_header is bounded by the gap end. _validation_problem does its own extra read for a slot longer than the read-ahead, under the existing MAX_VALIDATED_META_SIZE guard. A store-backed pack still costs one round trip per gap object, as before.
• Continuing past a rejected object by its own size can desync the rest of the gap, but nothing validates at a wrong offset, so that is conservative.

Checks run at c128990: ruff, black and git diff --check are clean.

tests	result
repository_test.py, repo_compress_cmd_test.py, compact_cmd_test.py	186 passed, 16 skipped
check_cmd_test.py, debug_cmds_test.py	44 passed, 47 skipped
repoobj_test.py	55 passed

[ThomasWaldmann]

@mr-raj12 ping?!

[mr-raj12]

Thanks, all four addressed in ac58e4c.

1. Dropped the obj_size equality check; the walk now requires only validate. Your table reproduces as all-yes on the new code:

gap copy	indexed copy	dropped
lz4, 234 B	lz4, 234 B	yes
lz4, 234 B	zstd,3, 239 B	yes
none, 926 B	lz4, 234 B	yes
obfuscate,110,lz4, 745 B	obfuscate,110,lz4, 1093 B	yes
obfuscate,250,lz4, 252 B	obfuscate,250,lz4, 252 B	yes

The "second source" bullet is gone from packs.rst and the docstrings, replaced by a paragraph saying why the sizes are deliberately not compared, so it does not get re-added. Your point that the index is no independent source holds even more directly than stated: borg compact builds it with build_chunkindex_from_repo, so it comes out of the same repository bytes. test_..._rejects_a_size_the_index_contradicts is renamed to test_..._rejects_an_inflated_data_size (the old name described the removed check) and reduced to its object_validator assertion, and test_..._reports_a_differently_sized_authoritative_copy covers the regression.

Worth noting the removal costs no safety property: _parse_header is passed the gap end, and that bound - not the size check - is what kept a dropped range from reaching an indexed object.

2. GAP_META_READAHEAD is gone, the gap walk reads with META_READ_SIZE. The 112-byte claim went with it.

3. Reworded, but not to "stored unencrypted" alone: in the authenticated-* modes the tag is keyed, it just binds a blob to its chunk id and nothing else. It now says the blobs are stored unencrypted and that validating does not establish that this repository wrote them, which is the framing the repair-walk section above already uses. The same over-claim was one paragraph up in the superseded_gap_ranges docstring, calling the chunk id a keyed MAC - that is wrong for the none-* modes and is fixed too.

4. Added CHUNK_ID_OFFSET next to the other two in repoobj_test.py and defined META_SIZE_OFFSET from it, so the offsets have one source; the new tests use the constants.

Full suite: 2993 passed, 1107 skipped. ruff, black and git diff --check clean. I could not build the docs locally (no sphinx here), so the packs.rst rewrite rests on CI.

[ThomasWaldmann]

review by claude fable 5.1 max (follow-up on ac58e4c)

The four points from the first review are addressed as requested; I re-ran the two-compression experiment on ac58e4c and every row is dropped now, with the dropped range equal to the gap copy's own authenticated size. Full suite here: 3011 passed, 993 skipped. The Sphinx build is clean for this change (the only two warnings are the theme's parallel-read notices), and the new section renders as intended.

On the inline comment about borg debug delete-obj in packs.rst: agreed, and there is a second problem in the same paragraph.

delete-obj belongs in the code, not in the format docs. The general rule stays in the docs: without a validator the walk drops no gap bytes. The delete-obj consequence is a TODO in do_debug_delete_obj, and it is cheap to resolve, possibly in this PR: do_debug_dump_repo_objs and do_debug_search_repo_objs already set up a key without depending on a manifest (list one object, hand its bytes to key_factory). delete-obj can do the same, build object_validator(RepoObj(key)) from it and pass it as validate to repository.delete(). It should fall back to validate=None with a warning when no key can be set up, so it stays usable as a rescue tool on a repository with an unreadable key or no objects.

Wrong claim in the same paragraph. "A blob that does not authenticate keeps its bytes, for borg check --repair to re-index." Repair cannot re-index such a blob: its validating walk skips it and leaves the bytes as a gap, as the repair-walk section above says, and rewriting a damaged pack is repository-level repair per the issue referenced there. The earlier revision's "anything else keeps its bytes" was true because it included unindexed valid blobs; narrowing the subject made the claim false. It should say the bytes are kept because compact is not where damaged bytes get handled.

A corrupt header in a gap blocks reclaim behind it for good. This is what the comment "the walk steps by obj_size before anything authenticates it: a wrong one lands the walk at a wrong offset, where nothing authenticates either" describes. Data is safe, but after the wrong step the walk breaks out of the gap, every rewrite copies the gap verbatim, and repair keeps a dropped object's bytes, so the same desync repeats on every compact and the superseded duplicates behind the corrupt object are never reclaimed. The repair walk already has the tool for this: _find_header scans forward for the next header that validates. The gap walk could reuse it. At least the comment should be a TODO rather than a statement of fact.

The walk gives up silently. Neither a non-authenticating candidate nor an unparsable header inside a gap produces a log line, so nobody learns that a pack holds bytes compact will never reclaim. iter_headers warns in the same situation. A warning naming the pack and offset would make the previous item visible.

The default silently turns reclaim off. delete(), compact_pack() and transform_pack() default to validate=None and then reclaim nothing. A future caller that forgets the argument loses reclaim without any signal. Making validate a required keyword argument, with delete-obj passing None explicitly, keeps the choice visible at each call site.

Checked and not a problem. No other caller lacks a validator: compact and repo-compress always have a manifest, and ArchiveChecker obtains a key (make_key) before verify_data() runs, so its delete calls always carry one. The remaining gaps are older than this PR: merge_packs copies whole files including superseded duplicates, and all-gap orphan packs are never rewritten by compact, which the existing TODO(#9868 follow-up) in compact_pack already tracks.

[ThomasWaldmann]

ping

[mr-raj12]

Addressed in c9c296f: delete-obj sets up the key (warns and drops no gap bytes without one), the re-index claim is gone, the gap walk warns where it keeps bytes, and validate is required. The resync is a TODO: _find_header can resume inside another object's unencrypted data and drop the gap objects after it.

[ThomasWaldmann]

• "Without the key nothing authenticates" in the docs is imprecise for the none-* modes, which have no key material and still validate by checksum. "Without a validator" is exact.

• A wrong passphrase degrades to the warning and still deletes. I verified that live. That is defensible for a rescue tool, but you may prefer an abort for a wrong passphrase specifically, as opposed to a missing key. Also new: the command prompts for a passphrase on an encrypted repository when none is in the environment, consistent with dump-repo-objs and stated in the PR text.

• Identifying the key type from the manifest or another object now exists three times: the checker's make_key, the new helper, and inline in the dump and search debug commands. A later cleanup could share one helper.

[mr-raj12]

Thanks for the review, and sorry for the slow reply here. All three points are handled in 8c6ea89, which went in before the merge, so noting it here for the record.

1. Docs, the no-validator case

docs/internals/packs.rst states it explicitly now, in the pack rewriting section:

Without a validator (validate=None), no gap bytes are dropped.

2. Wrong passphrase must not delete anything

You were right, this was a real problem. Two parts to it:

gap_validator() in archiver/debug_cmd.py catches only the "there is no key to use" errors, i.e. IntegrityError, RepoKeyNotFoundError, KeyfileInvalidError, UnsupportedKeyFormatError, and logs a warning in those cases. PassphraseWrong is a plain Error subclass and is deliberately not in that list, so it propagates and the command exits with rc 52.

The validator is also built once before the delete loop in do_debug_delete_obj() and not inside it. So a wrong passphrase aborts before the first repository.delete() call and nothing is deleted at all, not even partially. That was the part I had missed earlier.

Tests in testsuite/archiver/debug_cmds_test.py:

• test_debug_delete_obj_with_a_wrong_passphrase_deletes_nothing - checks rc 52 and that the pack size is unchanged afterwards
• test_debug_delete_obj_without_a_key_keeps_superseded_gap - no key, gap bytes are simply kept
• test_debug_delete_obj_with_invalid_ids_only_sets_up_no_key - no key setup if none of the given ids is valid, so no passphrase is asked for nothing
• test_debug_delete_obj_drops_superseded_gap - normal path still drops the gap

3. Key type identification existing in three places

Agreed, better to reduce it first and then extend. There is a single helper now, key_from_repository(repository, ids=None) in crypto/key.py. It reads the manifest first, then up to 999 chunk objects, and errors such as a wrong passphrase propagate to the caller.

All four places go through it: ArchiveChecker.make_key() (a thin wrapper now, it only passes the chunk ids), borg debug dump-repo-objs, borg debug search-repo-objs and the new gap_validator().

The only direct key_factory() caller left is in manifest.py, but that one is genuinely a different case, it already holds the manifest cdata and passes other= / ro_cls=. Folding it in would only make the helper more complicated, so I have left it as it is. Can revisit if you prefer otherwise.

Unit tests for the helper itself are in testsuite/crypto/key_test.py (test_key_from_repository_*), covering the manifest case, the fall through to chunk objects, the case where nothing identifies the key type, and that the key is loaded only once.