Skip to content

Commit 031c09f

Browse files
DerDreschnerDerDreschner
committed
feat(e2e-encryption): Allow cross-user key location fix
Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
1 parent dab92b3 commit 031c09f

2 files changed

Lines changed: 204 additions & 22 deletions

File tree

‎apps/encryption/lib/Command/FixKeyLocation.php‎

Lines changed: 107 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ protected function configure(): void {
6060
->setName('encryption:fix-key-location')
6161
->setDescription('Fix the location of encryption keys for external storage')
6262
->addOption('dry-run', null, InputOption::VALUE_NONE, "Only list files that require key migration, don't try to perform any migration")
63+
->addOption('personal', null, InputOption::VALUE_NONE, 'Also check the encrypted files in the personal space of the user and restore keys found in the key trees of other users')
6364
->addArgument('user', InputArgument::REQUIRED, 'User id to fix the key locations for');
6465
}
6566

@@ -108,6 +109,29 @@ protected function execute(InputInterface $input, OutputInterface $output): int
108109
}
109110
}
110111

112+
if ($input->getOption('personal')) {
113+
$userFolder = $this->rootFolder->getUserFolder($user->getUID());
114+
$personalMountPoint = $userFolder->getMountPoint()->getMountPoint();
115+
foreach ($this->getAllEncryptedFiles($userFolder) as $file) {
116+
/** @var File $file */
117+
// group folders, external storages and received shares are their own
118+
// mounts and follow the system wide handling
119+
if ($file->getMountPoint()->getMountPoint() !== $personalMountPoint) {
120+
continue;
121+
}
122+
try {
123+
$this->fixKeysForPersonalFile($user, $file, $dryRun, $output);
124+
} catch (\Throwable $e) {
125+
$failedPaths[] = $file->getPath();
126+
$this->logger->error('Failed to fix the key location of ' . $file->getPath(), [
127+
'app' => 'encryption',
128+
'exception' => $e,
129+
]);
130+
$output->writeln('<error>Failed to process ' . $file->getPath() . ': ' . $e->getMessage() . '</error>');
131+
}
132+
}
133+
}
134+
111135
if ($failedPaths !== []) {
112136
$output->writeln('');
113137
$output->writeln('<error>' . count($failedPaths) . ' file(s) could not be processed, see the log for details:</error>');
@@ -120,6 +144,46 @@ protected function execute(InputInterface $input, OutputInterface $output): int
120144
return self::SUCCESS;
121145
}
122146

