Skip to content

Commit 60aa0d2

Browse files
committed
encrypt oauth2 client secrets
Signed-off-by: Julien Veyssier <[email protected]>
1 parent ec4987f commit 60aa0d2

File tree

12 files changed

+202
-45
lines changed

12 files changed

+202
-45
lines changed

apps/oauth2/appinfo/info.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
<name>OAuth 2.0</name>
66
<summary>Allows OAuth2 compatible authentication from other web applications.</summary>
77
<description>The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications.</description>
8-
<version>1.13.0</version>
8+
<version>1.13.1</version>
99
<licence>agpl</licence>
1010
<author>Lukas Reschke</author>
1111
<namespace>OAuth2</namespace>

apps/oauth2/composer/composer/LICENSE

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
21
Copyright (c) Nils Adermann, Jordi Boggiano
32

43
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
1817
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
1918
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
2019
THE SOFTWARE.
21-

apps/oauth2/composer/composer/autoload_classmap.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,6 @@
1919
'OCA\\OAuth2\\Migration\\SetTokenExpiration' => $baseDir . '/../lib/Migration/SetTokenExpiration.php',
2020
'OCA\\OAuth2\\Migration\\Version010401Date20181207190718' => $baseDir . '/../lib/Migration/Version010401Date20181207190718.php',
2121
'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => $baseDir . '/../lib/Migration/Version010402Date20190107124745.php',
22+
'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.php',
2223
'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php',
2324
);

apps/oauth2/composer/composer/autoload_static.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ class ComposerStaticInitOAuth2
3434
'OCA\\OAuth2\\Migration\\SetTokenExpiration' => __DIR__ . '/..' . '/../lib/Migration/SetTokenExpiration.php',
3535
'OCA\\OAuth2\\Migration\\Version010401Date20181207190718' => __DIR__ . '/..' . '/../lib/Migration/Version010401Date20181207190718.php',
3636
'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => __DIR__ . '/..' . '/../lib/Migration/Version010402Date20190107124745.php',
37+
'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.php',
3738
'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php',
3839
);
3940

apps/oauth2/lib/Controller/OauthApiController.php

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
use OCP\IRequest;
4343
use OCP\Security\ICrypto;
4444
use OCP\Security\ISecureRandom;
45+
use Psr\Log\LoggerInterface;
4546

