Encrypt stored oidc provider client secrets and id4me client secrets

appinfo/info.xml

@@ -4,7 +4,7 @@
 	<name>OpenID Connect user backend</name>
 	<summary>Use an OpenID Connect backend to login to your Nextcloud</summary>
 	<description>Allows flexible configuration of an OIDC server as Nextcloud login user backend.</description>
-	<version>1.3.2</version>
+	<version>1.3.3</version>
 	<licence>agpl</licence>
 	<author>Roeland Jago Douma</author>
 	<author>Julius Härtl</author>

lib/Command/UpsertProvider.php

@@ -26,6 +26,7 @@
 use OCA\UserOIDC\Service\ProviderService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
+use OCP\Security\ICrypto;
 use \Symfony\Component\Console\Command\Command;
 use OCA\UserOIDC\Db\ProviderMapper;
 use Symfony\Component\Console\Helper\Table;
@@ -40,11 +41,18 @@ class UpsertProvider extends Command {
 	private $providerService;
 	/** @var ProviderMapper */
 	private $providerMapper;
-
-	public function __construct(ProviderService $providerService, ProviderMapper $providerMapper) {
+	/** @var ICrypto */
+	private $crypto;
+
+	public function __construct(
+		ProviderService $providerService,
+		ProviderMapper $providerMapper,
+		ICrypto $crypto
+	) {
 		parent::__construct();
 		$this->providerService = $providerService;
 		$this->providerMapper = $providerMapper;
+		$this->crypto = $crypto;
 	}
 
 	protected function configure() {
@@ -82,6 +90,9 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		$identifier = $input->getArgument('identifier');
 		$clientid = $input->getOption('clientid');
 		$clientsecret = $input->getOption('clientsecret');
+		if ($clientsecret !== null) {
+			$clientsecret = $this->crypto->encrypt($clientsecret);
+		}
 		$discoveryuri = $input->getOption('discoveryuri');
 		$scope = $input->getOption('scope');
 
@@ -128,10 +139,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 
 		$provider = $this->providerService->getProviderByIdentifier($identifier);
 		if ($provider !== null) {
-			// existing provider, keep values that are not set
-			$clientid = $clientid ?? $provider->getClientId();
-			$clientsecret = $clientsecret ?? $provider->getClientSecret();
-			$discoveryuri = $discoveryuri ?? $provider->getDiscoveryEndpoint();
+			// existing provider, keep values that are not set, the scope has to be set anyway
 			$scope = $scope ?? $provider->getScope();
 		} else {
 			// new provider default scope value

lib/Controller/Id4meController.php

@@ -46,6 +46,7 @@
 use OCP\IURLGenerator;
 use OCP\IUserManager;
 use OCP\IUserSession;
+use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 
 require_once __DIR__ . '/../../vendor/autoload.php';
@@ -61,37 +62,28 @@ class Id4meController extends BaseOidcController {
 
 	/** @var ISecureRandom */
 	private $random;
-
 	/** @var ISession */
 	private $session;
-
 	/** @var IClientService */
 	private $clientService;
-
 	/** @var IURLGenerator */
 	private $urlGenerator;
-
 	/** @var UserMapper */
 	private $userMapper;
-
 	/** @var IUserSession */
 	private $userSession;
-
 	/** @var IUserManager */
 	private $userManager;
-
 	/** @var Id4MeMapper */
 	private $id4MeMapper;
-
 	/** @var Service */
 	private $id4me;
-
 	/** @var IL10N */
 	private $l10n;
-	/**
-	 * @var LoggerInterface
-	 */
+	/** @var LoggerInterface */
 	private $logger;
+	/** @var ICrypto */
+	private $crypto;
 
 	public function __construct(
 		IRequest $request,
@@ -106,7 +98,8 @@ public function __construct(
 		IUserManager $userManager,
 		HttpClientHelper $clientHelper,
 		Id4MeMapper $id4MeMapper,
-		LoggerInterface $logger
+		LoggerInterface $logger,
+		ICrypto $crypto
 	) {
 		parent::__construct($request, $config);
 
@@ -121,6 +114,7 @@ public function __construct(
 		$this->id4MeMapper = $id4MeMapper;
 		$this->l10n = $l10n;
 		$this->logger = $logger;
+		$this->crypto = $crypto;
 	}
 
 	/**
@@ -198,7 +192,8 @@ private function registerClient(string $authorityName, OpenIdConfig $openIdConfi
 		$id4Me = new Id4Me();
 		$id4Me->setIdentifier($authorityName);
 		$id4Me->setClientId($client->getClientId());
-		$id4Me->setClientSecret($client->getClientSecret());
+		$encryptedClientSecret = $this->crypto->encrypt($client->getClientSecret());
+		$id4Me->setClientSecret($encryptedClientSecret);
 
 		return $this->id4MeMapper->insert($id4Me);
 	}
@@ -248,17 +243,25 @@ public function code(string $state = '', string $code = '', string $scope = '')
 			return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, ['authority_not_found' => $authorityName]);
 		}
 
+		try {
+			$id4meClientSecret = $this->crypto->decrypt($id4Me->getClientSecret());
+		} catch (\Exception $e) {
+			$this->logger->error('Failed to decrypt the id4me client secret', ['exception' => $e]);
+			$message = $this->l10n->t('Failed to decrypt the ID4ME provider client secret');
+			return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
+		}
+
 		$client = $this->clientService->newClient();
 		$result = $client->post(
 			$openIdConfig->getTokenEndpoint(),
 			[
 				'headers' => [
-					'Authorization' => 'Basic ' . base64_encode($id4Me->getClientId() . ':' . $id4Me->getClientSecret())
+					'Authorization' => 'Basic ' . base64_encode($id4Me->getClientId() . ':' . $id4meClientSecret)
 				],
 				'body' => [
 					'code' => $code,
 					'client_id' => $id4Me->getClientId(),
-					'client_secret' => $id4Me->getClientSecret(),
+					'client_secret' => $id4meClientSecret,
 					'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.id4me.code'),
 					'grant_type' => 'authorization_code',
 				],

lib/Controller/LoginController.php

@@ -59,6 +59,7 @@
 use OCP\IURLGenerator;
 use OCP\IUserManager;
 use OCP\IUserSession;
+use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 use OCP\Session\Exceptions\SessionNotAvailableException;
 
@@ -122,6 +123,10 @@ class LoginController extends BaseOidcController {
 
 	/** @var IL10N */
 	private $l10n;
+	/**
+	 * @var ICrypto
+	 */
+	private $crypto;
 
 	public function __construct(
 		IRequest $request,
@@ -142,7 +147,8 @@ public function __construct(
 		SessionMapper $sessionMapper,
 		ProvisioningService $provisioningService,
 		IL10N $l10n,
-		ILogger $logger
+		ILogger $logger,
+		ICrypto $crypto
 	) {
 		parent::__construct($request, $config);
 
@@ -165,6 +171,7 @@ public function __construct(
 		$this->provisioningService = $provisioningService;
 		$this->request = $request;
 		$this->l10n = $l10n;
+		$this->crypto = $crypto;
 	}
 
 	/**
@@ -353,6 +360,13 @@ public function code(string $state = '', string $code = '', string $scope = '',
 
 		$providerId = (int)$this->session->get(self::PROVIDERID);
 		$provider = $this->providerMapper->getProvider($providerId);
+		try {
+			$providerClientSecret = $this->crypto->decrypt($provider->getClientSecret());
+		} catch (\Exception $e) {
+			$this->logger->error('Failed to decrypt the client secret', ['exception' => $e]);
+			$message = $this->l10n->t('Failed to decrypt the OIDC provider client secret');
+			return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
+		}
 
 		$discovery = $this->discoveryService->obtainDiscovery($provider);
 
@@ -366,7 +380,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
 					'body' => [
 						'code' => $code,
 						'client_id' => $provider->getClientId(),
-						'client_secret' => $provider->getClientSecret(),
+						'client_secret' => $providerClientSecret,
 						'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.code'),
 						'grant_type' => 'authorization_code',
 					],

lib/Controller/SettingsController.php

@@ -35,6 +35,7 @@
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\IRequest;
+use OCP\Security\ICrypto;
 
 class SettingsController extends Controller {
 
@@ -44,18 +45,22 @@ class SettingsController extends Controller {
 	private $id4meService;
 	/** @var ProviderService */
 	private $providerService;
+	/** @var ICrypto */
+	private $crypto;
 
 	public function __construct(
 		IRequest $request,
 		ProviderMapper $providerMapper,
 		ID4MeService $id4meService,
-		ProviderService $providerService
+		ProviderService $providerService,
+		ICrypto $crypto
 		) {
 		parent::__construct(Application::APP_ID, $request);
 
 		$this->providerMapper = $providerMapper;
 		$this->id4meService = $id4meService;
 		$this->providerService = $providerService;
+		$this->crypto = $crypto;
 	}
 
 	public function createProvider(string $identifier, string $clientId, string $clientSecret, string $discoveryEndpoint,
@@ -67,7 +72,8 @@ public function createProvider(string $identifier, string $clientId, string $cli
 		$provider = new Provider();
 		$provider->setIdentifier($identifier);
 		$provider->setClientId($clientId);
-		$provider->setClientSecret($clientSecret);
+		$encryptedClientSecret = $this->crypto->encrypt($clientSecret);
+		$provider->setClientSecret($encryptedClientSecret);
 		$provider->setDiscoveryEndpoint($discoveryEndpoint);
 		$provider->setScope($scope);
 		$provider = $this->providerMapper->insert($provider);
@@ -88,7 +94,8 @@ public function updateProvider(int $providerId, string $identifier, string $clie
 		$provider->setIdentifier($identifier);
 		$provider->setClientId($clientId);
 		if ($clientSecret) {
-			$provider->setClientSecret($clientSecret);
+			$encryptedClientSecret = $this->crypto->encrypt($clientSecret);
+			$provider->setClientSecret($encryptedClientSecret);
 		}
 		$provider->setDiscoveryEndpoint($discoveryEndpoint);
 		$provider->setScope($scope);

lib/Migration/Version010303Date20230602125945.php

@@ -0,0 +1,98 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright 2023, Julien Veyssier <julien-nc@posteo.net>
+ *
+ * @author Julien Veyssier <julien-nc@posteo.net>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+namespace OCA\UserOIDC\Migration;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IDBConnection;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+use OCP\Security\ICrypto;
+
+class Version010303Date20230602125945 extends SimpleMigrationStep {
+
+	/**
+	 * @var IDBConnection
+	 */
+	private $connection;
+	/**
+	 * @var ICrypto
+	 */
+	private $crypto;
+
+	public function __construct(
+		IDBConnection $connection,
+		ICrypto $crypto
+	) {
+		$this->connection = $connection;
+		$this->crypto = $crypto;
+	}
+
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		foreach (['user_oidc_providers', 'user_oidc_id4me'] as $tableName) {
+			if ($schema->hasTable($tableName)) {
+				$table = $schema->getTable($tableName);
+				if ($table->hasColumn('client_secret')) {
+					$column = $table->getColumn('client_secret');
+					$column->setLength(512);
+					return $schema;
+				}
+			}
+		}
+
+		return null;
+	}
+
+	public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
+		// update secrets in user_oidc_providers and user_oidc_id4me
+		foreach (['user_oidc_providers', 'user_oidc_id4me'] as $tableName) {
+			$qbUpdate = $this->connection->getQueryBuilder();
+			$qbUpdate->update($tableName)
+				->set('client_secret', $qbUpdate->createParameter('updateSecret'))
+				->where(
+					$qbUpdate->expr()->eq('id', $qbUpdate->createParameter('updateId'))
+				);
+
+			$qbSelect = $this->connection->getQueryBuilder();
+			$qbSelect->select('id', 'client_secret')
+				->from($tableName);
+			$req = $qbSelect->executeQuery();
+			while ($row = $req->fetch()) {
+				$id = $row['id'];
+				$secret = $row['client_secret'];
+				$encryptedSecret = $this->crypto->encrypt($secret);
+				$qbUpdate->setParameter('updateSecret', $encryptedSecret, IQueryBuilder::PARAM_STR);
+				$qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT);
+				$qbUpdate->executeStatement();
+			}
+			$req->closeCursor();
+		}
+	}
+}