Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions app/Access/Controllers/OidcController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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')]));
}
}
27 changes: 27 additions & 0 deletions app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

namespace BookStack\Access\Oidc;

use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider;

/**
* Option provider that sends credentials via HTTP Basic Auth header
* and also includes client_id in the request body.
*/
class OidcHttpBasicWithClientIdOptionProvider extends HttpBasicAuthOptionProvider
{
public function getAccessTokenOptions($method, array $params)
{
$clientId = $params['client_id'] ?? null;

$options = parent::getAccessTokenOptions($method, $params);

if ($clientId) {
parse_str($options['body'] ?? '', $body);
$body['client_id'] = $clientId;
$options['body'] = http_build_query($body);
}

return $options;
}
}
6 changes: 3 additions & 3 deletions app/Access/Oidc/OidcIdToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ class OidcIdToken extends OidcJwtWithClaims implements ProvidesClaims
*
* @throws OidcInvalidTokenException
*/
public function validate(string $clientId): bool
public function validate(OidcProviderSettings $settings): bool
{
parent::validateCommonTokenDetails($clientId);
$this->validateTokenClaims($clientId);
parent::validateCommonTokenDetails($settings);
$this->validateTokenClaims($settings->clientId);

return true;
}
Expand Down
73 changes: 43 additions & 30 deletions app/Access/Oidc/OidcJwtWithClaims.php
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
*/
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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');
}
}
Expand All @@ -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');
}

/**
Expand Down
2 changes: 1 addition & 1 deletion app/Access/Oidc/OidcProviderSettings.php
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
5 changes: 2 additions & 3 deletions app/Access/Oidc/OidcService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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()}");
}
Expand Down
36 changes: 36 additions & 0 deletions tests/Helpers/OidcJwtHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Tests\Helpers;

use BookStack\Access\Oidc\OidcProviderSettings;
use phpseclib3\Crypt\RSA;

/**
Expand Down Expand Up @@ -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 [
Expand Down
41 changes: 30 additions & 11 deletions tests/Unit/OidcIdTokenTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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;
}
Expand All @@ -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()
Expand All @@ -83,23 +83,42 @@ 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()
{
$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()
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
}
}