Skip to content

Commit

Permalink
Merge pull request #636 from nextcloud/enh/noid/encrypt-client-secret
Browse files Browse the repository at this point in the history
Encrypt stored oidc provider client secrets and id4me client secrets
  • Loading branch information
julien-nc authored Jun 26, 2023
2 parents cc641f8 + baf9063 commit 780d900
Show file tree
Hide file tree
Showing 7 changed files with 175 additions and 31 deletions.
2 changes: 1 addition & 1 deletion appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<name>OpenID Connect user backend</name>
<summary>Use an OpenID Connect backend to login to your Nextcloud</summary>
<description>Allows flexible configuration of an OIDC server as Nextcloud login user backend.</description>
<version>1.3.2</version>
<version>1.3.3</version>
<licence>agpl</licence>
<author>Roeland Jago Douma</author>
<author>Julius Härtl</author>
Expand Down
20 changes: 14 additions & 6 deletions lib/Command/UpsertProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use OCA\UserOIDC\Service\ProviderService;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\Security\ICrypto;
use \Symfony\Component\Console\Command\Command;
use OCA\UserOIDC\Db\ProviderMapper;
use Symfony\Component\Console\Helper\Table;
Expand All @@ -40,11 +41,18 @@ class UpsertProvider extends Command {
private $providerService;
/** @var ProviderMapper */
private $providerMapper;

public function __construct(ProviderService $providerService, ProviderMapper $providerMapper) {
/** @var ICrypto */
private $crypto;

public function __construct(
ProviderService $providerService,
ProviderMapper $providerMapper,
ICrypto $crypto
) {
parent::__construct();
$this->providerService = $providerService;
$this->providerMapper = $providerMapper;
$this->crypto = $crypto;
}

protected function configure() {
Expand Down Expand Up @@ -82,6 +90,9 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$identifier = $input->getArgument('identifier');
$clientid = $input->getOption('clientid');
$clientsecret = $input->getOption('clientsecret');
if ($clientsecret !== null) {
$clientsecret = $this->crypto->encrypt($clientsecret);
}
$discoveryuri = $input->getOption('discoveryuri');
$scope = $input->getOption('scope');

Expand Down Expand Up @@ -128,10 +139,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {

$provider = $this->providerService->getProviderByIdentifier($identifier);
if ($provider !== null) {
// existing provider, keep values that are not set
$clientid = $clientid ?? $provider->getClientId();
$clientsecret = $clientsecret ?? $provider->getClientSecret();
$discoveryuri = $discoveryuri ?? $provider->getDiscoveryEndpoint();
// existing provider, keep values that are not set, the scope has to be set anyway
$scope = $scope ?? $provider->getScope();
} else {
// new provider default scope value
Expand Down
35 changes: 19 additions & 16 deletions lib/Controller/Id4meController.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;

require_once __DIR__ . '/../../vendor/autoload.php';
Expand All @@ -61,37 +62,28 @@ class Id4meController extends BaseOidcController {

/** @var ISecureRandom */
private $random;

/** @var ISession */
private $session;

/** @var IClientService */
private $clientService;

/** @var IURLGenerator */
private $urlGenerator;

/** @var UserMapper */
private $userMapper;

/** @var IUserSession */
private $userSession;

/** @var IUserManager */
private $userManager;

/** @var Id4MeMapper */
private $id4MeMapper;

/** @var Service */
private $id4me;

/** @var IL10N */
private $l10n;
/**
* @var LoggerInterface
*/
/** @var LoggerInterface */
private $logger;
/** @var ICrypto */
private $crypto;

public function __construct(
IRequest $request,
Expand All @@ -106,7 +98,8 @@ public function __construct(
IUserManager $userManager,
HttpClientHelper $clientHelper,
Id4MeMapper $id4MeMapper,
LoggerInterface $logger
LoggerInterface $logger,
ICrypto $crypto
) {
parent::__construct($request, $config);

Expand All @@ -121,6 +114,7 @@ public function __construct(
$this->id4MeMapper = $id4MeMapper;
$this->l10n = $l10n;
$this->logger = $logger;
$this->crypto = $crypto;
}

/**
Expand Down Expand Up @@ -198,7 +192,8 @@ private function registerClient(string $authorityName, OpenIdConfig $openIdConfi
$id4Me = new Id4Me();
$id4Me->setIdentifier($authorityName);
$id4Me->setClientId($client->getClientId());
$id4Me->setClientSecret($client->getClientSecret());
$encryptedClientSecret = $this->crypto->encrypt($client->getClientSecret());
$id4Me->setClientSecret($encryptedClientSecret);

return $this->id4MeMapper->insert($id4Me);
}
Expand Down Expand Up @@ -248,17 +243,25 @@ public function code(string $state = '', string $code = '', string $scope = '')
return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, ['authority_not_found' => $authorityName]);
}

try {
$id4meClientSecret = $this->crypto->decrypt($id4Me->getClientSecret());
} catch (\Exception $e) {
$this->logger->error('Failed to decrypt the id4me client secret', ['exception' => $e]);
$message = $this->l10n->t('Failed to decrypt the ID4ME provider client secret');
return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
}

$client = $this->clientService->newClient();
$result = $client->post(
$openIdConfig->getTokenEndpoint(),
[
'headers' => [
'Authorization' => 'Basic ' . base64_encode($id4Me->getClientId() . ':' . $id4Me->getClientSecret())
'Authorization' => 'Basic ' . base64_encode($id4Me->getClientId() . ':' . $id4meClientSecret)
],
'body' => [
'code' => $code,
'client_id' => $id4Me->getClientId(),
'client_secret' => $id4Me->getClientSecret(),
'client_secret' => $id4meClientSecret,
'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.id4me.code'),
'grant_type' => 'authorization_code',
],
Expand Down
18 changes: 16 additions & 2 deletions lib/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;
use OCP\Session\Exceptions\SessionNotAvailableException;

Expand Down Expand Up @@ -122,6 +123,10 @@ class LoginController extends BaseOidcController {

/** @var IL10N */
private $l10n;
/**
* @var ICrypto
*/
private $crypto;

public function __construct(
IRequest $request,
Expand All @@ -142,7 +147,8 @@ public function __construct(
SessionMapper $sessionMapper,
ProvisioningService $provisioningService,
IL10N $l10n,
ILogger $logger
ILogger $logger,
ICrypto $crypto
) {
parent::__construct($request, $config);

Expand All @@ -165,6 +171,7 @@ public function __construct(
$this->provisioningService = $provisioningService;
$this->request = $request;
$this->l10n = $l10n;
$this->crypto = $crypto;
}

/**
Expand Down Expand Up @@ -353,6 +360,13 @@ public function code(string $state = '', string $code = '', string $scope = '',

$providerId = (int)$this->session->get(self::PROVIDERID);
$provider = $this->providerMapper->getProvider($providerId);
try {
$providerClientSecret = $this->crypto->decrypt($provider->getClientSecret());
} catch (\Exception $e) {
$this->logger->error('Failed to decrypt the client secret', ['exception' => $e]);
$message = $this->l10n->t('Failed to decrypt the OIDC provider client secret');
return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
}

$discovery = $this->discoveryService->obtainDiscovery($provider);

Expand All @@ -366,7 +380,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
'body' => [
'code' => $code,
'client_id' => $provider->getClientId(),
'client_secret' => $provider->getClientSecret(),
'client_secret' => $providerClientSecret,
'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.code'),
'grant_type' => 'authorization_code',
],
Expand Down
13 changes: 10 additions & 3 deletions lib/Controller/SettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\IRequest;
use OCP\Security\ICrypto;

class SettingsController extends Controller {

Expand All @@ -44,18 +45,22 @@ class SettingsController extends Controller {
private $id4meService;
/** @var ProviderService */
private $providerService;
/** @var ICrypto */
private $crypto;

public function __construct(
IRequest $request,
ProviderMapper $providerMapper,
ID4MeService $id4meService,
ProviderService $providerService
ProviderService $providerService,
ICrypto $crypto
) {
parent::__construct(Application::APP_ID, $request);

$this->providerMapper = $providerMapper;
$this->id4meService = $id4meService;
$this->providerService = $providerService;
$this->crypto = $crypto;
}

public function createProvider(string $identifier, string $clientId, string $clientSecret, string $discoveryEndpoint,
Expand All @@ -67,7 +72,8 @@ public function createProvider(string $identifier, string $clientId, string $cli
$provider = new Provider();
$provider->setIdentifier($identifier);
$provider->setClientId($clientId);
$provider->setClientSecret($clientSecret);
$encryptedClientSecret = $this->crypto->encrypt($clientSecret);
$provider->setClientSecret($encryptedClientSecret);
$provider->setDiscoveryEndpoint($discoveryEndpoint);
$provider->setScope($scope);
$provider = $this->providerMapper->insert($provider);
Expand All @@ -88,7 +94,8 @@ public function updateProvider(int $providerId, string $identifier, string $clie
$provider->setIdentifier($identifier);
$provider->setClientId($clientId);
if ($clientSecret) {
$provider->setClientSecret($clientSecret);
$encryptedClientSecret = $this->crypto->encrypt($clientSecret);
$provider->setClientSecret($encryptedClientSecret);
}
$provider->setDiscoveryEndpoint($discoveryEndpoint);
$provider->setScope($scope);
Expand Down
98 changes: 98 additions & 0 deletions lib/Migration/Version010303Date20230602125945.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright 2023, Julien Veyssier <julien-nc@posteo.net>
*
* @author Julien Veyssier <julien-nc@posteo.net>
*
* @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 <http://www.gnu.org/licenses/>.
*
*/
namespace OCA\UserOIDC\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 Version010303Date20230602125945 extends SimpleMigrationStep {

/**
* @var IDBConnection
*/
private $connection;
/**
* @var ICrypto
*/
private $crypto;

public function __construct(
IDBConnection $connection,
ICrypto $crypto
) {
$this->connection = $connection;
$this->crypto = $crypto;
}

public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

foreach (['user_oidc_providers', 'user_oidc_id4me'] as $tableName) {
if ($schema->hasTable($tableName)) {
$table = $schema->getTable($tableName);
if ($table->hasColumn('client_secret')) {
$column = $table->getColumn('client_secret');
$column->setLength(512);
return $schema;
}
}
}

return null;
}

public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
// update secrets in user_oidc_providers and user_oidc_id4me
foreach (['user_oidc_providers', 'user_oidc_id4me'] as $tableName) {
$qbUpdate = $this->connection->getQueryBuilder();
$qbUpdate->update($tableName)
->set('client_secret', $qbUpdate->createParameter('updateSecret'))
->where(
$qbUpdate->expr()->eq('id', $qbUpdate->createParameter('updateId'))
);

$qbSelect = $this->connection->getQueryBuilder();
$qbSelect->select('id', 'client_secret')
->from($tableName);
$req = $qbSelect->executeQuery();
while ($row = $req->fetch()) {
$id = $row['id'];
$secret = $row['client_secret'];
$encryptedSecret = $this->crypto->encrypt($secret);
$qbUpdate->setParameter('updateSecret', $encryptedSecret, IQueryBuilder::PARAM_STR);
$qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT);
$qbUpdate->executeStatement();
}
$req->closeCursor();
}
}
}
Loading

0 comments on commit 780d900

Please sign in to comment.