diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php index 654ed692880..b9cd57243b4 100644 --- a/app/Access/Controllers/OidcController.php +++ b/app/Access/Controllers/OidcController.php @@ -55,6 +55,7 @@ public function callback(Request $request) } try { + $this->throwIfAuthorizationError($request); $this->oidcService->processAuthorizeResponse($request->query('code')); } catch (OidcException $oidcException) { $this->showErrorNotification($oidcException->getMessage()); @@ -72,4 +73,23 @@ public function logout() { return redirect($this->oidcService->logout()); } + + /** + * + * @throws OidcException + */ + private function throwIfAuthorizationError(Request $request): void + { + $errorCode = $request->query('error'); + if (!$errorCode) { + return; + } + + $errorDescription = $request->query('error_description'); + if ($errorDescription) { + throw new OidcException($errorDescription); + } + + throw new OidcException(trans('errors.oidc_fail_authed', ['system' => config('oidc.name')])); + } } diff --git a/app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php b/app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php new file mode 100644 index 00000000000..b5aca63f31b --- /dev/null +++ b/app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php @@ -0,0 +1,27 @@ +validateTokenClaims($clientId); + parent::validateCommonTokenDetails($settings); + $this->validateTokenClaims($settings->clientId); return true; } diff --git a/app/Access/Oidc/OidcJwtWithClaims.php b/app/Access/Oidc/OidcJwtWithClaims.php index 9d7eeead1a9..b3da04ee516 100644 --- a/app/Access/Oidc/OidcJwtWithClaims.php +++ b/app/Access/Oidc/OidcJwtWithClaims.php @@ -9,7 +9,9 @@ class OidcJwtWithClaims implements ProvidesClaims protected string $signature; protected string $issuer; protected array $tokenParts = []; - + protected array $acceptedSignatures = [self::hs256Signature, self::rs256Signature]; + private const hs256Signature = 'HS256' + , rs256Signature = 'RS256'; /** * @var array[]|string[] */ @@ -59,11 +61,11 @@ protected function base64UrlDecode(string $encoded): string * * @throws OidcInvalidTokenException */ - public function validateCommonTokenDetails(string $clientId): bool + public function validateCommonTokenDetails(OidcProviderSettings $settings): bool { $this->validateTokenStructure(); - $this->validateTokenSignature(); - $this->validateCommonClaims($clientId); + $this->validateTokenSignature($settings); + $this->validateCommonClaims($settings->clientId); return true; } @@ -102,12 +104,12 @@ public function replaceClaims(array $claims): void protected function validateTokenStructure(): void { foreach (['header', 'payload'] as $prop) { - if (empty($this->$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)) { + if (empty($this->signature) || !is_string($this->signature)) { throw new OidcInvalidTokenException('Could not parse out a valid signature within the provided token'); } } @@ -117,31 +119,42 @@ protected function validateTokenStructure(): void * * @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']}"); + protected function validateTokenSignature(OidcProviderSettings $settings): void { + $validSignatures = implode(', ',$this->acceptedSignatures); + switch ($this->header['alg']) { + case self::rs256Signature: + $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'); + case self::hs256Signature: + $secret = $settings->clientSecret; + $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1]; + $expectedSignature = hash_hmac('sha256', $contentToSign, $secret, true); + + if (hash_equals($expectedSignature, $this->signature)) { + return; + } + + throw new OidcInvalidTokenException('Token signature could not be validated using the provided secret'); + default: + throw new OidcInvalidTokenException("Only $validSignatures signatures validation are 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'); } /** diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index 71c3b573421..573d2beefe5 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -74,7 +74,7 @@ public function validate(): void { $this->validateInitial(); - $required = ['keys', 'tokenEndpoint', 'authorizationEndpoint']; + $required = ['tokenEndpoint', 'authorizationEndpoint']; 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 a84bd320513..60eb0a4d1fd 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -14,7 +14,6 @@ use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\User; use Illuminate\Support\Facades\Cache; -use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider; use League\OAuth2\Client\Provider\Exception\IdentityProviderException; /** @@ -140,7 +139,7 @@ protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvide 'redirectUri' => url('/oidc/callback'), ], [ 'httpClient' => $this->http->buildClient(5), - 'optionProvider' => new HttpBasicAuthOptionProvider(), + 'optionProvider' => new OidcHttpBasicWithClientIdOptionProvider(), ]); foreach ($this->getAdditionalScopes() as $scope) { @@ -199,7 +198,7 @@ protected function processAccessTokenCallback(OidcAccessToken $accessToken, Oidc } try { - $idToken->validate($settings->clientId); + $idToken->validate($settings); } catch (OidcInvalidTokenException $exception) { throw new OidcException("ID token validation failed with error: {$exception->getMessage()}"); } diff --git a/tests/Helpers/OidcJwtHelper.php b/tests/Helpers/OidcJwtHelper.php index 55a34d4dc96..97841a68896 100644 --- a/tests/Helpers/OidcJwtHelper.php +++ b/tests/Helpers/OidcJwtHelper.php @@ -2,6 +2,7 @@ namespace Tests\Helpers; +use BookStack\Access\Oidc\OidcProviderSettings; use phpseclib3\Crypt\RSA; /** @@ -119,6 +120,41 @@ public static function privatePemKey(): string -----END PRIVATE KEY-----'; } + public static function defaultSecret(): string + { + return 'test-client-secret-for-hs256'; + } + + /** + * Build a minimal OidcProviderSettings for use in token validation tests. + */ + public static function defaultProviderSettings(array $overrides = []): OidcProviderSettings + { + return new OidcProviderSettings(array_merge([ + 'issuer' => static::defaultIssuer(), + 'clientId' => static::defaultClientId(), + 'clientSecret' => static::defaultSecret(), + ], $overrides)); + } + + /** + * Build an HS256-signed ID token using the given secret. + */ + public static function hs256IdToken(string $secret, array $payloadOverrides = []): string + { + $payload = array_merge(static::defaultPayload(), $payloadOverrides); + $header = ['alg' => 'HS256', 'typ' => 'JWT']; + + $top = implode('.', [ + static::base64UrlEncode(json_encode($header)), + static::base64UrlEncode(json_encode($payload)), + ]); + + $signature = hash_hmac('sha256', $top, $secret, true); + + return $top . '.' . static::base64UrlEncode($signature); + } + public static function publicJwkKeyArray(): array { return [ diff --git a/tests/Unit/OidcIdTokenTest.php b/tests/Unit/OidcIdTokenTest.php index 73932326694..0eda9e937f9 100644 --- a/tests/Unit/OidcIdTokenTest.php +++ b/tests/Unit/OidcIdTokenTest.php @@ -15,7 +15,7 @@ public function test_valid_token_passes_validation() OidcJwtHelper::publicJwkKeyArray(), ]); - $this->assertTrue($token->validate('xxyyzz.aaa.bbccdd.123')); + $this->assertTrue($token->validate(OidcJwtHelper::defaultProviderSettings())); } public function test_get_claim_returns_value_if_existing() @@ -56,7 +56,7 @@ public function test_token_structure_error_cases() $err = null; try { - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } catch (\Exception $exception) { $err = $exception; } @@ -71,7 +71,7 @@ public function test_error_thrown_if_token_signature_not_validated_from_no_keys( $token = new OidcIdToken(OidcJwtHelper::idToken(), OidcJwtHelper::defaultIssuer(), []); $this->expectException(OidcInvalidTokenException::class); $this->expectExceptionMessage('Token signature could not be validated using the provided keys'); - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } public function test_error_thrown_if_token_signature_not_validated_from_non_matching_key() @@ -83,7 +83,7 @@ public function test_error_thrown_if_token_signature_not_validated_from_non_matc ]); $this->expectException(OidcInvalidTokenException::class); $this->expectExceptionMessage('Token signature could not be validated using the provided keys'); - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } public function test_error_thrown_if_invalid_key_provided() @@ -91,15 +91,34 @@ public function test_error_thrown_if_invalid_key_provided() $token = new OidcIdToken(OidcJwtHelper::idToken(), OidcJwtHelper::defaultIssuer(), ['url://example.com']); $this->expectException(OidcInvalidTokenException::class); $this->expectExceptionMessage('Unexpected type of key value provided'); - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } - public function test_error_thrown_if_token_algorithm_is_not_rs256() + public function test_error_thrown_if_token_algorithm_is_not_supported() { - $token = new OidcIdToken(OidcJwtHelper::idToken([], ['alg' => 'HS256']), OidcJwtHelper::defaultIssuer(), []); + $token = new OidcIdToken(OidcJwtHelper::idToken([], ['alg' => 'ES256']), OidcJwtHelper::defaultIssuer(), []); $this->expectException(OidcInvalidTokenException::class); - $this->expectExceptionMessage('Only RS256 signature validation is supported. Token reports using HS256'); - $token->validate('abc'); + $this->expectExceptionMessage('Only HS256, RS256 signatures validation are supported. Token reports using ES256'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); + } + + public function test_hs256_token_passes_validation_with_correct_secret() + { + $secret = OidcJwtHelper::defaultSecret(); + $token = new OidcIdToken(OidcJwtHelper::hs256IdToken($secret), OidcJwtHelper::defaultIssuer(), []); + $settings = OidcJwtHelper::defaultProviderSettings(['clientSecret' => $secret]); + + $this->assertTrue($token->validate($settings)); + } + + public function test_hs256_token_fails_validation_with_wrong_secret() + { + $token = new OidcIdToken(OidcJwtHelper::hs256IdToken('correct-secret'), OidcJwtHelper::defaultIssuer(), []); + $settings = OidcJwtHelper::defaultProviderSettings(['clientSecret' => 'wrong-secret']); + + $this->expectException(OidcInvalidTokenException::class); + $this->expectExceptionMessage('Token signature could not be validated using the provided secret'); + $token->validate($settings); } public function test_token_claim_error_cases() @@ -141,7 +160,7 @@ public function test_token_claim_error_cases() $err = null; try { - $token->validate('xxyyzz.aaa.bbccdd.123'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } catch (\Exception $exception) { $err = $exception; } @@ -160,7 +179,7 @@ public function test_keys_can_be_a_local_file_reference_to_pem_key() $testFilePath, ]); - $this->assertTrue($token->validate('xxyyzz.aaa.bbccdd.123')); + $this->assertTrue($token->validate(OidcJwtHelper::defaultProviderSettings())); unlink($testFilePath); } }