From 90011f59584820465aa92b213a2a11531ccc20cf Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Thu, 25 May 2023 15:13:40 +0200 Subject: [PATCH] [Security] Validate `aud` and `iss` claims on OidcTokenHandler --- .../AccessToken/OidcTokenHandlerFactory.php | 17 +++++++++--- .../Resources/config/schema/security-1.0.xsd | 12 ++++++++- .../security_authenticator_access_token.php | 5 ++-- .../Tests/Functional/AccessTokenTest.php | 2 +- .../app/AccessToken/config_oidc.yml | 1 + .../AccessToken/Oidc/OidcTokenHandler.php | 12 ++++----- .../AccessToken/Oidc/OidcTokenHandlerTest.php | 27 +++++++++---------- 7 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/AccessToken/OidcTokenHandlerFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/AccessToken/OidcTokenHandlerFactory.php index 86d2783cb8d78..18a5ee6ac3eef 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/AccessToken/OidcTokenHandlerFactory.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/AccessToken/OidcTokenHandlerFactory.php @@ -28,8 +28,9 @@ class OidcTokenHandlerFactory implements TokenHandlerFactoryInterface public function create(ContainerBuilder $container, string $id, array|string $config): void { $tokenHandlerDefinition = $container->setDefinition($id, (new ChildDefinition('security.access_token_handler.oidc')) + ->replaceArgument(2, $config['audience']) + ->replaceArgument(3, $config['issuers']) ->replaceArgument(4, $config['claim']) - ->replaceArgument(5, $config['audience']) ); if (!ContainerBuilder::willBeAvailable('web-token/jwt-core', Algorithm::class, ['symfony/security-bundle'])) { @@ -39,11 +40,14 @@ public function create(ContainerBuilder $container, string $id, array|string $co ->addError('You cannot use the "oidc" token handler since "web-token/jwt-core" is not installed. Try running "web-token/jwt-core".'); } + // @see Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\SignatureAlgorithmFactory + // for supported algorithms if (\in_array($config['algorithm'], ['ES256', 'ES384', 'ES512'], true)) { $tokenHandlerDefinition->replaceArgument(0, new Reference('security.access_token_handler.oidc.signature.'.$config['algorithm'])); } else { - $tokenHandlerDefinition->replaceArgument(0, (new ChildDefinition('security.access_token_handler.oidc.signature'))) - ->replaceArgument(0, $config['algorithm']); + $tokenHandlerDefinition->replaceArgument(0, (new ChildDefinition('security.access_token_handler.oidc.signature')) + ->replaceArgument(0, $config['algorithm']) + ); } $tokenHandlerDefinition->replaceArgument(1, (new ChildDefinition('security.access_token_handler.oidc.jwk')) @@ -68,7 +72,12 @@ public function addConfiguration(NodeBuilder $node): void ->end() ->scalarNode('audience') ->info('Audience set in the token, for validation purpose.') - ->defaultNull() + ->isRequired() + ->end() + ->arrayNode('issuers') + ->info('Issuers allowed to generate the token, for validation purpose.') + ->isRequired() + ->prototype('scalar')->end() ->end() ->scalarNode('algorithm') ->info('Algorithm used to sign the token.') diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/schema/security-1.0.xsd b/src/Symfony/Bundle/SecurityBundle/Resources/config/schema/security-1.0.xsd index 7798be2a28632..ab90f4b42606c 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/schema/security-1.0.xsd +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/schema/security-1.0.xsd @@ -335,12 +335,22 @@ + + + + - + + + + + + + diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php index 32eac0e580c57..3254f332aace0 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php @@ -69,10 +69,11 @@ ->args([ abstract_arg('signature algorithm'), abstract_arg('signature key'), + abstract_arg('audience'), + abstract_arg('issuers'), + 'sub', service('logger')->nullOnInvalid(), service('clock'), - 'sub', - null, ]) ->set('security.access_token_handler.oidc.jwk', JWK::class) diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AccessTokenTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AccessTokenTest.php index 8ca49ccc1430c..3deb91402165e 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AccessTokenTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AccessTokenTest.php @@ -343,7 +343,7 @@ public function testOidcSuccess() 'iat' => $time, 'nbf' => $time, 'exp' => $time + 3600, - 'iss' => 'https://www.example.com/', + 'iss' => 'https://www.example.com', 'aud' => 'Symfony OIDC', 'sub' => 'e21bf182-1538-406e-8ccb-e25a17aba39f', 'username' => 'dunglas', diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AccessToken/config_oidc.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AccessToken/config_oidc.yml index 920bc81f1e788..dd770e4520e41 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AccessToken/config_oidc.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AccessToken/config_oidc.yml @@ -23,6 +23,7 @@ security: oidc: claim: 'username' audience: 'Symfony OIDC' + issuers: [ 'https://www.example.com' ] algorithm: 'ES256' # tip: use https://mkjwk.org/ to generate a JWK key: '{"kty":"EC","d":"iA_TV2zvftni_9aFAQwFO_9aypfJFCSpcCyevDvz220","crv":"P-256","x":"0QEAsI1wGI-dmYatdUZoWSRWggLEpyzopuhwk-YUnA4","y":"KYl-qyZ26HobuYwlQh-r0iHX61thfP82qqEku7i0woo"}' diff --git a/src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php b/src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php index c103236ed2021..0623a60016a0d 100644 --- a/src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php +++ b/src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php @@ -41,10 +41,11 @@ final class OidcTokenHandler implements AccessTokenHandlerInterface public function __construct( private Algorithm $signatureAlgorithm, private JWK $jwk, - private ?LoggerInterface $logger = null, - private ClockInterface $clock = new Clock(), + private string $audience, + private array $issuers, private string $claim = 'sub', - private ?string $audience = null + private ?LoggerInterface $logger = null, + private ClockInterface $clock = new Clock() ) { } @@ -80,10 +81,9 @@ public function getUserBadgeFrom(string $accessToken): UserBadge new Checker\IssuedAtChecker(0, false, $this->clock), new Checker\NotBeforeChecker(0, false, $this->clock), new Checker\ExpirationTimeChecker(0, false, $this->clock), + new Checker\AudienceChecker($this->audience), + new Checker\IssuerChecker($this->issuers), ]; - if ($this->audience) { - $checkers[] = new Checker\AudienceChecker($this->audience); - } $claimCheckerManager = new ClaimCheckerManager($checkers); // if this check fails, an InvalidClaimException is thrown $claimCheckerManager->check($claims); diff --git a/src/Symfony/Component/Security/Http/Tests/AccessToken/Oidc/OidcTokenHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/AccessToken/Oidc/OidcTokenHandlerTest.php index 2d35bfb8aede3..8c8d86284b59a 100644 --- a/src/Symfony/Component/Security/Http/Tests/AccessToken/Oidc/OidcTokenHandlerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/AccessToken/Oidc/OidcTokenHandlerTest.php @@ -18,7 +18,6 @@ use Jose\Component\Signature\Serializer\CompactSerializer; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; -use Symfony\Component\Clock\Clock; use Symfony\Component\Security\Core\Exception\BadCredentialsException; use Symfony\Component\Security\Core\User\OidcUser; use Symfony\Component\Security\Http\AccessToken\Oidc\OidcTokenHandler; @@ -41,7 +40,7 @@ public function testGetsUserIdentifierFromSignedToken(string $claim, string $exp 'iat' => $time, 'nbf' => $time, 'exp' => $time + 3600, - 'iss' => 'https://www.example.com/', + 'iss' => 'https://www.example.com', 'aud' => self::AUDIENCE, 'sub' => 'e21bf182-1538-406e-8ccb-e25a17aba39f', 'email' => 'foo@example.com', @@ -55,10 +54,10 @@ public function testGetsUserIdentifierFromSignedToken(string $claim, string $exp $userBadge = (new OidcTokenHandler( new ES256(), $this->getJWK(), - $loggerMock, - new Clock(), + self::AUDIENCE, + ['https://www.example.com'], $claim, - self::AUDIENCE + $loggerMock, ))->getUserBadgeFrom($token); $actualUser = $userBadge->getUserLoader()(); @@ -89,10 +88,10 @@ public function testThrowsAnErrorIfTokenIsInvalid(string $token) (new OidcTokenHandler( new ES256(), $this->getJWK(), - $loggerMock, - new Clock(), + self::AUDIENCE, + ['https://www.example.com'], 'sub', - self::AUDIENCE + $loggerMock, ))->getUserBadgeFrom($token); } @@ -106,7 +105,7 @@ public static function getInvalidTokens(): iterable 'iat' => time() - 3600, 'nbf' => time() - 3600, 'exp' => time() - 3590, - 'iss' => 'https://www.example.com/', + 'iss' => 'https://www.example.com', 'aud' => self::AUDIENCE, 'sub' => 'e21bf182-1538-406e-8ccb-e25a17aba39f', 'email' => 'foo@example.com', @@ -118,7 +117,7 @@ public static function getInvalidTokens(): iterable 'iat' => time(), 'nbf' => time(), 'exp' => time() + 3590, - 'iss' => 'https://www.example.com/', + 'iss' => 'https://www.example.com', 'aud' => 'invalid', 'sub' => 'e21bf182-1538-406e-8ccb-e25a17aba39f', 'email' => 'foo@example.com', @@ -139,7 +138,7 @@ public function testThrowsAnErrorIfUserPropertyIsMissing() 'iat' => $time, 'nbf' => $time, 'exp' => $time + 3600, - 'iss' => 'https://www.example.com/', + 'iss' => 'https://www.example.com', 'aud' => self::AUDIENCE, 'sub' => 'e21bf182-1538-406e-8ccb-e25a17aba39f', ]; @@ -148,10 +147,10 @@ public function testThrowsAnErrorIfUserPropertyIsMissing() (new OidcTokenHandler( new ES256(), self::getJWK(), - $loggerMock, - new Clock(), + self::AUDIENCE, + ['https://www.example.com'], 'email', - self::AUDIENCE + $loggerMock, ))->getUserBadgeFrom($token); }