encrypt oauth2 client secrets

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>

apps/oauth2/appinfo/info.xml

@@ -5,7 +5,7 @@
 	<name>OAuth 2.0</name>
 	<summary>Allows OAuth2 compatible authentication from other web applications.</summary>
 	<description>The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications.</description>
-	<version>1.16.0</version>
+	<version>1.16.1</version>
 	<licence>agpl</licence>
 	<author>Lukas Reschke</author>
 	<namespace>OAuth2</namespace>

apps/oauth2/composer/composer/LICENSE

@@ -1,4 +1,3 @@
-
 Copyright (c) Nils Adermann, Jordi Boggiano
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -18,4 +17,3 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 THE SOFTWARE.
-

apps/oauth2/composer/composer/autoload_classmap.php

@@ -19,5 +19,6 @@ return array(
     'OCA\\OAuth2\\Migration\\SetTokenExpiration' => $baseDir . '/../lib/Migration/SetTokenExpiration.php',
     'OCA\\OAuth2\\Migration\\Version010401Date20181207190718' => $baseDir . '/../lib/Migration/Version010401Date20181207190718.php',
     'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => $baseDir . '/../lib/Migration/Version010402Date20190107124745.php',
+    'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.php',
     'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php',
 );

apps/oauth2/composer/composer/autoload_static.php

@@ -34,6 +34,7 @@ class ComposerStaticInitOAuth2
         'OCA\\OAuth2\\Migration\\SetTokenExpiration' => __DIR__ . '/..' . '/../lib/Migration/SetTokenExpiration.php',
         'OCA\\OAuth2\\Migration\\Version010401Date20181207190718' => __DIR__ . '/..' . '/../lib/Migration/Version010401Date20181207190718.php',
         'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => __DIR__ . '/..' . '/../lib/Migration/Version010402Date20190107124745.php',
+        'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.php',
         'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php',
     );
 

apps/oauth2/lib/Controller/OauthApiController.php

