From 1e06b61f591d74bfaa554578e2ca647d915fcb8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 29 Jun 2023 15:41:40 +0200 Subject: [PATCH 1/3] Migrate away from ILogger in encryption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit And modernize code a bit Signed-off-by: Côme Chilliet --- apps/encryption/lib/AppInfo/Application.php | 21 +-- .../lib/Command/FixEncryptedVersion.php | 43 ++---- apps/encryption/lib/Crypto/Crypt.php | 88 ++++-------- apps/encryption/lib/Crypto/Encryption.php | 76 +++-------- apps/encryption/lib/Hooks/UserHooks.php | 90 +++---------- apps/encryption/lib/KeyManager.php | 127 ++++-------------- apps/encryption/lib/Settings/Admin.php | 40 +----- apps/encryption/lib/Util.php | 55 ++------ .../tests/Command/FixEncryptedVersionTest.php | 3 +- apps/encryption/tests/Crypto/CryptTest.php | 6 +- .../tests/Crypto/EncryptionTest.php | 6 +- apps/encryption/tests/Hooks/UserHooksTest.php | 5 +- apps/encryption/tests/KeyManagerTest.php | 12 +- apps/encryption/tests/Settings/AdminTest.php | 6 +- apps/encryption/tests/UtilTest.php | 5 +- 15 files changed, 142 insertions(+), 441 deletions(-) diff --git a/apps/encryption/lib/AppInfo/Application.php b/apps/encryption/lib/AppInfo/Application.php index 63239a1bf9bd3..7d794ab15a6b3 100644 --- a/apps/encryption/lib/AppInfo/Application.php +++ b/apps/encryption/lib/AppInfo/Application.php @@ -39,6 +39,7 @@ use OCA\Encryption\Util; use OCP\Encryption\IManager; use OCP\IConfig; +use Psr\Log\LoggerInterface; class Application extends \OCP\AppFramework\App { /** @@ -69,7 +70,7 @@ public function registerHooks(IConfig $config) { $hookManager->registerHook([ new UserHooks($container->query(KeyManager::class), $server->getUserManager(), - $server->getLogger(), + $server->get(LoggerInterface::class), $container->query(Setup::class), $server->getUserSession(), $container->query(Util::class), @@ -93,15 +94,15 @@ public function registerEncryptionModule(IManager $encryptionManager) { Encryption::DISPLAY_NAME, function () use ($container) { return new Encryption( - $container->query(Crypt::class), - $container->query(KeyManager::class), - $container->query(Util::class), - $container->query(Session::class), - $container->query(EncryptAll::class), - $container->query(DecryptAll::class), - $container->getServer()->getLogger(), - $container->getServer()->getL10N($container->getAppName()) - ); + $container->query(Crypt::class), + $container->query(KeyManager::class), + $container->query(Util::class), + $container->query(Session::class), + $container->query(EncryptAll::class), + $container->query(DecryptAll::class), + $container->getServer()->get(LoggerInterface::class), + $container->getServer()->getL10N($container->getAppName()) + ); }); } } diff --git a/apps/encryption/lib/Command/FixEncryptedVersion.php b/apps/encryption/lib/Command/FixEncryptedVersion.php index a6f2a55bb7998..d0fbf1adb31b4 100644 --- a/apps/encryption/lib/Command/FixEncryptedVersion.php +++ b/apps/encryption/lib/Command/FixEncryptedVersion.php @@ -29,9 +29,9 @@ use OCP\Files\IRootFolder; use OCP\HintException; use OCP\IConfig; -use OCP\ILogger; 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,41 +39,16 @@ use Symfony\Component\Console\Output\OutputInterface; class FixEncryptedVersion extends Command { - /** @var IConfig */ - private $config; - - /** @var ILogger */ - private $logger; - - /** @var IRootFolder */ - private $rootFolder; - - /** @var IUserManager */ - private $userManager; - - /** @var Util */ - private $util; - - /** @var View */ - private $view; - - /** @var bool */ - private $supportLegacy; + private bool $supportLegacy; public function __construct( - IConfig $config, - ILogger $logger, - IRootFolder $rootFolder, - IUserManager $userManager, - Util $util, - View $view + private IConfig $config, + private LoggerInterface $logger, + private IRootFolder $rootFolder, + private IUserManager $userManager, + private Util $util, + private View $view, ) { - $this->config = $config; - $this->logger = $logger; - $this->rootFolder = $rootFolder; - $this->userManager = $userManager; - $this->util = $util; - $this->view = $view; $this->supportLegacy = false; parent::__construct(); @@ -134,7 +109,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return $this->runForUser($user, $pathOption, $output); } elseif ($all) { $result = 0; - $this->userManager->callForSeenUsers(function(IUser $user) use ($pathOption, $output, &$result) { + $this->userManager->callForSeenUsers(function (IUser $user) use ($pathOption, $output, &$result) { $output->writeln("Processing files for " . $user->getUID()); $result = $this->runForUser($user->getUID(), $pathOption, $output); return $result === 0; diff --git a/apps/encryption/lib/Crypto/Crypt.php b/apps/encryption/lib/Crypto/Crypt.php index cd2453e8c70c7..cc963f51b8687 100644 --- a/apps/encryption/lib/Crypto/Crypt.php +++ b/apps/encryption/lib/Crypto/Crypt.php @@ -40,8 +40,8 @@ use OCP\Encryption\Exceptions\GenericEncryptionException; use OCP\IConfig; use OCP\IL10N; -use OCP\ILogger; use OCP\IUserSession; +use Psr\Log\LoggerInterface; use phpseclib\Crypt\RC4; /** @@ -83,40 +83,24 @@ class Crypt { // default encoding format, old Nextcloud versions used base64 public const BINARY_ENCODING_FORMAT = 'binary'; - /** @var ILogger */ - private $logger; + private string $user; - /** @var string */ - private $user; + private ?string $currentCipher = null; - /** @var IConfig */ - private $config; - - /** @var IL10N */ - private $l; - - /** @var string|null */ - private $currentCipher; - - /** @var bool */ - private $supportLegacy; + private bool $supportLegacy; /** * Use the legacy base64 encoding instead of the more space-efficient binary encoding. */ private bool $useLegacyBase64Encoding; - /** - * @param ILogger $logger - * @param IUserSession $userSession - * @param IConfig $config - * @param IL10N $l - */ - public function __construct(ILogger $logger, IUserSession $userSession, IConfig $config, IL10N $l) { - $this->logger = $logger; - $this->user = $userSession && $userSession->isLoggedIn() ? $userSession->getUser()->getUID() : '"no user given"'; - $this->config = $config; - $this->l = $l; + public function __construct( + private LoggerInterface $logger, + IUserSession $userSession, + private IConfig $config, + private IL10N $l, + ) { + $this->user = $userSession->isLoggedIn() ? $userSession->getUser()->getUID() : '"no user given"'; $this->supportLegacy = $this->config->getSystemValueBool('encryption.legacy_format_support', false); $this->useLegacyBase64Encoding = $this->config->getSystemValueBool('encryption.use_legacy_base64_encoding', false); } @@ -127,15 +111,14 @@ public function __construct(ILogger $logger, IUserSession $userSession, IConfig * @return array|bool */ public function createKeyPair() { - $log = $this->logger; $res = $this->getOpenSSLPKey(); if (!$res) { - $log->error("Encryption Library couldn't generate users key-pair for {$this->user}", + $this->logger->error("Encryption Library couldn't generate users key-pair for {$this->user}", ['app' => 'encryption']); if (openssl_error_string()) { - $log->error('Encryption library openssl_pkey_new() fails: ' . openssl_error_string(), + $this->logger->error('Encryption library openssl_pkey_new() fails: ' . openssl_error_string(), ['app' => 'encryption']); } } elseif (openssl_pkey_export($res, @@ -150,10 +133,10 @@ public function createKeyPair() { 'privateKey' => $privateKey ]; } - $log->error('Encryption library couldn\'t export users private key, please check your servers OpenSSL configuration.' . $this->user, + $this->logger->error('Encryption library couldn\'t export users private key, please check your servers OpenSSL configuration.' . $this->user, ['app' => 'encryption']); if (openssl_error_string()) { - $log->error('Encryption Library:' . openssl_error_string(), + $this->logger->error('Encryption Library:' . openssl_error_string(), ['app' => 'encryption']); } @@ -172,10 +155,8 @@ public function getOpenSSLPKey() { /** * get openSSL Config - * - * @return array */ - private function getOpenSSLConfig() { + private function getOpenSSLConfig(): array { $config = ['private_key_bits' => 4096]; $config = array_merge( $config, @@ -240,10 +221,9 @@ public function generateHeader($keyFormat = self::DEFAULT_KEY_FORMAT) { * @param string $iv * @param string $passPhrase * @param string $cipher - * @return string * @throws EncryptionFailedException */ - private function encrypt($plainContent, $iv, $passPhrase = '', $cipher = self::DEFAULT_CIPHER) { + private function encrypt($plainContent, $iv, $passPhrase = '', $cipher = self::DEFAULT_CIPHER): string { $options = $this->useLegacyBase64Encoding ? 0 : OPENSSL_RAW_DATA; $encryptedContent = openssl_encrypt($plainContent, $cipher, @@ -264,10 +244,8 @@ private function encrypt($plainContent, $iv, $passPhrase = '', $cipher = self::D /** * return cipher either from config.php or the default cipher defined in * this class - * - * @return string */ - private function getCachedCipher() { + private function getCachedCipher(): string { if (isset($this->currentCipher)) { return $this->currentCipher; } @@ -336,18 +314,16 @@ public function getLegacyCipher() { /** * @param string $encryptedContent * @param string $iv - * @return string */ - private function concatIV($encryptedContent, $iv) { + private function concatIV($encryptedContent, $iv): string { return $encryptedContent . '00iv00' . $iv; } /** * @param string $encryptedContent * @param string $signature - * @return string */ - private function concatSig($encryptedContent, $signature) { + private function concatSig($encryptedContent, $signature): string { return $encryptedContent . '00sig00' . $signature; } @@ -357,19 +333,15 @@ private function concatSig($encryptedContent, $signature) { * encrypted content and is not used in any crypto primitive. * * @param string $data - * @return string */ - private function addPadding($data) { + private function addPadding($data): string { return $data . 'xxx'; } /** * generate password hash used to encrypt the users private key * - * @param string $password - * @param string $cipher * @param string $uid only used for user keys - * @return string */ protected function generatePasswordHash(string $password, string $cipher, string $uid = '', int $iterations = 600000): string { $instanceId = $this->config->getSystemValue('instanceid'); @@ -546,9 +518,8 @@ private function createSignature(string $data, string $passPhrase): string { * * @param string $padded * @param bool $hasSignature did the block contain a signature, in this case we use a different padding - * @return string|false */ - private function removePadding($padded, $hasSignature = false) { + private function removePadding($padded, $hasSignature = false): string|false { if ($hasSignature === false && substr($padded, -2) === 'xx') { return substr($padded, 0, -2); } elseif ($hasSignature === true && substr($padded, -3) === 'xxx') { @@ -564,9 +535,8 @@ private function removePadding($padded, $hasSignature = false) { * * @param string $catFile * @param string $cipher - * @return array */ - private function splitMetaData($catFile, $cipher) { + private function splitMetaData($catFile, $cipher): array { if ($this->hasSignature($catFile, $cipher)) { $catFile = $this->removePadding($catFile, true); $meta = substr($catFile, -93); @@ -593,10 +563,9 @@ private function splitMetaData($catFile, $cipher) { * * @param string $catFile * @param string $cipher - * @return bool * @throws GenericEncryptionException */ - private function hasSignature($catFile, $cipher) { + private function hasSignature($catFile, $cipher): bool { $skipSignatureCheck = $this->config->getSystemValueBool('encryption_skip_signature_check', false); $meta = substr($catFile, -93); @@ -617,12 +586,6 @@ private function hasSignature($catFile, $cipher) { /** - * @param string $encryptedContent - * @param string $iv - * @param string $passPhrase - * @param string $cipher - * @param boolean $binaryEncoding - * @return string * @throws DecryptionFailedException */ private function decrypt(string $encryptedContent, string $iv, string $passPhrase = '', string $cipher = self::DEFAULT_CIPHER, bool $binaryEncoding = false): string { @@ -669,10 +632,9 @@ protected function parseHeader($data) { /** * generate initialization vector * - * @return string * @throws GenericEncryptionException */ - private function generateIv() { + private function generateIv(): string { return random_bytes(16); } diff --git a/apps/encryption/lib/Crypto/Encryption.php b/apps/encryption/lib/Crypto/Encryption.php index 899d0f4315d26..1481d3a9a237e 100644 --- a/apps/encryption/lib/Crypto/Encryption.php +++ b/apps/encryption/lib/Crypto/Encryption.php @@ -42,7 +42,7 @@ use OCA\Encryption\Util; use OCP\Encryption\IEncryptionModule; use OCP\IL10N; -use OCP\ILogger; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -50,11 +50,6 @@ class Encryption implements IEncryptionModule { public const ID = 'OC_DEFAULT_MODULE'; public const DISPLAY_NAME = 'Default encryption module'; - /** - * @var Crypt - */ - private $crypt; - /** @var string */ private $cipher; @@ -64,8 +59,7 @@ class Encryption implements IEncryptionModule { /** @var string */ private $user; - /** @var array */ - private $owner; + private array $owner; /** @var string */ private $fileKey; @@ -73,76 +67,36 @@ class Encryption implements IEncryptionModule { /** @var string */ private $writeCache; - /** @var KeyManager */ - private $keyManager; - /** @var array */ private $accessList; /** @var boolean */ private $isWriteOperation; - /** @var Util */ - private $util; - - /** @var Session */ - private $session; - - /** @var ILogger */ - private $logger; - - /** @var IL10N */ - private $l; - - /** @var EncryptAll */ - private $encryptAll; - - /** @var bool */ - private $useMasterPassword; - - /** @var DecryptAll */ - private $decryptAll; + private bool $useMasterPassword; private bool $useLegacyBase64Encoding = false; /** @var int Current version of the file */ - private $version = 0; + private int $version = 0; private bool $useLegacyFileKey = true; /** @var array remember encryption signature version */ private static $rememberVersion = []; - - /** - * - * @param Crypt $crypt - * @param KeyManager $keyManager - * @param Util $util - * @param Session $session - * @param EncryptAll $encryptAll - * @param DecryptAll $decryptAll - * @param ILogger $logger - * @param IL10N $il10n - */ - public function __construct(Crypt $crypt, - KeyManager $keyManager, - Util $util, - Session $session, - EncryptAll $encryptAll, - DecryptAll $decryptAll, - ILogger $logger, - IL10N $il10n) { - $this->crypt = $crypt; - $this->keyManager = $keyManager; - $this->util = $util; - $this->session = $session; - $this->encryptAll = $encryptAll; - $this->decryptAll = $decryptAll; - $this->logger = $logger; - $this->l = $il10n; + public function __construct( + private Crypt $crypt, + private KeyManager $keyManager, + private Util $util, + private Session $session, + private EncryptAll $encryptAll, + private DecryptAll $decryptAll, + private LoggerInterface $logger, + private IL10N $l, + ) { $this->owner = []; - $this->useMasterPassword = $util->isMasterKeyEnabled(); + $this->useMasterPassword = $this->util->isMasterKeyEnabled(); } /** diff --git a/apps/encryption/lib/Hooks/UserHooks.php b/apps/encryption/lib/Hooks/UserHooks.php index a719d7bc12e1f..e9efc4c35c109 100644 --- a/apps/encryption/lib/Hooks/UserHooks.php +++ b/apps/encryption/lib/Hooks/UserHooks.php @@ -36,87 +36,29 @@ use OCA\Encryption\Users\Setup; use OCA\Encryption\Util; use OCP\Encryption\Exceptions\GenericEncryptionException; -use OCP\ILogger; use OCP\IUserManager; use OCP\IUserSession; use OCP\Util as OCUtil; +use Psr\Log\LoggerInterface; class UserHooks implements IHook { - /** * list of user for which we perform a password reset - * @var array - */ - protected static $passwordResetUsers = []; - - /** - * @var KeyManager - */ - private $keyManager; - /** - * @var IUserManager - */ - private $userManager; - /** - * @var ILogger - */ - private $logger; - /** - * @var Setup - */ - private $userSetup; - /** - * @var IUserSession - */ - private $userSession; - /** - * @var Util - */ - private $util; - /** - * @var Session - */ - private $session; - /** - * @var Recovery - */ - private $recovery; - /** - * @var Crypt + * @var array */ - private $crypt; - - /** - * UserHooks constructor. - * - * @param KeyManager $keyManager - * @param IUserManager $userManager - * @param ILogger $logger - * @param Setup $userSetup - * @param IUserSession $userSession - * @param Util $util - * @param Session $session - * @param Crypt $crypt - * @param Recovery $recovery - */ - public function __construct(KeyManager $keyManager, - IUserManager $userManager, - ILogger $logger, - Setup $userSetup, - IUserSession $userSession, - Util $util, - Session $session, - Crypt $crypt, - Recovery $recovery) { - $this->keyManager = $keyManager; - $this->userManager = $userManager; - $this->logger = $logger; - $this->userSetup = $userSetup; - $this->userSession = $userSession; - $this->util = $util; - $this->session = $session; - $this->recovery = $recovery; - $this->crypt = $crypt; + protected static array $passwordResetUsers = []; + + public function __construct( + private KeyManager $keyManager, + private IUserManager $userManager, + private LoggerInterface $logger, + private Setup $userSetup, + private IUserSession $userSession, + private Util $util, + private Session $session, + private Crypt $crypt, + private Recovery $recovery, + ) { } /** @@ -244,7 +186,6 @@ public function preSetPassphrase($params) { * @return boolean|null */ public function setPassphrase($params) { - // if we are in the process to resetting a user password, we have nothing // to do here if (isset(self::$passwordResetUsers[$params['uid']])) { @@ -298,7 +239,6 @@ public function setPassphrase($params) { || !$this->keyManager->userHasKeys($userId) || !$this->util->userHasFiles($userId) ) { - // backup old keys //$this->backupAllKeys('recovery'); diff --git a/apps/encryption/lib/KeyManager.php b/apps/encryption/lib/KeyManager.php index f0005b4576176..7d6380f3b8372 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -39,106 +39,34 @@ use OCA\Encryption\Exceptions\PublicKeyMissingException; use OCP\Encryption\Keys\IStorage; use OCP\IConfig; -use OCP\ILogger; use OCP\IUserSession; use OCP\Lock\ILockingProvider; +use Psr\Log\LoggerInterface; class KeyManager { - /** - * @var Session - */ - protected $session; - /** - * @var IStorage - */ - private $keyStorage; - /** - * @var Crypt - */ - private $crypt; - /** - * @var string - */ - private $recoveryKeyId; - /** - * @var string - */ - private $publicShareKeyId; - /** - * @var string - */ - private $masterKeyId; - /** - * @var string UserID - */ - private $keyId; - /** - * @var string - */ - private $publicKeyId = 'publicKey'; - /** - * @var string - */ - private $privateKeyId = 'privateKey'; + private string $recoveryKeyId; + private string $publicShareKeyId; + private string $masterKeyId; + private string $keyId; + private string $publicKeyId = 'publicKey'; + private string $privateKeyId = 'privateKey'; + private string $shareKeyId = 'shareKey'; + private string $fileKeyId = 'fileKey'; - /** - * @var string - */ - private $shareKeyId = 'shareKey'; - - /** - * @var string - */ - private $fileKeyId = 'fileKey'; - /** - * @var IConfig - */ - private $config; - /** - * @var ILogger - */ - private $log; - /** - * @var Util - */ - private $util; - - /** - * @var ILockingProvider - */ - private $lockingProvider; - - /** - * @param IStorage $keyStorage - * @param Crypt $crypt - * @param IConfig $config - * @param IUserSession $userSession - * @param Session $session - * @param ILogger $log - * @param Util $util - */ public function __construct( - IStorage $keyStorage, - Crypt $crypt, - IConfig $config, + private IStorage $keyStorage, + private Crypt $crypt, + private IConfig $config, IUserSession $userSession, - Session $session, - ILogger $log, - Util $util, - ILockingProvider $lockingProvider + private Session $session, + private LoggerInterface $logger, + private Util $util, + private ILockingProvider $lockingProvider, ) { - $this->util = $util; - $this->session = $session; - $this->keyStorage = $keyStorage; - $this->crypt = $crypt; - $this->config = $config; - $this->log = $log; - $this->lockingProvider = $lockingProvider; - $this->recoveryKeyId = $this->config->getAppValue('encryption', 'recoveryKeyId'); if (empty($this->recoveryKeyId)) { - $this->recoveryKeyId = 'recoveryKey_' . substr(md5(time()), 0, 8); + $this->recoveryKeyId = 'recoveryKey_' . substr(md5((string)time()), 0, 8); $this->config->setAppValue('encryption', 'recoveryKeyId', $this->recoveryKeyId); @@ -147,19 +75,18 @@ public function __construct( $this->publicShareKeyId = $this->config->getAppValue('encryption', 'publicShareKeyId'); if (empty($this->publicShareKeyId)) { - $this->publicShareKeyId = 'pubShare_' . substr(md5(time()), 0, 8); + $this->publicShareKeyId = 'pubShare_' . substr(md5((string)time()), 0, 8); $this->config->setAppValue('encryption', 'publicShareKeyId', $this->publicShareKeyId); } $this->masterKeyId = $this->config->getAppValue('encryption', 'masterKeyId'); if (empty($this->masterKeyId)) { - $this->masterKeyId = 'master_' . substr(md5(time()), 0, 8); + $this->masterKeyId = 'master_' . substr(md5((string)time()), 0, 8); $this->config->setAppValue('encryption', 'masterKeyId', $this->masterKeyId); } $this->keyId = $userSession->isLoggedIn() ? $userSession->getUser()->getUID() : false; - $this->log = $log; } /** @@ -223,10 +150,10 @@ public function validateMasterKey() { } $this->lockingProvider->releaseLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE); } elseif (empty($publicMasterKey)) { - $this->log->error('A private master key is available but the public key could not be found. This should never happen.'); + $this->logger->error('A private master key is available but the public key could not be found. This should never happen.'); return; } elseif (empty($privateMasterKey)) { - $this->log->error('A public master key is available but the private key could not be found. This should never happen.'); + $this->logger->error('A public master key is available but the private key could not be found. This should never happen.'); return; } @@ -405,11 +332,13 @@ public function init($uid, $passPhrase) { } catch (DecryptionFailedException $e) { return false; } catch (\Exception $e) { - $this->log->logException($e, [ - 'message' => 'Could not decrypt the private key from user "' . $uid . '"" during login. Assume password change on the user back-end.', - 'level' => ILogger::WARN, - 'app' => 'encryption', - ]); + $this->logger->warning( + 'Could not decrypt the private key from user "' . $uid . '"" during login. Assume password change on the user back-end.', + [ + 'app' => 'encryption', + 'exception' => $e, + ] + ); return false; } diff --git a/apps/encryption/lib/Settings/Admin.php b/apps/encryption/lib/Settings/Admin.php index e69379bb414cb..9ce765723d876 100644 --- a/apps/encryption/lib/Settings/Admin.php +++ b/apps/encryption/lib/Settings/Admin.php @@ -32,46 +32,21 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; use OCP\IL10N; -use OCP\ILogger; use OCP\ISession; use OCP\IUserManager; use OCP\IUserSession; use OCP\Settings\ISettings; +use Psr\Log\LoggerInterface; class Admin implements ISettings { - - /** @var IL10N */ - private $l; - - /** @var ILogger */ - private $logger; - - /** @var IUserSession */ - private $userSession; - - /** @var IConfig */ - private $config; - - /** @var IUserManager */ - private $userManager; - - /** @var ISession */ - private $session; - public function __construct( - IL10N $l, - ILogger $logger, - IUserSession $userSession, - IConfig $config, - IUserManager $userManager, - ISession $session + private IL10N $l, + private LoggerInterface $logger, + private IUserSession $userSession, + private IConfig $config, + private IUserManager $userManager, + private ISession $session ) { - $this->l = $l; - $this->logger = $logger; - $this->userSession = $userSession; - $this->config = $config; - $this->userManager = $userManager; - $this->session = $session; } /** @@ -87,7 +62,6 @@ public function getForm() { $util = new Util( new View(), $crypt, - $this->logger, $this->userSession, $this->config, $this->userManager); diff --git a/apps/encryption/lib/Util.php b/apps/encryption/lib/Util.php index 07bca9cbfdf3a..f1b43413be323 100644 --- a/apps/encryption/lib/Util.php +++ b/apps/encryption/lib/Util.php @@ -29,59 +29,24 @@ use OC\Files\View; use OCA\Encryption\Crypto\Crypt; use OCP\IConfig; -use OCP\ILogger; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; use OCP\PreConditionNotMetException; class Util { - /** - * @var View - */ - private $files; - /** - * @var Crypt - */ - private $crypt; - /** - * @var ILogger - */ - private $logger; - /** - * @var bool|IUser - */ - private $user; - /** - * @var IConfig - */ - private $config; - /** - * @var IUserManager - */ - private $userManager; + private IUser|false $user; - /** - * Util constructor. - * - * @param View $files - * @param Crypt $crypt - * @param ILogger $logger - * @param IUserSession $userSession - * @param IConfig $config - * @param IUserManager $userManager - */ - public function __construct(View $files, - Crypt $crypt, - ILogger $logger, - IUserSession $userSession, - IConfig $config, - IUserManager $userManager + public function __construct( + private View $files, + private Crypt $crypt, + IUserSession $userSession, + private IConfig $config, + private IUserManager $userManager, ) { $this->files = $files; $this->crypt = $crypt; - $this->logger = $logger; - $this->user = $userSession && $userSession->isLoggedIn() ? $userSession->getUser() : false; + $this->user = $userSession->isLoggedIn() ? $userSession->getUser() : false; $this->config = $config; $this->userManager = $userManager; } @@ -132,10 +97,8 @@ public function setEncryptHomeStorage($encryptHomeStorage) { /** * check if master key is enabled - * - * @return bool */ - public function isMasterKeyEnabled() { + public function isMasterKeyEnabled(): bool { $userMasterKey = $this->config->getAppValue('encryption', 'useMasterKey', '1'); return ($userMasterKey === '1'); } diff --git a/apps/encryption/tests/Command/FixEncryptedVersionTest.php b/apps/encryption/tests/Command/FixEncryptedVersionTest.php index 2a6c86ef5b23c..54e8b41ed6c0a 100644 --- a/apps/encryption/tests/Command/FixEncryptedVersionTest.php +++ b/apps/encryption/tests/Command/FixEncryptedVersionTest.php @@ -24,6 +24,7 @@ use OC\Files\View; use OCA\Encryption\Command\FixEncryptedVersion; use OCA\Encryption\Util; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Tester\CommandTester; use Test\TestCase; use Test\Traits\EncryptionTrait; @@ -70,7 +71,7 @@ public function setUp(): void { $this->fixEncryptedVersion = new FixEncryptedVersion( \OC::$server->getConfig(), - \OC::$server->getLogger(), + \OC::$server->get(LoggerInterface::class), \OC::$server->getRootFolder(), \OC::$server->getUserManager(), $this->util, diff --git a/apps/encryption/tests/Crypto/CryptTest.php b/apps/encryption/tests/Crypto/CryptTest.php index 0bb2c652d8b3b..06caa5aba1a59 100644 --- a/apps/encryption/tests/Crypto/CryptTest.php +++ b/apps/encryption/tests/Crypto/CryptTest.php @@ -29,12 +29,12 @@ use OCA\Encryption\Crypto\Crypt; use OCP\IConfig; use OCP\IL10N; -use OCP\ILogger; +use Psr\Log\LoggerInterface; use OCP\IUserSession; use Test\TestCase; class CryptTest extends TestCase { - /** @var \OCP\ILogger|\PHPUnit\Framework\MockObject\MockObject */ + /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ private $logger; /** @var \OCP\IUserSession|\PHPUnit\Framework\MockObject\MockObject */ @@ -52,7 +52,7 @@ class CryptTest extends TestCase { protected function setUp(): void { parent::setUp(); - $this->logger = $this->getMockBuilder(ILogger::class) + $this->logger = $this->getMockBuilder(LoggerInterface::class) ->disableOriginalConstructor() ->getMock(); $this->logger->expects($this->any()) diff --git a/apps/encryption/tests/Crypto/EncryptionTest.php b/apps/encryption/tests/Crypto/EncryptionTest.php index 675a8bf8e2921..115b90542783e 100644 --- a/apps/encryption/tests/Crypto/EncryptionTest.php +++ b/apps/encryption/tests/Crypto/EncryptionTest.php @@ -36,7 +36,7 @@ use OCA\Encryption\Util; use OCP\Files\Storage; use OCP\IL10N; -use OCP\ILogger; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Test\TestCase; @@ -63,7 +63,7 @@ class EncryptionTest extends TestCase { /** @var \OCA\Encryption\Util|\PHPUnit\Framework\MockObject\MockObject */ private $utilMock; - /** @var \OCP\ILogger|\PHPUnit\Framework\MockObject\MockObject */ + /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ private $loggerMock; /** @var \OCP\IL10N|\PHPUnit\Framework\MockObject\MockObject */ @@ -95,7 +95,7 @@ protected function setUp(): void { $this->decryptAllMock = $this->getMockBuilder(DecryptAll::class) ->disableOriginalConstructor() ->getMock(); - $this->loggerMock = $this->getMockBuilder(ILogger::class) + $this->loggerMock = $this->getMockBuilder(LoggerInterface::class) ->disableOriginalConstructor() ->getMock(); $this->l10nMock = $this->getMockBuilder(IL10N::class) diff --git a/apps/encryption/tests/Hooks/UserHooksTest.php b/apps/encryption/tests/Hooks/UserHooksTest.php index e06feb1e37061..add7e375f0495 100644 --- a/apps/encryption/tests/Hooks/UserHooksTest.php +++ b/apps/encryption/tests/Hooks/UserHooksTest.php @@ -34,7 +34,7 @@ use OCA\Encryption\Session; use OCA\Encryption\Users\Setup; use OCA\Encryption\Util; -use OCP\ILogger; +use Psr\Log\LoggerInterface; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; @@ -160,7 +160,6 @@ public function testPostPasswordReset() { * @dataProvider dataTestPreSetPassphrase */ public function testPreSetPassphrase($canChange) { - /** @var UserHooks | \PHPUnit\Framework\MockObject\MockObject $instance */ $instance = $this->getMockBuilder(UserHooks::class) ->setConstructorArgs( @@ -332,7 +331,7 @@ public function XtestSetPasswordNoUser() { protected function setUp(): void { parent::setUp(); - $this->loggerMock = $this->createMock(ILogger::class); + $this->loggerMock = $this->createMock(LoggerInterface::class); $this->keyManagerMock = $this->getMockBuilder(KeyManager::class) ->disableOriginalConstructor() ->getMock(); diff --git a/apps/encryption/tests/KeyManagerTest.php b/apps/encryption/tests/KeyManagerTest.php index be116318185a0..89956b7558c84 100644 --- a/apps/encryption/tests/KeyManagerTest.php +++ b/apps/encryption/tests/KeyManagerTest.php @@ -41,7 +41,7 @@ use OCP\Files\Cache\ICache; use OCP\Files\Storage; use OCP\IConfig; -use OCP\ILogger; +use Psr\Log\LoggerInterface; use OCP\IUserSession; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; @@ -73,7 +73,7 @@ class KeyManagerTest extends TestCase { /** @var \OCA\Encryption\Session|\PHPUnit\Framework\MockObject\MockObject */ private $sessionMock; - /** @var \OCP\ILogger|\PHPUnit\Framework\MockObject\MockObject */ + /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ private $logMock; /** @var \OCA\Encryption\Util|\PHPUnit\Framework\MockObject\MockObject */ @@ -101,7 +101,7 @@ protected function setUp(): void { $this->sessionMock = $this->getMockBuilder(Session::class) ->disableOriginalConstructor() ->getMock(); - $this->logMock = $this->createMock(ILogger::class); + $this->logMock = $this->createMock(LoggerInterface::class); $this->utilMock = $this->getMockBuilder(Util::class) ->disableOriginalConstructor() ->getMock(); @@ -600,6 +600,9 @@ public function testValidateMasterKey($masterKey) { )->setMethods(['getPublicMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword']) ->getMock(); + $this->utilMock->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + $instance->expects($this->once())->method('getPublicMasterKey') ->willReturn($masterKey); @@ -645,6 +648,9 @@ public function testValidateMasterKeyLocked() { )->setMethods(['getPublicMasterKey', 'getPrivateMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword']) ->getMock(); + $this->utilMock->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + $instance->expects($this->once())->method('getPublicMasterKey') ->willReturn(''); $instance->expects($this->once())->method('getPrivateMasterKey') diff --git a/apps/encryption/tests/Settings/AdminTest.php b/apps/encryption/tests/Settings/AdminTest.php index e4b2933dd3bdf..055b65e2e28af 100644 --- a/apps/encryption/tests/Settings/AdminTest.php +++ b/apps/encryption/tests/Settings/AdminTest.php @@ -29,7 +29,7 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; use OCP\IL10N; -use OCP\ILogger; +use Psr\Log\LoggerInterface; use OCP\ISession; use OCP\IUserManager; use OCP\IUserSession; @@ -40,7 +40,7 @@ class AdminTest extends TestCase { private $admin; /** @var IL10N */ private $l; - /** @var ILogger */ + /** @var LoggerInterface */ private $logger; /** @var IUserSession */ private $userSession; @@ -55,7 +55,7 @@ protected function setUp(): void { parent::setUp(); $this->l = $this->getMockBuilder(IL10N::class)->getMock(); - $this->logger = $this->getMockBuilder(ILogger::class)->getMock(); + $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $this->userSession = $this->getMockBuilder(IUserSession::class)->getMock(); $this->config = $this->getMockBuilder(IConfig::class)->getMock(); $this->userManager = $this->getMockBuilder(IUserManager::class)->getMock(); diff --git a/apps/encryption/tests/UtilTest.php b/apps/encryption/tests/UtilTest.php index 9d5d93809bcda..3c3a8ba371acd 100644 --- a/apps/encryption/tests/UtilTest.php +++ b/apps/encryption/tests/UtilTest.php @@ -33,7 +33,6 @@ use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage; use OCP\IConfig; -use OCP\ILogger; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; @@ -89,8 +88,6 @@ protected function setUp(): void { $cryptMock = $this->getMockBuilder(Crypt::class) ->disableOriginalConstructor() ->getMock(); - /** @var \OCP\ILogger $loggerMock */ - $loggerMock = $this->createMock(ILogger::class); $user = $this->createMock(IUser::class); $user->expects($this->any()) @@ -116,7 +113,7 @@ protected function setUp(): void { ->method('setUserValue') ->willReturnCallback([$this, 'setValueTester']); - $this->instance = new Util($this->filesMock, $cryptMock, $loggerMock, $userSessionMock, $this->configMock, $this->userManagerMock); + $this->instance = new Util($this->filesMock, $cryptMock, $userSessionMock, $this->configMock, $this->userManagerMock); } /** From 487f5963a1ee6fcc0cddc15638160d183a7a4e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 29 Jun 2023 17:09:45 +0200 Subject: [PATCH 2/3] Use Server::get instead of new to avoid troubles with encryption constructors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../Controller/ChangePasswordController.php | 32 ++----------------- 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/apps/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php index 20ec28220a5a8..10ad11935c652 100644 --- a/apps/settings/lib/Controller/ChangePasswordController.php +++ b/apps/settings/lib/Controller/ChangePasswordController.php @@ -171,36 +171,8 @@ public function changeUserPassword(string $username = null, string $password = n if ($this->appManager->isEnabledForUser('encryption')) { //handle the recovery case - $crypt = new \OCA\Encryption\Crypto\Crypt( - \OC::$server->getLogger(), - \OC::$server->getUserSession(), - \OC::$server->getConfig(), - \OC::$server->getL10N('encryption')); - $keyStorage = \OC::$server->getEncryptionKeyStorage(); - $util = new \OCA\Encryption\Util( - new \OC\Files\View(), - $crypt, - \OC::$server->getLogger(), - \OC::$server->getUserSession(), - \OC::$server->getConfig(), - \OC::$server->getUserManager()); - $keyManager = new \OCA\Encryption\KeyManager( - $keyStorage, - $crypt, - \OC::$server->getConfig(), - \OC::$server->getUserSession(), - new \OCA\Encryption\Session(\OC::$server->getSession()), - \OC::$server->getLogger(), - $util, - \OC::$server->getLockingProvider() - ); - $recovery = new \OCA\Encryption\Recovery( - \OC::$server->getUserSession(), - $crypt, - $keyManager, - \OC::$server->getConfig(), - \OC::$server->getEncryptionFilesHelper(), - new \OC\Files\View()); + $keyManager = \OCP\Server::get(\OCA\Encryption\KeyManager::class); + $recovery = \OCP\Server::get(\OCA\Encryption\Recovery::class); $recoveryAdminEnabled = $recovery->isRecoveryKeyEnabled(); $validRecoveryPassword = false; From 3e176f58af0e81588b20363dc36a295001284fc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 29 Jun 2023 17:14:17 +0200 Subject: [PATCH 3/3] Improve typing as suggested by review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/encryption/lib/Crypto/Crypt.php | 39 +++++----------------------- 1 file changed, 7 insertions(+), 32 deletions(-) diff --git a/apps/encryption/lib/Crypto/Crypt.php b/apps/encryption/lib/Crypto/Crypt.php index cc963f51b8687..ee01d632be8a9 100644 --- a/apps/encryption/lib/Crypto/Crypt.php +++ b/apps/encryption/lib/Crypto/Crypt.php @@ -153,9 +153,6 @@ public function getOpenSSLPKey() { return openssl_pkey_new($config); } - /** - * get openSSL Config - */ private function getOpenSSLConfig(): array { $config = ['private_key_bits' => 4096]; $config = array_merge( @@ -217,13 +214,9 @@ public function generateHeader($keyFormat = self::DEFAULT_KEY_FORMAT) { } /** - * @param string $plainContent - * @param string $iv - * @param string $passPhrase - * @param string $cipher * @throws EncryptionFailedException */ - private function encrypt($plainContent, $iv, $passPhrase = '', $cipher = self::DEFAULT_CIPHER): string { + private function encrypt(string $plainContent, string $iv, string $passPhrase = '', string $cipher = self::DEFAULT_CIPHER): string { $options = $this->useLegacyBase64Encoding ? 0 : OPENSSL_RAW_DATA; $encryptedContent = openssl_encrypt($plainContent, $cipher, @@ -311,19 +304,11 @@ public function getLegacyCipher() { return self::LEGACY_CIPHER; } - /** - * @param string $encryptedContent - * @param string $iv - */ - private function concatIV($encryptedContent, $iv): string { + private function concatIV(string $encryptedContent, string $iv): string { return $encryptedContent . '00iv00' . $iv; } - /** - * @param string $encryptedContent - * @param string $signature - */ - private function concatSig($encryptedContent, $signature): string { + private function concatSig(string $encryptedContent, string $signature): string { return $encryptedContent . '00sig00' . $signature; } @@ -331,10 +316,8 @@ private function concatSig($encryptedContent, $signature): string { * Note: This is _NOT_ a padding used for encryption purposes. It is solely * used to achieve the PHP stream size. It has _NOTHING_ to do with the * encrypted content and is not used in any crypto primitive. - * - * @param string $data */ - private function addPadding($data): string { + private function addPadding(string $data): string { return $data . 'xxx'; } @@ -514,12 +497,9 @@ private function createSignature(string $data, string $passPhrase): string { /** - * remove padding - * - * @param string $padded * @param bool $hasSignature did the block contain a signature, in this case we use a different padding */ - private function removePadding($padded, $hasSignature = false): string|false { + private function removePadding(string $padded, bool $hasSignature = false): string|false { if ($hasSignature === false && substr($padded, -2) === 'xx') { return substr($padded, 0, -2); } elseif ($hasSignature === true && substr($padded, -3) === 'xxx') { @@ -532,11 +512,8 @@ private function removePadding($padded, $hasSignature = false): string|false { * split meta data from encrypted file * Note: for now, we assume that the meta data always start with the iv * followed by the signature, if available - * - * @param string $catFile - * @param string $cipher */ - private function splitMetaData($catFile, $cipher): array { + private function splitMetaData(string $catFile, string $cipher): array { if ($this->hasSignature($catFile, $cipher)) { $catFile = $this->removePadding($catFile, true); $meta = substr($catFile, -93); @@ -561,11 +538,9 @@ private function splitMetaData($catFile, $cipher): array { /** * check if encrypted block is signed * - * @param string $catFile - * @param string $cipher * @throws GenericEncryptionException */ - private function hasSignature($catFile, $cipher): bool { + private function hasSignature(string $catFile, string $cipher): bool { $skipSignatureCheck = $this->config->getSystemValueBool('encryption_skip_signature_check', false); $meta = substr($catFile, -93);