]> source.dussan.org Git - nextcloud-server.git/commitdiff
Improve CertificateManager to not be user context dependent
authorMorris Jobke <hey@morrisjobke.de>
Sun, 5 Jul 2020 12:31:19 +0000 (14:31 +0200)
committerMorris Jobke <hey@morrisjobke.de>
Mon, 2 Nov 2020 23:13:01 +0000 (00:13 +0100)
* 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 <hey@morrisjobke.de>
12 files changed:
apps/dav/lib/CardDAV/SyncService.php
apps/files_sharing/lib/External/Manager.php
apps/files_sharing/lib/External/MountProvider.php
core/register_command.php
lib/private/Files/Storage/DAV.php
lib/private/Security/CertificateManager.php
lib/private/Server.php
lib/public/ICertificateManager.php
lib/public/IServerContainer.php
tests/lib/Http/Client/ClientTest.php
tests/lib/Security/CertificateManagerTest.php
tests/lib/ServerTest.php

index 1cd01066a7c194ac42554fa74e367f4d7199bf08..bcb20409524eba159a3d19fee9f672851fd95583 100644 (file)
@@ -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;
index c922754207c9c1b55807c4d6199410cf938a48e7..87146dd4268366840f7392b608d788af4c69fa19 100644 (file)
@@ -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);
        }
 
index ecff358abe503972777b05177348794bf1f3f7c1..eb8f1b8fde6d6be3da18cf1cf7714e909a632ef6 100644 (file)
@@ -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);
        }
index 840c73484bf62b943faa0b3342fea8bc5e89ae6c..af6bd6772516f1314f7c32d2008b96a2b9d7aa4c 100644 (file)
@@ -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));
index a6cfd77d98a52c75571cfce72390e7aa44db7313..974feee89958fd75fcf65d5762a29674fdf2be2e 100644 (file)
@@ -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, '/');
index e69132ff4df8c8cb4102c08abe200f9b181f63d8..ed873527d3c82ccb4d01b5c305134532e6d07fe9 100644 (file)
@@ -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);
        }
 
index 5de7808523e8557236a9945cdb547baa596c576f..8e2452b2f30ccbf090ebea7e3cf02d31e27e0841 100644 (file)
@@ -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);
        }
 
        /**
index 1c7949a89cf70376bd1b5998162fedceab802e82..da97dc105d02c217d9f2a58fdb148be0380b408a 100644 (file)
 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();
 }
index 215cfab84fd75eaaef24ccd4196c748e8d14371c..ed08b66b99478bc8a67800c1e10d9811c1260b09 100644 (file)
@@ -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
index a462a0848ae7284ac84d5569109ed4175161912e..78054725e8c7360fc0b671b3f8c00e4916e24a88 100644 (file)
@@ -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([
index 447ee8719c999499820bd76a26b0b8bdce8680f0..3af4b564612ca6033c2ecfc4ff99b7ab72186e41 100644 (file)
@@ -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],
                ];
        }
 }
index ada252473934e6268773d3a5732d2dfce6fac600..1054c4195b265f5b210035b3b38f4b4a00fffbc3 100644 (file)
@@ -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() {