]> source.dussan.org Git - nextcloud-server.git/commitdiff
fix(oauth2): store hashed secret instead of encrypted
authorJulien Veyssier <julien-nc@posteo.net>
Thu, 29 Aug 2024 15:28:01 +0000 (17:28 +0200)
committerJulien Veyssier <julien-nc@posteo.net>
Tue, 3 Sep 2024 01:18:30 +0000 (03:18 +0200)
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
12 files changed:
apps/oauth2/appinfo/info.xml
apps/oauth2/composer/composer/autoload_classmap.php
apps/oauth2/composer/composer/autoload_static.php
apps/oauth2/lib/Controller/OauthApiController.php
apps/oauth2/lib/Controller/SettingsController.php
apps/oauth2/lib/Migration/Version011901Date20240829164356.php [new file with mode: 0644]
apps/oauth2/lib/Settings/Admin.php
apps/oauth2/src/App.vue
apps/oauth2/src/components/OAuthItem.vue
apps/oauth2/tests/Controller/OauthApiControllerTest.php
apps/oauth2/tests/Controller/SettingsControllerTest.php
apps/oauth2/tests/Settings/AdminTest.php

index fd17afbb11f736df56986e57e223e7404723992a..65e0886c68a4e922ad9c734d7e8a4e094ea66c8a 100644 (file)
@@ -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.3</version>
+       <version>1.16.4</version>
        <licence>agpl</licence>
        <author>Lukas Reschke</author>
        <namespace>OAuth2</namespace>
index 536342c07511391df92faaa0e3cac2eeacf97920..cba0ef43dfc2ad6618a798bceb7045273a688d12 100644 (file)
@@ -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',
 );
index 1dc659778a3d385b2f55d5c6be209e175e056c5e..c4905bc96c477ded891376e77f3d8164dc45d66b 100644 (file)
@@ -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',
     );
 
index 46b68b1d5859ab9875c4d46371a14d2afbcc2301..0ce607edb5c22f2da512bc444b66ca21442b4aeb 100644 (file)
@@ -156,7 +156,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
@@ -165,7 +166,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);
index d49dd6da0381b88b040c1d7b2b096b2ae9f41eea..5d00e2338281031880ba1552d6086b3a6e1bf2cf 100644 (file)
@@ -72,8 +72,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 (file)
index 0000000..9ce9ff3
--- /dev/null
@@ -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();
+       }
+}
index 5cba6805f7cf1e72c4f4567a9c4f6db496215dbf..a44b0ff05b5524b59f820728973397b7c8d1fee7 100644 (file)
@@ -30,7 +30,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;
 
@@ -40,7 +39,6 @@ class Admin implements ISettings {
                private IInitialState $initialState,
                private ClientMapper $clientMapper,
                private IURLGenerator $urlGenerator,
-               private ICrypto $crypto,
                private LoggerInterface $logger,
        ) {
        }
@@ -51,13 +49,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]);
index 71cf22dbf89e24322c71529f7c68a3297ba5a97d..411643168f462d72fb0ad71dc708156d1faa2755 100644 (file)
                                        @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>
@@ -83,6 +87,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'
 
@@ -93,6 +98,7 @@ export default {
                NcSettingsSection,
                NcButton,
                NcTextField,
+               NcNoteCard,
        },
        props: {
                clients: {
@@ -109,6 +115,7 @@ export default {
                                error: false,
                        },
                        oauthDocLink: loadState('oauth2', 'oauth2-doc-link'),
+                       showSecretWarning: false,
                }
        },
        computed: {
@@ -136,6 +143,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 = ''
index 2a2eced77017fe8cdcc0ea98698a1207d4801382..4fc0d63b8e7704c7fb6bcefbabc810b1bae36149 100644 (file)
@@ -27,7 +27,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>
index eec38890e05b54805cc5902f889c4904e9d08217..ec4826b75e8709198f2f237aa8e3d96d71e065f9 100644 (file)
@@ -269,9 +269,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);
@@ -296,21 +307,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)
@@ -335,21 +345,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');
@@ -432,21 +441,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');
@@ -532,21 +540,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');
index f34707c554d1e2f5f09b86ddb4643c0c0b939a62..06eca4f0fb9d32a4b31194a5dcbc6b4b2ea34034 100644 (file)
@@ -102,13 +102,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
@@ -117,7 +117,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);
@@ -160,7 +160,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
index fb19a9fc6d177773c43eef591d4140c63fb7d7e2..55f8920b2021e242857de60a36da5dc5943e6cd1 100644 (file)
@@ -28,7 +28,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;
@@ -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 @@ class AdminTest extends TestCase {
                        $this->initialState,
                        $this->clientMapper,
                        $this->createMock(IURLGenerator::class),
-                       $this->createMock(ICrypto::class),
                        $this->createMock(LoggerInterface::class)
                );
        }