147+
/**
148+
* A personal file is healthy when its key sits in the tree of the user at the path
149+
* of the file. A missing key can only be restored there, the personal storage
150+
* carries the encryption wrapper, so the file decrypts transparently once the key
151+
* is back in place.
152+
*/
153+
private function fixKeysForPersonalFile(IUser $user, File $file, bool $dryRun, OutputInterface $output): void {
154+
if ($this->hasUserKey($user, $file)) {
155+
return;
156+
}
157+
if (!$this->isDataEncrypted($file)) {
158+
if ($dryRun) {
159+
$output->writeln('<info>' . $file->getPath() . ' needs to be marked as not encrypted</info>');
160+
} else {
161+
$this->markAsUnEncrypted($file);
162+
$output->writeln('<info>' . $file->getPath() . ' marked as not encrypted</info>');
163+
}
164+
return;
165+
}
166+
167+
$targetKeyPath = $this->getUserKeyPath($user, $file);
168+
$foundKey = $this->findKeyInUserTrees($user, $file, $targetKeyPath);
169+
if ($dryRun) {
170+
$output->write('<info>' . $file->getPath() . '</info> needs migration');
171+
if ($foundKey) {
172+
$output->writeln(', valid key found at <info>' . $foundKey . '</info>');
173+
} else {
174+
$output->writeln(' <error>❌ No key found</error>');
175+
}
176+
return;
177+
}
178+
$output->write('<info>Migrating key for ' . $file->getPath() . '</info>');
179+
if ($foundKey) {
180+
$this->rootView->copy($foundKey, $targetKeyPath);
181+
$output->writeln(' Migrated key from <info>' . $foundKey . '</info>');
182+
} else {
183+
$output->writeln(' <error>❌ No key found</error>');
184+
}
185+
}
186+
123187
private function fixKeysForFile(IUser $user, File $file, bool $dryRun, OutputInterface $output): void {
124188
$hasSystemKey = $this->hasSystemKey($file);
125189
$hasUserKey = $this->hasUserKey($user, $file);
@@ -283,7 +347,9 @@ private function tryReadFile(File $node): bool {
283347
}
284348
$data = fread($fh, 8192);
285349
fclose($fh);
286-
return $data !== false;
350+
// a broken unencrypted_size of 0 makes the stream return nothing at all
351+
// instead of failing, an empty read proves nothing about the key
352+
return $data !== false && $data !== '';
287353
} catch (\Exception) {
288354
return false;
289355
}
@@ -333,16 +399,41 @@ private function isDataEncrypted(File $node): bool {
333399
* Attempt to find a key (stored for user) for a file (that needs a system key) even when it's not stored in the expected location
334400
*/
335401
private function findUserKeyForSystemFile(IUser $user, File $node): ?string {
336-
$userKeyPath = $this->getUserBaseKeyPath($user);
337-
$possibleKeys = $this->findKeysByFileName($userKeyPath, $node->getName());
338-
foreach ($possibleKeys as $possibleKey) {
339-
if ($this->testSystemKey($user, $possibleKey, $node)) {
340-
return $possibleKey;
402+
return $this->findKeyInUserTrees($user, $node, $this->getSystemKeyPath($node));
403+
}
404+
405+
/**
406+
* Search the key trees of all users for a key that decrypts the file, the tree of
407+
* the given user first. Candidates are matched by file name and validated by a
408+
* decryption attempt with the key staged at the given path.
409+
*/
410+
private function findKeyInUserTrees(IUser $user, File $node, string $stageKeyPath): ?string {
411+
foreach ($this->getUserBaseKeyPaths($user) as $basePath) {
412+
foreach ($this->findKeysByFileName($basePath, $node->getName()) as $possibleKey) {
413+
if ($this->testKeyAtPath($node, $possibleKey, $stageKeyPath)) {
414+
return $possibleKey;
415+
}
341416
}
342417
}
343418
return null;
344419
}
345420

421+
/**
422+
* Base key paths of all users, the given user first. Users without a key tree are
423+
* skipped cheaply by the key search.
424+
*
425+
* @return \Generator<string>
426+
*/
427+
private function getUserBaseKeyPaths(IUser $firstUser): \Generator {
428+
yield $this->getUserBaseKeyPath($firstUser);
429+
430+
foreach ($this->userManager->search('') as $user) {
431+
if ($user->getUID() !== $firstUser->getUID()) {
432+
yield $this->keyRootDirectory . '/' . $user->getUID() . '/files_encryption/keys';
433+
}
434+
}
435+
}
436+
346437
/**
347438
* Attempt to find a key for a file even when it's not stored in the expected location
348439
*
@@ -376,19 +467,17 @@ private function findKeysByFileName(string $basePath, string $name) {
376467
}
377468

378469
/**
379-
* Test if the provided key is valid as a system key for the file
470+
* Test whether the key decrypts the file when staged at the given key path
380471
*/
381-
private function testSystemKey(IUser $user, string $key, File $node): bool {
382-
$systemKeyPath = $this->getSystemKeyPath($node);
383-
384-
if ($this->rootView->file_exists($systemKeyPath)) {
472+
private function testKeyAtPath(File $node, string $key, string $stageKeyPath): bool {
473+
if ($this->rootView->file_exists($stageKeyPath)) {
385474
// already has a key, reject new key
386475
return false;
387476
}
388477

389-
$this->rootView->copy($key, $systemKeyPath);
478+
$this->rootView->copy($key, $stageKeyPath);
390479
$isValid = $this->tryReadFile($node);
391-
$this->rootView->rmdir($systemKeyPath);
480+
$this->rootView->rmdir($stageKeyPath);
392481
return $isValid;
393482
}
394483

@@ -425,6 +514,11 @@ private function decryptWithSystemKey(File $node, string $key): void {
425514
if ($this->isDataEncrypted($decryptedNode)) {
426515
throw new \Exception($node->getPath() . ' still encrypted after attempting to decrypt with ' . $key);
427516
}
517+
// a broken unencrypted_size of 0 makes the decryption stream produce
518+
// nothing at all, an empty result for a non empty source is data loss
519+
if ($decryptedNode->getSize() === 0 && $node->getSize() > 0) {
520+
throw new \Exception($node->getPath() . ' decrypted to an empty file, refusing the result');
521+
}
428522
} catch (\Throwable $e) {
429523
// the target has to go first so the .bak can move back onto its name
430524
if ($decryptedNode !== null) {

‎apps/encryption/tests/Command/FixKeyLocationTest.php‎

Lines changed: 97 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,31 @@ private function getCommand(): TestableFixKeyLocation {
8787
);
8888
}
8989

90-
private function markAsEncrypted(TemporaryUnwrapped $storage, string $path): void {
90+
private function markAsEncrypted(TemporaryUnwrapped $storage, string $path, int $unencryptedSize): void {
9191
$cache = $storage->getCache();
92-
$cache->update($cache->get($path)->getId(), ['encrypted' => 1]);
92+
$cache->update($cache->get($path)->getId(), [
93+
'encrypted' => 1,
94+
'unencrypted_size' => $unencryptedSize,
95+
]);
96+
}
97+
98+
/**
99+
* A second user whose key tree can hold misplaced keys. The key tree is created
100+
* directly, no login needed, the encryption session of the first user stays
101+
* untouched.
102+
*/
103+
private function setUpSecondUser(): void {
104+
$this->createUser('test2', 'test2');
105+
}
106+
107+
private function moveKeyToSecondUserTree(string $keyPath, string $name): void {
108+
$rootView = new View();
109+
foreach (['/test2', '/test2/files_encryption', '/test2/files_encryption/keys'] as $dir) {
110+
if (!$rootView->file_exists($dir)) {
111+
$rootView->mkdir($dir);
112+
}
113+
}
114+
$rootView->rename(rtrim($keyPath, '/'), '/test2/files_encryption/keys/' . $name);
93115
}
94116

95117
/**
@@ -107,7 +129,7 @@ public function testKeyValidationReadsThroughEncryption(): void {
107129
// ciphertext under a path that has no key
108130
$view->file_put_contents('stray/stray.txt', $encryptedBackingStorage->file_get_contents('original.txt'));
109131
$strayStorage = $view->getFileInfo('stray/stray.txt')->getStorage();
110-
$this->markAsEncrypted($strayStorage, 'stray.txt');
132+
$this->markAsEncrypted($strayStorage, 'stray.txt', strlen('secret content'));
111133

112134
$userFolder = Server::get(IRootFolder::class)->getUserFolder('test1');
113135
$command = $this->getCommand();
@@ -137,7 +159,7 @@ public function testFailedDecryptionRollsBack(): void {
137159
$cipher = $encryptedBackingStorage->file_get_contents('original.txt');
138160
$view->file_put_contents('stray/broken.txt', $cipher);
139161
$strayStorage = $view->getFileInfo('stray/broken.txt')->getStorage();
140-
$this->markAsEncrypted($strayStorage, 'broken.txt');
162+
$this->markAsEncrypted($strayStorage, 'broken.txt', strlen('secret content'));
141163

142164
$userFolder = Server::get(IRootFolder::class)->getUserFolder('test1');
143165
$strayNode = $userFolder->get('stray/broken.txt');
@@ -153,11 +175,13 @@ public function testFailedDecryptionRollsBack(): void {
153175
$brokenKeyPath = self::invokePrivate($command, 'getUserKeyPath', [$user, $strayNode]);
154176
$rootView->copy($wrongKey, str_replace('broken.txt', 'broken.txt.bak', $brokenKeyPath));
155177

178+
$threw = false;
156179
try {
157180
self::invokePrivate($command, 'decryptWithSystemKey', [$strayNode, $wrongKey]);
158-
$this->fail('decrypting with the wrong key must fail');
159-
} catch (\Exception $e) {
181+
} catch (\Exception) {
182+
$threw = true;
160183
}
184+
$this->assertTrue($threw, 'decrypting with the wrong key must fail');
161185

162186
$this->assertTrue($view->file_exists('stray/broken.txt'), 'the original file has to be restored');
163187
$this->assertSame($cipher, $view->file_get_contents('stray/broken.txt'), 'the original content has to be intact');
@@ -169,6 +193,70 @@ public function testFailedDecryptionRollsBack(): void {
169193
$this->assertFalse($rootView->file_exists($systemKeyPathBak), 'no temporary system key must be left behind');
170194
}
171195

196+
/**
197+
* A key that only exists in the tree of another user must be found and validated.
198+
* The harness mounts are not system wide, the encryption wrapper resolves the keys
199+
* of the stray file through the user tree, so the search is exercised with that
200+
* staging path, the system wide flow only stages at a different location.
201+
*/
202+
public function testKeyFoundInAnotherUsersTree(): void {
203+
[
204+
'view' => $view,
205+
'encryptedBackingStorage' => $encryptedBackingStorage,
206+
] = $this->setUpMounts();
207+
$this->setUpSecondUser();
208+
209+
$view->file_put_contents('enc/original.txt', 'secret content');
210+
$view->file_put_contents('stray/orphan.txt', $encryptedBackingStorage->file_get_contents('original.txt'));
211+
$strayStorage = $view->getFileInfo('stray/orphan.txt')->getStorage();
212+
$this->markAsEncrypted($strayStorage, 'orphan.txt', strlen('secret content'));
213+
214+
$command = $this->getCommand();
215+
$user = Server::get(IUserManager::class)->get('test1');
216+
$userFolder = Server::get(IRootFolder::class)->getUserFolder('test1');
217+
$originalKey = self::invokePrivate($command, 'getUserKeyPath', [$user, $userFolder->get('enc/original.txt')]);
218+
// the key sits in ANOTHER user's tree, under the name of the stray file
219+
$this->moveKeyToSecondUserTree($originalKey, 'orphan.txt');
220+
221+
$strayNode = $userFolder->get('stray/orphan.txt');
222+
$stagePath = self::invokePrivate($command, 'getUserKeyPath', [$user, $strayNode]);
223+
$foundKey = self::invokePrivate($command, 'findKeyInUserTrees', [$user, $strayNode, $stagePath]);
224+
225+
$this->assertNotNull($foundKey, 'the key in the other tree has to be found');
226+
$this->assertStringContainsString('/test2/', $foundKey);
227+
}
228+
229+
/**
230+
* With --personal an encrypted file in the personal space whose key was lost is
231+
* restored from another user's tree, healthy files stay untouched.
232+
*/
233+
public function testPersonalFileKeyFoundInAnotherUsersTree(): void {
234+
$this->setUpMounts();
235+
$this->setUpSecondUser();
236+
237+
$view = new View('/test1/files');
238+
$view->file_put_contents('personal.txt', 'personal content');
239+
$view->file_put_contents('healthy.txt', 'healthy content');
240+
241+
$command = $this->getCommand();
242+
$user = Server::get(IUserManager::class)->get('test1');
243+
$userFolder = Server::get(IRootFolder::class)->getUserFolder('test1');
244+
$personalKey = self::invokePrivate($command, 'getUserKeyPath', [$user, $userFolder->get('personal.txt')]);
245+
$this->moveKeyToSecondUserTree($personalKey, 'personal.txt');
246+
247+
$command->systemMounts = [];
248+
$tester = new CommandTester($command);
249+
$exitCode = $tester->execute(['user' => 'test1', '--personal' => true]);
250+
$display = $tester->getDisplay();
251+
252+
$this->assertSame(Command::SUCCESS, $exitCode, $display);
253+
$this->assertStringContainsString('Migrated key from', $display);
254+
$this->assertEquals('personal content', $view->file_get_contents('personal.txt'));
255+
$rootView = new View();
256+
$this->assertTrue($rootView->file_exists($personalKey), 'the key has to be back at the path of the file');
257+
$this->assertStringNotContainsString('healthy.txt', $display, 'healthy files must not be touched');
258+
}
259+
172260
/**
173261
* One broken file must not abort the whole run, the remaining files still get
174262
* processed and the failure is reported.
@@ -180,17 +268,17 @@ public function testExecuteContinuesAfterFailure(): void {
180268

181269
$view->file_put_contents('stray/a-ghost.txt', 'gone');
182270
$strayStorage = $view->getFileInfo('stray/a-ghost.txt')->getStorage();
183-
$this->markAsEncrypted($strayStorage, 'a-ghost.txt');
271+
$this->markAsEncrypted($strayStorage, 'a-ghost.txt', strlen('gone'));
184272
// cache row without a backing file, reading it fails like an object store 404
185273
$strayStorage->unlink('a-ghost.txt');
186274

187275
$view->file_put_contents('stray/b-plain.txt', 'plain data');
188-
$this->markAsEncrypted($strayStorage, 'b-plain.txt');
276+
$this->markAsEncrypted($strayStorage, 'b-plain.txt', strlen('plain data'));
189277

190278
// ciphertext without any key while the user has no key directory at all,
191279
// the key search has to come up empty instead of erroring out
192280
$view->file_put_contents('stray/c-cipher.txt', 'HBEGIN:oc_encryption_module:OC_DEFAULT_MODULE:HEND');
193-
$this->markAsEncrypted($strayStorage, 'c-cipher.txt');
281+
$this->markAsEncrypted($strayStorage, 'c-cipher.txt', 8);
194282

195283
$command = $this->getCommand();
196284
$mount = $this->createMock(ICachedMountInfo::class);

0 commit comments

Comments
 (0)