compact: authenticate a gap object before dropping its bytes, #10093 - #10167
Open
mr-raj12 wants to merge 4 commits into
Open
compact: authenticate a gap object before dropping its bytes, #10093#10167mr-raj12 wants to merge 4 commits into
mr-raj12 wants to merge 4 commits into
Conversation
…e chunks index, borgbackup#8476 When check --repair rebuilds the chunks index from the packs, a corrupt object header now makes iter_headers resync rather than raise: it takes a validate function and scans forward for the next object, in 1 MiB windows that overlap by one header so a header on a window boundary is still found. Repository-only checks pass no validate and keep raising IntegrityError on a corrupt header. OBJ_MAGIC also occurs inside payloads, so a candidate is accepted only when it authenticates. For AEAD keys, decrypting the metadata authenticates it against the header's magic, version and chunk_id, so the walk confirms a chunk id from a few hundred bytes. Keys that authenticate by chunk_id == id_hash(content) (id_check_is_authentication) read the whole object and parse() at the "repair" id place; validate.needs_data selects between the two. Authentication needs the key, so check --repair makes it before the rebuild with manifest_only=True. A repair that cannot read the manifest has no key and walks without resyncing.
…one, borgbackup#8476 Every key mode covers the object header by the metadata slot's AAD, so parse_meta confirms a candidate and validate.needs_data is gone.
…up#8476 A meta_size or data_size corrupted to a value that keeps the object inside the pack leaves the header valid, so the walk only notices at the misaligned offset it jumps to; scanning from there loses the intact objects in between. An object is dropped when a recovered object starts inside it, its size field being wrong.
…kup#10093 superseded_gap_ranges computed the byte ranges compact drops from object headers as they are: magic, chunk id, meta_size and data_size were all taken on trust. A wrong data_size in a header whose chunk id is indexed elsewhere extended the dropped range past the object into the gap bytes behind it, and those were dropped without ever being looked at. Gaps are where the only copy of a chunk can sit (a backup that crashed before writing its index, or a stale index), so that is not free. Parse the header with PackReader._parse_header, so a header that does not parse or overruns its gap ends the walk over that gap. Read the metadata slot in the same request and require: - validate accepts the header and metadata slot. The slot's tag covers magic, version and chunk id as AAD, and meta_size through the length of the ciphertext slice it selects. - the object's total size equals the index entry's obj_size. data_size is outside what the tag covers and it sets how far the reported range reaches; the entry is a second source for it. Anything else keeps its bytes and the walk continues past it, so a corrupt length field can desync the walk but can no longer drop anything. validate is threaded through delete(), compact_pack() and transform_pack() from the callers that have a key (compact, repo-compress, check --repair). Without it no gap bytes are dropped at all, so "borg debug delete-obj", which opens the repository without a key, stops reclaiming superseded gap bytes. resync_validator moves from archive.py to repoobj.py as object_validator: it is plain RepoObj parsing, and repository.py's callers must not import archive.py for it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10167 +/- ##
==========================================
+ Coverage 86.55% 86.58% +0.03%
==========================================
Files 101 101
Lines 17996 18074 +78
Branches 2738 2754 +16
==========================================
+ Hits 15577 15650 +73
- Misses 1717 1719 +2
- Partials 702 705 +3 ☔ View full report in Codecov by Harness. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10093.
superseded_gap_rangestook the object headers in a pack's gaps at face value. magic, chunk id, meta_size and data_size all came out of the bytes being examined, and(offset, obj_size)was then handed to compact as a range to delete.A gap object's bytes now get dropped only if two independent things agree:
validateaccepts the header and metadata slot. The slot's tag covers magic, version and chunk id as AAD, and meta_size through the length of the ciphertext slice it selects.obj_size. data_size is the one field the tag does not cover, and it decides how far the dropped range reaches, so the index entry serves as a second source for it.The header goes through
PackReader._parse_headerfirst, so one that does not parse, or that overruns its gap, ends the walk over that gap. The metadata slot comes back in the same request as the header, so this costs no extra round trip in the common case.Fail either check and the object keeps its bytes; the walk just continues past it. A corrupt length field can still push the walk to a wrong offset, but at a wrong offset nothing passes both checks, so nothing gets dropped.
validateis threaded in from the callers that hold a key: compact, repo-compress, check --repair. Without one, nothing in a gap is dropped at all. Worth calling out:borg debug delete-objopens the repository without a key, so it still deletes its target object but no longer reclaims superseded gap bytes from the pack it rewrites.resync_validatormoves from archive.py to repoobj.py and is renamedobject_validator. It is plain RepoObj parsing, and the compact / repo-compress callers should not have to reach into archive.py for it.Based on #10094, which is where
_parse_headerand the validate callable come from. Merging that one first keeps this diff down to the #10093 change itself.