From dc479aae2d055dafddb250a382eb801a68d42afb Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Sun, 5 Jul 2020 14:31:19 +0200 Subject: Improve CertificateManager to not be user context dependent * removes the ability for users to import their own certificates (for external storage) * reliably returns the same certificate bundles system wide (and not depending on the user context and available sessions) The user specific certificates were broken in some cases anyways, as they are only loaded if the specific user is logged in and thus causing unexpected behavior for background jobs and other non-user triggered code paths. Signed-off-by: Morris Jobke --- apps/dav/lib/CardDAV/SyncService.php | 4 +- apps/files_sharing/lib/External/Manager.php | 2 +- apps/files_sharing/lib/External/MountProvider.php | 2 +- core/register_command.php | 6 +-- lib/private/Files/Storage/DAV.php | 3 -- lib/private/Security/CertificateManager.php | 66 ++++++----------------- lib/private/Server.php | 44 +++------------ lib/public/ICertificateManager.php | 14 +++-- lib/public/IServerContainer.php | 7 ++- tests/lib/Http/Client/ClientTest.php | 6 +-- tests/lib/Security/CertificateManagerTest.php | 61 +++++++-------------- tests/lib/ServerTest.php | 4 +- 12 files changed, 61 insertions(+), 158 deletions(-) diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 1cd01066a7c..bcb20409524 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -31,7 +31,6 @@ namespace OCA\DAV\CardDAV; use OC\Accounts\AccountManager; use OCP\AppFramework\Http; -use OCP\ICertificateManager; use OCP\ILogger; use OCP\IUser; use OCP\IUserManager; @@ -155,8 +154,7 @@ class SyncService { return $this->certPath; } - /** @var ICertificateManager $certManager */ - $certManager = \OC::$server->getCertificateManager(null); + $certManager = \OC::$server->getCertificateManager(); $certPath = $certManager->getAbsoluteBundlePath(); if (file_exists($certPath)) { $this->certPath = $certPath; diff --git a/apps/files_sharing/lib/External/Manager.php b/apps/files_sharing/lib/External/Manager.php index c922754207c..87146dd4268 100644 --- a/apps/files_sharing/lib/External/Manager.php +++ b/apps/files_sharing/lib/External/Manager.php @@ -441,7 +441,7 @@ class Manager { $data['manager'] = $this; $mountPoint = '/' . $this->uid . '/files' . $data['mountpoint']; $data['mountpoint'] = $mountPoint; - $data['certificateManager'] = \OC::$server->getCertificateManager($this->uid); + $data['certificateManager'] = \OC::$server->getCertificateManager(); return new Mount(self::STORAGE, $mountPoint, $data, $this, $this->storageLoader); } diff --git a/apps/files_sharing/lib/External/MountProvider.php b/apps/files_sharing/lib/External/MountProvider.php index ecff358abe5..eb8f1b8fde6 100644 --- a/apps/files_sharing/lib/External/MountProvider.php +++ b/apps/files_sharing/lib/External/MountProvider.php @@ -66,7 +66,7 @@ class MountProvider implements IMountProvider { $mountPoint = '/' . $user->getUID() . '/files/' . ltrim($data['mountpoint'], '/'); $data['mountpoint'] = $mountPoint; $data['cloudId'] = $this->cloudIdManager->getCloudId($data['owner'], $data['remote']); - $data['certificateManager'] = \OC::$server->getCertificateManager($user->getUID()); + $data['certificateManager'] = \OC::$server->getCertificateManager(); $data['HttpClientService'] = \OC::$server->getHTTPClientService(); return new Mount(self::STORAGE, $mountPoint, $data, $manager, $storageFactory); } diff --git a/core/register_command.php b/core/register_command.php index 840c73484bf..af6bd677251 100644 --- a/core/register_command.php +++ b/core/register_command.php @@ -183,9 +183,9 @@ if (\OC::$server->getConfig()->getSystemValue('installed', false)) { $application->add(new OC\Core\Command\Group\AddUser(\OC::$server->getUserManager(), \OC::$server->getGroupManager())); $application->add(new OC\Core\Command\Group\RemoveUser(\OC::$server->getUserManager(), \OC::$server->getGroupManager())); - $application->add(new OC\Core\Command\Security\ListCertificates(\OC::$server->getCertificateManager(null), \OC::$server->getL10N('core'))); - $application->add(new OC\Core\Command\Security\ImportCertificate(\OC::$server->getCertificateManager(null))); - $application->add(new OC\Core\Command\Security\RemoveCertificate(\OC::$server->getCertificateManager(null))); + $application->add(new OC\Core\Command\Security\ListCertificates(\OC::$server->getCertificateManager(), \OC::$server->getL10N('core'))); + $application->add(new OC\Core\Command\Security\ImportCertificate(\OC::$server->getCertificateManager())); + $application->add(new OC\Core\Command\Security\RemoveCertificate(\OC::$server->getCertificateManager())); $application->add(new OC\Core\Command\Security\ResetBruteforceAttempts(\OC::$server->getBruteForceThrottler())); } else { $application->add(\OC::$server->get(\OC\Core\Command\Maintenance\Install::class)); diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php index a6cfd77d98a..974feee8995 100644 --- a/lib/private/Files/Storage/DAV.php +++ b/lib/private/Files/Storage/DAV.php @@ -122,9 +122,6 @@ class DAV extends Common { if ($this->secure === true) { // inject mock for testing $this->certManager = \OC::$server->getCertificateManager(); - if (is_null($this->certManager)) { //no user - $this->certManager = \OC::$server->getCertificateManager(null); - } } $this->root = $params['root'] ?? '/'; $this->root = '/' . ltrim($this->root, '/'); diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php index e69132ff4df..ed873527d3c 100644 --- a/lib/private/Security/CertificateManager.php +++ b/lib/private/Security/CertificateManager.php @@ -39,11 +39,6 @@ use OCP\Security\ISecureRandom; * Manage trusted certificates for users */ class CertificateManager implements ICertificateManager { - /** - * @var string - */ - protected $uid; - /** * @var \OC\Files\View */ @@ -63,18 +58,15 @@ class CertificateManager implements ICertificateManager { protected $random; /** - * @param string $uid * @param \OC\Files\View $view relative to data/ * @param IConfig $config * @param ILogger $logger * @param ISecureRandom $random */ - public function __construct($uid, - \OC\Files\View $view, + public function __construct(\OC\Files\View $view, IConfig $config, ILogger $logger, ISecureRandom $random) { - $this->uid = $uid; $this->view = $view; $this->config = $config; $this->logger = $logger; @@ -148,7 +140,7 @@ class CertificateManager implements ICertificateManager { fwrite($fhCerts, $defaultCertificates); // Append the system certificate bundle - $systemBundle = $this->getCertificateBundle(null); + $systemBundle = $this->getCertificateBundle(); if ($systemBundle !== $certPath && $this->view->file_exists($systemBundle)) { $systemCertificates = $this->view->file_get_contents($systemBundle); fwrite($fhCerts, $systemCertificates); @@ -207,73 +199,45 @@ class CertificateManager implements ICertificateManager { } /** - * Get the path to the certificate bundle for this user + * Get the path to the certificate bundle * - * @param string|null $uid (optional) user to get the certificate bundle for, use `null` to get the system bundle * @return string */ - public function getCertificateBundle($uid = '') { - if ($uid === '') { - $uid = $this->uid; - } - return $this->getPathToCertificates($uid) . 'rootcerts.crt'; + public function getCertificateBundle() { + return $this->getPathToCertificates() . 'rootcerts.crt'; } /** - * Get the full local path to the certificate bundle for this user + * Get the full local path to the certificate bundle * - * @param string $uid (optional) user to get the certificate bundle for, use `null` to get the system bundle * @return string */ - public function getAbsoluteBundlePath($uid = '') { - if ($uid === '') { - $uid = $this->uid; - } - if ($this->needsRebundling($uid)) { - if (is_null($uid)) { - $manager = new CertificateManager(null, $this->view, $this->config, $this->logger, $this->random); - $manager->createCertificateBundle(); - } else { - $this->createCertificateBundle(); - } + public function getAbsoluteBundlePath() { + if ($this->needsRebundling()) { + $this->createCertificateBundle(); } - return $this->view->getLocalFile($this->getCertificateBundle($uid)); + return $this->view->getLocalFile($this->getCertificateBundle()); } /** - * @param string|null $uid (optional) user to get the certificate path for, use `null` to get the system path * @return string */ - private function getPathToCertificates($uid = '') { - if ($uid === '') { - $uid = $this->uid; - } - return is_null($uid) ? '/files_external/' : '/' . $uid . '/files_external/'; + private function getPathToCertificates() { + return '/files_external/'; } /** * Check if we need to re-bundle the certificates because one of the sources has updated * - * @param string $uid (optional) user to get the certificate path for, use `null` to get the system path * @return bool */ - private function needsRebundling($uid = '') { - if ($uid === '') { - $uid = $this->uid; - } - $sourceMTimes = [$this->getFilemtimeOfCaBundle()]; - $targetBundle = $this->getCertificateBundle($uid); + private function needsRebundling() { + $targetBundle = $this->getCertificateBundle(); if (!$this->view->file_exists($targetBundle)) { return true; } - if (!is_null($uid)) { // also depend on the system bundle - $sourceMTimes[] = $this->view->filemtime($this->getCertificateBundle(null)); - } - - $sourceMTime = array_reduce($sourceMTimes, function ($max, $mtime) { - return max($max, $mtime); - }, 0); + $sourceMTime = $this->getFilemtimeOfCaBundle(); return $sourceMTime > $this->view->filemtime($targetBundle); } diff --git a/lib/private/Server.php b/lib/private/Server.php index 5de7808523e..8e2452b2f30 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -178,6 +178,7 @@ use OCP\IAppConfig; use OCP\IAvatarManager; use OCP\ICache; use OCP\ICacheFactory; +use OCP\ICertificateManager; use OCP\IDateTimeFormatter; use OCP\IDateTimeZone; use OCP\IDBConnection; @@ -823,23 +824,8 @@ class Server extends ServerContainer implements IServerContainer { /** @deprecated 19.0.0 */ $this->registerDeprecatedAlias('DatabaseConnection', IDBConnection::class); - - $this->registerService(IClientService::class, function (ContainerInterface $c) { - $user = \OC_User::getUser(); - $uid = $user ? $user : null; - return new ClientService( - $c->get(\OCP\IConfig::class), - $c->get(ILogger::class), - new \OC\Security\CertificateManager( - $uid, - new View(), - $c->get(\OCP\IConfig::class), - $c->get(ILogger::class), - $c->get(ISecureRandom::class) - ) - ); - }); - /** @deprecated 19.0.0 */ + $this->registerAlias(ICertificateManager::class, CertificateManager::class); + $this->registerAlias(IClientService::class, ClientService::class); $this->registerDeprecatedAlias('HttpClientService', IClientService::class); $this->registerService(IEventLogger::class, function (ContainerInterface $c) { $eventLogger = new EventLogger(); @@ -1840,28 +1826,12 @@ class Server extends ServerContainer implements IServerContainer { } /** - * Get the certificate manager for the user + * Get the certificate manager * - * @param string $userId (optional) if not specified the current loggedin user is used, use null to get the system certificate manager - * @return \OCP\ICertificateManager | null if $uid is null and no user is logged in - * @deprecated 20.0.0 + * @return \OCP\ICertificateManager */ - public function getCertificateManager($userId = '') { - if ($userId === '') { - $userSession = $this->get(IUserSession::class); - $user = $userSession->getUser(); - if (is_null($user)) { - return null; - } - $userId = $user->getUID(); - } - return new CertificateManager( - $userId, - new View(), - $this->get(\OCP\IConfig::class), - $this->get(ILogger::class), - $this->get(ISecureRandom::class) - ); + public function getCertificateManager() { + return $this->get(ICertificateManager::class); } /** diff --git a/lib/public/ICertificateManager.php b/lib/public/ICertificateManager.php index 1c7949a89cf..da97dc105d0 100644 --- a/lib/public/ICertificateManager.php +++ b/lib/public/ICertificateManager.php @@ -25,12 +25,12 @@ namespace OCP; /** - * Manage trusted certificates for users + * Manage trusted certificates * @since 8.0.0 */ interface ICertificateManager { /** - * Returns all certificates trusted by the user + * Returns all certificates trusted by the system * * @return \OCP\ICertificate[] * @since 8.0.0 @@ -53,20 +53,18 @@ interface ICertificateManager { public function removeCertificate($name); /** - * Get the path to the certificate bundle for this user + * Get the path to the certificate bundle * - * @param string $uid (optional) user to get the certificate bundle for, use `null` to get the system bundle (since 9.0.0) * @return string * @since 8.0.0 */ - public function getCertificateBundle($uid = ''); + public function getCertificateBundle(); /** - * Get the full local path to the certificate bundle for this user + * Get the full local path to the certificate bundle * - * @param string $uid (optional) user to get the certificate bundle for, use `null` to get the system bundle * @return string * @since 9.0.0 */ - public function getAbsoluteBundlePath($uid = ''); + public function getAbsoluteBundlePath(); } diff --git a/lib/public/IServerContainer.php b/lib/public/IServerContainer.php index 215cfab84fd..ed08b66b994 100644 --- a/lib/public/IServerContainer.php +++ b/lib/public/IServerContainer.php @@ -387,14 +387,13 @@ interface IServerContainer extends ContainerInterface, IContainer { public function getSearch(); /** - * Get the certificate manager for the user + * Get the certificate manager * - * @param string $userId (optional) if not specified the current loggedin user is used, use null to get the system certificate manager - * @return \OCP\ICertificateManager | null if $userId is null and no user is logged in + * @return \OCP\ICertificateManager * @since 8.0.0 * @deprecated 20.0.0 have it injected or fetch it through \Psr\Container\ContainerInterface::get */ - public function getCertificateManager($userId = null); + public function getCertificateManager(); /** * Create a new event source diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index a462a0848ae..78054725e8c 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -281,7 +281,7 @@ class ClientTest extends \Test\TestCase { $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') - ->with(null) + ->with() ->willReturn('/my/path.crt'); $this->defaultRequestOptions = [ @@ -493,7 +493,7 @@ class ClientTest extends \Test\TestCase { $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') - ->with(null) + ->with() ->willReturn('/my/path.crt'); $this->assertEquals([ @@ -529,7 +529,7 @@ class ClientTest extends \Test\TestCase { $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') - ->with(null) + ->with() ->willReturn('/my/path.crt'); $this->assertEquals([ diff --git a/tests/lib/Security/CertificateManagerTest.php b/tests/lib/Security/CertificateManagerTest.php index 447ee8719c9..3af4b564612 100644 --- a/tests/lib/Security/CertificateManagerTest.php +++ b/tests/lib/Security/CertificateManagerTest.php @@ -53,7 +53,6 @@ class CertificateManagerTest extends \Test\TestCase { ->willReturn('random'); $this->certificateManager = new CertificateManager( - $this->username, new \OC\Files\View(), $config, $this->createMock(ILogger::class), @@ -132,22 +131,18 @@ class CertificateManagerTest extends \Test\TestCase { } public function testGetCertificateBundle() { - $this->assertSame('/' . $this->username . '/files_external/rootcerts.crt', $this->certificateManager->getCertificateBundle()); + $this->assertSame('/files_external/rootcerts.crt', $this->certificateManager->getCertificateBundle()); } /** * @dataProvider dataTestNeedRebundling * - * @param string $uid * @param int $CaBundleMtime - * @param int $systemWideMtime * @param int $targetBundleMtime * @param int $targetBundleExists * @param bool $expected */ - public function testNeedRebundling($uid, - $CaBundleMtime, - $systemWideMtime, + public function testNeedRebundling($CaBundleMtime, $targetBundleMtime, $targetBundleExists, $expected @@ -158,31 +153,24 @@ class CertificateManagerTest extends \Test\TestCase { /** @var CertificateManager | \PHPUnit\Framework\MockObject\MockObject $certificateManager */ $certificateManager = $this->getMockBuilder('OC\Security\CertificateManager') - ->setConstructorArgs([$uid, $view, $config, $this->createMock(ILogger::class), $this->random]) + ->setConstructorArgs([$view, $config, $this->createMock(ILogger::class), $this->random]) ->setMethods(['getFilemtimeOfCaBundle', 'getCertificateBundle']) ->getMock(); $certificateManager->expects($this->any())->method('getFilemtimeOfCaBundle') ->willReturn($CaBundleMtime); - $certificateManager->expects($this->at(1))->method('getCertificateBundle') - ->with($uid)->willReturn('targetBundlePath'); + $certificateManager->expects($this->at(0))->method('getCertificateBundle') + ->willReturn('targetBundlePath'); $view->expects($this->any())->method('file_exists') ->with('targetBundlePath') ->willReturn($targetBundleExists); - if ($uid !== null && $targetBundleExists) { - $certificateManager->expects($this->at(2))->method('getCertificateBundle') - ->with(null)->willReturn('SystemBundlePath'); - } - $view->expects($this->any())->method('filemtime') - ->willReturnCallback(function ($path) use ($systemWideMtime, $targetBundleMtime) { - if ($path === 'SystemBundlePath') { - return $systemWideMtime; - } elseif ($path === 'targetBundlePath') { + ->willReturnCallback(function ($path) use ($targetBundleMtime) { + if ($path === 'targetBundlePath') { return $targetBundleMtime; } throw new \Exception('unexpected path'); @@ -190,37 +178,26 @@ class CertificateManagerTest extends \Test\TestCase { $this->assertSame($expected, - $this->invokePrivate($certificateManager, 'needsRebundling', [$uid]) + $this->invokePrivate($certificateManager, 'needsRebundling') ); } public function dataTestNeedRebundling() { return [ - //values: uid, CaBundleMtime, systemWideMtime, targetBundleMtime, targetBundleExists, expected - - // compare minimum of CaBundleMtime and systemWideMtime with targetBundleMtime - ['user1', 10, 20, 30, true, false], - ['user1', 10, 20, 15, true, true], - ['user1', 10, 5, 30, true, false], - ['user1', 10, 5, 8, true, true], + //values: CaBundleMtime, targetBundleMtime, targetBundleExists, expected - // if no user exists we ignore 'systemWideMtime' because this is the bundle we re-build - [null, 10, 20, 30, true, false], - [null, 10, 20, 15, true, false], - [null, 10, 20, 8, true, true], - [null, 10, 5, 30, true, false], - [null, 10, 5, 8, true, true], + [10, 30, true, false], + [10, 15, true, false], + [10, 8, true, true], + [10, 30, true, false], + [10, 8, true, true], // if no target bundle exists we always build a new one - ['user1', 10, 20, 30, false, true], - ['user1', 10, 20, 15, false, true], - ['user1', 10, 5, 30, false, true], - ['user1', 10, 5, 8, false, true], - [null, 10, 20, 30, false, true], - [null, 10, 20, 15, false, true], - [null, 10, 20, 8, false, true], - [null, 10, 5, 30, false, true], - [null, 10, 5, 8, false, true], + [10, 30, false, true], + [10, 15, false, true], + [10, 8, false, true], + [10, 30, false, true], + [10, 8, false, true], ]; } } diff --git a/tests/lib/ServerTest.php b/tests/lib/ServerTest.php index ada25247393..1054c4195b2 100644 --- a/tests/lib/ServerTest.php +++ b/tests/lib/ServerTest.php @@ -175,8 +175,8 @@ class ServerTest extends \Test\TestCase { } public function testGetCertificateManager() { - $this->assertInstanceOf('\OC\Security\CertificateManager', $this->server->getCertificateManager('test'), 'service returned by "getCertificateManager" did not return the right class'); - $this->assertInstanceOf('\OCP\ICertificateManager', $this->server->getCertificateManager('test'), 'service returned by "getCertificateManager" did not return the right class'); + $this->assertInstanceOf('\OC\Security\CertificateManager', $this->server->getCertificateManager(), 'service returned by "getCertificateManager" did not return the right class'); + $this->assertInstanceOf('\OCP\ICertificateManager', $this->server->getCertificateManager(), 'service returned by "getCertificateManager" did not return the right class'); } public function testCreateEventSource() { -- cgit v1.2.3 From 54b9f639a6cec14236f432c9907edb18d323d94d Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sun, 1 Nov 2020 21:05:36 +0100 Subject: Always return the default path if we can Just check in the certifcate manager. So every part of the system that request the certificatebundle gets the defaullt one (the 99% case) if we can. Signed-off-by: Roeland Jago Douma --- lib/private/Http/Client/Client.php | 4 ---- lib/private/Security/CertificateManager.php | 28 ++++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 35171810a68..5ac29afe31d 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -105,10 +105,6 @@ class Client implements IClient { return \OC::$SERVERROOT . '/resources/config/ca-bundle.crt'; } - if ($this->certificateManager->listCertificates() === []) { - return \OC::$SERVERROOT . '/resources/config/ca-bundle.crt'; - } - return $this->certificateManager->getAbsoluteBundlePath(); } diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php index ed873527d3c..ef0c6563320 100644 --- a/lib/private/Security/CertificateManager.php +++ b/lib/private/Security/CertificateManager.php @@ -104,6 +104,29 @@ class CertificateManager implements ICertificateManager { return $result; } + private function hasCertificates(): bool { + if (!$this->config->getSystemValue('installed', false)) { + return false; + } + + $path = $this->getPathToCertificates() . 'uploads/'; + if (!$this->view->is_dir($path)) { + return false; + } + $result = []; + $handle = $this->view->opendir($path); + if (!is_resource($handle)) { + return false; + } + while (false !== ($file = readdir($handle))) { + if ($file !== '.' && $file !== '..') { + return true; + } + } + closedir($handle); + return false; + } + /** * create the certificate bundle of all trusted certificated */ @@ -213,9 +236,14 @@ class CertificateManager implements ICertificateManager { * @return string */ public function getAbsoluteBundlePath() { + if (!$this->hasCertificates()) { + return \OC::$SERVERROOT . '/resources/config/ca-bundle.crt'; + } + if ($this->needsRebundling()) { $this->createCertificateBundle(); } + return $this->view->getLocalFile($this->getCertificateBundle()); } -- cgit v1.2.3 From 1c496a5a356249dc5bd7c1bbb64ae262b2f8fdab Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Tue, 3 Nov 2020 00:00:05 +0100 Subject: Add a background job that checks for potential user imported SSL certificates and shows a warning in the admin settings Signed-off-by: Morris Jobke --- .../composer/composer/autoload_classmap.php | 1 + .../settings/composer/composer/autoload_static.php | 1 + .../lib/Controller/CheckSetupController.php | 4 ++ .../lib/SetupChecks/CheckUserCertificates.php | 80 ++++++++++++++++++++++ .../tests/Controller/CheckSetupControllerTest.php | 13 ++-- core/BackgroundJobs/CheckForUserCertificates.php | 79 +++++++++++++++++++++ core/js/setupchecks.js | 10 +++ lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + lib/private/Repair.php | 2 + .../Repair/NC21/AddCheckForUserCertificatesJob.php | 61 +++++++++++++++++ version.php | 2 +- 12 files changed, 252 insertions(+), 5 deletions(-) create mode 100644 apps/settings/lib/SetupChecks/CheckUserCertificates.php create mode 100644 core/BackgroundJobs/CheckForUserCertificates.php create mode 100644 lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 3ccd7d9d030..fbe1d1aa4d0 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -56,6 +56,7 @@ return array( 'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => $baseDir . '/../lib/Settings/Personal/Security/TwoFactor.php', 'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => $baseDir . '/../lib/Settings/Personal/Security/WebAuthn.php', 'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => $baseDir . '/../lib/Settings/Personal/ServerDevNotice.php', + 'OCA\\Settings\\SetupChecks\\CheckUserCertificates' => $baseDir . '/../lib/SetupChecks/CheckUserCertificates.php', 'OCA\\Settings\\SetupChecks\\LegacySSEKeyFormat' => $baseDir . '/../lib/SetupChecks/LegacySSEKeyFormat.php', 'OCA\\Settings\\SetupChecks\\PhpDefaultCharset' => $baseDir . '/../lib/SetupChecks/PhpDefaultCharset.php', 'OCA\\Settings\\SetupChecks\\PhpOutputBuffering' => $baseDir . '/../lib/SetupChecks/PhpOutputBuffering.php', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index bf831a81cd4..dda9fa778cc 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/apps/settings/composer/composer/autoload_static.php @@ -71,6 +71,7 @@ class ComposerStaticInitSettings 'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/TwoFactor.php', 'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/WebAuthn.php', 'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => __DIR__ . '/..' . '/../lib/Settings/Personal/ServerDevNotice.php', + 'OCA\\Settings\\SetupChecks\\CheckUserCertificates' => __DIR__ . '/..' . '/../lib/SetupChecks/CheckUserCertificates.php', 'OCA\\Settings\\SetupChecks\\LegacySSEKeyFormat' => __DIR__ . '/..' . '/../lib/SetupChecks/LegacySSEKeyFormat.php', 'OCA\\Settings\\SetupChecks\\PhpDefaultCharset' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpDefaultCharset.php', 'OCA\\Settings\\SetupChecks\\PhpOutputBuffering' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpOutputBuffering.php', diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 76b97eb9dc4..0f9dd84febb 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -53,6 +53,7 @@ use OC\DB\SchemaWrapper; use OC\IntegrityCheck\Checker; use OC\Lock\NoopLockingProvider; use OC\MemoryInfo; +use OCA\Settings\SetupChecks\CheckUserCertificates; use OCA\Settings\SetupChecks\LegacySSEKeyFormat; use OCA\Settings\SetupChecks\PhpDefaultCharset; use OCA\Settings\SetupChecks\PhpOutputBuffering; @@ -692,6 +693,8 @@ Raw output $phpDefaultCharset = new PhpDefaultCharset(); $phpOutputBuffering = new PhpOutputBuffering(); $legacySSEKeyFormat = new LegacySSEKeyFormat($this->l10n, $this->config, $this->urlGenerator); + $checkUserCertificates = new CheckUserCertificates($this->l10n, $this->config, $this->urlGenerator); + return new DataResponse( [ 'isGetenvServerWorking' => !empty(getenv('PATH')), @@ -734,6 +737,7 @@ Raw output PhpDefaultCharset::class => ['pass' => $phpDefaultCharset->run(), 'description' => $phpDefaultCharset->description(), 'severity' => $phpDefaultCharset->severity()], PhpOutputBuffering::class => ['pass' => $phpOutputBuffering->run(), 'description' => $phpOutputBuffering->description(), 'severity' => $phpOutputBuffering->severity()], LegacySSEKeyFormat::class => ['pass' => $legacySSEKeyFormat->run(), 'description' => $legacySSEKeyFormat->description(), 'severity' => $legacySSEKeyFormat->severity(), 'linkToDocumentation' => $legacySSEKeyFormat->linkToDocumentation()], + CheckUserCertificates::class => ['pass' => $checkUserCertificates->run(), 'description' => $checkUserCertificates->description(), 'severity' => $checkUserCertificates->severity(), 'elements' => $checkUserCertificates->elements()], ] ); } diff --git a/apps/settings/lib/SetupChecks/CheckUserCertificates.php b/apps/settings/lib/SetupChecks/CheckUserCertificates.php new file mode 100644 index 00000000000..cbe6c91996a --- /dev/null +++ b/apps/settings/lib/SetupChecks/CheckUserCertificates.php @@ -0,0 +1,80 @@ + + * + * @author Morris Jobke + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Settings\SetupChecks; + +use OCP\IConfig; +use OCP\IL10N; +use OCP\IURLGenerator; + +class CheckUserCertificates { + /** @var IL10N */ + private $l10n; + /** @var string */ + private $configValue; + /** @var IURLGenerator */ + private $urlGenerator; + + public function __construct(IL10N $l10n, IConfig $config, IURLGenerator $urlGenerator) { + $this->l10n = $l10n; + $configValue = $config->getAppValue('files_external', 'user_certificate_scan', false); + if (!is_string($configValue)) { + $configValue = ''; + } + $this->configValue = $configValue; + $this->urlGenerator = $urlGenerator; + } + + public function description(): string { + if ($this->configValue === '') { + return ''; + } + if ($this->configValue === 'not-run-yet') { + return $this->l10n->t('A background job is pending that checks for user imported SSL certificates. Please check back later.'); + } + return $this->l10n->t('There are some user imported SSL certificates present, that are not used anymore with Nextcloud 21. They can be imported on the command line via "occ security:certificates:import" command. Their paths inside the data directory are shown below.'); + } + + public function severity(): string { + return 'warning'; + } + + public function run(): bool { + // all fine if neither "not-run-yet" nor a result + return $this->configValue === ''; + } + + public function elements(): array { + if ($this->configValue === '' || $this->configValue === 'not-run-yet') { + return []; + } + $data = json_decode($this->configValue); + if (!is_array($data)) { + return []; + } + return $data; + } +} diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 64d27dcda89..dbc3fc7d0db 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -382,22 +382,26 @@ class CheckSetupControllerTest extends TestCase { public function testCheck() { $this->config->expects($this->at(0)) + ->method('getAppValue') + ->with('files_external', 'user_certificate_scan', false) + ->willReturn('["a", "b"]'); + $this->config->expects($this->at(1)) ->method('getAppValue') ->with('core', 'cronErrors') ->willReturn(''); - $this->config->expects($this->at(2)) + $this->config->expects($this->at(3)) ->method('getSystemValue') ->with('connectivity_check_domains', ['www.nextcloud.com', 'www.startpage.com', 'www.eff.org', 'www.edri.org']) ->willReturn(['www.nextcloud.com', 'www.startpage.com', 'www.eff.org', 'www.edri.org']); - $this->config->expects($this->at(3)) + $this->config->expects($this->at(4)) ->method('getSystemValue') ->with('memcache.local', null) ->willReturn('SomeProvider'); - $this->config->expects($this->at(4)) + $this->config->expects($this->at(5)) ->method('getSystemValue') ->with('has_internet_connection', true) ->willReturn(true); - $this->config->expects($this->at(5)) + $this->config->expects($this->at(6)) ->method('getSystemValue') ->with('appstoreenabled', true) ->willReturn(false); @@ -594,6 +598,7 @@ class CheckSetupControllerTest extends TestCase { 'OCA\Settings\SetupChecks\PhpDefaultCharset' => ['pass' => true, 'description' => 'PHP configuration option default_charset should be UTF-8', 'severity' => 'warning'], 'OCA\Settings\SetupChecks\PhpOutputBuffering' => ['pass' => true, 'description' => 'PHP configuration option output_buffering must be disabled', 'severity' => 'error'], 'OCA\Settings\SetupChecks\LegacySSEKeyFormat' => ['pass' => true, 'description' => 'The old server-side-encryption format is enabled. We recommend disabling this.', 'severity' => 'warning', 'linkToDocumentation' => ''], + 'OCA\Settings\SetupChecks\CheckUserCertificates' => ['pass' => false, 'description' => 'There are some user imported SSL certificates present, that are not used anymore with Nextcloud 21. They can be imported on the command line via "occ security:certificates:import" command. Their paths inside the data directory are shown below.', 'severity' => 'warning', 'elements' => ['a', 'b']], 'imageMagickLacksSVGSupport' => false, ] ); diff --git a/core/BackgroundJobs/CheckForUserCertificates.php b/core/BackgroundJobs/CheckForUserCertificates.php new file mode 100644 index 00000000000..8b106c8ce74 --- /dev/null +++ b/core/BackgroundJobs/CheckForUserCertificates.php @@ -0,0 +1,79 @@ + + * + * @author Morris Jobke + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\Core\BackgroundJobs; + +use OC\BackgroundJob\QueuedJob; +use OCP\Files\Folder; +use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; +use OCP\IConfig; +use OCP\IUser; +use OCP\IUserManager; + +class CheckForUserCertificates extends QueuedJob { + + /** @var IConfig */ + protected $config; + /** @var IUserManager */ + private $userManager; + /** @var IRootFolder */ + private $rootFolder; + + public function __construct(IConfig $config, IUserManager $userManager, IRootFolder $rootFolder) { + $this->config = $config; + $this->userManager = $userManager; + $this->rootFolder = $rootFolder; + } + + /** + * Checks all user directories for old user uploaded certificates + */ + public function run($arguments) { + $uploadList = []; + $this->userManager->callForSeenUsers(function (IUser $user) use (&$uploadList) { + $userId = $user->getUID(); + try { + \OC_Util::setupFS($userId); + $filesExternalUploadsFolder = $this->rootFolder->get($userId . '/files_external/uploads'); + } catch (NotFoundException $e) { + \OC_Util::tearDownFS(); + return; + } + if ($filesExternalUploadsFolder instanceof Folder) { + $files = $filesExternalUploadsFolder->getDirectoryListing(); + foreach ($files as $file) { + $filename = $file->getName(); + $uploadList[] = "$userId/files_external/uploads/$filename"; + } + } + \OC_Util::tearDownFS(); + }); + + if (empty($uploadList)) { + $this->config->deleteAppValue('files_external', 'user_certificate_scan'); + } else { + $this->config->setAppValue('files_external', 'user_certificate_scan', json_encode($uploadList)); + } + } +} diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index cd933d5f603..204401064f9 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -488,6 +488,7 @@ OC.SetupChecks.addGenericSetupCheck(data, 'OCA\\Settings\\SetupChecks\\PhpDefaultCharset', messages) OC.SetupChecks.addGenericSetupCheck(data, 'OCA\\Settings\\SetupChecks\\PhpOutputBuffering', messages) OC.SetupChecks.addGenericSetupCheck(data, 'OCA\\Settings\\SetupChecks\\LegacySSEKeyFormat', messages) + OC.SetupChecks.addGenericSetupCheck(data, 'OCA\\Settings\\SetupChecks\\CheckUserCertificates', messages) } else { messages.push({ @@ -520,6 +521,15 @@ if (setupCheck.linkToDocumentation) { message += ' ' + t('core', 'For more details see the documentation.', {docLink: setupCheck.linkToDocumentation}); } + if (setupCheck.elements) { + message += '
    ' + setupCheck.elements.forEach(function(element){ + message += '
  • '; + message += element + message += '
    • '; + }); + message += '
' + } if (!setupCheck.pass) { messages.push({ diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index aa2b5100b79..f9479cc6d21 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -769,6 +769,7 @@ return array( 'OC\\Contacts\\ContactsMenu\\Providers\\EMailProvider' => $baseDir . '/lib/private/Contacts/ContactsMenu/Providers/EMailProvider.php', 'OC\\Core\\Application' => $baseDir . '/core/Application.php', 'OC\\Core\\BackgroundJobs\\BackgroundCleanupUpdaterBackupsJob' => $baseDir . '/core/BackgroundJobs/BackgroundCleanupUpdaterBackupsJob.php', + 'OC\\Core\\BackgroundJobs\\CheckForUserCertificates' => $baseDir . '/core/BackgroundJobs/CheckForUserCertificates.php', 'OC\\Core\\BackgroundJobs\\CleanupLoginFlowV2' => $baseDir . '/core/BackgroundJobs/CleanupLoginFlowV2.php', 'OC\\Core\\Command\\App\\CheckCode' => $baseDir . '/core/Command/App/CheckCode.php', 'OC\\Core\\Command\\App\\Disable' => $baseDir . '/core/Command/App/Disable.php', @@ -1254,6 +1255,7 @@ return array( 'OC\\Repair\\NC20\\EncryptionLegacyCipher' => $baseDir . '/lib/private/Repair/NC20/EncryptionLegacyCipher.php', 'OC\\Repair\\NC20\\EncryptionMigration' => $baseDir . '/lib/private/Repair/NC20/EncryptionMigration.php', 'OC\\Repair\\NC20\\ShippedDashboardEnable' => $baseDir . '/lib/private/Repair/NC20/ShippedDashboardEnable.php', + 'OC\\Repair\\NC21\\AddCheckForUserCertificatesJob' => $baseDir . '/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php', 'OC\\Repair\\OldGroupMembershipShares' => $baseDir . '/lib/private/Repair/OldGroupMembershipShares.php', 'OC\\Repair\\Owncloud\\DropAccountTermsTable' => $baseDir . '/lib/private/Repair/Owncloud/DropAccountTermsTable.php', 'OC\\Repair\\Owncloud\\SaveAccountsTableData' => $baseDir . '/lib/private/Repair/Owncloud/SaveAccountsTableData.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 2eef38f5d89..a0336fc6bd1 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -798,6 +798,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Contacts\\ContactsMenu\\Providers\\EMailProvider' => __DIR__ . '/../../..' . '/lib/private/Contacts/ContactsMenu/Providers/EMailProvider.php', 'OC\\Core\\Application' => __DIR__ . '/../../..' . '/core/Application.php', 'OC\\Core\\BackgroundJobs\\BackgroundCleanupUpdaterBackupsJob' => __DIR__ . '/../../..' . '/core/BackgroundJobs/BackgroundCleanupUpdaterBackupsJob.php', + 'OC\\Core\\BackgroundJobs\\CheckForUserCertificates' => __DIR__ . '/../../..' . '/core/BackgroundJobs/CheckForUserCertificates.php', 'OC\\Core\\BackgroundJobs\\CleanupLoginFlowV2' => __DIR__ . '/../../..' . '/core/BackgroundJobs/CleanupLoginFlowV2.php', 'OC\\Core\\Command\\App\\CheckCode' => __DIR__ . '/../../..' . '/core/Command/App/CheckCode.php', 'OC\\Core\\Command\\App\\Disable' => __DIR__ . '/../../..' . '/core/Command/App/Disable.php', @@ -1283,6 +1284,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Repair\\NC20\\EncryptionLegacyCipher' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/EncryptionLegacyCipher.php', 'OC\\Repair\\NC20\\EncryptionMigration' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/EncryptionMigration.php', 'OC\\Repair\\NC20\\ShippedDashboardEnable' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/ShippedDashboardEnable.php', + 'OC\\Repair\\NC21\\AddCheckForUserCertificatesJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php', 'OC\\Repair\\OldGroupMembershipShares' => __DIR__ . '/../../..' . '/lib/private/Repair/OldGroupMembershipShares.php', 'OC\\Repair\\Owncloud\\DropAccountTermsTable' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/DropAccountTermsTable.php', 'OC\\Repair\\Owncloud\\SaveAccountsTableData' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/SaveAccountsTableData.php', diff --git a/lib/private/Repair.php b/lib/private/Repair.php index eb66ed9dd55..2b9b14b58b6 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -52,6 +52,7 @@ use OC\Repair\NC18\ResetGeneratedAvatarFlag; use OC\Repair\NC20\EncryptionLegacyCipher; use OC\Repair\NC20\EncryptionMigration; use OC\Repair\NC20\ShippedDashboardEnable; +use OC\Repair\NC21\AddCheckForUserCertificatesJob; use OC\Repair\OldGroupMembershipShares; use OC\Repair\Owncloud\DropAccountTermsTable; use OC\Repair\Owncloud\SaveAccountsTableData; @@ -164,6 +165,7 @@ class Repair implements IOutput { \OC::$server->query(EncryptionMigration::class), \OC::$server->get(ShippedDashboardEnable::class), \OC::$server->get(AddBruteForceCleanupJob::class), + \OC::$server->get(AddCheckForUserCertificatesJob::class), ]; } diff --git a/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php b/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php new file mode 100644 index 00000000000..df6637e3948 --- /dev/null +++ b/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php @@ -0,0 +1,61 @@ + + * + * @author Morris Jobke + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\Repair\NC21; + +use OC\Core\BackgroundJobs\CheckForUserCertificates; +use OCP\BackgroundJob\IJobList; +use OCP\IConfig; +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; + +class AddCheckForUserCertificatesJob implements IRepairStep { + + /** @var IJobList */ + protected $jobList; + /** @var IConfig */ + private $config; + + public function __construct(IConfig $config, IJobList $jobList) { + $this->jobList = $jobList; + $this->config = $config; + } + + public function getName() { + return 'Queue a one-time job to check for user uploaded certificates'; + } + + private function shouldRun() { + $versionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0.0'); + + // was added to 21.0.0.2 + return version_compare($versionFromBeforeUpdate, '21.0.0.2', '<'); + } + + public function run(IOutput $output) { + if ($this->shouldRun()) { + $this->config->setAppValue('files_external', 'user_certificate_scan', 'not-run-yet'); + $this->jobList->add(CheckForUserCertificates::class); + } + } +} diff --git a/version.php b/version.php index 41639d6a54a..280a04eec01 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel // when updating major/minor version number. -$OC_Version = [21, 0, 0, 1]; +$OC_Version = [21, 0, 0, 2]; // The human readable string $OC_VersionString = '21.0.0 alpha'; -- cgit v1.2.3 From 9435ec2b4ed503bfb978028f21446aa6c6b75712 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Tue, 3 Nov 2020 00:11:27 +0100 Subject: Remove unneeded service - found by Psalm Signed-off-by: Morris Jobke --- apps/settings/lib/AppInfo/Application.php | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php index 2471400cb07..fe9ba63b012 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/apps/settings/lib/AppInfo/Application.php @@ -98,16 +98,6 @@ class Application extends App implements IBootstrap { } return $isSubAdmin; }); - $context->registerService('userCertificateManager', function (IAppContainer $appContainer) { - /** @var IServerContainer $serverContainer */ - $serverContainer = $appContainer->get(IServerContainer::class); - return $serverContainer->getCertificateManager(); - }, false); - $context->registerService('systemCertificateManager', function (IAppContainer $appContainer) { - /** @var IServerContainer $serverContainer */ - $serverContainer = $appContainer->query('ServerContainer'); - return $serverContainer->getCertificateManager(null); - }, false); $context->registerService(IProvider::class, function (IAppContainer $appContainer) { /** @var IServerContainer $serverContainer */ $serverContainer = $appContainer->query(IServerContainer::class); -- cgit v1.2.3