fix(oauth2): store hashed secret instead of encrypted

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

12 files changed, 118 insertions, 55 deletions

diff --git a/apps/oauth2/appinfo/info.xml b/apps/oauth2/appinfo/info.xml
index f4eede380b1..b6f14720de1 100644
--- a/apps/oauth2/appinfo/info.xml
+++ b/apps/oauth2/appinfo/info.xml
@@ -9,7 +9,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.19.0</version>
+	<version>1.19.1</version>
 	<licence>agpl</licence>
 	<author>Lukas Reschke</author>
 	<namespace>OAuth2</namespace>
diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php
index 536342c0751..cba0ef43dfc 100644
--- a/apps/oauth2/composer/composer/autoload_classmap.php
+++ b/apps/oauth2/composer/composer/autoload_classmap.php
@@ -23,5 +23,6 @@ return array(
     'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.php',
     'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => $baseDir . '/../lib/Migration/Version011602Date20230613160650.php',
     'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => $baseDir . '/../lib/Migration/Version011603Date20230620111039.php',
+    'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => $baseDir . '/../lib/Migration/Version011901Date20240829164356.php',
     'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php',
 );
diff --git a/apps/oauth2/composer/composer/autoload_static.php b/apps/oauth2/composer/composer/autoload_static.php
index 1dc659778a3..c4905bc96c4 100644
--- a/apps/oauth2/composer/composer/autoload_static.php
+++ b/apps/oauth2/composer/composer/autoload_static.php
@@ -38,6 +38,7 @@ class ComposerStaticInitOAuth2
         'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.php',
         'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => __DIR__ . '/..' . '/../lib/Migration/Version011602Date20230613160650.php',
         'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => __DIR__ . '/..' . '/../lib/Migration/Version011603Date20230620111039.php',
+        'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => __DIR__ . '/..' . '/../lib/Migration/Version011901Date20240829164356.php',
         'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php',
     );
 
diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php
index ec54e867314..d763779053a 100644
--- a/apps/oauth2/lib/Controller/OauthApiController.php
+++ b/apps/oauth2/lib/Controller/OauthApiController.php
@@ -138,7 +138,8 @@ class OauthApiController extends Controller {
 		}
 
 		try {
-			$storedClientSecret = $this->crypto->decrypt($client->getSecret());
+			$storedClientSecretHash = $client->getSecret();
+			$clientSecretHash = bin2hex($this->crypto->calculateHMAC($client_secret));
 		} catch (\Exception $e) {
 			$this->logger->error('OAuth client secret decryption error', ['exception' => $e]);
 			// we don't throttle here because it might not be a bruteforce attack
@@ -147,7 +148,7 @@ class OauthApiController extends Controller {
 			], Http::STATUS_BAD_REQUEST);
 		}
 		// The client id and secret must match. Else we don't provide an access token!
-		if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) {
+		if ($client->getClientIdentifier() !== $client_id || $storedClientSecretHash !== $clientSecretHash) {
 			$response = new JSONResponse([
 				'error' => 'invalid_client',
 			], Http::STATUS_BAD_REQUEST);
diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php
index ce85598d08d..f16b26696c4 100644
--- a/apps/oauth2/lib/Controller/SettingsController.php
+++ b/apps/oauth2/lib/Controller/SettingsController.php
@@ -50,8 +50,8 @@ class SettingsController extends Controller {
 		$client->setName($name);
 		$client->setRedirectUri($redirectUri);
 		$secret = $this->secureRandom->generate(64, self::validChars);
-		$encryptedSecret = $this->crypto->encrypt($secret);
-		$client->setSecret($encryptedSecret);
+		$hashedSecret = bin2hex($this->crypto->calculateHMAC($secret));
+		$client->setSecret($hashedSecret);
 		$client->setClientIdentifier($this->secureRandom->generate(64, self::validChars));
 		$client = $this->clientMapper->insert($client);
 
diff --git a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php
new file mode 100644
index 00000000000..9ce9ff371cb
--- /dev/null
+++ b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php
@@ -0,0 +1,49 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\OAuth2\Migration;
+
+use Closure;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IDBConnection;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+use OCP\Security\ICrypto;
+
+class Version011901Date20240829164356 extends SimpleMigrationStep {
+
+	public function __construct(
+		private IDBConnection $connection,
+		private ICrypto $crypto,
+	) {
+	}
+
+	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'];
+			$storedEncryptedSecret = $row['secret'];
+			$secret = $this->crypto->decrypt($storedEncryptedSecret);
+			$hashedSecret = bin2hex($this->crypto->calculateHMAC($secret));
+			$qbUpdate->setParameter('updateSecret', $hashedSecret, IQueryBuilder::PARAM_STR);
+			$qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT);
+			$qbUpdate->executeStatement();
+		}
+		$req->closeCursor();
+	}
+}
diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php
index eae10098a59..93b6b7bcc3f 100644
--- a/apps/oauth2/lib/Settings/Admin.php
+++ b/apps/oauth2/lib/Settings/Admin.php
@@ -12,7 +12,6 @@ use OCA\OAuth2\Db\ClientMapper;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
 use OCP\IURLGenerator;