@@ -42,40 +42,23 @@ use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IRequest;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
+use Psr\Log\LoggerInterface;
 
 class OauthApiController extends Controller {
-	/** @var AccessTokenMapper */
-	private $accessTokenMapper;
-	/** @var ClientMapper */
-	private $clientMapper;
-	/** @var ICrypto */
-	private $crypto;
-	/** @var TokenProvider */
-	private $tokenProvider;
-	/** @var ISecureRandom */
-	private $secureRandom;
-	/** @var ITimeFactory */
-	private $time;
-	/** @var Throttler */
-	private $throttler;
 
-	public function __construct(string $appName,
-								IRequest $request,
-								ICrypto $crypto,
-								AccessTokenMapper $accessTokenMapper,
-								ClientMapper $clientMapper,
-								TokenProvider $tokenProvider,
-								ISecureRandom $secureRandom,
-								ITimeFactory $time,
-								Throttler $throttler) {
+	public function __construct(
+		string $appName,
+		IRequest $request,
+		private ICrypto $crypto,
+		private AccessTokenMapper $accessTokenMapper,
+		private ClientMapper $clientMapper,
+		private TokenProvider $tokenProvider,
+		private ISecureRandom $secureRandom,
+		private ITimeFactory $time,
+		private LoggerInterface $logger,
+		private Throttler $throttler
+	) {
 		parent::__construct($appName, $request);
-		$this->crypto = $crypto;
-		$this->accessTokenMapper = $accessTokenMapper;
-		$this->clientMapper = $clientMapper;
-		$this->tokenProvider = $tokenProvider;
-		$this->secureRandom = $secureRandom;
-		$this->time = $time;
-		$this->throttler = $throttler;
 	}
 
 	/**
@@ -124,8 +107,16 @@ class OauthApiController extends Controller {
 			$client_secret = $this->request->server['PHP_AUTH_PW'];
 		}
 
+		try {
+			$storedClientSecret = $this->crypto->decrypt($client->getSecret());
+		} catch (\Exception $e) {
+			$this->logger->error('OAuth client secret decryption error', ['exception' => $e]);
+			return new JSONResponse([
+				'error' => 'invalid_client',
+			], Http::STATUS_BAD_REQUEST);
+		}
 		// The client id and secret must match. Else we don't provide an access token!
-		if ($client->getClientIdentifier() !== $client_id || $client->getSecret() !== $client_secret) {
+		if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) {
 			return new JSONResponse([
 				'error' => 'invalid_client',
 			], Http::STATUS_BAD_REQUEST);

apps/oauth2/lib/Controller/SettingsController.php

@@ -41,41 +41,25 @@ use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 
 class SettingsController extends Controller {
-	/** @var ClientMapper */
-	private $clientMapper;
-	/** @var ISecureRandom */
-	private $secureRandom;
-	/** @var AccessTokenMapper  */
-	private $accessTokenMapper;
-	/** @var IL10N */
-	private $l;
-	/** @var IAuthTokenProvider */
-	private $tokenProvider;
-	/**
-	 * @var IUserManager
-	 */
-	private $userManager;
+
 	public const validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
 
-	public function __construct(string $appName,
-								IRequest $request,
-								ClientMapper $clientMapper,
-								ISecureRandom $secureRandom,
-								AccessTokenMapper $accessTokenMapper,
-								IL10N $l,
-								IAuthTokenProvider $tokenProvider,
-								IUserManager $userManager
+	public function __construct(
+		string $appName,
+		IRequest $request,
+		private ClientMapper $clientMapper,
+		private ISecureRandom $secureRandom,
+		private AccessTokenMapper $accessTokenMapper,
+		private IL10N $l,
+		private IAuthTokenProvider $tokenProvider,
+		private IUserManager $userManager,
+		private ICrypto $crypto
 	) {
 		parent::__construct($appName, $request);
-		$this->secureRandom = $secureRandom;
-		$this->clientMapper = $clientMapper;
-		$this->accessTokenMapper = $accessTokenMapper;
-		$this->l = $l;
-		$this->tokenProvider = $tokenProvider;
-		$this->userManager = $userManager;
 	}
 
 	public function addClient(string $name,
@@ -87,7 +71,9 @@ class SettingsController extends Controller {
 		$client = new Client();
 		$client->setName($name);
 		$client->setRedirectUri($redirectUri);
-		$client->setSecret($this->secureRandom->generate(64, self::validChars));
+		$secret = $this->secureRandom->generate(64, self::validChars);
+		$encryptedSecret = $this->crypto->encrypt($secret);
+		$client->setSecret($encryptedSecret);
 		$client->setClientIdentifier($this->secureRandom->generate(64, self::validChars));
 		$client = $this->clientMapper->insert($client);
 
@@ -96,7 +82,7 @@ class SettingsController extends Controller {
 			'name' => $client->getName(),
 			'redirectUri' => $client->getRedirectUri(),
 			'clientId' => $client->getClientIdentifier(),
-			'clientSecret' => $client->getSecret(),
+			'clientSecret' => $secret,
 		];
 
 		return new JSONResponse($result);

apps/oauth2/lib/Migration/Version011601Date20230522143227.php

@@ -0,0 +1,82 @@
+<?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\OAuth2\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 Version011601Date20230522143227 extends SimpleMigrationStep {
+
+	public function __construct(
+		private IDBConnection $connection,
+		private ICrypto $crypto,
+	) {
+	}
+
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		if ($schema->hasTable('oauth2_clients')) {
+			$table = $schema->getTable('oauth2_clients');
+			if ($table->hasColumn('secret')) {
+				$column = $table->getColumn('secret');
+				$column->setLength(256);
+				return $schema;
+			}
+		}
+
+		return null;
+	}
+
+	public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
+		$qbUpdate = $this->connection->getQueryBuilder();
+		$qbUpdate->update('oauth2_clients')
+			->set('secret', $qbUpdate->createParameter('updateSecret'))
+			->where(
+				$qbUpdate->expr()->eq('id', $qbUpdate->createParameter('updateId'))
+			);
+
+		$qbSelect = $this->connection->getQueryBuilder();
+		$qbSelect->select('id', 'secret')
+			->from('oauth2_clients');
+		$req = $qbSelect->executeQuery();
+		while ($row = $req->fetch()) {
+			$id = $row['id'];
+			$secret = $row['secret'];
+			$encryptedSecret = $this->crypto->encrypt($secret);
+			$qbUpdate->setParameter('updateSecret', $encryptedSecret, IQueryBuilder::PARAM_STR);
+			$qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT);
+			$qbUpdate->executeStatement();
+		}
+		$req->closeCursor();
+	}
+}

apps/oauth2/lib/Settings/Admin.php

@@ -29,22 +29,20 @@ namespace OCA\OAuth2\Settings;
 use OCA\OAuth2\Db\ClientMapper;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
+use OCP\Security\ICrypto;
 use OCP\Settings\ISettings;
 use OCP\IURLGenerator;
+use Psr\Log\LoggerInterface;
 
 class Admin implements ISettings {
-	private IInitialState $initialState;
-	private ClientMapper $clientMapper;
-	private IURLGenerator $urlGenerator;
 
 	public function __construct(
-		IInitialState $initialState,
-		ClientMapper $clientMapper,
-		IURLGenerator $urlGenerator
+		private IInitialState $initialState,
+		private ClientMapper $clientMapper,
+		private IURLGenerator $urlGenerator,
+		private ICrypto $crypto,
+		private LoggerInterface $logger,
 	) {
-		$this->initialState = $initialState;
-		$this->clientMapper = $clientMapper;
-		$this->urlGenerator = $urlGenerator;
 	}
 
 	public function getForm(): TemplateResponse {
@@ -52,13 +50,18 @@ class Admin implements ISettings {
 		$result = [];
 
 		foreach ($clients as $client) {
-			$result[] = [
-				'id' => $client->getId(),
-				'name' => $client->getName(),
-				'redirectUri' => $client->getRedirectUri(),
-				'clientId' => $client->getClientIdentifier(),
-				'clientSecret' => $client->getSecret(),
-			];
+			try {
+				$secret = $this->crypto->decrypt($client->getSecret());
+				$result[] = [
+					'id' => $client->getId(),
+					'name' => $client->getName(),
+					'redirectUri' => $client->getRedirectUri(),
+					'clientId' => $client->getClientIdentifier(),
+					'clientSecret' => $secret,
+				];
+			} catch (\Exception $e) {
+				$this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]);
+			}
 		}
 		$this->initialState->provideInitialState('clients', $result);
 		$this->initialState->provideInitialState('oauth2-doc-link', $this->urlGenerator->linkToDocs('admin-oauth2'));

apps/oauth2/tests/Controller/OauthApiControllerTest.php

@@ -43,6 +43,7 @@ use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IRequest;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /* We have to use this to add a property to the mocked request and avoid warnings about dynamic properties on PHP>=8.2 */
@@ -67,6 +68,8 @@ class OauthApiControllerTest extends TestCase {
 	private $time;
 	/** @var Throttler|\PHPUnit\Framework\MockObject\MockObject */
 	private $throttler;
+	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
+	private $logger;
 	/** @var OauthApiController */
 	private $oauthApiController;
 
@@ -81,6 +84,7 @@ class OauthApiControllerTest extends TestCase {
 		$this->secureRandom = $this->createMock(ISecureRandom::class);
 		$this->time = $this->createMock(ITimeFactory::class);
 		$this->throttler = $this->createMock(Throttler::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->oauthApiController = new OauthApiController(
 			'oauth2',
@@ -91,6 +95,7 @@ class OauthApiControllerTest extends TestCase {
 			$this->tokenProvider,
 			$this->secureRandom,
 			$this->time,
+			$this->logger,
 			$this->throttler
 		);
 	}

apps/oauth2/tests/Controller/SettingsControllerTest.php

@@ -38,6 +38,7 @@ use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 use Test\TestCase;
 
@@ -61,6 +62,8 @@ class SettingsControllerTest extends TestCase {
 	private $settingsController;
 	/** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */
 	private $l;
+	/** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */
+	private $crypto;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -71,6 +74,7 @@ class SettingsControllerTest extends TestCase {
 		$this->accessTokenMapper = $this->createMock(AccessTokenMapper::class);
 		$this->authTokenProvider = $this->createMock(IAuthTokenProvider::class);
 		$this->userManager = $this->createMock(IUserManager::class);
+		$this->crypto = $this->createMock(ICrypto::class);
 		$this->l = $this->createMock(IL10N::class);
 		$this->l->method('t')
 			->willReturnArgument(0);
@@ -82,7 +86,8 @@ class SettingsControllerTest extends TestCase {
 			$this->accessTokenMapper,
 			$this->l,
 			$this->authTokenProvider,
-			$this->userManager
+			$this->userManager,
+			$this->crypto
 		);
 
 	}
@@ -96,6 +101,11 @@ class SettingsControllerTest extends TestCase {
 				'MySecret',
 				'MyClientIdentifier');
 
+		$this->crypto
+			->expects($this->once())
+			->method('encrypt')
+			->willReturn('MyEncryptedSecret');
+
 		$client = new Client();
 		$client->setName('My Client Name');
 		$client->setRedirectUri('https://example.com/');
@@ -108,7 +118,7 @@ class SettingsControllerTest extends TestCase {
 			->with($this->callback(function (Client $c) {
 				return $c->getName() === 'My Client Name' &&
 					$c->getRedirectUri() === 'https://example.com/' &&
-					$c->getSecret() === 'MySecret' &&
+					$c->getSecret() === 'MyEncryptedSecret' &&
 					$c->getClientIdentifier() === 'MyClientIdentifier';
 			}))->willReturnCallback(function (Client $c) {
 				$c->setId(42);
@@ -175,7 +185,8 @@ class SettingsControllerTest extends TestCase {
 			$this->accessTokenMapper,
 			$this->l,
 			$tokenProviderMock,
-			$userManager
+			$userManager,
+			$this->crypto
 		);
 
 		$result = $settingsController->deleteClient(123);

lib/composer/composer/LICENSE

@@ -1,4 +1,3 @@
-
 Copyright (c) Nils Adermann, Jordi Boggiano
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -18,4 +17,3 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 THE SOFTWARE.
-