fix(oauth2): store hashed secret instead of encrypted

Signed-off-by: Julien Veyssier <[email protected]>

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.15.2</version>
+	<version>1.15.3</version>
 	<licence>agpl</licence>
 	<author>Lukas Reschke</author>
 	<namespace>OAuth2</namespace>

apps/oauth2/composer/composer/autoload_classmap.php

@@ -22,5 +22,6 @@
     'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => $baseDir . '/../lib/Migration/Version010402Date20190107124745.php',
     'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.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',
 );

apps/oauth2/composer/composer/autoload_static.php

@@ -37,6 +37,7 @@ class ComposerStaticInitOAuth2
         'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => __DIR__ . '/..' . '/../lib/Migration/Version010402Date20190107124745.php',
         'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.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',
     );
 

apps/oauth2/lib/Controller/OauthApiController.php

@@ -149,7 +149,8 @@ public function getToken(
 		}
 
 		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
@@ -158,7 +159,7 @@ public function getToken(
 			], 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);

apps/oauth2/lib/Controller/SettingsController.php

@@ -72,8 +72,8 @@ public function addClient(string $name,
 		$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);
 

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

apps/oauth2/lib/Settings/Admin.php

@@ -29,9 +29,9 @@
 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 OCP\IURLGenerator;
 use Psr\Log\LoggerInterface;
 
 class Admin implements ISettings {
@@ -40,7 +40,6 @@ public function __construct(
 		private IInitialState $initialState,
 		private ClientMapper $clientMapper,
 		private IURLGenerator $urlGenerator,
-		private ICrypto $crypto,
 		private LoggerInterface $logger,
 	) {
 	}
@@ -51,13 +50,12 @@ public function getForm(): TemplateResponse {
 
 		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]);

apps/oauth2/src/App.vue

@@ -39,6 +39,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>
@@ -68,6 +72,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'
 
 export default {
@@ -76,6 +81,7 @@ export default {
 		OAuthItem,
 		NcSettingsSection,
 		NcButton,
+		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 = ''

apps/oauth2/src/components/OAuthItem.vue

@@ -37,7 +37,7 @@
 				</tr>
 				<tr>
 					<td>{{ t('oauth2', 'Secret') }}</td>
-					<td><code>{{ renderedSecret }}</code><a class="icon-toggle has-tooltip" :title="t('oauth2', 'Show client secret')" @click="toggleSecret" /></td>
+					<td><code>{{ renderedSecret }}</code><a v-if="clientSecret !== ''" class="icon-toggle has-tooltip" :title="t('oauth2', 'Show client secret')" @click="toggleSecret" /></td>
 				</tr>
 			</table>
 		</td>

apps/oauth2/tests/Controller/OauthApiControllerTest.php

@@ -265,9 +265,20 @@ public function testRefreshTokenInvalidClient($clientId, $clientSecret) {
 			->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);
@@ -292,21 +303,20 @@ public function testRefreshTokenInvalidAppToken() {
 
 		$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)
@@ -331,21 +341,20 @@ public function testRefreshTokenValidAppToken() {
 
 		$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');
@@ -428,21 +437,20 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
 
 		$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');
@@ -528,21 +536,20 @@ public function testRefreshTokenExpiredAppToken() {
 
 		$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');

apps/oauth2/tests/Controller/SettingsControllerTest.php

@@ -103,13 +103,13 @@ public function testAddClient() {
 
 		$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
@@ -118,7 +118,7 @@ public function testAddClient() {
 			->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);
@@ -161,7 +161,7 @@ public function testDeleteClient() {
 		$client->setId(123);
 		$client->setName('My Client Name');
 		$client->setRedirectUri('https://example.com/');
-		$client->setSecret('MySecret');
+		$client->setSecret('MyHashedSecret');
 		$client->setClientIdentifier('MyClientIdentifier');
 
 		$this->clientMapper

apps/oauth2/tests/Settings/AdminTest.php

@@ -28,7 +28,6 @@
 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;
@@ -38,7 +37,7 @@ class AdminTest extends TestCase {
 	/** @var Admin|MockObject */
 	private $admin;
 
-	/** @var IInitialStateService|MockObject */
+	/** @var IInitialState|MockObject */
 	private $initialState;
 
 	/** @var ClientMapper|MockObject */
@@ -54,7 +53,6 @@ protected function setUp(): void {
 			$this->initialState,
 			$this->clientMapper,
 			$this->createMock(IURLGenerator::class),
-			$this->createMock(ICrypto::class),
 			$this->createMock(LoggerInterface::class)
 		);
 	}