-use OCP\Security\ICrypto;
 use OCP\Settings\ISettings;
 use Psr\Log\LoggerInterface;
 
@@ -22,7 +21,6 @@ class Admin implements ISettings {
 		private IInitialState $initialState,
 		private ClientMapper $clientMapper,
 		private IURLGenerator $urlGenerator,
-		private ICrypto $crypto,
 		private LoggerInterface $logger,
 	) {
 	}
@@ -33,13 +31,12 @@ class Admin implements ISettings {
 
 		foreach ($clients as $client) {
 			try {
-				$secret = $this->crypto->decrypt($client->getSecret());
 				$result[] = [
 					'id' => $client->getId(),
 					'name' => $client->getName(),
 					'redirectUri' => $client->getRedirectUri(),
 					'clientId' => $client->getClientIdentifier(),
-					'clientSecret' => $secret,
+					'clientSecret' => '',
 				];
 			} catch (\Exception $e) {
 				$this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]);
diff --git a/apps/oauth2/src/App.vue b/apps/oauth2/src/App.vue
index 5498d7ec071..255da9a0943 100644
--- a/apps/oauth2/src/App.vue
+++ b/apps/oauth2/src/App.vue
@@ -33,6 +33,10 @@
 					@delete="deleteClient" />
 			</tbody>
 		</table>
+		<NcNoteCard v-if="showSecretWarning"
+			type="warning">
+			{{ t('oauth2', 'Make sure you store the secret key, it cannot be recovered.') }}
+		</NcNoteCard>
 
 		<br>
 		<h3>{{ t('oauth2', 'Add client') }}</h3>
@@ -66,6 +70,7 @@ import { generateUrl } from '@nextcloud/router'
 import { getCapabilities } from '@nextcloud/capabilities'
 import NcSettingsSection from '@nextcloud/vue/dist/Components/NcSettingsSection.js'
 import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
+import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js'
 import { loadState } from '@nextcloud/initial-state'
 import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
 
@@ -76,6 +81,7 @@ export default {
 		NcSettingsSection,
 		NcButton,
 		NcTextField,
+		NcNoteCard,
 	},
 	props: {
 		clients: {
@@ -92,6 +98,7 @@ export default {
 				error: false,
 			},
 			oauthDocLink: loadState('oauth2', 'oauth2-doc-link'),
+			showSecretWarning: false,
 		}
 	},
 	computed: {
@@ -119,6 +126,7 @@ export default {
 			).then(response => {
 				// eslint-disable-next-line vue/no-mutating-props
 				this.clients.push(response.data)
+				this.showSecretWarning = true
 
 				this.newClient.name = ''
 				this.newClient.redirectUri = ''
diff --git a/apps/oauth2/src/components/OAuthItem.vue b/apps/oauth2/src/components/OAuthItem.vue
index c22a2f73d74..567cf88b63c 100644
--- a/apps/oauth2/src/components/OAuthItem.vue
+++ b/apps/oauth2/src/components/OAuthItem.vue
@@ -10,7 +10,8 @@
 		<td>
 			<div class="action-secret">
 				<code>{{ renderedSecret }}</code>
-				<NcButton type="tertiary-no-background"
+				<NcButton v-if="clientSecret !== ''"
+					type="tertiary-no-background"
 					:aria-label="toggleAriaLabel"
 					@click="toggleSecret">
 					<template #icon>
diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php
index 369b5f38e0b..d881ccbb69b 100644
--- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php
+++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php
@@ -249,9 +249,20 @@ class OauthApiControllerTest extends TestCase {
 			->with('validrefresh')
 			->willReturn($accessToken);
 
+		$this->crypto
+			->method('calculateHMAC')
+			->with($this->callback(function (string $text) {
+				return $text === 'clientSecret' || $text === 'invalidClientSecret';
+			}))
+			->willReturnCallback(function (string $text) {
+				return $text === 'clientSecret'
+					? 'hashedClientSecret'
+					: 'hashedInvalidClientSecret';
+			});
+
 		$client = new Client();
 		$client->setClientIdentifier('clientId');
-		$client->setSecret('clientSecret');
+		$client->setSecret('hashedClientSecret');
 		$this->clientMapper->method('getByUid')
 			->with(42)
 			->willReturn($client);
@@ -276,21 +287,20 @@ class OauthApiControllerTest extends TestCase {
 
 		$client = new Client();
 		$client->setClientIdentifier('clientId');
-		$client->setSecret('encryptedClientSecret');
+		$client->setSecret('hashedClientSecret');
 		$this->clientMapper->method('getByUid')
 			->with(42)
 			->willReturn($client);
 
 		$this->crypto
 			->method('decrypt')
-			->with($this->callback(function (string $text) {
-				return $text === 'encryptedClientSecret' || $text === 'encryptedToken';
-			}))
-			->willReturnCallback(function (string $text) {
-				return $text === 'encryptedClientSecret'
-					? 'clientSecret'
-					: ($text === 'encryptedToken' ? 'decryptedToken' : '');
-			});
+			->with('encryptedToken')
+			->willReturn('decryptedToken');
+
+		$this->crypto
+			->method('calculateHMAC')
+			->with('clientSecret')
+			->willReturn('hashedClientSecret');
 
 		$this->tokenProvider->method('getTokenById')
 			->with(1337)
@@ -315,21 +325,20 @@ class OauthApiControllerTest extends TestCase {
 
 		$client = new Client();
 		$client->setClientIdentifier('clientId');
-		$client->setSecret('encryptedClientSecret');
+		$client->setSecret('hashedClientSecret');
 		$this->clientMapper->method('getByUid')
 			->with(42)
 			->willReturn($client);
 
 		$this->crypto
 			->method('decrypt')
-			->with($this->callback(function (string $text) {
-				return $text === 'encryptedClientSecret' || $text === 'encryptedToken';
-			}))
-			->willReturnCallback(function (string $text) {
-				return $text === 'encryptedClientSecret'
-					? 'clientSecret'
-					: ($text === 'encryptedToken' ? 'decryptedToken' : '');
-			});
+			->with('encryptedToken')
+			->willReturn('decryptedToken');
+
+		$this->crypto
+			->method('calculateHMAC')
+			->with('clientSecret')
+			->willReturn('hashedClientSecret');
 
 		$appToken = new PublicKeyToken();
 		$appToken->setUid('userId');
@@ -412,21 +421,20 @@ class OauthApiControllerTest extends TestCase {
 
 		$client = new Client();
 		$client->setClientIdentifier('clientId');
-		$client->setSecret('encryptedClientSecret');
+		$client->setSecret('hashedClientSecret');
 		$this->clientMapper->method('getByUid')
 			->with(42)
 			->willReturn($client);
 
 		$this->crypto
 			->method('decrypt')
-			->with($this->callback(function (string $text) {
-				return $text === 'encryptedClientSecret' || $text === 'encryptedToken';
-			}))
-			->willReturnCallback(function (string $text) {
-				return $text === 'encryptedClientSecret'
-					? 'clientSecret'
-					: ($text === 'encryptedToken' ? 'decryptedToken' : '');
-			});
+			->with('encryptedToken')
+			->willReturn('decryptedToken');
+
+		$this->crypto
+			->method('calculateHMAC')
+			->with('clientSecret')
+			->willReturn('hashedClientSecret');
 
 		$appToken = new PublicKeyToken();
 		$appToken->setUid('userId');
@@ -512,21 +520,20 @@ class OauthApiControllerTest extends TestCase {
 
 		$client = new Client();
 		$client->setClientIdentifier('clientId');
-		$client->setSecret('encryptedClientSecret');
+		$client->setSecret('hashedClientSecret');
 		$this->clientMapper->method('getByUid')
 			->with(42)
 			->willReturn($client);
 
 		$this->crypto
 			->method('decrypt')
-			->with($this->callback(function (string $text) {
-				return $text === 'encryptedClientSecret' || $text === 'encryptedToken';
-			}))
-			->willReturnCallback(function (string $text) {
-				return $text === 'encryptedClientSecret'
-					? 'clientSecret'
-					: ($text === 'encryptedToken' ? 'decryptedToken' : '');
-			});
+			->with('encryptedToken')
+			->willReturn('decryptedToken');
+
+		$this->crypto
+			->method('calculateHMAC')
+			->with('clientSecret')
+			->willReturn('hashedClientSecret');
 
 		$appToken = new PublicKeyToken();
 		$appToken->setUid('userId');
diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php
index 181cfbfa5b7..ec382edc1d6 100644
--- a/apps/oauth2/tests/Controller/SettingsControllerTest.php
+++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php
@@ -81,13 +81,13 @@ class SettingsControllerTest extends TestCase {
 
 		$this->crypto
 			->expects($this->once())
-			->method('encrypt')
-			->willReturn('MyEncryptedSecret');
+			->method('calculateHMAC')
+			->willReturn('MyHashedSecret');
 
 		$client = new Client();
 		$client->setName('My Client Name');
 		$client->setRedirectUri('https://example.com/');
-		$client->setSecret('MySecret');
+		$client->setSecret('MyHashedSecret');
 		$client->setClientIdentifier('MyClientIdentifier');
 
 		$this->clientMapper
@@ -96,7 +96,7 @@ class SettingsControllerTest extends TestCase {
 			->with($this->callback(function (Client $c) {
 				return $c->getName() === 'My Client Name' &&
 					$c->getRedirectUri() === 'https://example.com/' &&
-					$c->getSecret() === 'MyEncryptedSecret' &&
+					$c->getSecret() === 'MyHashedSecret' &&
 					$c->getClientIdentifier() === 'MyClientIdentifier';
 			}))->willReturnCallback(function (Client $c) {
 				$c->setId(42);
@@ -142,7 +142,7 @@ class SettingsControllerTest extends TestCase {
 		$client->setId(123);
 		$client->setName('My Client Name');
 		$client->setRedirectUri('https://example.com/');
-		$client->setSecret('MySecret');
+		$client->setSecret('MyHashedSecret');
 		$client->setClientIdentifier('MyClientIdentifier');
 
 		$this->clientMapper
diff --git a/apps/oauth2/tests/Settings/AdminTest.php b/apps/oauth2/tests/Settings/AdminTest.php
index 6774302949f..9cbefb74671 100644
--- a/apps/oauth2/tests/Settings/AdminTest.php
+++ b/apps/oauth2/tests/Settings/AdminTest.php
@@ -10,7 +10,6 @@ use OCA\OAuth2\Settings\Admin;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
 use OCP\IURLGenerator;
-use OCP\Security\ICrypto;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
@@ -20,7 +19,7 @@ class AdminTest extends TestCase {
 	/** @var Admin|MockObject */
 	private $admin;
 
-	/** @var IInitialStateService|MockObject */
+	/** @var IInitialState|MockObject */
 	private $initialState;
 
 	/** @var ClientMapper|MockObject */
@@ -36,7 +35,6 @@ class AdminTest extends TestCase {
 			$this->initialState,
 			$this->clientMapper,
 			$this->createMock(IURLGenerator::class),
-			$this->createMock(ICrypto::class),
 			$this->createMock(LoggerInterface::class)
 		);
 	}