From 042fdae686a5b24514b17a5efc4976167c7dfd0a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Sun, 8 Nov 2026 18:25:59 +0100 Subject: [PATCH] Review fixes: the longest deletion delay when the location's settings cannot be read, a damaged passphrase slot does not stop the next one - Unreadable settings or an invalid delay on the location: 365 days, so nothing leaves the Bin sooner than on the old server - A passphrase change that still cannot reach a location is logged; removing a location clears its flag - A damaged slot (missing parts, invalid base64) is skipped and the next slot is tried - Tests: damaged settings scenario, damaged slot, upgrade test checks the migration and the time zone --- CHANGELOG.md | 5 +++-- lib/Command/TargetAdd.php | 2 +- lib/Crypto/KeyRing.php | 10 ++++++++-- lib/Service/TargetService.php | 19 ++++++++++++++----- tests/Unit/Crypto/KeyRingTest.php | 14 ++++++++++++++ tests/integration/recovery-scenarios.sh | 10 ++++++++++ tests/integration/upgrade-from-release.sh | 6 +++++- 7 files changed, 55 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21825e8..0b698da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,8 +34,9 @@ shown under 1.0.0. The git tag of a release candidate goes after the date. `occ backup:target:add` finds the backups on the location and takes their key from there with one of the old server's passphrases. Backups then wait until a new recovery kit is confirmed. The deletion delay is now also stored on the location, sealed with the key, from its first backup - with this version; a location backed up only by an older version gives the default of 7 days, - and so does a settings file that cannot be read (the command then says so). + with this version; a location backed up only by an older version gives the default of 7 days. + When the settings on the location cannot be read, the longest delay (365 days) is taken, so + nothing leaves the Bin sooner than on the old server; the command says so. - A passphrase change that cannot reach a location (offline at the time) now reaches it at the location's next backup, so a new server can open it with the new passphrase. - `occ backup:target:add --repair-config`: when the config file of a location is gone, the recovery diff --git a/lib/Command/TargetAdd.php b/lib/Command/TargetAdd.php index e8b447f..4c6d50a 100644 --- a/lib/Command/TargetAdd.php +++ b/lib/Command/TargetAdd.php @@ -164,7 +164,7 @@ HELP); $days = 'Deletion delay for forgotten snapshots: ' . $this->keys->deleteDelayDays() . ' days'; $output->writeln(match (true) { ($r['deleteDelayDays'] ?? null) !== null => $days . ' (from the location).', - $r['settingsUnreadable'] ?? false => '' . $days . ' (the default: the settings file on the location could not be read, it may be damaged). If this server used a longer delay, removed restore points leave the Bin sooner than before.', + $r['settingsUnreadable'] ?? false => '' . $days . ' (the longest: the settings on the location could not be read or hold no valid delay, so nothing leaves the Bin sooner than on the old server; removed restore points take space for that long).', default => $days . ' (the default: the location has none stored, its backups were made before NG Backup 1.1.0).', }); $output->writeln('Next: occ backup:list and occ backup:restore:full. Before backups run again, download a new recovery kit and confirm it (occ backup:key:kit, occ backup:key:confirm).'); diff --git a/lib/Crypto/KeyRing.php b/lib/Crypto/KeyRing.php index 92de548..6b847ba 100644 --- a/lib/Crypto/KeyRing.php +++ b/lib/Crypto/KeyRing.php @@ -87,14 +87,20 @@ final class KeyRing { foreach ($field['slots'] as $slot) { try { return self::unwrap($slot['key'], $passphrase); - } catch (CryptoException) { + } catch (CryptoException|\SodiumException|\TypeError|\ValueError) { + // a wrong passphrase for this slot, or a damaged slot: the next one may still open } } throw new CryptoException('Wrong passphrase or damaged key'); } - /** @param array{v:int, salt:string, ops:int, mem:int, nonce:string, key:string} $wrapped */ + /** @param array $wrapped from a kit file or a location's config: checked here, not trusted */ public static function unwrap(array $wrapped, #[\SensitiveParameter] string $passphrase): self { + if (!isset($wrapped['salt'], $wrapped['nonce'], $wrapped['key'], $wrapped['ops'], $wrapped['mem']) + || !is_string($wrapped['salt']) || !is_string($wrapped['nonce']) || !is_string($wrapped['key']) + || !is_int($wrapped['ops']) || !is_int($wrapped['mem'])) { + throw new CryptoException('Damaged key file: a part of the key is missing'); + } $b64 = static fn (string $s): string => sodium_base642bin($s, SODIUM_BASE64_VARIANT_ORIGINAL); // ops/mem come from unauthenticated JSON (kit file, repository config): bound them, so a // damaged or crafted file cannot make the KDF use unbounded memory or time. diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index f8a7967..c3481f1 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -27,6 +27,9 @@ use OCP\Server; * classes but its enabled/disabled state is left as the admin set it. */ final class TargetService { + /** Deletion delay taken when a location's own cannot be read: nothing leaves the Bin sooner than on the old server. */ + public const LONGEST_DELETE_DELAY = 365; + public function __construct( private TargetMapper $mapper, private KeyService $keys, @@ -195,13 +198,16 @@ final class TargetService { throw new \RuntimeException('The key in the config at this location does not open its backups: the config is damaged or was replaced. Only the recovery kit can help now: occ backup:key:import , then this command again.'); } $settings = Repository::settingsAt($storageBackend, $keys); - $delay = $settings['delete_delay_days'] ?? null; - $delay = is_int($delay) ? $delay : null; - $this->keys->adoptKey($keys, $wrapped, $delay, $replaceUnusedKey); + $stored = $settings['delete_delay_days'] ?? null; + $delay = is_int($stored) && $stored >= 1 && $stored <= 365 ? $stored : null; + // Settings that are there but cannot be read, or hold no valid delay (also none at all): the old server's delay + // is unknown, so take the longest one; a shorter guess could empty the Bin too soon. + $unknown = ($settings['unreadable'] ?? false) === true || ($settings !== [] && $delay === null); + $this->keys->adoptKey($keys, $wrapped, $delay ?? ($unknown ? self::LONGEST_DELETE_DELAY : null), $replaceUnusedKey); if ($old !== null) { $this->alerts->securityEvent('key_replaced', ['old' => $old, 'new' => $this->keys->fingerprint()]); } - return $this->add($name, $backend, $auth, $options, $basePath, $progress) + ['deleteDelayDays' => $delay, 'settingsUnreadable' => ($settings['unreadable'] ?? false) === true]; + return $this->add($name, $backend, $auth, $options, $basePath, $progress) + ['deleteDelayDays' => $delay, 'settingsUnreadable' => $unknown]; } /** A passphrase change that missed this location (offline at the time) reaches it now. A failure is tried again at the next backup. */ @@ -212,7 +218,9 @@ final class TargetService { try { $repo->updateWrappedKey($this->keys->wrappedKey()); $this->keys->setWrappedKeyPending($target->getId(), false); - } catch (\Throwable) { + } catch (\Throwable $e) { + Server::get(\Psr\Log\LoggerInterface::class)->warning('NG Backup: the last passphrase change still does not reach ' . $target->getName() + . ' (it keeps the previous passphrases); tried again at its next backup: ' . $e->getMessage(), ['exception' => $e]); } } @@ -442,6 +450,7 @@ final class TargetService { /** Remove the target from NG Backup. The backups on the target itself are not touched. */ public function remove(Target $target): void { $this->mapper->delete($target); + $this->keys->setWrappedKeyPending($target->getId(), false); // The cached index of its repository is only of use while a location points at it $repositoryId = $target->getRepositoryId(); if ($repositoryId !== null && !array_filter($this->list(), fn (Target $t) => $t->getRepositoryId() === $repositoryId)) { diff --git a/tests/Unit/Crypto/KeyRingTest.php b/tests/Unit/Crypto/KeyRingTest.php index 51a72f0..ae34ebc 100644 --- a/tests/Unit/Crypto/KeyRingTest.php +++ b/tests/Unit/Crypto/KeyRingTest.php @@ -34,6 +34,20 @@ class KeyRingTest extends TestCase { new KeyRing('short'); } + public function testADamagedSlotDoesNotStopTheNextOne(): void { + $keys = KeyRing::generate(); + $broken = $keys->wrap('passphrase in the safe'); + $broken['salt'] = '*** not base64 ***'; + $field = ['slots' => [ + ['slot' => 1, 'label' => 'Safe', 'key' => $broken], + ['slot' => 2, 'label' => 'Missing parts', 'key' => ['v' => 1]], + ['slot' => 3, 'label' => 'CTO', 'key' => $keys->wrap('passphrase of the cto')], + ]]; + $this->assertSame($keys->blobId('x'), KeyRing::unwrapAny($field, 'passphrase of the cto')->blobId('x')); + $this->expectException(CryptoException::class); + KeyRing::unwrapAny($field, 'passphrase in the safe'); + } + public function testAnySlotOpensTheSameKey(): void { $keys = KeyRing::generate(); $field = ['slots' => [ diff --git a/tests/integration/recovery-scenarios.sh b/tests/integration/recovery-scenarios.sh index f6f2eae..2431d81 100755 --- a/tests/integration/recovery-scenarios.sh +++ b/tests/integration/recovery-scenarios.sh @@ -10,6 +10,7 @@ # changes nothing; backups wait for a new kit # no-kit-old as no-kit, on a location backed up only before 1.1.0 (no settings file): # the deletion delay is the default, and the admin is told so +# no-kit-damaged as no-kit, with a damaged settings file: the longest deletion delay, and why # no-config-no-kit the location's config is gone and there is no kit: told that only the kit helps # no-config-kit config gone, kit there: --repair-config writes it again, the backups open # other-kit the kit of another server: refused, told to use the old server's kit @@ -150,6 +151,15 @@ expect "$OUT" "7 days (the default" "no-kit-old: the default delay and why" docker exec ngb-rs-new test -f /mnt/shared/no-kit-old/repo/settings || fail "no-kit-old: the settings were not written when the location was added" ok "no-kit-old: key from the location, default delay with the reason, settings written now" +# no-kit-damaged: the settings file is there but damaged: the longest delay, and the admin is told +scenario no-kit-damaged +docker exec ngb-rs-new sh -c 'rm /tmp/kit.json && printf garbage >> /mnt/shared/no-kit-damaged/repo/settings' +# shellcheck disable=SC2046 +OUT=$(run_on ngb-rs-new backup:target:add offsite $(location no-kit-damaged) --passphrase-file=/tmp/pass 2>&1) || { printf '%s\n' "$OUT"; fail "no-kit-damaged: target add"; } +expect "$OUT" "fingerprint $FP" "no-kit-damaged" +expect "$OUT" "365 days (the longest" "no-kit-damaged: the longest delay and why" +ok "no-kit-damaged: key from the location, the longest delay with the reason" + # no-config-no-kit scenario no-config-no-kit docker exec ngb-rs-new rm /tmp/kit.json /mnt/shared/no-config-no-kit/repo/config diff --git a/tests/integration/upgrade-from-release.sh b/tests/integration/upgrade-from-release.sh index 089aba0..5d37bee 100644 --- a/tests/integration/upgrade-from-release.sh +++ b/tests/integration/upgrade-from-release.sh @@ -56,6 +56,7 @@ occ backup:key:kit --output=/tmp/kit.json >/dev/null CODE=$(occ backup:key:kit | php -r 'echo json_decode(stream_get_contents(STDIN), true)["confirmation_code"];') occ backup:key:confirm -u admin --code="$CODE" --confirm="Yes, I confirm" >/dev/null || fail "confirm" occ config:app:set ng_backup schedule_time --value=03:15 >/dev/null +occ config:app:set ng_backup schedule_timezone --value=America/Toronto >/dev/null occ backup:target:add offsite local -a null::null -o datadir=/mnt/shared/loc --path=repo >/dev/null || fail "target add" docker exec -u www-data $N sh -c 'mkdir -p data/admin/files/Docs && head -c 3000000 /dev/urandom > data/admin/files/Docs/before.bin' occ files:scan admin >/dev/null @@ -76,9 +77,12 @@ OUT=$(occ upgrade 2>&1) || { printf '%s\n' "$OUT"; fail "occ upgrade"; } VER=$(occ app:list --output=json | php -r '$a = json_decode(stream_get_contents(STDIN), true); echo $a["enabled"]["ng_backup"] ?? "";') [ -z "$NEWVER" ] || [ "$VER" = "$NEWVER" ] || fail "after the upgrade the app has version '$VER'" ok "occ upgrade to $VER" +# Before anything else touches the key: only the migration can have marked it as used +[ "$(occ config:app:get ng_backup master_key_used 2>/dev/null)" = 1 ] || [ "$(occ config:app:get ng_backup master_key_used 2>/dev/null)" = true ] || fail "the migration did not mark the key from before the upgrade as used" +ok "the key from before the upgrade counts as used" SCHED=$(occ backup:target:schedule offsite) -printf '%s\n' "$SCHED" | grep -q "^offsite: 03:15 " || { printf '%s\n' "$SCHED"; fail "the location did not keep 03:15 as its schedule"; } +printf '%s\n' "$SCHED" | grep -qx "offsite: 03:15 (America/Toronto)" || { printf '%s\n' "$SCHED"; fail "the location did not keep 03:15 as its schedule"; } occ backup:list offsite | grep -q "$OLDSNAP" || fail "the old restore point is not listed" occ backup:verify offsite "$OLDSNAP" >/dev/null || fail "the old restore point does not verify" occ backup:status >/dev/null || fail "backup:status"