4647
class OauthApiController extends Controller {
4748
/** @var AccessTokenMapper */
@@ -58,6 +59,8 @@ class OauthApiController extends Controller {
5859
private $time;
5960
/** @var Throttler */
6061
private $throttler;
62+
/** @var LoggerInterface */
63+
private $logger;
6164

6265
public function __construct(string $appName,
6366
IRequest $request,
@@ -67,6 +70,7 @@ public function __construct(string $appName,
6770
TokenProvider $tokenProvider,
6871
ISecureRandom $secureRandom,
6972
ITimeFactory $time,
73+
LoggerInterface $logger,
7074
Throttler $throttler) {
7175
parent::__construct($appName, $request);
7276
$this->crypto = $crypto;
@@ -76,6 +80,7 @@ public function __construct(string $appName,
7680
$this->secureRandom = $secureRandom;
7781
$this->time = $time;
7882
$this->throttler = $throttler;
83+
$this->logger = $logger;
7984
}
8085

8186
/**
@@ -124,8 +129,16 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client
124129
$client_secret = $this->request->server['PHP_AUTH_PW'];
125130
}
126131

132+
try {
133+
$storedClientSecret = $this->crypto->decrypt($client->getSecret());
134+
} catch (\Exception $e) {
135+
$this->logger->error('OAuth client secret decryption error', ['exception' => $e]);
136+
return new JSONResponse([
137+
'error' => 'invalid_client',
138+
], Http::STATUS_BAD_REQUEST);
139+
}
127140
// The client id and secret must match. Else we don't provide an access token!
128-
if ($client->getClientIdentifier() !== $client_id || $client->getSecret() !== $client_secret) {
141+
if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) {
129142
return new JSONResponse([
130143
'error' => 'invalid_client',
131144
], Http::STATUS_BAD_REQUEST);

apps/oauth2/lib/Controller/SettingsController.php

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
use OCP\IL10N;
4040
use OCP\IRequest;
4141
use OCP\Security\ISecureRandom;
42+
use OCP\Security\ICrypto;
4243

4344
class SettingsController extends Controller {
4445
/** @var ClientMapper */
@@ -49,6 +50,8 @@ class SettingsController extends Controller {
4950
private $accessTokenMapper;
5051
/** @var IL10N */
5152
private $l;
53+
/** @var ICrypto */
54+
private $crypto;
5255

5356
public const validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
5457

@@ -57,13 +60,15 @@ public function __construct(string $appName,
5760
ClientMapper $clientMapper,
5861
ISecureRandom $secureRandom,
5962
AccessTokenMapper $accessTokenMapper,
60-
IL10N $l
63+
IL10N $l,
64+
ICrypto $crypto
6165
) {
6266
parent::__construct($appName, $request);
6367
$this->secureRandom = $secureRandom;
6468
$this->clientMapper = $clientMapper;
6569
$this->accessTokenMapper = $accessTokenMapper;
6670
$this->l = $l;
71+
$this->crypto = $crypto;
6772
}
6873

6974
public function addClient(string $name,
@@ -75,7 +80,9 @@ public function addClient(string $name,
7580
$client = new Client();
7681
$client->setName($name);
7782
$client->setRedirectUri($redirectUri);
78-
$client->setSecret($this->secureRandom->generate(64, self::validChars));
83+
$secret = $this->secureRandom->generate(64, self::validChars);
84+
$encryptedSecret = $this->crypto->encrypt($secret);
85+
$client->setSecret($encryptedSecret);
7986
$client->setClientIdentifier($this->secureRandom->generate(64, self::validChars));
8087
$client = $this->clientMapper->insert($client);
8188

@@ -84,7 +91,7 @@ public function addClient(string $name,
8491
'name' => $client->getName(),
8592
'redirectUri' => $client->getRedirectUri(),
8693
'clientId' => $client->getClientIdentifier(),
87-
'clientSecret' => $client->getSecret(),
94+
'clientSecret' => $secret,
8895
];
8996

9097
return new JSONResponse($result);
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* @copyright Copyright 2023, Julien Veyssier <[email protected]>
7+
*
8+
* @author Julien Veyssier <[email protected]>
9+
*
10+
* @license GNU AGPL version 3 or any later version
11+
*
12+
* This program is free software: you can redistribute it and/or modify
13+
* it under the terms of the GNU Affero General Public License as
14+
* published by the Free Software Foundation, either version 3 of the
15+
* License, or (at your option) any later version.
16+
*
17+
* This program is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU Affero General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU Affero General Public License
23+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
24+
*
25+
*/
26+
namespace OCA\OAuth2\Migration;
27+
28+
use Closure;
29+
use OCP\DB\ISchemaWrapper;
30+
use OCP\DB\QueryBuilder\IQueryBuilder;
31+
use OCP\IDBConnection;
32+
use OCP\Migration\IOutput;
33+
use OCP\Migration\SimpleMigrationStep;
34+
use OCP\Security\ICrypto;
35+
36+
class Version011601Date20230522143227 extends SimpleMigrationStep {
37+
private IDBConnection $connection;
38+
private ICrypto $crypto;
39+
40+
public function __construct(IDBConnection $connection, ICrypto $crypto) {
41+
$this->connection = $connection;
42+
$this->crypto = $crypto;
43+
}
44+
45+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) {
46+
/** @var ISchemaWrapper $schema */
47+
$schema = $schemaClosure();
48+
49+
if ($schema->hasTable('oauth2_clients')) {
50+
$table = $schema->getTable('oauth2_clients');
51+
if ($table->hasColumn('secret')) {
52+
$column = $table->getColumn('secret');
53+
$column->setLength(256);
54+
return $schema;
55+
}
56+
}
57+
58+
return null;
59+
}
60+
61+
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
62+
$qbUpdate = $this->connection->getQueryBuilder();
63+
$qbUpdate->update('oauth2_clients')
64+
->set('secret', $qbUpdate->createParameter('updateSecret'))
65+
->where(
66+
$qbUpdate->expr()->eq('id', $qbUpdate->createParameter('updateId'))
67+
);
68+
69+
$qbSelect = $this->connection->getQueryBuilder();
70+
$qbSelect->select('id', 'secret')
71+
->from('oauth2_clients');
72+
$req = $qbSelect->executeQuery();
73+
while ($row = $req->fetch()) {
74+
$id = $row['id'];
75+
$secret = $row['secret'];
76+
$encryptedSecret = $this->crypto->encrypt($secret);
77+
$qbUpdate->setParameter('updateSecret', $encryptedSecret, IQueryBuilder::PARAM_STR);
78+
$qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT);
79+
$qbUpdate->executeStatement();
80+
}
81+
$req->closeCursor();
82+
}
83+
}

apps/oauth2/lib/Settings/Admin.php

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,36 +29,49 @@
2929
use OCA\OAuth2\Db\ClientMapper;
3030
use OCP\AppFramework\Http\TemplateResponse;
3131
use OCP\AppFramework\Services\IInitialState;
32+
use OCP\Security\ICrypto;
3233
use OCP\Settings\ISettings;
3334
use OCP\IURLGenerator;
35+
use Psr\Log\LoggerInterface;
3436

3537
class Admin implements ISettings {
3638
private IInitialState $initialState;
3739
private ClientMapper $clientMapper;
3840
private IURLGenerator $urlGenerator;
41+
private ICrypto $crypto;
42+
private LoggerInterface $logger;
3943

4044
public function __construct(
4145
IInitialState $initialState,
4246
ClientMapper $clientMapper,
43-
IURLGenerator $urlGenerator
47+
IURLGenerator $urlGenerator,
48+
ICrypto $crypto,
49+
LoggerInterface $logger
4450
) {
4551
$this->initialState = $initialState;
4652
$this->clientMapper = $clientMapper;
4753
$this->urlGenerator = $urlGenerator;
54+
$this->crypto = $crypto;
55+
$this->logger = $logger;
4856
}
4957

5058
public function getForm(): TemplateResponse {
5159
$clients = $this->clientMapper->getClients();
5260
$result = [];
5361

5462
foreach ($clients as $client) {
55-
$result[] = [
56-
'id' => $client->getId(),
57-
'name' => $client->getName(),
58-
'redirectUri' => $client->getRedirectUri(),
59-
'clientId' => $client->getClientIdentifier(),
60-
'clientSecret' => $client->getSecret(),
61-
];
63+
try {
64+
$secret = $this->crypto->decrypt($client->getSecret());
65+
$result[] = [
66+
'id' => $client->getId(),
67+
'name' => $client->getName(),
68+
'redirectUri' => $client->getRedirectUri(),
69+
'clientId' => $client->getClientIdentifier(),
70+
'clientSecret' => $secret,
71+
];
72+
} catch (\Exception $e) {
73+
$this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]);
74+
}
6275
}
6376
$this->initialState->provideInitialState('clients', $result);
6477
$this->initialState->provideInitialState('oauth2-doc-link', $this->urlGenerator->linkToDocs('admin-oauth2'));

0 commit comments

Comments
 (0)