From dedbf11e695c4b9b3d8191e506a2b8a5a431e3ef Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Mon, 14 Sep 2026 19:37:16 +0530 Subject: [PATCH] check --repair: validate the repository index rebuild with the key, #9901 Repair reads the key and aborts if it can not; objects that fail validation are not indexed and are reported as errors. --- src/borg/archive.py | 9 +- src/borg/archiver/check_cmd.py | 31 ++++- src/borg/repository.py | 62 +++++++--- src/borg/testsuite/archiver/check_cmd_test.py | 117 ++++++++++-------- src/borg/testsuite/repository_test.py | 51 +++++++- 5 files changed, 180 insertions(+), 90 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index f8769f526d..da266d6258 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2267,14 +2267,7 @@ def check( # decryption per object and it needs the key, so read the key here if we do not have it yet. # manifest_only=True: the other key source make_key uses is self.chunks, built just below. if repair and self.key is None: - try: - self.key = self.make_key(repository, manifest_only=True) - except IntegrityError as err: - logger.warning( - f"Could not read the key ({err}), so the rebuild can not validate object headers: " - "a pack with a corrupt object header is indexed up to that header and the rest " - "of it is dropped." - ) + self.key = self.make_key(repository, manifest_only=True) if self.key is not None: # the validator decrypts metadata slots, so it needs a RepoObj built from the key. self.repo_objs = RepoObj(self.key) diff --git a/src/borg/archiver/check_cmd.py b/src/borg/archiver/check_cmd.py index fd3a18beed..25940602f1 100644 --- a/src/borg/archiver/check_cmd.py +++ b/src/borg/archiver/check_cmd.py @@ -3,10 +3,12 @@ from ._common import with_repository, Highlander from ..archive import ArchiveChecker from ..constants import * # NOQA +from ..crypto.key import key_from_repository from ..helpers import set_ec, EXIT_WARNING, CancelledByUser, CommandError, Error, IntegrityError from ..helpers import relative_time_marker_validator, yes, ArchiveFormatter, sig_int from ..helpers.argparsing import ArgumentParser from ..helpers.time import archive_ts_now, calculate_relative_offset +from ..repoobj import RepoObj, object_validator from ..logger import create_logger @@ -69,7 +71,10 @@ def do_check(self, args, repository): try: archive_checker.key = archive_checker.make_key(repository, manifest_only=True) except IntegrityError: - pass # will try to make key later again + if args.repair: + # repair needs the key to validate the index rebuild. The manifest did not give it, + # so read it from the objects the chunk index lists. + archive_checker.key = key_from_repository(repository) if args.format is not None: format = args.format else: @@ -78,8 +83,18 @@ def do_check(self, args, repository): # the repository check has finished, which can take hours. ArchiveFormatter.validate_format(format) if not args.archives_only: + validate = None # the object validator for the index rebuild, which only a repair does + if args.repair: + # ids=(): read the key from the manifest only. Chunk objects are found through the index, + # which this check may find corrupt. + key = archive_checker.key if not args.repo_only else key_from_repository(repository, ()) + validate = object_validator(RepoObj(key)) if not repository.check( - repair=args.repair, max_duration=args.max_duration, max_age=max_age, repo_only=args.repo_only + repair=args.repair, + max_duration=args.max_duration, + max_age=max_age, + repo_only=args.repo_only, + validate=validate, ): set_ec(EXIT_WARNING) if sig_int: # repository check interrupted; skip the archive check @@ -244,10 +259,14 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser): In practice, repair mode hooks into both the repository and archive checks: 1. When checking the repository's consistency, repair mode rebuilds the repository - index from the packs if the index is corrupt, provided every pack is intact. If - any pack is corrupt, the repository check leaves the index and the packs untouched - and reports the corruption; salvaging a corrupt pack's still-intact objects is not - implemented yet (refs #8572). + index from the packs if the index is corrupt, provided every pack matches its + store hash. If any pack fails its store hash, the repository check leaves the + index and the packs untouched and reports it; salvaging the intact objects of + such a pack is not implemented yet (refs #8572). The rebuild authenticates + each object's header and metadata with the key, leaves an object that fails + this out of the index and reports it as an error. Repair therefore always + needs the key, ``--repository-only`` included, and aborts if the key can not + be read. 2. When checking the consistency and correctness of archives, repair mode might remove whole archives from the manifest if their archive metadata chunk is diff --git a/src/borg/repository.py b/src/borg/repository.py index 58e1cba5b4..c9ddc97022 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1160,7 +1160,7 @@ def info(self): info = dict(id=self.id, version=self.version) return info - def check(self, repair=False, max_duration=0, max_age=0, repo_only=False): + def check(self, repair=False, max_duration=0, max_age=0, repo_only=False, validate=None): """Check repository consistency. packs/ and index/ objects are named by the store hash of their content, so a pack or index @@ -1174,10 +1174,11 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False): far too slow and expensive for a routine (e.g. cron) check. With repair=True and a corrupt index, and if every pack is intact, the index is rebuilt from the packs' object headers and persisted; on a full check the archives phase rebuilds and re-persists it afterwards, see - ArchiveChecker.finish. Packs are verified by the store hash, which is content-addressing rather than a - MAC, so this rebuild detects accidental corruption but not tampering, refs #9901, #10026. If any - pack is corrupt the index is left unchanged, refs #8572, #10026. Pack ids found corrupt are kept - in cache/checked-packs, refs #9696. + ArchiveChecker.finish. Packs are verified by the store hash, which is content-addressing rather + than a MAC, so that check detects accidental corruption but not tampering; the rebuild therefore + checks every object with validate, see below, refs #9901, #10026. If any pack is corrupt the index + is left unchanged, refs #8572, #10026. Pack ids found corrupt are kept in cache/checked-packs, + refs #9696. A pack recorded corrupt fails the check, also on a partial run that stops before re-reaching it. The record clears at the check that finds the pack intact again or gone (removed by @@ -1195,10 +1196,20 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False): max_age, accepting a future timestamp up to MAX_CLOCK_SKEW (clock skew). Results are recorded regardless of max_age. - repo_only: whether this is a repository-only run. In repair mode it sets the return value for a - corrupt pack, which repair does not fix: fail if repo_only, else defer (a full check's archives - phase can repair a corrupt pack holding metadata, or file content with --verify-data). + repo_only: whether this is a repository-only run. In repair mode it sets the return value for + damage repair does not fix, i.e. a corrupt pack or a skipped pack byte range (see validate): + fail if repo_only, else defer (a full check's archives phase can repair a corrupt pack holding + metadata, or file content with --verify-data, and reports chunks the archives reference but the + index lacks). + + validate: validate(chunk_id, obj) -> bool, True if obj (an object's header plus its metadata + slot) is the repo object with id chunk_id, see repoobj.object_validator. Required if repair. + The index rebuild checks every object with it and leaves an object it rejects out of the index. + A skipped pack byte range is the pack content the rebuild discards at one place: a rejected + object plus the bytes up to the next object it accepts, or the rest of the pack if it accepts + none. Each skipped range counts as one error. """ + assert validate is not None or not repair def verify(namespace, name): # name is the store hash of the object's content, so it is intact iff store.hash() matches. @@ -1238,6 +1249,7 @@ def store_list(namespace): pack_files = pack_errors = pack_skipped = 0 missing_pack_ids = [] # packs referenced by the index but absent from packs/ (refs #9898) index_repaired = False + drops = 0 # number of pack byte ranges the index rebuild skipped packs_scanned = False # index and packs get separate progress indicators, each running from 0% to 100%. # the index is checked first and in full, on partial checks too: it is small, and index errors @@ -1363,19 +1375,22 @@ def recorded_ts(info): # run: sig_int breaks the loop early, so "no pack errors" must be paired with "all packs # scanned" (pack_files == len(pack_infos)) to not rebuild from unverified packs. if index_errors and pack_errors == 0 and not sig_int and pack_files == len(pack_infos): + + def note_drop(): + nonlocal drops + drops += 1 + # the exclusive check lock keeps the pack set fixed, so re-listing packs/ inside # build_chunkindex_from_repo matches this verification. write_immediately persists the # index and drops the corrupt fragments. - # the walk gets no validator: validating needs the key, which a Repository does not - # have. A pack is named by the store hash of its content, so a pack damaged in the store - # fails verify() above and pack_errors > 0 keeps it out of here. A pack that matches - # its name and still has a bad object header makes iter_headers raise, see #10026. - build_chunkindex_from_repo(self, slow_rebuild=True, write_immediately=True) + build_chunkindex_from_repo( + self, slow_rebuild=True, validate=validate, on_drop=note_drop, write_immediately=True + ) self.invalidate_chunk_index() # the rebuilt index is persisted; drop the in-memory copy index_repaired = True else: logger.error("Repository index is corrupted and must be repaired; skipping the pack check.") - objs_errors = index_errors + pack_errors + len(missing_pack_ids) + objs_errors = index_errors + pack_errors + len(missing_pack_ids) + drops summary = ( f"Checked {index_files} index files ({index_errors} errors) " f"and {pack_files} packs ({pack_errors} errors)." @@ -1394,6 +1409,8 @@ def recorded_ts(info): ) if index_repaired: logger.info("Repository index was corrupted and has been rebuilt from the packs.") + if drops: + logger.error(f"The index rebuild skipped {drops} pack byte range(s) it could not authenticate.") # corrupt_ids() includes packs recorded corrupt in earlier runs; report them only when this # run scanned the packs. corrupt_ids = tracker.corrupt_ids() if packs_scanned else [] @@ -1410,7 +1427,7 @@ def recorded_ts(info): logger.info(f"{done} {mode} repository check, no problems found{so_far}.") elif not repair: logger.error(f"{done} {mode} repository check, errors found{so_far}.") - elif index_repaired and not (pack_errors or corrupt_ids or missing_pack_ids): + elif index_repaired and not (pack_errors or corrupt_ids or missing_pack_ids or drops): # the index was the only problem and it has been rebuilt from the packs. logger.info(f"{done} {mode} repository check, repaired{so_far}.") elif pack_errors or corrupt_ids: @@ -1423,6 +1440,14 @@ def recorded_ts(info): # a full check's archives phase reads archive/item metadata (and file content with # --verify-data), so it repairs a corrupt pack holding such objects; warn rather than fail. logger.warning(f"{done} {mode} repository check, corrupt pack(s) found{so_far}.") + elif drops: + # a full check's archives phase reports the chunks the archives reference but the index + # lacks, so warn only. + log = logger.error if repo_only else logger.warning + log( + f"{done} {mode} repository check, " + f"index rebuilt without pack byte range(s) it could not authenticate{so_far}." + ) elif index_errors and not index_repaired: # the index is corrupt but was not rebuilt, e.g. the pack verification was interrupted # before every pack was confirmed intact; the corrupt index is left in place. @@ -1430,12 +1455,13 @@ def recorded_ts(info): else: # index-referenced packs are missing, so their chunks are lost. logger.error(f"{done} {mode} repository check, errors found{so_far}.") - # in repair mode a corrupt index left unrebuilt is a failure; a corrupt or missing pack fails - # only a repository-only run, while a full check defers it to the archives phase. + # in repair mode a corrupt index left unrebuilt is a failure; a corrupt or missing pack, or a + # skipped pack byte range, fails only a repository-only run, while a full check defers it to the + # archives phase. if repair: if index_errors and not index_repaired: return False - return not (repo_only and (pack_errors or corrupt_ids or missing_pack_ids)) + return not (repo_only and (pack_errors or corrupt_ids or missing_pack_ids or drops)) return not problems def list(self, limit=None, marker=None): diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index bf9f90874b..8706c12059 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -6,13 +6,14 @@ import pytest -from ...crypto.key import STORE_HASH_NAME +from ...crypto.key import store_hash, STORE_HASH_NAME from ... import archive as archive_module from ...archive import Archive, ArchiveChecker, ChunkBuffer from ...cache import Cache, delete_chunkindex_from_repo from ...constants import * # NOQA from ...helpers import bin_to_hex, msgpack, CommandError, CorruptPack, Error, IntegrityError, sig_int from ...helpers import BackupDamagedChunksError +from ...helpers.passphrase import PassphraseWrong from ...item import Item from ...manifest import Archives, Manifest from ...repoobj import RepoObj @@ -624,6 +625,56 @@ def test_check_repair_rebuilds_corrupt_index(archivers, request): assert "archive1" in cmd(archiver, "repo-list") # and remains usable +def tamper_object_keeping_pack_name(repository): + """Flip a byte in the metadata slot of the 2nd object of a pack holding more than 2 objects, and store + the pack under the store hash of its new content, so its content still matches its name. Corrupt + every index fragment. Return the ids of the changed object and of the object after it. + """ + by_pack = {} + for chunk_id, entry in repository.chunks.iteritems(): + by_pack.setdefault(entry.pack_id, []).append((entry.obj_offset, chunk_id)) + pack_id, objects = next((p, sorted(o)) for p, o in by_pack.items() if len(o) > 2) + (offset, tampered_id), (_, neighbour_id) = objects[1], objects[2] + old_name = "packs/" + bin_to_hex(pack_id) + data = corrupt(repository.store_load(old_name), offset + RepoObj.obj_header.size) + repository.store_store("packs/" + store_hash(data).hexdigest(), data) + repository.store_delete(old_name) + for info in repository.store_list("index"): + name = f"index/{info.name}" + repository.store_store(name, corrupt(repository.store_load(name), 0)) + return tampered_id, neighbour_id + + +def test_check_repository_only_repair_validates_index_rebuild(archivers, request): + """--repository-only --repair leaves an object that fails validation out of the index (#9901).""" + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("inspects the store directly") + check_cmd_setup(archiver) + with Repository(archiver.repository_location, exclusive=True) as repository: + tampered_id, neighbour_id = tamper_object_keeping_pack_name(repository) + output = cmd(archiver, "check", "-v", "--repository-only", "--repair", exit_code=1) + assert "does not authenticate" in output + assert "continuing at the object at offset" in output + assert "index rebuilt without pack byte range(s) it could not authenticate" in output + with Repository(archiver.repository_location) as repository: + assert tampered_id not in repository.chunks + assert neighbour_id in repository.chunks + + +def test_check_repository_only_repair_aborts_on_wrong_passphrase(archivers, request, monkeypatch): + """--repair aborts on a wrong passphrase (#9901).""" + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("inspects the store directly") + check_cmd_setup(archiver) + with Repository(archiver.repository_location, exclusive=True) as repository: + tamper_object_keeping_pack_name(repository) + monkeypatch.setenv("BORG_PASSPHRASE", "definitely-not-the-passphrase") + with pytest.raises(PassphraseWrong): + cmd(archiver, "check", "-v", "--repository-only", "--repair") + + @pytest.mark.skip(reason="TODO: repair does not yet rewrite store-corrupted packs, refs #8572") def test_manifest_rebuild_corrupted_chunk(archivers, request): archiver = request.getfixturevalue(archivers) @@ -772,62 +823,22 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama assert f"Store object packs/{bin_to_hex(pack_id)} is corrupted" in output -def test_repair_without_the_key_rebuilds_without_validating(archivers, request, monkeypatch): - """--repair that can not read the key says so and rebuilds the chunks index without validating. - - make_key gives up with an IntegrityError when the manifest yields no key, which a badly damaged - repository can do. The rebuild then walks the object headers alone, and a pack with a corrupt - object header is indexed up to that header, the rest of it dropped. - """ +def test_check_repair_validates_index_rebuild(archivers, request): + """--repair leaves an object that fails validation out of the index and keeps the object after it (#9901).""" archiver = request.getfixturevalue(archivers) if archiver.get_kind() != "local": - pytest.skip("patches in-process archive internals") + pytest.skip("inspects the store directly") check_cmd_setup(archiver) - - # two objects no archive references: they go into a pack of their own, so the damage below - # stays out of the objects the check reads back. - kept_id = b"kept-chunk".ljust(32, b".") # object ids are 32 bytes long - damaged_id = b"damaged-chunk".ljust(32, b".") - with Repository(archiver.repository_location, exclusive=True) as repository: - repository.put(kept_id, fchunk(b"kept", chunk_id=kept_id)) - repository.put(damaged_id, fchunk(b"damaged", chunk_id=damaged_id)) - repository.flush() with Repository(archiver.repository_location, exclusive=True) as repository: - kept, damaged = repository.chunks[kept_id], repository.chunks[damaged_id] - assert kept.pack_id == damaged.pack_id and kept.obj_offset < damaged.obj_offset - damaged_offset = damaged.obj_offset - key = "packs/" + bin_to_hex(damaged.pack_id) - repository.store_store(key, corrupt(repository.store_load(key), damaged_offset)) - - real_make_key = ArchiveChecker.make_key - - def make_key(self, repository, manifest_only=False): - if manifest_only: # the read that yields the validator's key; the later full read succeeds - raise IntegrityError("no key") - return real_make_key(self, repository, manifest_only=manifest_only) - - real_build = archive_module.build_chunkindex_from_repo - validators = [] - indexes = [] - - def build_chunkindex_from_repo(repository, **kwargs): - validators.append(kwargs.get("validate")) - index = real_build(repository, **kwargs) - indexes.append(index) - return index - - monkeypatch.setattr(ArchiveChecker, "make_key", make_key) - monkeypatch.setattr(archive_module, "build_chunkindex_from_repo", build_chunkindex_from_repo) - output = cmd(archiver, "check", "--repair", exit_code=0) - assert "Could not read the key (" in output - assert validators[0] is None # the rebuild got no validator, so it walked the headers alone - assert f"no object header at offset {damaged_offset}, no validator to resync with" in output - assert "Archive consistency check complete, problems found." in output # the drop is a problem - # indexes[0] is the keyless rebuild: it indexed the pack up to the damaged header. The objects - # put above are not encrypted repo objects, so the validating rebuild in finish() drops them. - assert kept_id in indexes[0] - assert damaged_id not in indexes[0] - cmd(archiver, "list", "archive1", exit_code=0) # the archives are still readable + tampered_id, neighbour_id = tamper_object_keeping_pack_name(repository) + output = cmd(archiver, "check", "-v", "--repair", exit_code=0) + assert "does not authenticate" in output + assert "continuing at the object at offset" in output + assert "index rebuilt without pack byte range(s) it could not authenticate" in output + assert "Archive consistency check complete, problems found." in output + with Repository(archiver.repository_location) as repository: + assert tampered_id not in repository.chunks + assert neighbour_id in repository.chunks def test_check_without_repair_does_not_drop_a_pack_tail(archivers, request, monkeypatch): diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index da00f35416..2b3c2aa5c2 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -132,6 +132,11 @@ def pdchunk(chunk): return pchunk(chunk)[0] +def validate_any(chunk_id, obj): + # A validator for check(repair=True) that accepts every object. + return True + + def test_basic_operations(repo_fixtures, request): with get_repository_from_fixture(repo_fixtures, request) as repository: for x in range(100): @@ -1121,13 +1126,48 @@ def test_check_repair_rebuilds_corrupt_index(tmp_path): repository.store_store(name, bytes(data)) assert repository.check(repair=False) is False # read-only check reports the corrupt index with reopen(repository) as repository: - assert repository.check(repair=True) is True # repair rebuilds the index from the packs + assert repository.check(repair=True, validate=validate_any) is True # repair rebuilds the index from the packs with reopen(repository) as repository: assert repository.check(repair=False) is True # the rebuilt index passes a read-only check for i, cid in enumerate(ids): assert pdchunk(repository.get(cid)) == bytes([i]) * 20 # every chunk is indexed and resolves +@pytest.mark.parametrize("repo_only", [True, False]) +def test_check_repair_rebuild_validates_objects(tmp_path, caplog, repo_only): + # check(repair=True, validate=...) does not index an object validate rejects and reports it, refs + # #9901. That fails a repository-only run only. + location = os.fspath(tmp_path / "repo") + ids = [H(x) for x in range(10)] + rejected_id = ids[4] + with Repository(location, exclusive=True, create=True) as repository: + for i, cid in enumerate(ids): + repository.put(cid, fchunk(bytes([i]) * 20, chunk_id=cid)) + repository.flush() + with reopen(repository) as repository: + for info in repository.store_list("index"): # corrupt every index fragment + name = f"index/{info.name}" + data = bytearray(repository.store_load(name)) + data[0] ^= 0xFF + repository.store_store(name, bytes(data)) + validated = [] + + def validate(chunk_id, obj): + validated.append(chunk_id) + return chunk_id != rejected_id + + caplog.set_level(logging.INFO) + with reopen(repository) as repository: + assert repository.check(repair=True, repo_only=repo_only, validate=validate) is not repo_only + assert set(ids) <= set(validated) + assert "skipped 1 pack byte range(s) it could not authenticate" in caplog.text + with reopen(repository) as repository: + assert rejected_id not in repository.chunks + for i, cid in enumerate(ids): + if cid != rejected_id: + assert pdchunk(repository.get(cid)) == bytes([i]) * 20 # every other object is indexed + + def test_check_repair_refuses_when_pack_corrupt(tmp_path): # A repair that finds any corrupt pack leaves the index and the pack untouched (no lossy rebuild, # nothing dropped) and fails on a repository-only run, refs #8572, #10026. @@ -1149,7 +1189,7 @@ def test_check_repair_refuses_when_pack_corrupt(tmp_path): repository.store_store(name, bytes(idata)) with reopen(repository) as repository: # a repository-only repair cannot fix a corrupt pack, so it fails. - assert repository.check(repair=True, repo_only=True) is False + assert repository.check(repair=True, repo_only=True, validate=validate_any) is False # the corrupt pack is left in place, not dropped. assert bad_pack_name in [f"packs/{info.name}" for info in repository.store_list("packs")] with reopen(repository) as repository: @@ -1174,7 +1214,8 @@ def test_check_repair_leaves_index_when_interrupted(tmp_path, caplog, monkeypatc with reopen(repository) as repository: monkeypatch.setattr("borg.repository.sig_int", True) # simulate a SIGINT before the pack loop with caplog.at_level(logging.ERROR, logger="borg.repository"): - assert repository.check(repair=True) is False # interrupted: index not rebuilt, so it fails + # interrupted: index not rebuilt, so it fails + assert repository.check(repair=True, validate=validate_any) is False assert "index still corrupt" in caplog.text with reopen(repository) as repository: assert repository.check(repair=False) is False # repair left the index corrupt @@ -1195,12 +1236,12 @@ def test_check_repair_reports_missing_pack_as_error(tmp_path, caplog): with reopen(repository) as repository: # a repository-only repair cannot recover the lost chunks, so it fails and reports the error. with caplog.at_level(logging.ERROR, logger="borg.repository"): - assert repository.check(repair=True, repo_only=True) is False + assert repository.check(repair=True, repo_only=True, validate=validate_any) is False assert f"Missing pack: {bin_to_hex(pack_id)}" in caplog.text assert "errors found" in caplog.text with reopen(repository) as repository: # a full check defers the missing pack to the archives phase, so the repository phase passes. - assert repository.check(repair=True, repo_only=False) is True + assert repository.check(repair=True, repo_only=False, validate=validate_any) is True def test_check_warns_on_invalid_chunk_index(tmp_path, caplog):