diff --git a/appinfo/info.xml b/appinfo/info.xml index 99b63507..492f05cc 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -4,7 +4,7 @@ OpenID Connect user backend Use an OpenID Connect backend to login to your Nextcloud Allows flexible configuration of an OIDC server as Nextcloud login user backend. - 1.3.2 + 1.3.3 agpl Roeland Jago Douma Julius Härtl diff --git a/lib/Command/UpsertProvider.php b/lib/Command/UpsertProvider.php index f164fdfb..932574ad 100644 --- a/lib/Command/UpsertProvider.php +++ b/lib/Command/UpsertProvider.php @@ -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; @@ -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() { @@ -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'); @@ -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 diff --git a/lib/Controller/Id4meController.php b/lib/Controller/Id4meController.php index 3c97a711..c39bf4b9 100644 --- a/lib/Controller/Id4meController.php +++ b/lib/Controller/Id4meController.php @@ -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'; @@ -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, @@ -106,7 +98,8 @@ public function __construct( IUserManager $userManager, HttpClientHelper $clientHelper, Id4MeMapper $id4MeMapper, - LoggerInterface $logger + LoggerInterface $logger, + ICrypto $crypto ) { parent::__construct($request, $config); @@ -121,6 +114,7 @@ public function __construct( $this->id4MeMapper = $id4MeMapper; $this->l10n = $l10n; $this->logger = $logger; + $this->crypto = $crypto; } /** @@ -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); } @@ -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', ], diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index 20761ffd..2c2fbbf0 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -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; @@ -122,6 +123,10 @@ class LoginController extends BaseOidcController { /** @var IL10N */ private $l10n; + /** + * @var ICrypto + */ + private $crypto; public function __construct( IRequest $request, @@ -142,7 +147,8 @@ public function __construct( SessionMapper $sessionMapper, ProvisioningService $provisioningService, IL10N $l10n, - ILogger $logger + ILogger $logger, + ICrypto $crypto ) { parent::__construct($request, $config); @@ -165,6 +171,7 @@ public function __construct( $this->provisioningService = $provisioningService; $this->request = $request; $this->l10n = $l10n; + $this->crypto = $crypto; } /** @@ -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); @@ -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', ], diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 4a366cb9..4fb4d601 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -35,6 +35,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; +use OCP\Security\ICrypto; class SettingsController extends Controller { @@ -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, @@ -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); @@ -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); diff --git a/lib/Migration/Version010303Date20230602125945.php b/lib/Migration/Version010303Date20230602125945.php new file mode 100644 index 00000000..ef05e65f --- /dev/null +++ b/lib/Migration/Version010303Date20230602125945.php @@ -0,0 +1,98 @@ + + * + * @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\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(); + } + } +} diff --git a/lib/Service/OIDCService.php b/lib/Service/OIDCService.php index 5e1bbfe2..be489123 100644 --- a/lib/Service/OIDCService.php +++ b/lib/Service/OIDCService.php @@ -27,6 +27,7 @@ use OCA\UserOIDC\Db\Provider; use OCP\Http\Client\IClientService; +use OCP\Security\ICrypto; use Psr\Log\LoggerInterface; use Throwable; @@ -34,14 +35,21 @@ class OIDCService { /** @var LoggerInterface */ private $logger; - /** @var IClientService */ private $clientService; + /** @var ICrypto */ + private $crypto; - public function __construct(DiscoveryService $discoveryService, LoggerInterface $logger, IClientService $clientService) { + public function __construct( + DiscoveryService $discoveryService, + LoggerInterface $logger, + IClientService $clientService, + ICrypto $crypto + ) { $this->discoveryService = $discoveryService; $this->logger = $logger; $this->clientService = $clientService; + $this->crypto = $crypto; } public function userinfo(Provider $provider, string $accessToken): array { @@ -65,6 +73,12 @@ public function userinfo(Provider $provider, string $accessToken): array { } public function introspection(Provider $provider, string $accessToken): array { + try { + $providerClientSecret = $this->crypto->decrypt($provider->getClientSecret()); + } catch (\Exception $e) { + $this->logger->error('Failed to decrypt the client secret', ['exception' => $e]); + return []; + } $url = $this->discoveryService->obtainDiscovery($provider)['introspection_endpoint'] ?? null; if ($url === null) { return []; @@ -74,7 +88,7 @@ public function introspection(Provider $provider, string $accessToken): array { $this->logger->debug('Fetching user info endpoint'); $options = [ 'headers' => [ - 'Authorization' => base64_encode($provider->getClientId() . ':' . $provider->getClientSecret()), + 'Authorization' => base64_encode($provider->getClientId() . ':' . $providerClientSecret), ], 'body' => [ 'token' => $accessToken,