do not verify the chunk id on every read from an AEAD repo, fixes #9994 - #9995
Open
ThomasWaldmann wants to merge 1 commit into
Open
do not verify the chunk id on every read from an AEAD repo, fixes #9994#9995ThomasWaldmann wants to merge 1 commit into
ThomasWaldmann wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9995 +/- ##
==========================================
+ Coverage 85.88% 85.97% +0.09%
==========================================
Files 95 95
Lines 17088 17124 +36
Branches 2616 2621 +5
==========================================
+ Hits 14676 14723 +47
+ Misses 1674 1667 -7
+ Partials 738 734 -4 ☔ View full report in Codecov by Harness. |
ThomasWaldmann
force-pushed
the
always-assert-id-9994
branch
2 times, most recently
from
August 1, 2026 19:26
2d1faa9 to
b721f0b
Compare
ThomasWaldmann
force-pushed
the
always-assert-id-9994
branch
from
August 1, 2026 19:54
b721f0b to
a6ec929
Compare
…gbackup#9994 For the AEAD encryption modes, the chunk id is part of the AAD, so a successful decryption already proves that a repo key holder deliberately stored this exact ciphertext for this exact chunk id: a malicious repository can not swap, splice or substitute objects, whether we recompute the id hash over the plaintext or not. What assert_id adds there is only the detection of chunks whose content does not match their id, which only a malicious/compromised borg client that had the borg key could have written (see borgbackup#7362, which made the check mandatory). Whether that extra full-plaintext hash pass is worth it depends on where borg reads, so the new BORG_ASSERT_ID env var takes a comma-separated list of the places that shall verify the chunk id: read the general read path: extract, mount, export-tar, diff, ... repair borg check --repair transfer borg transfer (everything read from the source repo) rechunk borg recreate --chunker-params (re-chunking reads) Default (env var not set): "repair,transfer,rechunk", i.e. every place that re-anchors content and would thus make a violation unnoticeable afterwards, but not the hot read path. Unknown names are an error. BORG_ASSERT_ID=read,repair,transfer,rechunk gives the old behaviour of always verifying. The id is verified no matter what the env var says: - in borg check --verify-data - the audit that re-certifies the id/content invariant for all chunks of the repository is what makes not verifying elsewhere defensible, so it is not configurable (there is no "verify_data" place name, giving one is an error). - for "authenticated" and "none" mode repos and for borg 1.x repo objects: there is no AEAD there, so assert_id IS the read path authentication (KeyBase.id_check_is_authentication) and skipping it would demote "authenticated" to "none". Measured on an Apple M3 Pro (18GB RAM, hw accelerated sha256), reading one 20GiB incompressible file back via "borg export-tar arch - > /dev/null", aes256-ocb, --compression none, 4 runs alternating between the settings: id hash BORG_ASSERT_ID run times [s] median throughput sha256 default 20.43 21.13 20.79 20.93 20.86 982 MiB/s sha256 ...,read 29.42 28.91 28.82 28.05* 28.87 709 MiB/s blake3 default 19.99 19.97 20.00 19.93 19.98 1025 MiB/s blake3 ...,read 24.70 24.95* 24.72 24.60 24.71 829 MiB/s *) repeated run: the original one (31.64s resp. 28.37s) was disturbed by unrelated background load on the test machine. i.e. 28% faster for sha256 id hashes and 19% faster for blake3 ones (the file is much bigger than the RAM, so this includes reading it from the SSD every time). With the default, running borg check --verify-data periodically is the recommended audit that re-certifies the id/content invariant for all chunks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ThomasWaldmann
force-pushed
the
always-assert-id-9994
branch
from
August 1, 2026 20:06
a6ec929 to
87b8d61
Compare
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
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 #9994.
What this does
With the AEAD ciphersuites, the chunk id is part of the AAD (and, for repo objects v2, the header AAD + slot tag), so a successful decryption already proves that a holder of the repo key deliberately stored this exact ciphertext for this exact chunk id. A malicious or buggy repository therefore cannot swap, splice, truncate or substitute objects, whether or not we additionally recompute the id hash over the plaintext.
What
assert_id()adds on top of that is only the detection of chunks whose content does not match their id — which only a malicious or compromised borg client that had the borg key could have written (see #7362, which made the check mandatory in borg2 back then).Whether that extra full-plaintext hash pass is worth it depends on where borg reads, so this PR adds
BORG_ASSERT_ID, a comma-separated list of the places that shall verify the chunk id:readborg extract,borg mount,borg export-tar,borg diff, ...repairborg check --repair(rebuilds archives by re-packing the item metadata stream into new chunks with fresh ids, recreates manifest / archives directory entries from what it reads)transferborg transfer, for everything it reads from the source repository (a trust boundary)rechunkborg recreate --chunker-params ...(new ids are computed from the content read, so a violation would not be noticeable afterwards)Default (variable not set):
repair,transfer,rechunk— every place that re-anchors content, but not the hot read path. Unknown place names are an error.BORG_ASSERT_ID=read,repair,transfer,rechunkrestores the "always verify" behaviour of borg 2.0.0b22 and before.The chunk id is verified no matter what the variable says:
borg check --verify-data. That audit re-certifies the id/content invariant for all chunks of the repository and is what makes not verifying elsewhere defensible, so it is not configurable - there is noverify_dataplace name, and giving one is an error.authenticatedandnonemode repositories and for borg 1.x repo objects: there is no AEAD there, soassert_id()is the read path's authentication and switching it off would silently demoteauthenticatedtonone. This is a key class property,KeyBase.id_check_is_authentication(defaultTrue,Falseonly inAEADKeyBase), so any other/future key class is safe by default.Measurement
Apple M3 Pro, 18 GB RAM (hw accelerated sha256), one 20 GiB incompressible file,
--encryption=aes256-ocb --compression none, read back withborg export-tar arch - > /dev/null. 4 runs per setting, alternating between the two settings so that any drift hits both equally:BORG_ASSERT_ID...,read...,read*) repeated run: the original one (31.64 s resp. 28.37 s) was disturbed by unrelated background load on the test machine.
So 28% faster for
hmac-sha256id hashes and 19% faster forblake3ones. The file is much bigger than the RAM, so every run also reads 20 GiB from the SSD - for a repo whose data is in the page cache (or on faster storage), the relative difference is bigger.With the default, running
borg check --verify-dataperiodically is the recommended audit: it re-certifies the invariant for all chunks of the repository in the background, instead of on every read.Implementation
RepoObj.parse()is the single choke point (nothing else callskey.assert_id()). It verifies if the key class says the id check is the read path authentication, if the place of this read is mandatory (ASSERT_ID_PLACES_MANDATORY, currentlyverify_data), or if it is in the configured set. Reads are attributed toreadunless a command sets its own place for a repository (RepoObj.set_assert_id_place("transfer"|"rechunk"|"repair")) - which also covers the reads that go throughDownloadPipeline.fetch_many(), e.g.--chunker-paramsre-chunking - or unless a single call names its place (parse(..., assert_id_place="verify_data")).Tests
New unit tests in
repoobj_test.pyfor the place logic: the default set, parsing of the env var (including whitespace, the empty value, an invalid name and the rejectedverify_data), thatverify_dataverifies whatever the variable says, and that non-AEAD keys verify at every place no matter what is configured. Plus end-to-end tests that plant a chunk whose content does not match its id — i.e. what an evil client could write, which the AEAD layer authenticates happily — and check thatborg extractonly notices it withreadin the list, whilecheck --verify-data,check --repair,transfer(with and without--chunker-params) andrecreate --chunker-paramsnotice it with the default.Full test suite passes locally (excluding remote/binary variants).
🤖 Generated with Claude Code