From ba309bc43fde62a9df951622f1f1713f7f640ed3 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 10 Sep 2026 15:37:38 +0200 Subject: [PATCH] fix(encryption): cap and invalidate key-cache For large operations the cache infinitly grows and invalid entries were never invalidated. So memory cap the cache and remove outdated entries. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- lib/private/Encryption/Keys/Storage.php | 36 ++++- tests/lib/Encryption/Keys/StorageTest.php | 167 ++++++++++++++++++++++ 2 files changed, 197 insertions(+), 6 deletions(-) diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index cce22b9138a21..91f87aca12d85 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -12,6 +12,7 @@ use OC\Files\View; use OC\ServerNotAvailableException; use OC\User\NoUserException; +use OCP\Cache\CappedMemoryCache; use OCP\Encryption\Keys\IStorage; use OCP\IConfig; use OCP\Security\ICrypto; @@ -40,8 +41,8 @@ class Storage implements IStorage { /** @var string */ private $backup_base_dir; - /** @var array */ - private $keyCache = []; + /** @var CappedMemoryCache */ + private CappedMemoryCache $keyCache; /** @var ICrypto */ private $crypto; @@ -57,6 +58,7 @@ public function __construct(View $view, Util $util, ICrypto $crypto, IConfig $co $this->view = $view; $this->util = $util; + $this->keyCache = new CappedMemoryCache(); $this->encryption_base_dir = '/files_encryption'; $this->keys_base_dir = $this->encryption_base_dir . '/keys'; $this->backup_base_dir = $this->encryption_base_dir . '/backup'; @@ -138,6 +140,7 @@ public function setSystemUserKey($keyId, $key, $encryptionModuleId) { public function deleteUserKey($uid, $keyId, $encryptionModuleId) { try { $path = $this->constructUserKeyPath($encryptionModuleId, $keyId, $uid); + $this->keyCache->remove($path); return !$this->view->file_exists($path) || $this->view->unlink($path); } catch (NoUserException $e) { // this exception can come from initMountPoints() from setupUserMounts() @@ -158,6 +161,7 @@ public function deleteUserKey($uid, $keyId, $encryptionModuleId) { */ public function deleteFileKey($path, $keyId, $encryptionModuleId) { $keyDir = $this->util->getFileKeyDir($encryptionModuleId, $path); + $this->keyCache->remove($keyDir . $keyId); return !$this->view->file_exists($keyDir . $keyId) || $this->view->unlink($keyDir . $keyId); } @@ -166,6 +170,7 @@ public function deleteFileKey($path, $keyId, $encryptionModuleId) { */ public function deleteAllFileKeys($path) { $keyDir = $this->util->getFileKeyDir('', $path); + $this->clearCachedKeysBelow($keyDir); return !$this->view->file_exists($keyDir) || $this->view->deleteAll($keyDir); } @@ -174,9 +179,24 @@ public function deleteAllFileKeys($path) { */ public function deleteSystemUserKey($keyId, $encryptionModuleId) { $path = $this->constructUserKeyPath($encryptionModuleId, $keyId, null); + $this->keyCache->remove($path); return !$this->view->file_exists($path) || $this->view->unlink($path); } + /** + * Drop all cached keys stored inside the given key directory + * + * @param string $keyDir path to a key directory, with or without trailing slash + */ + private function clearCachedKeysBelow(string $keyDir): void { + $prefix = rtrim($keyDir, '/') . '/'; + foreach (array_keys($this->keyCache->getData()) as $cachedPath) { + if (str_starts_with((string)$cachedPath, $prefix)) { + $this->keyCache->remove($cachedPath); + } + } + } + /** * construct path to users key * @@ -246,8 +266,9 @@ private function getKey($path): array { ]; if ($this->view->file_exists($path)) { - if (isset($this->keyCache[$path])) { - $key = $this->keyCache[$path]; + $cachedKey = $this->keyCache->get($path); + if ($cachedKey !== null) { + $key = $cachedKey; } else { $data = $this->view->file_get_contents($path); @@ -297,7 +318,7 @@ private function getKey($path): array { } } - $this->keyCache[$path] = $key; + $this->keyCache->set($path, $key); } } @@ -328,7 +349,7 @@ private function setKey($path, $key) { $result = $this->view->file_put_contents($path, $data); if (is_int($result) && $result > 0) { - $this->keyCache[$path] = $key; + $this->keyCache->set($path, $key); return true; } @@ -348,6 +369,8 @@ public function renameKeys($source, $target) { if ($this->view->file_exists($sourcePath)) { $this->keySetPreparation(dirname($targetPath)); + $this->clearCachedKeysBelow($sourcePath); + $this->clearCachedKeysBelow($targetPath); $this->view->rename($sourcePath, $targetPath); return true; @@ -370,6 +393,7 @@ public function copyKeys($source, $target) { if ($this->view->file_exists($sourcePath)) { $this->keySetPreparation(dirname($targetPath)); + $this->clearCachedKeysBelow($targetPath); $this->view->copy($sourcePath, $targetPath); return true; } diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index 333d8d8ce2199..bb4606064ece5 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -11,6 +11,7 @@ use OC\Encryption\Keys\Storage; use OC\Encryption\Util; use OC\Files\View; +use OCP\Cache\CappedMemoryCache; use OCP\IConfig; use OCP\Security\ICrypto; use PHPUnit\Framework\MockObject\MockObject; @@ -408,6 +409,172 @@ public function testDeleteFileKey(): void { ); } + /** + * Set up the mocks needed to read file keys for arbitrary paths + */ + private function mockFileKeyEnvironment(): void { + $this->config->method('getSystemValueString') + ->with('version') + ->willReturn('20.0.0.2'); + $this->config->method('getSystemValueBool') + ->willReturn(true); + $this->util->method('getUidAndFilename') + ->willReturnCallback([$this, 'getUidAndFilenameCallback']); + $this->util->method('stripPartialFileExtension') + ->willReturnArgument(0); + $this->util->method('isSystemWideMountPoint') + ->willReturn(false); + $this->view->method('file_exists') + ->willReturn(true); + $this->view->method('is_dir') + ->willReturn(true); + } + + /** + * Make the view return a dummy key for every path and collect the read paths + * + * @param string[] $reads + */ + private function trackKeyReads(array &$reads, ?string $uid = null): void { + $this->view->method('file_get_contents') + ->willReturnCallback(function (string $path) use (&$reads, $uid): string { + $reads[] = $path; + return json_encode(['key' => base64_encode('key'), 'uid' => $uid]); + }); + } + + public function testGetFileKeyIsCached(): void { + $this->mockFileKeyEnvironment(); + $reads = []; + $this->trackKeyReads($reads); + + $this->storage->getFileKey('/user1/files/foo.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/foo.txt', 'fileKey', 'encModule'); + + $this->assertSame(['/user1/files_encryption/keys/files/foo.txt/encModule/fileKey'], $reads); + } + + public function testKeyCacheIsCapped(): void { + $this->mockFileKeyEnvironment(); + $reads = []; + $this->trackKeyReads($reads); + + for ($i = 0; $i < 600; $i++) { + $this->storage->getFileKey('/user1/files/foo' . $i . '.txt', 'fileKey', 'encModule'); + } + + /** @var CappedMemoryCache $keyCache */ + $keyCache = self::invokePrivate($this->storage, 'keyCache'); + $this->assertCount(512, $keyCache->getData()); + } + + public function testDeleteFileKeyInvalidatesCache(): void { + $this->mockFileKeyEnvironment(); + $this->view->method('unlink')->willReturn(true); + $reads = []; + $this->trackKeyReads($reads); + + $this->storage->getFileKey('/user1/files/foo.txt', 'fileKey', 'encModule'); + $this->assertTrue($this->storage->deleteFileKey('/user1/files/foo.txt', 'fileKey', 'encModule')); + $reads = []; + + $this->storage->getFileKey('/user1/files/foo.txt', 'fileKey', 'encModule'); + + $this->assertSame(['/user1/files_encryption/keys/files/foo.txt/encModule/fileKey'], $reads); + } + + public function testDeleteAllFileKeysInvalidatesCache(): void { + $this->mockFileKeyEnvironment(); + $this->view->method('deleteAll')->willReturn(true); + $reads = []; + $this->trackKeyReads($reads); + + $this->storage->getFileKey('/user1/files/foo.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/foo.txt', 'otherKey', 'encModule'); + // a sibling sharing the name prefix must stay cached + $this->storage->getFileKey('/user1/files/foobar.txt', 'fileKey', 'encModule'); + $this->assertTrue($this->storage->deleteAllFileKeys('/user1/files/foo.txt')); + $reads = []; + + $this->storage->getFileKey('/user1/files/foo.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/foo.txt', 'otherKey', 'encModule'); + $this->storage->getFileKey('/user1/files/foobar.txt', 'fileKey', 'encModule'); + + $this->assertSame([ + '/user1/files_encryption/keys/files/foo.txt/encModule/fileKey', + '/user1/files_encryption/keys/files/foo.txt/encModule/otherKey', + ], $reads); + } + + public function testRenameKeysInvalidatesCache(): void { + $this->mockFileKeyEnvironment(); + $this->view->method('rename')->willReturn(true); + $reads = []; + $this->trackKeyReads($reads); + + $this->storage->getFileKey('/user1/files/source.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/target.txt', 'fileKey', 'encModule'); + $this->assertTrue($this->storage->renameKeys('/user1/files/source.txt', '/user1/files/target.txt')); + $reads = []; + + $this->storage->getFileKey('/user1/files/source.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/target.txt', 'fileKey', 'encModule'); + + $this->assertSame([ + '/user1/files_encryption/keys/files/source.txt/encModule/fileKey', + '/user1/files_encryption/keys/files/target.txt/encModule/fileKey', + ], $reads); + } + + public function testCopyKeysInvalidatesTargetCache(): void { + $this->mockFileKeyEnvironment(); + $this->view->method('copy')->willReturn(true); + $reads = []; + $this->trackKeyReads($reads); + + $this->storage->getFileKey('/user1/files/source.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/target.txt', 'fileKey', 'encModule'); + $this->assertTrue($this->storage->copyKeys('/user1/files/source.txt', '/user1/files/target.txt')); + $reads = []; + + $this->storage->getFileKey('/user1/files/source.txt', 'fileKey', 'encModule'); + $this->storage->getFileKey('/user1/files/target.txt', 'fileKey', 'encModule'); + + $this->assertSame([ + '/user1/files_encryption/keys/files/target.txt/encModule/fileKey', + ], $reads); + } + + public function testDeleteUserKeyInvalidatesCache(): void { + $this->mockFileKeyEnvironment(); + $this->view->method('unlink')->willReturn(true); + $reads = []; + $this->trackKeyReads($reads, 'user1'); + + $this->storage->getUserKey('user1', 'publicKey', 'encModule'); + $this->assertTrue($this->storage->deleteUserKey('user1', 'publicKey', 'encModule')); + $reads = []; + + $this->storage->getUserKey('user1', 'publicKey', 'encModule'); + + $this->assertSame(['/user1/files_encryption/encModule/user1.publicKey'], $reads); + } + + public function testDeleteSystemUserKeyInvalidatesCache(): void { + $this->mockFileKeyEnvironment(); + $this->view->method('unlink')->willReturn(true); + $reads = []; + $this->trackKeyReads($reads); + + $this->storage->getSystemUserKey('shareKey_56884', 'encModule'); + $this->assertTrue($this->storage->deleteSystemUserKey('shareKey_56884', 'encModule')); + $reads = []; + + $this->storage->getSystemUserKey('shareKey_56884', 'encModule'); + + $this->assertSame(['/files_encryption/encModule/shareKey_56884'], $reads); + } + #[\PHPUnit\Framework\Attributes\DataProvider('dataProviderCopyRename')] public function testRenameKeys($source, $target, $systemWideMountSource, $systemWideMountTarget, $expectedSource, $expectedTarget): void { $this->view->expects($this->any())