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 <hey@morrisjobke.de>

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;

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);
 	}
 

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);
 	}

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));

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, '/');

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);
 	}
 

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);
 	}
 
 	/**

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();
 }

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

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([

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],
 		];
 	}
 }

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() {