diff --git a/docs/DISASTER-RECOVERY.md b/docs/DISASTER-RECOVERY.md index ba0bdb7..c7ffc48 100644 --- a/docs/DISASTER-RECOVERY.md +++ b/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: diff --git a/lib/Crypto/KeyRing.php b/lib/Crypto/KeyRing.php index 6b847ba..ec6cb99 100644 --- a/lib/Crypto/KeyRing.php +++ b/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'); } diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index c3481f1..8f578a0 100644 --- a/lib/Service/TargetService.php +++ b/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]); } } diff --git a/tests/Unit/Crypto/KeyRingTest.php b/tests/Unit/Crypto/KeyRingTest.php index ae34ebc..3f35174 100644 --- a/tests/Unit/Crypto/KeyRingTest.php +++ b/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();