From f6c7a7f186993b5b2bd27cffd9bf73e3a2727c84 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Fri, 10 May 2024 11:03:34 +0200 Subject: [PATCH 1/3] fix: add issuer, audience and azp checks in bearer token validator, make it possible to disable audience check via config Signed-off-by: Julien Veyssier --- README.md | 10 +++++ lib/User/Validator/SelfEncodedValidator.php | 41 ++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6cdcca34..f787c1fc 100644 --- a/README.md +++ b/README.md @@ -175,6 +175,16 @@ it is possible to disable the classic "self-encoded" validation: ], ``` +### Disable audience check in bearer token validation + +The `audience` and `azp` token claims will be checked when validating a bearer token for authenticated API requests. +You can disable this check with this config value: +``` php +'user_oidc' => [ + 'selfencoded_bearer_validation_audience_check' => false, +], +``` + ## Building the app Requirements for building: diff --git a/lib/User/Validator/SelfEncodedValidator.php b/lib/User/Validator/SelfEncodedValidator.php index 0e462fe9..63993540 100644 --- a/lib/User/Validator/SelfEncodedValidator.php +++ b/lib/User/Validator/SelfEncodedValidator.php @@ -31,6 +31,7 @@ use OCA\UserOIDC\User\Provisioning\SelfEncodedTokenProvisioning; use OCA\UserOIDC\Vendor\Firebase\JWT\JWT; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; use Psr\Log\LoggerInterface; use Throwable; @@ -42,11 +43,19 @@ class SelfEncodedValidator implements IBearerTokenValidator { private $logger; /** @var ITimeFactory */ private $timeFactory; + /** @var IConfig */ + private $config; - public function __construct(DiscoveryService $discoveryService, LoggerInterface $logger, ITimeFactory $timeFactory) { + public function __construct( + DiscoveryService $discoveryService, + LoggerInterface $logger, + ITimeFactory $timeFactory, + IConfig $config + ) { $this->discoveryService = $discoveryService; $this->logger = $logger; $this->timeFactory = $timeFactory; + $this->config = $config; } public function isValidBearerToken(Provider $provider, string $bearerToken): ?string { @@ -70,6 +79,36 @@ public function isValidBearerToken(Provider $provider, string $bearerToken): ?st return null; } + $discovery = $this->discoveryService->obtainDiscovery($provider); + if ($payload->iss !== $discovery['issuer']) { + $this->logger->debug('This token is issued by the wrong issuer, it does not match the one from the discovery endpoint'); + return null; + } + + $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); + $checkAudience = !isset($oidcSystemConfig['selfencoded_bearer_validation_audience_check']) + || !in_array($oidcSystemConfig['selfencoded_bearer_validation_audience_check'], [false, 'false', 0, '0'], true); + if ($checkAudience) { + if (!($payload->aud === $provider->getClientId() || in_array($provider->getClientId(), $payload->aud, true))) { + $this->logger->debug('This token is not for us, the audience does not match the client ID'); + return null; + } + + // If the ID Token contains multiple audiences, the Client SHOULD verify that an azp Claim is present. + // If an azp (authorized party) Claim is present, the Client SHOULD verify that its client_id is the Claim Value. + if (is_array($payload->aud) && count($payload->aud) > 1) { + if (isset($payload->azp)) { + if ($payload->azp !== $provider->getClientId()) { + $this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID'); + return null; + } + } else { + $this->logger->debug('Multiple audiences but no authorized party (azp) in the id token'); + return null; + } + } + } + // find the user ID if (!isset($payload->{$uidAttribute})) { return null; From 134c14eb02cf457cf2cd1a44f3b493f05b16655f Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Thu, 23 May 2024 17:12:15 +0200 Subject: [PATCH 2/3] make audience check safer Signed-off-by: Julien Veyssier --- lib/Controller/LoginController.php | 7 ++++++- lib/User/Validator/SelfEncodedValidator.php | 11 ++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index d817bb7e..55cc1f6a 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -455,7 +455,12 @@ public function code(string $state = '', string $code = '', string $scope = '', } // Verify audience - if (!($idTokenPayload->aud === $provider->getClientId() || in_array($provider->getClientId(), $idTokenPayload->aud, true))) { + $tokenAudience = $idTokenPayload->aud; + $providerClientId = $provider->getClientId(); + if ( + (is_string($tokenAudience) && $tokenAudience !== $providerClientId) + || (is_array($tokenAudience) && !in_array($providerClientId, $tokenAudience, true)) + ) { $this->logger->debug('This token is not for us'); $message = $this->l10n->t('The audience does not match ours'); return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_audience' => $idTokenPayload->aud]); diff --git a/lib/User/Validator/SelfEncodedValidator.php b/lib/User/Validator/SelfEncodedValidator.php index 63993540..c68fb1cd 100644 --- a/lib/User/Validator/SelfEncodedValidator.php +++ b/lib/User/Validator/SelfEncodedValidator.php @@ -89,16 +89,21 @@ public function isValidBearerToken(Provider $provider, string $bearerToken): ?st $checkAudience = !isset($oidcSystemConfig['selfencoded_bearer_validation_audience_check']) || !in_array($oidcSystemConfig['selfencoded_bearer_validation_audience_check'], [false, 'false', 0, '0'], true); if ($checkAudience) { - if (!($payload->aud === $provider->getClientId() || in_array($provider->getClientId(), $payload->aud, true))) { + $tokenAudience = $payload->aud; + $providerClientId = $provider->getClientId(); + if ( + (is_string($tokenAudience) && $tokenAudience !== $providerClientId) + || (is_array($tokenAudience) && !in_array($providerClientId, $tokenAudience, true)) + ) { $this->logger->debug('This token is not for us, the audience does not match the client ID'); return null; } // If the ID Token contains multiple audiences, the Client SHOULD verify that an azp Claim is present. // If an azp (authorized party) Claim is present, the Client SHOULD verify that its client_id is the Claim Value. - if (is_array($payload->aud) && count($payload->aud) > 1) { + if (is_array($tokenAudience) && count($tokenAudience) > 1) { if (isset($payload->azp)) { - if ($payload->azp !== $provider->getClientId()) { + if ($payload->azp !== $providerClientId) { $this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID'); return null; } From 91e7712f5e2f4ca7f6d969500d60350ed9b060d5 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Tue, 4 Jun 2024 12:13:03 +0200 Subject: [PATCH 3/3] adjust the azp check to OpenID specs Signed-off-by: Julien Veyssier --- lib/Controller/LoginController.php | 20 ++++++-------------- lib/User/Validator/SelfEncodedValidator.php | 17 +++++------------ 2 files changed, 11 insertions(+), 26 deletions(-) diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index 55cc1f6a..c11550a5 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -466,20 +466,12 @@ public function code(string $state = '', string $code = '', string $scope = '', return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_audience' => $idTokenPayload->aud]); } - // If the ID Token contains multiple audiences, the Client SHOULD verify that an azp Claim is present. - // If an azp (authorized party) Claim is present, the Client SHOULD verify that its client_id is the Claim Value. - if (is_array($idTokenPayload->aud) && count($idTokenPayload->aud) > 1) { - if (isset($idTokenPayload->azp)) { - if ($idTokenPayload->azp !== $provider->getClientId()) { - $this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID'); - $message = $this->l10n->t('The authorized party does not match ours'); - return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_azp' => $idTokenPayload->azp]); - } - } else { - $this->logger->debug('Multiple audiences but no authorized party (azp) in the id token'); - $message = $this->l10n->t('No authorized party'); - return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['missing_azp']); - } + // ref https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation + // If the azp claim is present, it should be the client ID + if (isset($idTokenPayload->azp) && $idTokenPayload->azp !== $provider->getClientId()) { + $this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID'); + $message = $this->l10n->t('The authorized party does not match ours'); + return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_azp' => $idTokenPayload->azp]); } if (isset($idTokenPayload->nonce) && $idTokenPayload->nonce !== $this->session->get(self::NONCE)) { diff --git a/lib/User/Validator/SelfEncodedValidator.php b/lib/User/Validator/SelfEncodedValidator.php index c68fb1cd..6e479745 100644 --- a/lib/User/Validator/SelfEncodedValidator.php +++ b/lib/User/Validator/SelfEncodedValidator.php @@ -99,18 +99,11 @@ public function isValidBearerToken(Provider $provider, string $bearerToken): ?st return null; } - // If the ID Token contains multiple audiences, the Client SHOULD verify that an azp Claim is present. - // If an azp (authorized party) Claim is present, the Client SHOULD verify that its client_id is the Claim Value. - if (is_array($tokenAudience) && count($tokenAudience) > 1) { - if (isset($payload->azp)) { - if ($payload->azp !== $providerClientId) { - $this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID'); - return null; - } - } else { - $this->logger->debug('Multiple audiences but no authorized party (azp) in the id token'); - return null; - } + // ref https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation + // If the azp claim is present, it should be the client ID + if (isset($payload->azp) && $payload->azp !== $providerClientId) { + $this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID'); + return null; } }