Review fixes: a malformed key reads as damaged, removing a location always reports it, a key from before 1.1.0 in the recovery guide

- Invalid base64 or a wrong salt/nonce length in a key gives the damaged-key error, so the recovery options are shown
- Tidying up after a removed location cannot fail the removal anymore, so the removal event is always sent
- Recovery guide: a key made before 1.1.0 counts as used and cannot make way

docs/DISASTER-RECOVERY.md

@@ -91,7 +91,9 @@ As long as that key was never used (no location added with it, no backup), it ca
 key of your backups. NG Backup says so when it refuses the location. Add `--replace-unused-key` to
 `occ backup:key:import` (with the kit) or to `occ backup:target:add` (without the kit; it asks for
 a passphrase of the old server). The passphrases and kit of the unused key then stop working. A key
-that was used for a location is never replaced: its own backups depend on it.
+that was used for a location is never replaced: its own backups depend on it. A key made with a
+version before 1.1.0 always counts as used (those versions did not keep track), also when it never
+protected a location; recover onto a fresh Nextcloud installation instead.
 
 **6. List the restore points** and pick one, normally the newest:
 

lib/Crypto/KeyRing.php

@@ -108,10 +108,15 @@ final class KeyRing {
 			|| $wrapped['mem'] < SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE || $wrapped['mem'] > SODIUM_CRYPTO_PWHASH_MEMLIMIT_SENSITIVE) {
 			throw new CryptoException('Unsupported key derivation parameters (damaged or foreign key file)');
 		}
-		$kek = sodium_crypto_pwhash(SODIUM_CRYPTO_SECRETBOX_KEYBYTES, $passphrase, $b64($wrapped['salt']),
-			$wrapped['ops'], $wrapped['mem'], SODIUM_CRYPTO_PWHASH_ALG_ARGON2ID13);
-		$master = sodium_crypto_secretbox_open($b64($wrapped['key']), $b64($wrapped['nonce']), $kek);
-		sodium_memzero($kek);
+		try {
+			$kek = sodium_crypto_pwhash(SODIUM_CRYPTO_SECRETBOX_KEYBYTES, $passphrase, $b64($wrapped['salt']),
+				$wrapped['ops'], $wrapped['mem'], SODIUM_CRYPTO_PWHASH_ALG_ARGON2ID13);
+			$master = sodium_crypto_secretbox_open($b64($wrapped['key']), $b64($wrapped['nonce']), $kek);
+			sodium_memzero($kek);
+		} catch (\SodiumException|\ValueError $e) {
+			// invalid base64 or a salt/nonce of the wrong length: damaged, so the caller shows its recovery options
+			throw new CryptoException('Damaged key file: ' . $e->getMessage(), 0, $e);
+		}
 		if ($master === false) {
 			throw new CryptoException('Wrong passphrase or damaged key');
 		}

lib/Service/TargetService.php

@@ -450,11 +450,18 @@ 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)) {
-			$this->indexCache->forget($repositoryId);
+		// The location is gone now: tidying up must not fail the removal, or the caller would skip
+		// the removal event and a retry would no longer find the location
+		try {
+			$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)) {
+				$this->indexCache->forget($repositoryId);
+			}
+		} catch (\Throwable $e) {
+			Server::get(\Psr\Log\LoggerInterface::class)->warning('NG Backup: location ' . $target->getName()
+				. ' removed, tidying up after it failed: ' . $e->getMessage(), ['exception' => $e]);
 		}
 	}
 

tests/Unit/Crypto/KeyRingTest.php

@@ -22,6 +22,14 @@ class KeyRingTest extends TestCase {
 		KeyRing::unwrap($wrapped, 'wrong');
 	}
 
+	public function testInvalidBase64IsADamagedKey(): void {
+		$wrapped = KeyRing::generate()->wrap('right');
+		$wrapped['salt'] = '***not base64***';
+		$this->expectException(CryptoException::class);
+		$this->expectExceptionMessage('Damaged key file');
+		KeyRing::unwrapAny($wrapped, 'right');
+	}
+
 	public function testBlobIdsAreKeyed(): void {
 		$this->assertNotSame(KeyRing::generate()->blobId('same'), KeyRing::generate()->blobId('same'));
 		$k = KeyRing::generate();