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

@@ -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

lib/Command/TargetAdd.php

@@ -164,7 +164,7 @@ HELP);
 			$days = 'Deletion delay for forgotten snapshots: <info>' . $this->keys->deleteDelayDays() . '</info> days';
 			$output->writeln(match (true) {
 				($r['deleteDelayDays'] ?? null) !== null => $days . ' (from the location).',
-				$r['settingsUnreadable'] ?? false => '<comment>' . $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.</comment>',
+				$r['settingsUnreadable'] ?? false => '<comment>' . $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).</comment>',
 				default => $days . ' (the default: the location has none stored, its backups were made before NG Backup 1.1.0).',
 			});
 			$output->writeln('Next: <info>occ backup:list</info> and <info>occ backup:restore:full</info>. Before backups run again, download a new recovery kit and confirm it (<info>occ backup:key:kit</info>, <info>occ backup:key:confirm</info>).');

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<string, mixed> $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.

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 <kit file>, 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)) {

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' => [

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

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"