diff --git a/apps/encryption/lib/Command/FixKeyLocation.php b/apps/encryption/lib/Command/FixKeyLocation.php index a869f967277c9..9372f492b706f 100644 --- a/apps/encryption/lib/Command/FixKeyLocation.php +++ b/apps/encryption/lib/Command/FixKeyLocation.php @@ -20,8 +20,10 @@ use OCP\Files\IRootFolder; use OCP\Files\ISetupManager; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\IUser; use OCP\IUserManager; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -39,6 +41,7 @@ public function __construct( private readonly Util $encryptionUtil, private readonly IRootFolder $rootFolder, private readonly ISetupManager $setupManager, + private readonly LoggerInterface $logger, IManager $encryptionManager, ) { $this->keyRootDirectory = rtrim($this->encryptionUtil->getKeyStorageRoot(), '/'); @@ -75,8 +78,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->setupManager->setupForUser($user); $mounts = $this->getSystemMountsForUser($user); + $failedPaths = []; foreach ($mounts as $mount) { - $mountRootFolder = $this->rootFolder->get($mount->getMountPoint()); + try { + $mountRootFolder = $this->rootFolder->get($mount->getMountPoint()); + } catch (NotFoundException $e) { + $this->logger->warning('Mount point of user ' . $user->getUID() . ' not found: ' . $mount->getMountPoint(), [ + 'app' => 'encryption', + 'exception' => $e, + ]); + $output->writeln('System wide mount point not found, skipping: ' . $mount->getMountPoint() . ''); + continue; + } if (!$mountRootFolder instanceof Folder) { $output->writeln('System wide mount point is not a directory, skipping: ' . $mount->getMountPoint() . ''); continue; @@ -85,75 +98,97 @@ protected function execute(InputInterface $input, OutputInterface $output): int $files = $this->getAllEncryptedFiles($mountRootFolder); foreach ($files as $file) { /** @var File $file */ - $hasSystemKey = $this->hasSystemKey($file); - $hasUserKey = $this->hasUserKey($user, $file); - if (!$hasSystemKey) { - if ($hasUserKey) { - // key was stored incorrectly as user key, migrate - - if ($dryRun) { - $output->writeln('' . $file->getPath() . ' needs migration'); + try { + $this->fixKeysForFile($user, $file, $dryRun, $output); + } catch (\Throwable $e) { + $failedPaths[] = $file->getPath(); + $this->logger->error('Failed to fix the key location of ' . $file->getPath(), [ + 'app' => 'encryption', + 'exception' => $e, + ]); + $output->writeln('Failed to process ' . $file->getPath() . ': ' . $e->getMessage() . ''); + } + } + } + + if ($failedPaths !== []) { + $output->writeln(''); + $output->writeln('' . count($failedPaths) . ' file(s) could not be processed, see the log for details:'); + foreach ($failedPaths as $failedPath) { + $output->writeln(' - ' . $failedPath . ''); + } + return self::FAILURE; + } + + return self::SUCCESS; + } + + private function fixKeysForFile(IUser $user, File $file, bool $dryRun, OutputInterface $output): void { + $hasSystemKey = $this->hasSystemKey($file); + $hasUserKey = $this->hasUserKey($user, $file); + if (!$hasSystemKey) { + if ($hasUserKey) { + // key was stored incorrectly as user key, migrate + + if ($dryRun) { + $output->writeln('' . $file->getPath() . ' needs migration'); + } else { + $output->write('Migrating key for ' . $file->getPath() . ' '); + if ($this->copyUserKeyToSystemAndValidate($user, $file)) { + $output->writeln(''); + } else { + $output->writeln('❌'); + $output->writeln(' Failed to validate key for ' . $file->getPath() . ', key will not be migrated'); + } + } + } else { + // no matching key, probably from a broken cross-storage move + + $shouldBeEncrypted = $file->getStorage()->instanceOfStorage(Encryption::class); + $isActuallyEncrypted = $this->isDataEncrypted($file); + if ($isActuallyEncrypted) { + if ($dryRun) { + if ($shouldBeEncrypted) { + $output->write('' . $file->getPath() . ' needs migration'); } else { - $output->write('Migrating key for ' . $file->getPath() . ' '); - if ($this->copyUserKeyToSystemAndValidate($user, $file)) { - $output->writeln(''); - } else { - $output->writeln('❌'); - $output->writeln(' Failed to validate key for ' . $file->getPath() . ', key will not be migrated'); - } + $output->write('' . $file->getPath() . ' needs decryption'); + } + $foundKey = $this->findUserKeyForSystemFile($user, $file); + if ($foundKey) { + $output->writeln(', valid key found at ' . $foundKey . ''); + } else { + $output->writeln(' ❌ No key found'); } } else { - // no matching key, probably from a broken cross-storage move - - $shouldBeEncrypted = $file->getStorage()->instanceOfStorage(Encryption::class); - $isActuallyEncrypted = $this->isDataEncrypted($file); - if ($isActuallyEncrypted) { - if ($dryRun) { - if ($shouldBeEncrypted) { - $output->write('' . $file->getPath() . ' needs migration'); - } else { - $output->write('' . $file->getPath() . ' needs decryption'); - } - $foundKey = $this->findUserKeyForSystemFile($user, $file); - if ($foundKey) { - $output->writeln(', valid key found at ' . $foundKey . ''); - } else { - $output->writeln(' ❌ No key found'); - } - } else { - if ($shouldBeEncrypted) { - $output->write('Migrating key for ' . $file->getPath() . ''); - } else { - $output->write('Decrypting ' . $file->getPath() . ''); - } - $foundKey = $this->findUserKeyForSystemFile($user, $file); - if ($foundKey) { - if ($shouldBeEncrypted) { - $systemKeyPath = $this->getSystemKeyPath($file); - $this->rootView->copy($foundKey, $systemKeyPath); - $output->writeln(' Migrated key from ' . $foundKey . ''); - } else { - $this->decryptWithSystemKey($file, $foundKey); - $output->writeln(' Decrypted with key from ' . $foundKey . ''); - } - } else { - $output->writeln(' ❌ No key found'); - } - } + if ($shouldBeEncrypted) { + $output->write('Migrating key for ' . $file->getPath() . ''); } else { - if ($dryRun) { - $output->writeln('' . $file->getPath() . ' needs to be marked as not encrypted'); + $output->write('Decrypting ' . $file->getPath() . ''); + } + $foundKey = $this->findUserKeyForSystemFile($user, $file); + if ($foundKey) { + if ($shouldBeEncrypted) { + $systemKeyPath = $this->getSystemKeyPath($file); + $this->rootView->copy($foundKey, $systemKeyPath); + $output->writeln(' Migrated key from ' . $foundKey . ''); } else { - $this->markAsUnEncrypted($file); - $output->writeln('' . $file->getPath() . ' marked as not encrypted'); + $this->decryptWithSystemKey($file, $foundKey); + $output->writeln(' Decrypted with key from ' . $foundKey . ''); } + } else { + $output->writeln(' ❌ No key found'); } } + } else { + if ($dryRun) { + $output->writeln('' . $file->getPath() . ' needs to be marked as not encrypted'); + } else { + $this->markAsUnEncrypted($file); + $output->writeln('' . $file->getPath() . ' marked as not encrypted'); + } } } } - - return self::SUCCESS; } private function getUserRelativePath(string $path): string { @@ -168,7 +203,7 @@ private function getUserRelativePath(string $path): string { /** * @return ICachedMountInfo[] */ - private function getSystemMountsForUser(IUser $user): array { + protected function getSystemMountsForUser(IUser $user): array { return array_filter($this->userMountCache->getMountsForUser($user), function (ICachedMountInfo $mount) use ( $user ) { @@ -241,15 +276,19 @@ private function copyUserKeyToSystemAndValidate(IUser $user, File $node): bool { private function tryReadFile(File $node): bool { try { - $fh = $node->fopen('r'); - // read a single chunk - $data = fread($fh, 8192); - if ($data === false) { + // a raw read on an unwrapped mount would succeed with any key + $storage = $node->getStorage(); + if (!$storage->instanceOfStorage(Encryption::class)) { + $storage = $this->encryptionManager->forceWrapStorage($node->getMountPoint(), $storage); + } + $fh = $storage->fopen($node->getInternalPath(), 'r'); + if ($fh === false) { return false; - } else { - return true; } - } catch (\Exception $e) { + $data = fread($fh, 8192); + fclose($fh); + return $data !== false; + } catch (\Exception) { return false; } } @@ -314,6 +353,10 @@ private function findUserKeyForSystemFile(IUser $user, File $node): ?string { * @return \Generator */ private function findKeysByFileName(string $basePath, string $name) { + if (!$this->rootView->is_dir($basePath)) { + // no keys stored for the user at all + return; + } if ($this->rootView->is_dir($basePath . '/' . $name . '/OC_DEFAULT_MODULE')) { yield $basePath . '/' . $name; } else { @@ -327,6 +370,7 @@ private function findKeysByFileName(string $basePath, string $name) { $childPath = $basePath . '/' . $child; // recurse if the child is not a key folder + /** @psalm-suppress InternalMethod */ if ($this->rootView->is_dir($childPath) && !is_dir($childPath . '/OC_DEFAULT_MODULE')) { yield from $this->findKeysByFileName($childPath, $name); } @@ -363,6 +407,7 @@ private function decryptWithSystemKey(File $node, string $key): void { $systemKeyPath = $this->getSystemKeyPath($node); $this->rootView->copy($key, $systemKeyPath); + $decryptedNode = null; try { if (!$storage->instanceOfStorage(Encryption::class)) { $storage = $this->encryptionManager->forceWrapStorage($node->getMountPoint(), $storage); @@ -380,19 +425,30 @@ private function decryptWithSystemKey(File $node, string $key): void { fclose($source); $decryptedNode->getStorage()->getScanner()->scan($decryptedNode->getInternalPath()); - } catch (\Exception $e) { + + if ($this->isDataEncrypted($decryptedNode)) { + throw new \Exception($node->getPath() . ' still encrypted after attempting to decrypt with ' . $key); + } + } catch (\Throwable $e) { + // the target has to go first so the .bak can move back onto its name + if ($decryptedNode !== null) { + try { + $decryptedNode->delete(); + } catch (\Throwable $cleanupError) { + $this->logger->warning('Failed to remove the partial decryption target ' . $decryptedNode->getPath(), [ + 'app' => 'encryption', + 'exception' => $cleanupError, + ]); + } + } $this->rootView->rmdir($systemKeyPath); - // remove the .bak + // move the backup back onto the original name $node->move(substr($node->getPath(), 0, -4)); throw $e; } - if ($this->isDataEncrypted($decryptedNode)) { - throw new \Exception($node->getPath() . ' still encrypted after attempting to decrypt with ' . $key); - } - $this->markAsUnEncrypted($decryptedNode); $this->rootView->rmdir($systemKeyPath); diff --git a/apps/encryption/tests/Command/FixKeyLocationTest.php b/apps/encryption/tests/Command/FixKeyLocationTest.php new file mode 100644 index 0000000000000..ee4a5867f5811 --- /dev/null +++ b/apps/encryption/tests/Command/FixKeyLocationTest.php @@ -0,0 +1,217 @@ +systemMounts; + } +} + +#[\PHPUnit\Framework\Attributes\Group(name: 'DB')] +class FixKeyLocationTest extends TestCase { + use MountProviderTrait; + use EncryptionTrait; + use UserTrait; + + /** + * One mount with the encryption wrapper and one without, so ciphertext can be + * placed on a mount that cannot decrypt it, like a broken cross storage move + * leaves it behind. + * + * @return array{view: View, encryptedBackingStorage: Temporary, strayStorage: TemporaryUnwrapped} + */ + private function setUpMounts(): array { + Server::get(KeyManager::class)->validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + + $encryptedBacking = new Temporary(); + $stray = new TemporaryUnwrapped(); + + $this->registerMount('test1', $encryptedBacking, '/test1/files/enc'); + $this->registerMount('test1', $stray, '/test1/files/stray'); + + $this->loginWithEncryption('test1'); + + return [ + 'view' => new View('/test1/files'), + 'encryptedBackingStorage' => $encryptedBacking, + 'strayStorage' => $stray, + ]; + } + + private function getCommand(): TestableFixKeyLocation { + return new TestableFixKeyLocation( + Server::get(IUserManager::class), + Server::get(IUserMountCache::class), + Server::get(EncryptionUtil::class), + Server::get(IRootFolder::class), + Server::get(ISetupManager::class), + Server::get(LoggerInterface::class), + Server::get(IManager::class), + ); + } + + private function markAsEncrypted(TemporaryUnwrapped $storage, string $path): void { + $cache = $storage->getCache(); + $cache->update($cache->get($path)->getId(), ['encrypted' => 1]); + } + + /** + * Key validation must read through the encryption wrapper: a raw read succeeds + * with any key, so on an unwrapped mount the first candidate key would always + * "validate" and a wrong key only fails later, during the actual decryption. + */ + public function testKeyValidationReadsThroughEncryption(): void { + [ + 'view' => $view, + 'encryptedBackingStorage' => $encryptedBackingStorage, + ] = $this->setUpMounts(); + + $view->file_put_contents('enc/original.txt', 'secret content'); + // ciphertext under a path that has no key + $view->file_put_contents('stray/stray.txt', $encryptedBackingStorage->file_get_contents('original.txt')); + $strayStorage = $view->getFileInfo('stray/stray.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'stray.txt'); + + $userFolder = Server::get(IRootFolder::class)->getUserFolder('test1'); + $command = $this->getCommand(); + + $this->assertFalse( + self::invokePrivate($command, 'tryReadFile', [$userFolder->get('stray/stray.txt')]), + 'ciphertext without a key must not validate' + ); + $this->assertTrue( + self::invokePrivate($command, 'tryReadFile', [$userFolder->get('enc/original.txt')]), + 'a readable encrypted file must validate' + ); + } + + /** + * A failed decryption must roll everything back: no .bak left behind, no partial + * target, no temporary system key. + */ + public function testFailedDecryptionRollsBack(): void { + [ + 'view' => $view, + 'encryptedBackingStorage' => $encryptedBackingStorage, + ] = $this->setUpMounts(); + + $view->file_put_contents('enc/original.txt', 'secret content'); + $view->file_put_contents('enc/other.txt', 'other content'); + $cipher = $encryptedBackingStorage->file_get_contents('original.txt'); + $view->file_put_contents('stray/broken.txt', $cipher); + $strayStorage = $view->getFileInfo('stray/broken.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'broken.txt'); + + $userFolder = Server::get(IRootFolder::class)->getUserFolder('test1'); + $strayNode = $userFolder->get('stray/broken.txt'); + $command = $this->getCommand(); + $user = Server::get(IUserManager::class)->get('test1'); + + // the key of a different file cannot decrypt this ciphertext + $wrongKey = self::invokePrivate($command, 'getUserKeyPath', [$user, $userFolder->get('enc/other.txt')]); + $rootView = new View(); + $this->assertTrue($rootView->file_exists($wrongKey), 'test setup: key of the other file has to exist'); + // the decryption reads under the .bak name, stage the wrong key where the + // wrapper will look for it so the failure is a real signature mismatch + $brokenKeyPath = self::invokePrivate($command, 'getUserKeyPath', [$user, $strayNode]); + $rootView->copy($wrongKey, str_replace('broken.txt', 'broken.txt.bak', $brokenKeyPath)); + + try { + self::invokePrivate($command, 'decryptWithSystemKey', [$strayNode, $wrongKey]); + $this->fail('decrypting with the wrong key must fail'); + } catch (\Exception $e) { + } + + $this->assertTrue($view->file_exists('stray/broken.txt'), 'the original file has to be restored'); + $this->assertSame($cipher, $view->file_get_contents('stray/broken.txt'), 'the original content has to be intact'); + $this->assertFalse($view->file_exists('stray/broken.txt.bak'), 'no .bak must be left behind'); + + $systemKeyPath = self::invokePrivate($command, 'getSystemKeyPath', [$strayNode]); + $systemKeyPathBak = str_replace('broken.txt', 'broken.txt.bak', $systemKeyPath); + $this->assertFalse($rootView->file_exists($systemKeyPath), 'no temporary system key must be left behind'); + $this->assertFalse($rootView->file_exists($systemKeyPathBak), 'no temporary system key must be left behind'); + } + + /** + * One broken file must not abort the whole run, the remaining files still get + * processed and the failure is reported. + */ + public function testExecuteContinuesAfterFailure(): void { + [ + 'view' => $view, + ] = $this->setUpMounts(); + + $view->file_put_contents('stray/a-ghost.txt', 'gone'); + $strayStorage = $view->getFileInfo('stray/a-ghost.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'a-ghost.txt'); + // cache row without a backing file, reading it fails like an object store 404 + $strayStorage->unlink('a-ghost.txt'); + + $view->file_put_contents('stray/b-plain.txt', 'plain data'); + $this->markAsEncrypted($strayStorage, 'b-plain.txt'); + + // ciphertext without any key while the user has no key directory at all, + // the key search has to come up empty instead of erroring out + $view->file_put_contents('stray/c-cipher.txt', 'HBEGIN:oc_encryption_module:OC_DEFAULT_MODULE:HEND'); + $this->markAsEncrypted($strayStorage, 'c-cipher.txt'); + + $command = $this->getCommand(); + $mount = $this->createMock(ICachedMountInfo::class); + $mount->method('getMountPoint')->willReturn('/test1/files/stray/'); + $command->systemMounts = [$mount]; + + $tester = new CommandTester($command); + $exitCode = $tester->execute(['user' => 'test1']); + $display = $tester->getDisplay(); + + $this->assertSame(Command::FAILURE, $exitCode, 'failures have to be reflected in the exit code'); + $this->assertStringContainsString('Failed to process', $display); + $this->assertStringContainsString('could not be processed', $display); + $this->assertStringContainsString(' - /test1/files/stray/a-ghost.txt', $display, 'the summary has to name the affected file'); + $this->assertFalse( + $strayStorage->getCache()->get('b-plain.txt')->isEncrypted(), + 'the remaining file was not processed, the run stopped at the broken one' + ); + $this->assertStringContainsString('No key found', $display, 'a missing key directory has to read as "no candidates"'); + $this->assertStringNotContainsString('c-cipher.txt: ', $display, 'the missing key directory failed the file'); + } +}