From f5514dde9a170594a2e9886f09b39217ef638994 Mon Sep 17 00:00:00 2001 From: Edward Ly Date: Sun, 6 Oct 2024 16:06:57 -0700 Subject: [PATCH 1/2] fix: re-enable PKCE by default Signed-off-by: Edward Ly --- README.md | 4 ++-- lib/Controller/LoginController.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index cdc60ed8..aebd9d9a 100644 --- a/README.md +++ b/README.md @@ -88,10 +88,10 @@ sudo -u www-data php var/www/nextcloud/occ config:app:set --value=0 user_oidc al This app supports PKCE (Proof Key for Code Exchange). https://datatracker.ietf.org/doc/html/rfc7636 -It is disabled by default and can be enabled in `config.php`: +It is enabled by default, but can be disabled in `config.php`: ``` php 'user_oidc' => [ - 'use_pkce' => true, + 'use_pkce' => false, ], ``` diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index 7dd585f9..d4063b2e 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -232,7 +232,7 @@ public function login(int $providerId, ?string $redirectUrl = null) { $this->session->set(self::NONCE, $nonce); $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); - $isPkceEnabled = isset($oidcSystemConfig['use_pkce']) && $oidcSystemConfig['use_pkce']; + $isPkceEnabled = $oidcSystemConfig['use_pkce'] ?? true; if ($isPkceEnabled) { // PKCE code_challenge see https://datatracker.ietf.org/doc/html/rfc7636 $code_verifier = $this->random->generate(128, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER); @@ -396,7 +396,7 @@ public function code(string $state = '', string $code = '', string $scope = '', } $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); - $isPkceEnabled = isset($oidcSystemConfig['use_pkce']) && $oidcSystemConfig['use_pkce']; + $isPkceEnabled = $oidcSystemConfig['use_pkce'] ?? true; $discovery = $this->discoveryService->obtainDiscovery($provider); From f06bfc6d104d2a13e6a96e736a9455c001701777 Mon Sep 17 00:00:00 2001 From: Edward Ly Date: Wed, 9 Oct 2024 12:03:40 -0700 Subject: [PATCH 2/2] fix(LoginController): check for PKCE support from OIDC provider Signed-off-by: Edward Ly --- README.md | 4 ++- lib/Controller/LoginController.php | 48 ++++++++++++++++-------------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index aebd9d9a..5d6e775b 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,9 @@ sudo -u www-data php var/www/nextcloud/occ config:app:set --value=0 user_oidc al This app supports PKCE (Proof Key for Code Exchange). https://datatracker.ietf.org/doc/html/rfc7636 -It is enabled by default, but can be disabled in `config.php`: +Unless PKCE is not supported by the configured OpenID Connect provider, +it is enabled by default. +You can also manually disable it in `config.php`: ``` php 'user_oidc' => [ 'use_pkce' => false, diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index d4063b2e..75ffbc3b 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -224,6 +224,24 @@ public function login(int $providerId, ?string $redirectUrl = null) { return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['provider_not_found' => $providerId]); } + // pass discovery query parameters also on to the authentication + $data = []; + $discoveryUrl = parse_url($provider->getDiscoveryEndpoint()); + if (isset($discoveryUrl['query'])) { + $this->logger->debug('Add custom discovery query: ' . $discoveryUrl['query']); + $discoveryQuery = []; + parse_str($discoveryUrl['query'], $discoveryQuery); + $data += $discoveryQuery; + } + + try { + $discovery = $this->discoveryService->obtainDiscovery($provider); + } catch (\Exception $e) { + $this->logger->error('Could not reach the provider at URL ' . $provider->getDiscoveryEndpoint(), ['exception' => $e]); + $message = $this->l10n->t('Could not reach the OpenID Connect provider.'); + return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['reason' => 'provider unreachable']); + } + $state = $this->random->generate(32, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER); $this->session->set(self::STATE, $state); $this->session->set(self::REDIRECT_AFTER_LOGIN, $redirectUrl); @@ -232,7 +250,9 @@ public function login(int $providerId, ?string $redirectUrl = null) { $this->session->set(self::NONCE, $nonce); $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); - $isPkceEnabled = $oidcSystemConfig['use_pkce'] ?? true; + $isPkceSupported = in_array('S256', $discovery['code_challenge_methods_supported'] ?? [], true); + $isPkceEnabled = $isPkceSupported && ($oidcSystemConfig['use_pkce'] ?? true); + if ($isPkceEnabled) { // PKCE code_challenge see https://datatracker.ietf.org/doc/html/rfc7636 $code_verifier = $this->random->generate(128, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER); @@ -295,7 +315,7 @@ public function login(int $providerId, ?string $redirectUrl = null) { } } - $data = [ + $data += [ 'client_id' => $provider->getClientId(), 'response_type' => 'code', 'scope' => trim($provider->getScope()), @@ -308,23 +328,6 @@ public function login(int $providerId, ?string $redirectUrl = null) { $data['code_challenge'] = $this->toCodeChallenge($code_verifier); $data['code_challenge_method'] = 'S256'; } - // pass discovery query parameters also on to the authentication - $discoveryUrl = parse_url($provider->getDiscoveryEndpoint()); - if (isset($discoveryUrl['query'])) { - $this->logger->debug('Add custom discovery query: ' . $discoveryUrl['query']); - $discoveryQuery = []; - parse_str($discoveryUrl['query'], $discoveryQuery); - $data += $discoveryQuery; - } - - try { - $discovery = $this->discoveryService->obtainDiscovery($provider); - } catch (\Exception $e) { - $this->logger->error('Could not reach the provider at URL ' . $provider->getDiscoveryEndpoint(), ['exception' => $e]); - $message = $this->l10n->t('Could not reach the OpenID Connect provider.'); - return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['reason' => 'provider unreachable']); - } - $authorizationUrl = $this->discoveryService->buildAuthorizationUrl($discovery['authorization_endpoint'], $data); $this->logger->debug('Redirecting user to: ' . $authorizationUrl); @@ -395,13 +398,14 @@ public function code(string $state = '', string $code = '', string $scope = '', return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false); } - $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); - $isPkceEnabled = $oidcSystemConfig['use_pkce'] ?? true; - $discovery = $this->discoveryService->obtainDiscovery($provider); $this->logger->debug('Obtainting data from: ' . $discovery['token_endpoint']); + $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); + $isPkceSupported = in_array('S256', $discovery['code_challenge_methods_supported'] ?? [], true); + $isPkceEnabled = $isPkceSupported && ($oidcSystemConfig['use_pkce'] ?? true); + $client = $this->clientService->newClient(); try { $requestBody = [