From a5456d1ea2391042cf382785f9f982949f6b19d1 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Mon, 22 May 2023 15:39:56 +0200 Subject: [PATCH] encrypt client secrets Signed-off-by: Julien Veyssier --- apps/oauth2/composer/composer/LICENSE | 2 - .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../lib/Controller/OauthApiController.php | 3 +- .../lib/Controller/SettingsController.php | 46 ++++------- .../Version011601Date20230522143227.php | 82 +++++++++++++++++++ apps/oauth2/lib/Settings/Admin.php | 17 ++-- .../Controller/SettingsControllerTest.php | 10 ++- lib/composer/composer/LICENSE | 2 - 9 files changed, 117 insertions(+), 47 deletions(-) create mode 100644 apps/oauth2/lib/Migration/Version011601Date20230522143227.php diff --git a/apps/oauth2/composer/composer/LICENSE b/apps/oauth2/composer/composer/LICENSE index f27399a042d95..62ecfd8d0046b 100644 --- a/apps/oauth2/composer/composer/LICENSE +++ b/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. - diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php index d760d7cd57929..09cacb20335f9 100644 --- a/apps/oauth2/composer/composer/autoload_classmap.php +++ b/apps/oauth2/composer/composer/autoload_classmap.php @@ -19,5 +19,6 @@ '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', ); diff --git a/apps/oauth2/composer/composer/autoload_static.php b/apps/oauth2/composer/composer/autoload_static.php index c8f3a388c786f..1442093e32fb9 100644 --- a/apps/oauth2/composer/composer/autoload_static.php +++ b/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', ); diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 910fdc9943230..924927b462779 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -125,7 +125,8 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client } // The client id and secret must match. Else we don't provide an access token! - if ($client->getClientIdentifier() !== $client_id || $client->getSecret() !== $client_secret) { + $storedClientSecret = $this->crypto->decrypt($client->getSecret()); + if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) { return 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 c24308140ecbb..3dcda337917ce 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -41,41 +41,25 @@ 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 @@ public function addClient(string $name, $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 @@ public function addClient(string $name, 'name' => $client->getName(), 'redirectUri' => $client->getRedirectUri(), 'clientId' => $client->getClientIdentifier(), - 'clientSecret' => $client->getSecret(), + 'clientSecret' => $secret, ]; return new JSONResponse($result); diff --git a/apps/oauth2/lib/Migration/Version011601Date20230522143227.php b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php new file mode 100644 index 0000000000000..e258224bb399b --- /dev/null +++ b/apps/oauth2/lib/Migration/Version011601Date20230522143227.php @@ -0,0 +1,82 @@ + + * + * @author Julien Veyssier + * + * @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 . + * + */ +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(); + } +} diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php index aa2bd6db0121f..3d84a614d8cd4 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -29,22 +29,18 @@ 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; 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 ) { - $this->initialState = $initialState; - $this->clientMapper = $clientMapper; - $this->urlGenerator = $urlGenerator; } public function getForm(): TemplateResponse { @@ -52,12 +48,13 @@ public function getForm(): TemplateResponse { $result = []; foreach ($clients as $client) { + $secret = $this->crypto->decrypt($client->getSecret()); $result[] = [ 'id' => $client->getId(), 'name' => $client->getName(), 'redirectUri' => $client->getRedirectUri(), 'clientId' => $client->getClientIdentifier(), - 'clientSecret' => $client->getSecret(), + 'clientSecret' => $secret, ]; } $this->initialState->provideInitialState('clients', $result); diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php index e79d7cbe34ef1..f1975ac07a95b 100644 --- a/apps/oauth2/tests/Controller/SettingsControllerTest.php +++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php @@ -38,6 +38,7 @@ 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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $this->accessTokenMapper, $this->l, $this->authTokenProvider, - $this->userManager + $this->userManager, + $this->crypto ); } @@ -175,7 +180,8 @@ public function testDeleteClient() { $this->accessTokenMapper, $this->l, $tokenProviderMock, - $userManager + $userManager, + $this->crypto ); $result = $settingsController->deleteClient(123); diff --git a/lib/composer/composer/LICENSE b/lib/composer/composer/LICENSE index f27399a042d95..62ecfd8d0046b 100644 --- a/lib/composer/composer/LICENSE +++ b/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. -