From c76d12d1ded72b5bf74a51fdb1647f87bb935edc Mon Sep 17 00:00:00 2001 From: "Luke T. Shumaker" Date: Fri, 15 Dec 2023 10:58:20 -0700 Subject: [PATCH 1/9] Oidc: Properly query the UserInfo Endpoint BooksStack's OIDC Client requests the 'profile' and 'email' scope values in order to have access to the 'name', 'email', and other claims. It looks for these claims in the ID Token that is returned along with the Access Token. However, the OIDC-core specification section 5.4 [1] only requires that the Provider include those claims in the ID Token *if* an Access Token is not also issued. If an Access Token is issued, the Provider can leave out those claims from the ID Token, and the Client is supposed to obtain them by submitting the Access Token to the UserInfo Endpoint. So I suppose it's just good luck that the OIDC Providers that BookStack has been tested with just so happen to also stick those claims in the ID Token even though they don't have to. But others (in particular: https://login.infomaniak.com) don't do so, and require fetching the UserInfo Endpoint.) A workaround is currently possible by having the user write a theme with a ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook that fetches the UserInfo Endpoint. This workaround isn't great, for a few reasons: 1. Asking the user to implement core parts of the OIDC protocol is silly. 2. The user either needs to re-fetch the .well-known/openid-configuration file to discover the endpoint (adding yet another round-trip to each login) or hard-code the endpoint, which is fragile. 3. The hook doesn't receive the HTTP client configuration. So, have BookStack's OidcService fetch the UserInfo Endpoint and inject those claims into the ID Token, if a UserInfo Endpoint is defined. Two points about this: - Injecting them into the ID Token's claims is the most obvious approach given the current code structure; though I'm not sure it is the best approach, perhaps it should instead fetch the user info in processAuthorizationResponse() and pass that as an argument to processAccessTokenCallback() which would then need a bit of restructuring. But this made sense because it's also how the ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook works. - OIDC *requires* that a UserInfo Endpoint exists, so why bother with that "if a UserInfo Endpoint is defined" bit? Simply out of an abundance of caution that there's an existing BookStack user that is relying on it not fetching the UserInfo Endpoint in order to work with a non-compliant OIDC Provider. [1]: https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims --- .env.example.complete | 1 + app/Access/Oidc/OidcProviderSettings.php | 7 ++++- app/Access/Oidc/OidcService.php | 12 ++++++++ app/Config/oidc.php | 1 + tests/Auth/OidcTest.php | 38 ++++++++++++++++++++++-- 5 files changed, 56 insertions(+), 3 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index e8520a24c..124296818 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -267,6 +267,7 @@ OIDC_ISSUER_DISCOVER=false OIDC_PUBLIC_KEY=null OIDC_AUTH_ENDPOINT=null OIDC_TOKEN_ENDPOINT=null +OIDC_USERINFO_ENDPOINT=null OIDC_ADDITIONAL_SCOPES=null OIDC_DUMP_USER_DETAILS=false OIDC_USER_TO_GROUPS=false diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index bea6a523e..49ccab6f0 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -22,6 +22,7 @@ class OidcProviderSettings public ?string $authorizationEndpoint; public ?string $tokenEndpoint; public ?string $endSessionEndpoint; + public ?string $userinfoEndpoint; /** * @var string[]|array[] @@ -128,6 +129,10 @@ class OidcProviderSettings $discoveredSettings['tokenEndpoint'] = $result['token_endpoint']; } + if (!empty($result['userinfo_endpoint'])) { + $discoveredSettings['userinfoEndpoint'] = $result['userinfo_endpoint']; + } + if (!empty($result['jwks_uri'])) { $keys = $this->loadKeysFromUri($result['jwks_uri'], $httpClient); $discoveredSettings['keys'] = $this->filterKeys($keys); @@ -177,7 +182,7 @@ class OidcProviderSettings */ public function arrayForProvider(): array { - $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint']; + $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint']; $settings = []; foreach ($settingKeys as $setting) { $settings[$setting] = $this->$setting; diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index f1e5b25af..244957991 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -85,6 +85,7 @@ class OidcService 'authorizationEndpoint' => $config['authorization_endpoint'], 'tokenEndpoint' => $config['token_endpoint'], 'endSessionEndpoint' => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null, + 'userinfoEndpoint' => $config['userinfo_endpoint'], ]); // Use keys if configured @@ -228,6 +229,17 @@ class OidcService session()->put("oidc_id_token", $idTokenText); + if (!empty($settings->userinfoEndpoint)) { + $provider = $this->getProvider($settings); + $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); + $response = $provider->getParsedResponse($request); + $claims = $idToken->getAllClaims(); + foreach ($response as $key => $value) { + $claims[$key] = $value; + } + $idToken->replaceClaims($claims); + } + $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [ 'access_token' => $accessToken->getToken(), 'expires_in' => $accessToken->getExpires(), diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 7f8f40d41..8b5470931 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -35,6 +35,7 @@ return [ // OAuth2 endpoints. 'authorization_endpoint' => env('OIDC_AUTH_ENDPOINT', null), 'token_endpoint' => env('OIDC_TOKEN_ENDPOINT', null), + 'userinfo_endpoint' => env('OIDC_USERINFO_ENDPOINT', null), // OIDC RP-Initiated Logout endpoint URL. // A false value force-disables RP-Initiated Logout. diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index dbf26f1bd..f47a20100 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -668,11 +668,44 @@ class OidcTest extends TestCase protected function runLogin($claimOverrides = []): TestResponse { + // These two variables should perhaps be arguments instead of + // assuming that they're tied to whether discovery is enabled, + // but that's how the tests are written for now. + $claimsInIdToken = !config('oidc.discover'); + $tokenEndpoint = config('oidc.discover') + ? OidcJwtHelper::defaultIssuer() . '/oidc/token' + : 'https://oidc.local/token'; + $this->post('/oidc/login'); $state = session()->get('oidc_state'); - $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides)]); - return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + $providerResponses = [$this->getMockAuthorizationResponse($claimsInIdToken ? $claimOverrides : [])]; + if (!$claimsInIdToken) { + $providerResponses[] = new Response(200, [ + 'Content-Type' => 'application/json', + 'Cache-Control' => 'no-cache, no-store', + 'Pragma' => 'no-cache', + ], json_encode($claimOverrides)); + } + + $transactions = $this->mockHttpClient($providerResponses); + + $response = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + + if (auth()->check()) { + $this->assertEquals($claimsInIdToken ? 1 : 2, $transactions->requestCount()); + $tokenRequest = $transactions->requestAt(0); + $this->assertEquals($tokenEndpoint, (string) $tokenRequest->getUri()); + $this->assertEquals('POST', $tokenRequest->getMethod()); + if (!$claimsInIdToken) { + $userinfoRequest = $transactions->requestAt(1); + $this->assertEquals(OidcJwtHelper::defaultIssuer() . '/oidc/userinfo', (string) $userinfoRequest->getUri()); + $this->assertEquals('GET', $userinfoRequest->getMethod()); + $this->assertEquals('Bearer abc123', $userinfoRequest->getHeader('Authorization')[0]); + } + } + + return $response; } protected function getAutoDiscoveryResponse($responseOverrides = []): Response @@ -684,6 +717,7 @@ class OidcTest extends TestCase ], json_encode(array_merge([ 'token_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/token', 'authorization_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/authorize', + 'userinfo_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/userinfo', 'jwks_uri' => OidcJwtHelper::defaultIssuer() . '/oidc/keys', 'issuer' => OidcJwtHelper::defaultIssuer(), 'end_session_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/logout', From d640411adb4d828cffefd1248407eb93db2eaee2 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 16 Apr 2024 15:19:51 +0100 Subject: [PATCH 2/9] OIDC: Cleaned up provider settings, added extra validation - Added endpoint validation to ensure HTTPS as per spec - Added some missing types - Removed redirectUri from OidcProviderSettings since it's not a provider-based setting, but a setting for the oauth client, so extracted that back to service. --- app/Access/Oidc/OidcProviderSettings.php | 19 +++++++++++++------ app/Access/Oidc/OidcService.php | 6 ++++-- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index 49ccab6f0..616bf1012 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -18,7 +18,6 @@ class OidcProviderSettings public string $issuer; public string $clientId; public string $clientSecret; - public ?string $redirectUri; public ?string $authorizationEndpoint; public ?string $tokenEndpoint; public ?string $endSessionEndpoint; @@ -38,7 +37,7 @@ class OidcProviderSettings /** * Apply an array of settings to populate setting properties within this class. */ - protected function applySettingsFromArray(array $settingsArray) + protected function applySettingsFromArray(array $settingsArray): void { foreach ($settingsArray as $key => $value) { if (property_exists($this, $key)) { @@ -52,7 +51,7 @@ class OidcProviderSettings * * @throws InvalidArgumentException */ - protected function validateInitial() + protected function validateInitial(): void { $required = ['clientId', 'clientSecret', 'redirectUri', 'issuer']; foreach ($required as $prop) { @@ -74,12 +73,20 @@ class OidcProviderSettings public function validate(): void { $this->validateInitial(); + $required = ['keys', 'tokenEndpoint', 'authorizationEndpoint']; foreach ($required as $prop) { if (empty($this->$prop)) { throw new InvalidArgumentException("Missing required configuration \"{$prop}\" value"); } } + + $endpointProperties = ['tokenEndpoint', 'authorizationEndpoint', 'userinfoEndpoint']; + foreach ($endpointProperties as $prop) { + if (is_string($this->$prop) && !str_starts_with($this->$prop, 'https://')) { + throw new InvalidArgumentException("Endpoint value for \"{$prop}\" must start with https://"); + } + } } /** @@ -87,7 +94,7 @@ class OidcProviderSettings * * @throws OidcIssuerDiscoveryException */ - public function discoverFromIssuer(ClientInterface $httpClient, Repository $cache, int $cacheMinutes) + public function discoverFromIssuer(ClientInterface $httpClient, Repository $cache, int $cacheMinutes): void { try { $cacheKey = 'oidc-discovery::' . $this->issuer; @@ -180,9 +187,9 @@ class OidcProviderSettings /** * Get the settings needed by an OAuth provider, as a key=>value array. */ - public function arrayForProvider(): array + public function arrayForOAuthProvider(): array { - $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint']; + $settingKeys = ['clientId', 'clientSecret', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint']; $settings = []; foreach ($settingKeys as $setting) { $settings[$setting] = $this->$setting; diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 467e31417..00ac2b6dc 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -91,7 +91,6 @@ class OidcService 'issuer' => $config['issuer'], 'clientId' => $config['client_id'], 'clientSecret' => $config['client_secret'], - 'redirectUri' => url('/oidc/callback'), 'authorizationEndpoint' => $config['authorization_endpoint'], 'tokenEndpoint' => $config['token_endpoint'], 'endSessionEndpoint' => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null, @@ -130,7 +129,10 @@ class OidcService */ protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvider { - $provider = new OidcOAuthProvider($settings->arrayForProvider(), [ + $provider = new OidcOAuthProvider([ + ...$settings->arrayForOAuthProvider(), + 'redirectUri' => url('/oidc/callback'), + ], [ 'httpClient' => $this->http->buildClient(5), 'optionProvider' => new HttpBasicAuthOptionProvider(), ]); From 9183e7f2fed7c06c538e5e7258467fe0508538ca Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 16 Apr 2024 15:52:55 +0100 Subject: [PATCH 3/9] OIDC Userinfo: Labelled changes to be made during review --- app/Access/Oidc/OidcService.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 00ac2b6dc..a7f31e56b 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -241,14 +241,23 @@ class OidcService session()->put("oidc_id_token", $idTokenText); + // TODO - This should not affect id token validation + // TODO - Should only call if we're missing properties if (!empty($settings->userinfoEndpoint)) { $provider = $this->getProvider($settings); $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); $response = $provider->getParsedResponse($request); + // TODO - Ensure response content-type is "application/json" before using in this way (5.3.2) + // TODO - The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used. (5.3.2) + // TODO - Response validation (5.3.4) + // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. + // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. + // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. $claims = $idToken->getAllClaims(); foreach ($response as $key => $value) { $claims[$key] = $value; } + // TODO - Should maybe remain separate from IdToken completely $idToken->replaceClaims($claims); } From a71c8c60b7b6dc0bc20938029b14a86ab9cc95cd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 16 Apr 2024 18:10:32 +0100 Subject: [PATCH 4/9] OIDC: Extracted user detail handling to own OidcUserDetails class Allows a proper defined object instead of an array an extracts related logic out of OidcService. Updated userinfo to only be called if we're missing details. --- app/Access/Oidc/OidcService.php | 123 +++++++--------------------- app/Access/Oidc/OidcUserDetails.php | 83 +++++++++++++++++++ 2 files changed, 114 insertions(+), 92 deletions(-) create mode 100644 app/Access/Oidc/OidcUserDetails.php diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index a7f31e56b..5a73484c1 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -12,7 +12,6 @@ use BookStack\Facades\Theme; use BookStack\Http\HttpRequestService; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\User; -use Illuminate\Support\Arr; use Illuminate\Support\Facades\Cache; use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider; use League\OAuth2\Client\Provider\Exception\IdentityProviderException; @@ -159,69 +158,6 @@ class OidcService return array_filter($scopeArr); } - /** - * Calculate the display name. - */ - protected function getUserDisplayName(OidcIdToken $token, string $defaultValue): string - { - $displayNameAttrString = $this->config()['display_name_claims'] ?? ''; - $displayNameAttrs = explode('|', $displayNameAttrString); - - $displayName = []; - foreach ($displayNameAttrs as $dnAttr) { - $dnComponent = $token->getClaim($dnAttr) ?? ''; - if ($dnComponent !== '') { - $displayName[] = $dnComponent; - } - } - - if (count($displayName) == 0) { - $displayName[] = $defaultValue; - } - - return implode(' ', $displayName); - } - - /** - * Extract the assigned groups from the id token. - * - * @return string[] - */ - protected function getUserGroups(OidcIdToken $token): array - { - $groupsAttr = $this->config()['groups_claim']; - if (empty($groupsAttr)) { - return []; - } - - $groupsList = Arr::get($token->getAllClaims(), $groupsAttr); - if (!is_array($groupsList)) { - return []; - } - - return array_values(array_filter($groupsList, function ($val) { - return is_string($val); - })); - } - - /** - * Extract the details of a user from an ID token. - * - * @return array{name: string, email: string, external_id: string, groups: string[]} - */ - protected function getUserDetails(OidcIdToken $token): array - { - $idClaim = $this->config()['external_id_claim']; - $id = $token->getClaim($idClaim); - - return [ - 'external_id' => $id, - 'email' => $token->getClaim('email'), - 'name' => $this->getUserDisplayName($token, $id), - 'groups' => $this->getUserGroups($token), - ]; - } - /** * Processes a received access token for a user. Login the user when * they exist, optionally registering them automatically. @@ -241,26 +177,6 @@ class OidcService session()->put("oidc_id_token", $idTokenText); - // TODO - This should not affect id token validation - // TODO - Should only call if we're missing properties - if (!empty($settings->userinfoEndpoint)) { - $provider = $this->getProvider($settings); - $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); - $response = $provider->getParsedResponse($request); - // TODO - Ensure response content-type is "application/json" before using in this way (5.3.2) - // TODO - The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used. (5.3.2) - // TODO - Response validation (5.3.4) - // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. - // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. - // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. - $claims = $idToken->getAllClaims(); - foreach ($response as $key => $value) { - $claims[$key] = $value; - } - // TODO - Should maybe remain separate from IdToken completely - $idToken->replaceClaims($claims); - } - $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [ 'access_token' => $accessToken->getToken(), 'expires_in' => $accessToken->getExpires(), @@ -281,31 +197,54 @@ class OidcService throw new OidcException("ID token validate failed with error: {$exception->getMessage()}"); } - $userDetails = $this->getUserDetails($idToken); - $isLoggedIn = auth()->check(); + $userDetails = OidcUserDetails::fromToken( + $idToken, + $this->config()['external_id_claim'], + $this->config()['display_name_claims'] ?? '', + $this->config()['groups_claim'] ?? '' + ); - if (empty($userDetails['email'])) { + // TODO - This should not affect id token validation + if (!$userDetails->isFullyPopulated($this->shouldSyncGroups()) && !empty($settings->userinfoEndpoint)) { + $provider = $this->getProvider($settings); + $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); + $response = $provider->getParsedResponse($request); + // TODO - Ensure response content-type is "application/json" before using in this way (5.3.2) + // TODO - The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used. (5.3.2) + // TODO - Response validation (5.3.4) + // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. + // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. + // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. + $claims = $idToken->getAllClaims(); + foreach ($response as $key => $value) { + $claims[$key] = $value; + } + // TODO - Should maybe remain separate from IdToken completely + $idToken->replaceClaims($claims); + } + + if (empty($userDetails->email)) { throw new OidcException(trans('errors.oidc_no_email_address')); } + $isLoggedIn = auth()->check(); if ($isLoggedIn) { throw new OidcException(trans('errors.oidc_already_logged_in')); } try { $user = $this->registrationService->findOrRegister( - $userDetails['name'], - $userDetails['email'], - $userDetails['external_id'] + $userDetails->name, + $userDetails->email, + $userDetails->externalId ); } catch (UserRegistrationException $exception) { throw new OidcException($exception->getMessage()); } if ($this->shouldSyncGroups()) { - $groups = $userDetails['groups']; $detachExisting = $this->config()['remove_from_groups']; - $this->groupService->syncUserWithFoundGroups($user, $groups, $detachExisting); + $this->groupService->syncUserWithFoundGroups($user, $userDetails->groups ?? [], $detachExisting); } $this->loginService->login($user, 'oidc'); diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php new file mode 100644 index 000000000..1fb40ddc2 --- /dev/null +++ b/app/Access/Oidc/OidcUserDetails.php @@ -0,0 +1,83 @@ +externalId) + || empty($this->email) + || empty($this->name) + || ($groupSyncActive && empty($this->groups)); + + return !$hasEmpty; + } + + /** + * Populate user details from OidcIdToken data. + */ + public static function fromToken( + OidcIdToken $token, + string $idClaim, + string $displayNameClaims, + string $groupsClaim, + ): static { + $id = $token->getClaim($idClaim); + + return new self( + externalId: $id, + email: $token->getClaim('email'), + name: static::getUserDisplayName($displayNameClaims, $token, $id), + groups: static::getUserGroups($groupsClaim, $token), + ); + } + + protected static function getUserDisplayName(string $displayNameClaims, OidcIdToken $token, string $defaultValue): string + { + $displayNameClaimParts = explode('|', $displayNameClaims); + + $displayName = []; + foreach ($displayNameClaimParts as $claim) { + $component = $token->getClaim(trim($claim)) ?? ''; + if ($component !== '') { + $displayName[] = $component; + } + } + + if (count($displayName) === 0) { + $displayName[] = $defaultValue; + } + + return implode(' ', $displayName); + } + + protected static function getUserGroups(string $groupsClaim, OidcIdToken $token): array + { + if (empty($groupsClaim)) { + return []; + } + + $groupsList = Arr::get($token->getAllClaims(), $groupsClaim); + if (!is_array($groupsList)) { + return []; + } + + return array_values(array_filter($groupsList, function ($val) { + return is_string($val); + })); + } +} From 7d7cd32ca72397b635f7be597ad467ca27cffe6e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 17 Apr 2024 18:23:58 +0100 Subject: [PATCH 5/9] OIDC Userinfo: Added userinfo data validation, seperated from id token Wrapped userinfo response in its own class for additional handling and validation. Updated userdetails to take abstract claim data, to be populated by either userinfo data or id token data. --- app/Access/Oidc/OidcIdToken.php | 6 +-- app/Access/Oidc/OidcService.php | 64 ++++++++++++++---------- app/Access/Oidc/OidcUserDetails.php | 22 ++++---- app/Access/Oidc/OidcUserinfoResponse.php | 54 ++++++++++++++++++++ app/Access/Oidc/ProvidesClaims.php | 17 +++++++ 5 files changed, 119 insertions(+), 44 deletions(-) create mode 100644 app/Access/Oidc/OidcUserinfoResponse.php create mode 100644 app/Access/Oidc/ProvidesClaims.php diff --git a/app/Access/Oidc/OidcIdToken.php b/app/Access/Oidc/OidcIdToken.php index 5a395022a..b1da998a5 100644 --- a/app/Access/Oidc/OidcIdToken.php +++ b/app/Access/Oidc/OidcIdToken.php @@ -2,7 +2,7 @@ namespace BookStack\Access\Oidc; -class OidcIdToken +class OidcIdToken implements ProvidesClaims { protected array $header; protected array $payload; @@ -71,10 +71,8 @@ class OidcIdToken /** * Fetch a specific claim from this token. * Returns null if it is null or does not exist. - * - * @return mixed|null */ - public function getClaim(string $claim) + public function getClaim(string $claim): mixed { return $this->payload[$claim] ?? null; } diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 5a73484c1..fba6dc9a8 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -194,35 +194,10 @@ class OidcService try { $idToken->validate($settings->clientId); } catch (OidcInvalidTokenException $exception) { - throw new OidcException("ID token validate failed with error: {$exception->getMessage()}"); - } - - $userDetails = OidcUserDetails::fromToken( - $idToken, - $this->config()['external_id_claim'], - $this->config()['display_name_claims'] ?? '', - $this->config()['groups_claim'] ?? '' - ); - - // TODO - This should not affect id token validation - if (!$userDetails->isFullyPopulated($this->shouldSyncGroups()) && !empty($settings->userinfoEndpoint)) { - $provider = $this->getProvider($settings); - $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); - $response = $provider->getParsedResponse($request); - // TODO - Ensure response content-type is "application/json" before using in this way (5.3.2) - // TODO - The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used. (5.3.2) - // TODO - Response validation (5.3.4) - // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. - // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. - // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. - $claims = $idToken->getAllClaims(); - foreach ($response as $key => $value) { - $claims[$key] = $value; - } - // TODO - Should maybe remain separate from IdToken completely - $idToken->replaceClaims($claims); + throw new OidcException("ID token validation failed with error: {$exception->getMessage()}"); } + $userDetails = $this->getUserDetailsFromToken($idToken, $accessToken, $settings); if (empty($userDetails->email)) { throw new OidcException(trans('errors.oidc_no_email_address')); } @@ -252,6 +227,41 @@ class OidcService return $user; } + /** + * @throws OidcException + */ + protected function getUserDetailsFromToken(OidcIdToken $idToken, OidcAccessToken $accessToken, OidcProviderSettings $settings): OidcUserDetails + { + $userDetails = new OidcUserDetails(); + $userDetails->populate( + $idToken, + $this->config()['external_id_claim'], + $this->config()['display_name_claims'] ?? '', + $this->config()['groups_claim'] ?? '' + ); + + if (!$userDetails->isFullyPopulated($this->shouldSyncGroups()) && !empty($settings->userinfoEndpoint)) { + $provider = $this->getProvider($settings); + $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); + $response = new OidcUserinfoResponse($provider->getResponse($request)); + + try { + $response->validate($idToken->getClaim('sub')); + } catch (OidcInvalidTokenException $exception) { + throw new OidcException("Userinfo endpoint response validation failed with error: {$exception->getMessage()}"); + } + + $userDetails->populate( + $response, + $this->config()['external_id_claim'], + $this->config()['display_name_claims'] ?? '', + $this->config()['groups_claim'] ?? '' + ); + } + + return $userDetails; + } + /** * Get the OIDC config from the application. */ diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php index 1fb40ddc2..172bc9ceb 100644 --- a/app/Access/Oidc/OidcUserDetails.php +++ b/app/Access/Oidc/OidcUserDetails.php @@ -30,23 +30,19 @@ class OidcUserDetails /** * Populate user details from OidcIdToken data. */ - public static function fromToken( - OidcIdToken $token, + public function populate( + ProvidesClaims $claims, string $idClaim, string $displayNameClaims, string $groupsClaim, - ): static { - $id = $token->getClaim($idClaim); - - return new self( - externalId: $id, - email: $token->getClaim('email'), - name: static::getUserDisplayName($displayNameClaims, $token, $id), - groups: static::getUserGroups($groupsClaim, $token), - ); + ): void { + $this->externalId = $claims->getClaim($idClaim) ?? $this->externalId; + $this->email = $claims->getClaim('email') ?? $this->email; + $this->name = static::getUserDisplayName($displayNameClaims, $claims, $this->externalId) ?? $this->name; + $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups; } - protected static function getUserDisplayName(string $displayNameClaims, OidcIdToken $token, string $defaultValue): string + protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token, string $defaultValue): string { $displayNameClaimParts = explode('|', $displayNameClaims); @@ -65,7 +61,7 @@ class OidcUserDetails return implode(' ', $displayName); } - protected static function getUserGroups(string $groupsClaim, OidcIdToken $token): array + protected static function getUserGroups(string $groupsClaim, ProvidesClaims $token): array { if (empty($groupsClaim)) { return []; diff --git a/app/Access/Oidc/OidcUserinfoResponse.php b/app/Access/Oidc/OidcUserinfoResponse.php new file mode 100644 index 000000000..7c7760434 --- /dev/null +++ b/app/Access/Oidc/OidcUserinfoResponse.php @@ -0,0 +1,54 @@ +getHeader('Content-Type')[0] === 'application/json') { + $this->claims = json_decode($response->getBody()->getContents(), true); + } + + // TODO - Support JWTs + // TODO - Response validation (5.3.4): + // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. + // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. + // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. + } + + /** + * @throws OidcInvalidTokenException + */ + public function validate(string $idTokenSub): bool + { + $sub = $this->getClaim('sub'); + + // Spec: v1.0 5.3.2: The sub (subject) Claim MUST always be returned in the UserInfo Response. + if (!is_string($sub) || empty($sub)) { + throw new OidcInvalidTokenException("No valid subject value found in userinfo data"); + } + + // Spec: v1.0 5.3.2: The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; + // if they do not match, the UserInfo Response values MUST NOT be used. + if ($idTokenSub !== $sub) { + throw new OidcInvalidTokenException("Subject value provided in the userinfo endpoint does not match the provided ID token value"); + } + + return true; + } + + public function getClaim(string $claim): mixed + { + return $this->claims[$claim] ?? null; + } + + public function getAllClaims(): array + { + return $this->claims; + } +} diff --git a/app/Access/Oidc/ProvidesClaims.php b/app/Access/Oidc/ProvidesClaims.php new file mode 100644 index 000000000..a3cf51655 --- /dev/null +++ b/app/Access/Oidc/ProvidesClaims.php @@ -0,0 +1,17 @@ + Date: Wed, 17 Apr 2024 23:24:57 +0100 Subject: [PATCH 6/9] OIDC Userinfo: Started writing tests to cover userinfo calling --- app/Access/Oidc/OidcProviderSettings.php | 2 +- app/Access/Oidc/OidcService.php | 3 + app/Access/Oidc/OidcUserDetails.php | 10 +- tests/Auth/OidcTest.php | 142 +++++++++++++++++------ 4 files changed, 111 insertions(+), 46 deletions(-) diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index 616bf1012..71c3b5734 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -53,7 +53,7 @@ class OidcProviderSettings */ protected function validateInitial(): void { - $required = ['clientId', 'clientSecret', 'redirectUri', 'issuer']; + $required = ['clientId', 'clientSecret', 'issuer']; foreach ($required as $prop) { if (empty($this->$prop)) { throw new InvalidArgumentException("Missing required configuration \"{$prop}\" value"); diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index fba6dc9a8..6d024ae32 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -201,6 +201,9 @@ class OidcService if (empty($userDetails->email)) { throw new OidcException(trans('errors.oidc_no_email_address')); } + if (empty($userDetails->name)) { + $userDetails->name = $userDetails->externalId; + } $isLoggedIn = auth()->check(); if ($isLoggedIn) { diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php index 172bc9ceb..bccc49ee4 100644 --- a/app/Access/Oidc/OidcUserDetails.php +++ b/app/Access/Oidc/OidcUserDetails.php @@ -28,7 +28,7 @@ class OidcUserDetails } /** - * Populate user details from OidcIdToken data. + * Populate user details from the given claim data. */ public function populate( ProvidesClaims $claims, @@ -38,11 +38,11 @@ class OidcUserDetails ): void { $this->externalId = $claims->getClaim($idClaim) ?? $this->externalId; $this->email = $claims->getClaim('email') ?? $this->email; - $this->name = static::getUserDisplayName($displayNameClaims, $claims, $this->externalId) ?? $this->name; + $this->name = static::getUserDisplayName($displayNameClaims, $claims) ?? $this->name; $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups; } - protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token, string $defaultValue): string + protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token): string { $displayNameClaimParts = explode('|', $displayNameClaims); @@ -54,10 +54,6 @@ class OidcUserDetails } } - if (count($displayName) === 0) { - $displayName[] = $defaultValue; - } - return implode(' ', $displayName); } diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 661722983..9ed3fa7b9 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -37,6 +37,7 @@ class OidcTest extends TestCase 'oidc.issuer' => OidcJwtHelper::defaultIssuer(), 'oidc.authorization_endpoint' => 'https://oidc.local/auth', 'oidc.token_endpoint' => 'https://oidc.local/token', + 'oidc.userinfo_endpoint' => 'https://oidc.local/userinfo', 'oidc.discover' => false, 'oidc.dump_user_details' => false, 'oidc.additional_scopes' => '', @@ -208,6 +209,8 @@ class OidcTest extends TestCase public function test_auth_fails_if_no_email_exists_in_user_data() { + config()->set('oidc.userinfo_endpoint', null); + $this->runLogin([ 'email' => '', 'sub' => 'benny505', @@ -270,10 +273,38 @@ class OidcTest extends TestCase ]); $resp = $this->followRedirects($resp); - $resp->assertSeeText('ID token validate failed with error: Missing token subject value'); + $resp->assertSeeText('ID token validation failed with error: Missing token subject value'); $this->assertFalse(auth()->check()); } + public function test_auth_fails_if_endpoints_start_with_https() + { + $endpointConfigKeys = [ + 'oidc.token_endpoint' => 'tokenEndpoint', + 'oidc.authorization_endpoint' => 'authorizationEndpoint', + 'oidc.userinfo_endpoint' => 'userinfoEndpoint', + ]; + + foreach ($endpointConfigKeys as $endpointConfigKey => $endpointName) { + $logger = $this->withTestLogger(); + $original = config()->get($endpointConfigKey); + $new = str_replace('https://', 'http://', $original); + config()->set($endpointConfigKey, $new); + + $this->withoutExceptionHandling(); + $err = null; + try { + $resp = $this->runLogin(); + $resp->assertRedirect('/login'); + } catch (\Exception $exception) { + $err = $exception; + } + $this->assertEquals("Endpoint value for \"{$endpointName}\" must start with https://", $err->getMessage()); + + config()->set($endpointConfigKey, $original); + } + } + public function test_auth_login_with_autodiscovery() { $this->withAutodiscovery(); @@ -689,57 +720,92 @@ class OidcTest extends TestCase $this->assertEquals($pkceCode, $bodyParams['code_verifier']); } - protected function withAutodiscovery() + public function test_userinfo_endpoint_used_if_missing_claims_in_id_token() + { + config()->set('oidc.display_name_claims', 'first_name|last_name'); + $this->post('/oidc/login'); + $state = session()->get('oidc_state'); + + $client = $this->mockHttpClient([ + $this->getMockAuthorizationResponse(['name' => null]), + new Response(200, [ + 'Content-Type' => 'application/json', + ], json_encode([ + 'sub' => OidcJwtHelper::defaultPayload()['sub'], + 'first_name' => 'Barry', + 'last_name' => 'Userinfo', + ])) + ]); + + $resp = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + $resp->assertRedirect('/'); + $this->assertEquals(2, $client->requestCount()); + + $userinfoRequest = $client->requestAt(1); + $this->assertEquals('GET', $userinfoRequest->getMethod()); + $this->assertEquals('https://oidc.local/userinfo', (string) $userinfoRequest->getUri()); + + $this->assertEquals('Barry Userinfo', user()->name); + } + + public function test_userinfo_endpoint_fetch_with_different_sub_throws_error() + { + $userinfoResponseData = ['sub' => 'dcba4321']; + $userinfoResponse = new Response(200, ['Content-Type' => 'application/json'], json_encode($userinfoResponseData)); + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: Subject value provided in the userinfo endpoint does not match the provided ID token value'); + } + + public function test_userinfo_endpoint_fetch_returning_no_sub_throws_error() + { + $userinfoResponseData = ['name' => 'testing']; + $userinfoResponse = new Response(200, ['Content-Type' => 'application/json'], json_encode($userinfoResponseData)); + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: No valid subject value found in userinfo data'); + } + + public function test_userinfo_endpoint_fetch_can_parsed_nested_groups() + { + config()->set([ + 'oidc.user_to_groups' => true, + 'oidc.groups_claim' => 'my.nested.groups.attr', + 'oidc.remove_from_groups' => false, + ]); + + $roleA = Role::factory()->create(['display_name' => 'Ducks']); + $userinfoResponseData = [ + 'sub' => OidcJwtHelper::defaultPayload()['sub'], + 'my' => ['nested' => ['groups' => ['attr' => ['Ducks', 'Donkeys']]]] + ]; + $userinfoResponse = new Response(200, ['Content-Type' => 'application/json'], json_encode($userinfoResponseData)); + $resp = $this->runLogin(['groups' => null], [$userinfoResponse]); + $resp->assertRedirect('/'); + + $user = User::where('email', OidcJwtHelper::defaultPayload()['email'])->first(); + $this->assertTrue($user->hasRole($roleA->id)); + } + + protected function withAutodiscovery(): void { config()->set([ 'oidc.issuer' => OidcJwtHelper::defaultIssuer(), 'oidc.discover' => true, 'oidc.authorization_endpoint' => null, 'oidc.token_endpoint' => null, + 'oidc.userinfo_endpoint' => null, 'oidc.jwt_public_key' => null, ]); } - protected function runLogin($claimOverrides = []): TestResponse + protected function runLogin($claimOverrides = [], $additionalHttpResponses = []): TestResponse { - // These two variables should perhaps be arguments instead of - // assuming that they're tied to whether discovery is enabled, - // but that's how the tests are written for now. - $claimsInIdToken = !config('oidc.discover'); - $tokenEndpoint = config('oidc.discover') - ? OidcJwtHelper::defaultIssuer() . '/oidc/token' - : 'https://oidc.local/token'; - $this->post('/oidc/login'); $state = session()->get('oidc_state'); + $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides), ...$additionalHttpResponses]); - $providerResponses = [$this->getMockAuthorizationResponse($claimsInIdToken ? $claimOverrides : [])]; - if (!$claimsInIdToken) { - $providerResponses[] = new Response(200, [ - 'Content-Type' => 'application/json', - 'Cache-Control' => 'no-cache, no-store', - 'Pragma' => 'no-cache', - ], json_encode($claimOverrides)); - } - - $transactions = $this->mockHttpClient($providerResponses); - - $response = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); - - if (auth()->check()) { - $this->assertEquals($claimsInIdToken ? 1 : 2, $transactions->requestCount()); - $tokenRequest = $transactions->requestAt(0); - $this->assertEquals($tokenEndpoint, (string) $tokenRequest->getUri()); - $this->assertEquals('POST', $tokenRequest->getMethod()); - if (!$claimsInIdToken) { - $userinfoRequest = $transactions->requestAt(1); - $this->assertEquals(OidcJwtHelper::defaultIssuer() . '/oidc/userinfo', (string) $userinfoRequest->getUri()); - $this->assertEquals('GET', $userinfoRequest->getMethod()); - $this->assertEquals('Bearer abc123', $userinfoRequest->getHeader('Authorization')[0]); - } - } - - return $response; + return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); } protected function getAutoDiscoveryResponse($responseOverrides = []): Response From b18cee3dc4cec028ff9efa69dab960649bb38425 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 19 Apr 2024 14:12:27 +0100 Subject: [PATCH 7/9] OIDC Userinfo: Added JWT signed response support Not yet tested, nor checked all response validations. --- app/Access/Oidc/OidcIdToken.php | 144 +------------------ app/Access/Oidc/OidcJwtWithClaims.php | 174 +++++++++++++++++++++++ app/Access/Oidc/OidcService.php | 6 +- app/Access/Oidc/OidcUserinfoResponse.php | 16 ++- 4 files changed, 196 insertions(+), 144 deletions(-) create mode 100644 app/Access/Oidc/OidcJwtWithClaims.php diff --git a/app/Access/Oidc/OidcIdToken.php b/app/Access/Oidc/OidcIdToken.php index b1da998a5..f3fad0e1e 100644 --- a/app/Access/Oidc/OidcIdToken.php +++ b/app/Access/Oidc/OidcIdToken.php @@ -2,58 +2,8 @@ namespace BookStack\Access\Oidc; -class OidcIdToken implements ProvidesClaims +class OidcIdToken extends OidcJwtWithClaims implements ProvidesClaims { - protected array $header; - protected array $payload; - protected string $signature; - protected string $issuer; - protected array $tokenParts = []; - - /** - * @var array[]|string[] - */ - protected array $keys; - - public function __construct(string $token, string $issuer, array $keys) - { - $this->keys = $keys; - $this->issuer = $issuer; - $this->parse($token); - } - - /** - * Parse the token content into its components. - */ - protected function parse(string $token): void - { - $this->tokenParts = explode('.', $token); - $this->header = $this->parseEncodedTokenPart($this->tokenParts[0]); - $this->payload = $this->parseEncodedTokenPart($this->tokenParts[1] ?? ''); - $this->signature = $this->base64UrlDecode($this->tokenParts[2] ?? '') ?: ''; - } - - /** - * Parse a Base64-JSON encoded token part. - * Returns the data as a key-value array or empty array upon error. - */ - protected function parseEncodedTokenPart(string $part): array - { - $json = $this->base64UrlDecode($part) ?: '{}'; - $decoded = json_decode($json, true); - - return is_array($decoded) ? $decoded : []; - } - - /** - * Base64URL decode. Needs some character conversions to be compatible - * with PHP's default base64 handling. - */ - protected function base64UrlDecode(string $encoded): string - { - return base64_decode(strtr($encoded, '-_', '+/')); - } - /** * Validate all possible parts of the id token. * @@ -61,89 +11,12 @@ class OidcIdToken implements ProvidesClaims */ public function validate(string $clientId): bool { - $this->validateTokenStructure(); - $this->validateTokenSignature(); + parent::validateCommonClaims(); $this->validateTokenClaims($clientId); return true; } - /** - * Fetch a specific claim from this token. - * Returns null if it is null or does not exist. - */ - public function getClaim(string $claim): mixed - { - return $this->payload[$claim] ?? null; - } - - /** - * Get all returned claims within the token. - */ - public function getAllClaims(): array - { - return $this->payload; - } - - /** - * Replace the existing claim data of this token with that provided. - */ - public function replaceClaims(array $claims): void - { - $this->payload = $claims; - } - - /** - * Validate the structure of the given token and ensure we have the required pieces. - * As per https://datatracker.ietf.org/doc/html/rfc7519#section-7.2. - * - * @throws OidcInvalidTokenException - */ - protected function validateTokenStructure(): void - { - foreach (['header', 'payload'] as $prop) { - if (empty($this->$prop) || !is_array($this->$prop)) { - throw new OidcInvalidTokenException("Could not parse out a valid {$prop} within the provided token"); - } - } - - if (empty($this->signature) || !is_string($this->signature)) { - throw new OidcInvalidTokenException('Could not parse out a valid signature within the provided token'); - } - } - - /** - * Validate the signature of the given token and ensure it validates against the provided key. - * - * @throws OidcInvalidTokenException - */ - protected function validateTokenSignature(): void - { - if ($this->header['alg'] !== 'RS256') { - throw new OidcInvalidTokenException("Only RS256 signature validation is supported. Token reports using {$this->header['alg']}"); - } - - $parsedKeys = array_map(function ($key) { - try { - return new OidcJwtSigningKey($key); - } catch (OidcInvalidKeyException $e) { - throw new OidcInvalidTokenException('Failed to read signing key with error: ' . $e->getMessage()); - } - }, $this->keys); - - $parsedKeys = array_filter($parsedKeys); - - $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1]; - /** @var OidcJwtSigningKey $parsedKey */ - foreach ($parsedKeys as $parsedKey) { - if ($parsedKey->verify($contentToSign, $this->signature)) { - return; - } - } - - throw new OidcInvalidTokenException('Token signature could not be validated using the provided keys'); - } - /** * Validate the claims of the token. * As per https://openid.net/specs/openid-connect-basic-1_0.html#IDTokenValidation. @@ -154,27 +27,18 @@ class OidcIdToken implements ProvidesClaims { // 1. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) // MUST exactly match the value of the iss (issuer) Claim. - if (empty($this->payload['iss']) || $this->issuer !== $this->payload['iss']) { - throw new OidcInvalidTokenException('Missing or non-matching token issuer value'); - } + // Already done in parent. // 2. The Client MUST validate that the aud (audience) Claim contains its client_id value registered // at the Issuer identified by the iss (issuer) Claim as an audience. The ID Token MUST be rejected // if the ID Token does not list the Client as a valid audience, or if it contains additional // audiences not trusted by the Client. - if (empty($this->payload['aud'])) { - throw new OidcInvalidTokenException('Missing token audience value'); - } - + // Partially done in parent. $aud = is_string($this->payload['aud']) ? [$this->payload['aud']] : $this->payload['aud']; if (count($aud) !== 1) { throw new OidcInvalidTokenException('Token audience value has ' . count($aud) . ' values, Expected 1'); } - if ($aud[0] !== $clientId) { - throw new OidcInvalidTokenException('Token audience value did not match the expected client_id'); - } - // 3. If the ID Token contains multiple audiences, the Client SHOULD verify that an azp Claim is present. // NOTE: Addressed by enforcing a count of 1 above. diff --git a/app/Access/Oidc/OidcJwtWithClaims.php b/app/Access/Oidc/OidcJwtWithClaims.php new file mode 100644 index 000000000..cc13936ab --- /dev/null +++ b/app/Access/Oidc/OidcJwtWithClaims.php @@ -0,0 +1,174 @@ +keys = $keys; + $this->issuer = $issuer; + $this->parse($token); + } + + /** + * Parse the token content into its components. + */ + protected function parse(string $token): void + { + $this->tokenParts = explode('.', $token); + $this->header = $this->parseEncodedTokenPart($this->tokenParts[0]); + $this->payload = $this->parseEncodedTokenPart($this->tokenParts[1] ?? ''); + $this->signature = $this->base64UrlDecode($this->tokenParts[2] ?? '') ?: ''; + } + + /** + * Parse a Base64-JSON encoded token part. + * Returns the data as a key-value array or empty array upon error. + */ + protected function parseEncodedTokenPart(string $part): array + { + $json = $this->base64UrlDecode($part) ?: '{}'; + $decoded = json_decode($json, true); + + return is_array($decoded) ? $decoded : []; + } + + /** + * Base64URL decode. Needs some character conversions to be compatible + * with PHP's default base64 handling. + */ + protected function base64UrlDecode(string $encoded): string + { + return base64_decode(strtr($encoded, '-_', '+/')); + } + + /** + * Validate common parts of OIDC JWT tokens. + * + * @throws OidcInvalidTokenException + */ + protected function validateCommonTokenDetails(): bool + { + $this->validateTokenStructure(); + $this->validateTokenSignature(); + $this->validateCommonClaims(); + + return true; + } + + /** + * Fetch a specific claim from this token. + * Returns null if it is null or does not exist. + */ + public function getClaim(string $claim): mixed + { + return $this->payload[$claim] ?? null; + } + + /** + * Get all returned claims within the token. + */ + public function getAllClaims(): array + { + return $this->payload; + } + + /** + * Replace the existing claim data of this token with that provided. + */ + public function replaceClaims(array $claims): void + { + $this->payload = $claims; + } + + /** + * Validate the structure of the given token and ensure we have the required pieces. + * As per https://datatracker.ietf.org/doc/html/rfc7519#section-7.2. + * + * @throws OidcInvalidTokenException + */ + protected function validateTokenStructure(): void + { + foreach (['header', 'payload'] as $prop) { + if (empty($this->$prop) || !is_array($this->$prop)) { + throw new OidcInvalidTokenException("Could not parse out a valid {$prop} within the provided token"); + } + } + + if (empty($this->signature) || !is_string($this->signature)) { + throw new OidcInvalidTokenException('Could not parse out a valid signature within the provided token'); + } + } + + /** + * Validate the signature of the given token and ensure it validates against the provided key. + * + * @throws OidcInvalidTokenException + */ + protected function validateTokenSignature(): void + { + if ($this->header['alg'] !== 'RS256') { + throw new OidcInvalidTokenException("Only RS256 signature validation is supported. Token reports using {$this->header['alg']}"); + } + + $parsedKeys = array_map(function ($key) { + try { + return new OidcJwtSigningKey($key); + } catch (OidcInvalidKeyException $e) { + throw new OidcInvalidTokenException('Failed to read signing key with error: ' . $e->getMessage()); + } + }, $this->keys); + + $parsedKeys = array_filter($parsedKeys); + + $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1]; + /** @var OidcJwtSigningKey $parsedKey */ + foreach ($parsedKeys as $parsedKey) { + if ($parsedKey->verify($contentToSign, $this->signature)) { + return; + } + } + + throw new OidcInvalidTokenException('Token signature could not be validated using the provided keys'); + } + + /** + * Validate common claims for OIDC JWT tokens. + * As per https://openid.net/specs/openid-connect-basic-1_0.html#IDTokenValidation + * and https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse + * + * @throws OidcInvalidTokenException + */ + public function validateCommonClaims(): void + { + // 1. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) + // MUST exactly match the value of the iss (issuer) Claim. + if (empty($this->payload['iss']) || $this->issuer !== $this->payload['iss']) { + throw new OidcInvalidTokenException('Missing or non-matching token issuer value'); + } + + // 2. The Client MUST validate that the aud (audience) Claim contains its client_id value registered + // at the Issuer identified by the iss (issuer) Claim as an audience. The ID Token MUST be rejected + // if the ID Token does not list the Client as a valid audience. + if (empty($this->payload['aud'])) { + throw new OidcInvalidTokenException('Missing token audience value'); + } + + $aud = is_string($this->payload['aud']) ? [$this->payload['aud']] : $this->payload['aud']; + if (!in_array($this->payload['aud'], $aud, true)) { + throw new OidcInvalidTokenException('Token audience value did not match the expected client_id'); + } + } +} diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 6d024ae32..6bb326e4b 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -246,7 +246,11 @@ class OidcService if (!$userDetails->isFullyPopulated($this->shouldSyncGroups()) && !empty($settings->userinfoEndpoint)) { $provider = $this->getProvider($settings); $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); - $response = new OidcUserinfoResponse($provider->getResponse($request)); + $response = new OidcUserinfoResponse( + $provider->getResponse($request), + $settings->issuer, + $settings->keys, + ); try { $response->validate($idToken->getClaim('sub')); diff --git a/app/Access/Oidc/OidcUserinfoResponse.php b/app/Access/Oidc/OidcUserinfoResponse.php index 7c7760434..bb6c2454a 100644 --- a/app/Access/Oidc/OidcUserinfoResponse.php +++ b/app/Access/Oidc/OidcUserinfoResponse.php @@ -7,14 +7,20 @@ use Psr\Http\Message\ResponseInterface; class OidcUserinfoResponse implements ProvidesClaims { protected array $claims = []; + protected ?OidcJwtWithClaims $jwt = null; - public function __construct(ResponseInterface $response) + public function __construct(ResponseInterface $response, string $issuer, array $keys) { - if ($response->getHeader('Content-Type')[0] === 'application/json') { + $contentType = $response->getHeader('Content-Type')[0]; + if ($contentType === 'application/json') { $this->claims = json_decode($response->getBody()->getContents(), true); } - // TODO - Support JWTs + if ($contentType === 'application/jwt') { + $this->jwt = new OidcJwtWithClaims($response->getBody()->getContents(), $issuer, $keys); + $this->claims = $this->jwt->getAllClaims(); + } + // TODO - Response validation (5.3.4): // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. @@ -26,6 +32,10 @@ class OidcUserinfoResponse implements ProvidesClaims */ public function validate(string $idTokenSub): bool { + if (!is_null($this->jwt)) { + $this->jwt->validateCommonClaims(); + } + $sub = $this->getClaim('sub'); // Spec: v1.0 5.3.2: The sub (subject) Claim MUST always be returned in the UserInfo Response. From 0958909cd999be772c045aada5bc426dffb0a0b1 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 19 Apr 2024 15:05:00 +0100 Subject: [PATCH 8/9] OIDC Userinfo: Added additional tests to cover jwks usage --- app/Access/Oidc/OidcJwtWithClaims.php | 4 +- app/Access/Oidc/OidcUserinfoResponse.php | 15 +++--- tests/Auth/OidcTest.php | 62 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/app/Access/Oidc/OidcJwtWithClaims.php b/app/Access/Oidc/OidcJwtWithClaims.php index cc13936ab..393ac5f0e 100644 --- a/app/Access/Oidc/OidcJwtWithClaims.php +++ b/app/Access/Oidc/OidcJwtWithClaims.php @@ -59,7 +59,7 @@ class OidcJwtWithClaims implements ProvidesClaims * * @throws OidcInvalidTokenException */ - protected function validateCommonTokenDetails(): bool + public function validateCommonTokenDetails(): bool { $this->validateTokenStructure(); $this->validateTokenSignature(); @@ -151,7 +151,7 @@ class OidcJwtWithClaims implements ProvidesClaims * * @throws OidcInvalidTokenException */ - public function validateCommonClaims(): void + protected function validateCommonClaims(): void { // 1. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) // MUST exactly match the value of the iss (issuer) Claim. diff --git a/app/Access/Oidc/OidcUserinfoResponse.php b/app/Access/Oidc/OidcUserinfoResponse.php index bb6c2454a..0026d2f0a 100644 --- a/app/Access/Oidc/OidcUserinfoResponse.php +++ b/app/Access/Oidc/OidcUserinfoResponse.php @@ -20,11 +20,6 @@ class OidcUserinfoResponse implements ProvidesClaims $this->jwt = new OidcJwtWithClaims($response->getBody()->getContents(), $issuer, $keys); $this->claims = $this->jwt->getAllClaims(); } - - // TODO - Response validation (5.3.4): - // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. - // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. - // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. } /** @@ -33,7 +28,7 @@ class OidcUserinfoResponse implements ProvidesClaims public function validate(string $idTokenSub): bool { if (!is_null($this->jwt)) { - $this->jwt->validateCommonClaims(); + $this->jwt->validateCommonTokenDetails(); } $sub = $this->getClaim('sub'); @@ -49,6 +44,14 @@ class OidcUserinfoResponse implements ProvidesClaims throw new OidcInvalidTokenException("Subject value provided in the userinfo endpoint does not match the provided ID token value"); } + // Spec v1.0 5.3.4 Defines the following: + // Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. + // This is effectively done as part of the HTTP request we're making through CURLOPT_SSL_VERIFYHOST on the request. + // If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. + // We don't currently support JWT encryption for OIDC + // If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. + // This is done as part of the validateCommonClaims above. + return true; } diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 9ed3fa7b9..9bde71c80 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -787,6 +787,68 @@ class OidcTest extends TestCase $this->assertTrue($user->hasRole($roleA->id)); } + public function test_userinfo_endpoint_jwks_response_handled() + { + $userinfoResponseData = OidcJwtHelper::idToken(['name' => 'Barry Jwks']); + $userinfoResponse = new Response(200, ['Content-Type' => 'application/jwt'], $userinfoResponseData); + + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/'); + + $user = User::where('email', OidcJwtHelper::defaultPayload()['email'])->first(); + $this->assertEquals('Barry Jwks', $user->name); + } + + public function test_userinfo_endpoint_jwks_response_returning_no_sub_throws() + { + $userinfoResponseData = OidcJwtHelper::idToken(['sub' => null]); + $userinfoResponse = new Response(200, ['Content-Type' => 'application/jwt'], $userinfoResponseData); + + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: No valid subject value found in userinfo data'); + } + + public function test_userinfo_endpoint_jwks_response_returning_non_matching_sub_throws() + { + $userinfoResponseData = OidcJwtHelper::idToken(['sub' => 'zzz123']); + $userinfoResponse = new Response(200, ['Content-Type' => 'application/jwt'], $userinfoResponseData); + + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: Subject value provided in the userinfo endpoint does not match the provided ID token value'); + } + + public function test_userinfo_endpoint_jwks_response_with_invalid_signature_throws() + { + $userinfoResponseData = OidcJwtHelper::idToken(); + $exploded = explode('.', $userinfoResponseData); + $exploded[2] = base64_encode(base64_decode($exploded[2]) . 'ABC'); + $userinfoResponse = new Response(200, ['Content-Type' => 'application/jwt'], implode('.', $exploded)); + + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: Token signature could not be validated using the provided keys'); + } + + public function test_userinfo_endpoint_jwks_response_with_invalid_signature_alg_throws() + { + $userinfoResponseData = OidcJwtHelper::idToken([], ['alg' => 'ZZ512']); + $userinfoResponse = new Response(200, ['Content-Type' => 'application/jwt'], $userinfoResponseData); + + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: Only RS256 signature validation is supported. Token reports using ZZ512'); + } + + public function test_userinfo_endpoint_response_with_invalid_content_type_throws() + { + $userinfoResponse = new Response(200, ['Content-Type' => 'application/beans'], json_encode(OidcJwtHelper::defaultPayload())); + $resp = $this->runLogin(['name' => null], [$userinfoResponse]); + $resp->assertRedirect('/login'); + $this->assertSessionError('Userinfo endpoint response validation failed with error: No valid subject value found in userinfo data'); + } + protected function withAutodiscovery(): void { config()->set([ From 8b14a701a4792ecc0f8dc500d78e5810597eb4ac Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 19 Apr 2024 16:43:51 +0100 Subject: [PATCH 9/9] OIDC Userinfo: Fixed issues with validation logic from changes Also updated test to suit validation changes --- app/Access/Oidc/OidcIdToken.php | 2 +- app/Access/Oidc/OidcJwtWithClaims.php | 8 ++++---- app/Access/Oidc/OidcService.php | 2 +- app/Access/Oidc/OidcUserinfoResponse.php | 4 ++-- tests/Unit/OidcIdTokenTest.php | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/Access/Oidc/OidcIdToken.php b/app/Access/Oidc/OidcIdToken.php index f3fad0e1e..68a8aa611 100644 --- a/app/Access/Oidc/OidcIdToken.php +++ b/app/Access/Oidc/OidcIdToken.php @@ -11,7 +11,7 @@ class OidcIdToken extends OidcJwtWithClaims implements ProvidesClaims */ public function validate(string $clientId): bool { - parent::validateCommonClaims(); + parent::validateCommonTokenDetails($clientId); $this->validateTokenClaims($clientId); return true; diff --git a/app/Access/Oidc/OidcJwtWithClaims.php b/app/Access/Oidc/OidcJwtWithClaims.php index 393ac5f0e..06c04d81e 100644 --- a/app/Access/Oidc/OidcJwtWithClaims.php +++ b/app/Access/Oidc/OidcJwtWithClaims.php @@ -59,11 +59,11 @@ class OidcJwtWithClaims implements ProvidesClaims * * @throws OidcInvalidTokenException */ - public function validateCommonTokenDetails(): bool + public function validateCommonTokenDetails(string $clientId): bool { $this->validateTokenStructure(); $this->validateTokenSignature(); - $this->validateCommonClaims(); + $this->validateCommonClaims($clientId); return true; } @@ -151,7 +151,7 @@ class OidcJwtWithClaims implements ProvidesClaims * * @throws OidcInvalidTokenException */ - protected function validateCommonClaims(): void + protected function validateCommonClaims(string $clientId): void { // 1. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) // MUST exactly match the value of the iss (issuer) Claim. @@ -167,7 +167,7 @@ class OidcJwtWithClaims implements ProvidesClaims } $aud = is_string($this->payload['aud']) ? [$this->payload['aud']] : $this->payload['aud']; - if (!in_array($this->payload['aud'], $aud, true)) { + if (!in_array($clientId, $aud, true)) { throw new OidcInvalidTokenException('Token audience value did not match the expected client_id'); } } diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 6bb326e4b..7c1760649 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -253,7 +253,7 @@ class OidcService ); try { - $response->validate($idToken->getClaim('sub')); + $response->validate($idToken->getClaim('sub'), $settings->clientId); } catch (OidcInvalidTokenException $exception) { throw new OidcException("Userinfo endpoint response validation failed with error: {$exception->getMessage()}"); } diff --git a/app/Access/Oidc/OidcUserinfoResponse.php b/app/Access/Oidc/OidcUserinfoResponse.php index 0026d2f0a..9aded654e 100644 --- a/app/Access/Oidc/OidcUserinfoResponse.php +++ b/app/Access/Oidc/OidcUserinfoResponse.php @@ -25,10 +25,10 @@ class OidcUserinfoResponse implements ProvidesClaims /** * @throws OidcInvalidTokenException */ - public function validate(string $idTokenSub): bool + public function validate(string $idTokenSub, string $clientId): bool { if (!is_null($this->jwt)) { - $this->jwt->validateCommonTokenDetails(); + $this->jwt->validateCommonTokenDetails($clientId); } $sub = $this->getClaim('sub'); diff --git a/tests/Unit/OidcIdTokenTest.php b/tests/Unit/OidcIdTokenTest.php index 6302f84c7..739323266 100644 --- a/tests/Unit/OidcIdTokenTest.php +++ b/tests/Unit/OidcIdTokenTest.php @@ -113,7 +113,7 @@ class OidcIdTokenTest extends TestCase // 2. aud claim present ['Missing token audience value', ['aud' => null]], // 2. aud claim validates all values against those expected (Only expect single) - ['Token audience value has 2 values, Expected 1', ['aud' => ['abc', 'def']]], + ['Token audience value has 2 values, Expected 1', ['aud' => ['xxyyzz.aaa.bbccdd.123', 'def']]], // 2. aud claim matches client id ['Token audience value did not match the expected client_id', ['aud' => 'xxyyzz.aaa.bbccdd.456']], // 4. azp claim matches client id if present