From 4d1d474f50fddb194478a195ca4064d4841fb0f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Wed, 18 Sep 2024 18:58:37 +0200 Subject: [PATCH] Introduce Protocol Cache for Client Verifier token reuse check --- routing/services/services.yml | 2 + src/Factories/CacheFactory.php | 62 ++++++++++++++----- src/Factories/RequestRulesManagerFactory.php | 10 ++- src/ModuleConfig.php | 10 +++ .../Rules/ClientAuthenticationRule.php | 24 ++++++- .../RequestRules/Rules/ClientIdRule.php | 2 +- src/Services/Container.php | 11 +++- src/Utils/ProtocolCache.php | 11 ++++ ...RuleTest.php => RequestObjectRuleTest.php} | 2 +- 9 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 src/Utils/ProtocolCache.php rename tests/unit/src/Server/RequestRules/Rules/{RequestParameterRuleTest.php => RequestObjectRuleTest.php} (99%) diff --git a/routing/services/services.yml b/routing/services/services.yml index c1f01a6c..031398e7 100644 --- a/routing/services/services.yml +++ b/routing/services/services.yml @@ -90,6 +90,8 @@ services: factory: ['@SimpleSAML\Module\oidc\Factories\ClaimTranslatorExtractorFactory', 'build'] SimpleSAML\Module\oidc\Utils\FederationCache: factory: ['@SimpleSAML\Module\oidc\Factories\CacheFactory', 'forFederation'] # Can return null + SimpleSAML\Module\oidc\Utils\ProtocolCache: + factory: ['@SimpleSAML\Module\oidc\Factories\CacheFactory', 'forProtocol'] # Can return null # Use (already available) Laminas\Diactoros package as PSR HTTP Factories. Laminas\Diactoros\ServerRequestFactory: ~ diff --git a/src/Factories/CacheFactory.php b/src/Factories/CacheFactory.php index 527f789b..dafe05a8 100644 --- a/src/Factories/CacheFactory.php +++ b/src/Factories/CacheFactory.php @@ -9,6 +9,7 @@ use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\ClassInstanceBuilder; use SimpleSAML\Module\oidc\Utils\FederationCache; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\Psr16Cache; @@ -24,31 +25,62 @@ public function __construct( /** * @throws \SimpleSAML\Module\oidc\OidcException */ - public function forFederation(): ?FederationCache - { - $class = $this->moduleConfig->getFederationCacheAdapterClass(); - - if (is_null($class)) { - return null; - } - + protected function buildAdapterInstance( + string $class, + array $args = [], + ): AdapterInterface { try { - $instance = $this->classInstanceBuilder->build( - $class, - $this->moduleConfig->getFederationCacheAdapterArguments(), - ); + $instance = $this->classInstanceBuilder->build($class, $args); } catch (\Throwable $exception) { - $message = "Error building federation cache instance: " . $exception->getMessage(); + $message = "Error building cache adapter instance: " . $exception->getMessage(); $this->loggerService->error($message); throw new OidcException($message); } if (!is_a($instance, AdapterInterface::class)) { - $message = "Unexpected federation cache adapter class: $class. Expected type: " . AdapterInterface::class; + $message = "Unexpected cache adapter class: $class. Expected type: " . AdapterInterface::class; $this->loggerService->error($message); throw new OidcException($message); } - return new FederationCache(new Psr16Cache($instance)); + return $instance; + } + + /** + * @throws \SimpleSAML\Module\oidc\OidcException + */ + public function forFederation(): ?FederationCache + { + $class = $this->moduleConfig->getFederationCacheAdapterClass(); + + if (is_null($class)) { + return null; + } + + $adapter = $this->buildAdapterInstance( + $class, + $this->moduleConfig->getFederationCacheAdapterArguments(), + ); + + return new FederationCache(new Psr16Cache($adapter)); + } + + /** + * @throws \SimpleSAML\Module\oidc\OidcException + */ + public function forProtocol(): ?ProtocolCache + { + $class = $this->moduleConfig->getProtocolCacheAdapterClass(); + + if (is_null($class)) { + return null; + } + + $adapter = $this->buildAdapterInstance( + $class, + $this->moduleConfig->getProtocolCacheAdapterArguments(), + ); + + return new ProtocolCache(new Psr16Cache($adapter)); } } diff --git a/src/Factories/RequestRulesManagerFactory.php b/src/Factories/RequestRulesManagerFactory.php index 4bab6d0d..4a95962e 100644 --- a/src/Factories/RequestRulesManagerFactory.php +++ b/src/Factories/RequestRulesManagerFactory.php @@ -36,6 +36,7 @@ use SimpleSAML\Module\oidc\Utils\ClaimTranslatorExtractor; use SimpleSAML\Module\oidc\Utils\FederationCache; use SimpleSAML\Module\oidc\Utils\JwksResolver; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Federation; @@ -57,6 +58,7 @@ public function __construct( private readonly Helpers $helpers, private readonly JwksResolver $jwksResolver, private readonly ?FederationCache $federationCache = null, + private readonly ?ProtocolCache $protocolCache = null, ) { } @@ -104,7 +106,13 @@ private function getDefaultRules(): array new UiLocalesRule($this->requestParamsResolver), new AcrValuesRule($this->requestParamsResolver), new ScopeOfflineAccessRule($this->requestParamsResolver), - new ClientAuthenticationRule($this->requestParamsResolver, $this->moduleConfig, $this->jwksResolver), + new ClientAuthenticationRule( + $this->requestParamsResolver, + $this->moduleConfig, + $this->jwksResolver, + $this->helpers, + $this->protocolCache, + ), new CodeVerifierRule($this->requestParamsResolver), ]; } diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 9220194c..bc014702 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -606,4 +606,14 @@ public function getTrustAnchorJwks(string $trustAnchorId): ?array sprintf('Unexpected JWKS format for Trust Anchor %s: %s', $trustAnchorId, var_export($jwks, true)), ); } + + public function getProtocolCacheAdapterClass(): ?string + { + return $this->config()->getOptionalString(self::OPTION_PROTOCOL_CACHE_ADAPTER, null); + } + + public function getProtocolCacheAdapterArguments(): array + { + return $this->config()->getOptionalArray(self::OPTION_PROTOCOL_CACHE_ADAPTER_ARGUMENTS, []); + } } diff --git a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php index 9cd6307c..b7ab74cb 100644 --- a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php +++ b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php @@ -6,6 +6,7 @@ use Psr\Http\Message\ServerRequestInterface; use SimpleSAML\Module\oidc\Codebooks\RoutesEnum; +use SimpleSAML\Module\oidc\Helpers; use SimpleSAML\Module\oidc\ModuleConfig; use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; use SimpleSAML\Module\oidc\Server\RequestRules\Interfaces\ResultBagInterface; @@ -13,6 +14,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\Result; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\JwksResolver; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Codebooks\ClientAssertionTypesEnum; use SimpleSAML\OpenID\Codebooks\HttpMethodsEnum; @@ -20,10 +22,14 @@ class ClientAuthenticationRule extends AbstractRule { + protected const KEY_CLIENT_ASSERTION_JTI = 'client_assertion_jti'; + public function __construct( RequestParamsResolver $requestParamsResolver, protected ModuleConfig $moduleConfig, protected JwksResolver $jwksResolver, + protected Helpers $helpers, + protected ?ProtocolCache $protocolCache, ) { parent::__construct($requestParamsResolver); } @@ -87,8 +93,14 @@ public function checkRule( $clientAssertion = $this->requestParamsResolver->parseClientAssertionToken($clientAssertionParam); - /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */ - $client = $currentResultBag->getOrFail(ClientIdRule::class)->getValue(); + // Check if the Client Assertion token has already been used. Only applicable if we have cache available. + if ($this->protocolCache) { + ($this->protocolCache->has(self::KEY_CLIENT_ASSERTION_JTI, $clientAssertion->getJwtId()) === false) + || throw OidcServerException::invalidRequest( + ParamsEnum::ClientAssertion->value, + 'Client Assertion reused.', + ); + } ($jwks = $this->jwksResolver->forClient($client)) || throw OidcServerException::accessDenied( 'Can not validate Client Assertion, client JWKS not available.', @@ -122,6 +134,14 @@ public function checkRule( (!empty(array_intersect($expectedAudience, $clientAssertion->getAudience()))) || throw OidcServerException::accessDenied('Invalid Client Assertion Audience claim.'); + // Everything seems ok. Save it in cache so we can check for reuse. + $this->protocolCache?->set( + $clientAssertion->getJwtId(), + $this->helpers->dateTime()->getSecondsToExpirationTime($clientAssertion->getExpirationTime()), + self::KEY_CLIENT_ASSERTION_JTI, + $clientAssertion->getJwtId(), + ); + return new Result($this->getKey(), ParamsEnum::ClientAssertion->value); } } diff --git a/src/Server/RequestRules/Rules/ClientIdRule.php b/src/Server/RequestRules/Rules/ClientIdRule.php index c2dc57a6..814fe369 100644 --- a/src/Server/RequestRules/Rules/ClientIdRule.php +++ b/src/Server/RequestRules/Rules/ClientIdRule.php @@ -109,7 +109,7 @@ public function checkRule( // Check for reuse of the Request Object. Request Object MUST only be used once (by OpenID Federation spec). if ($this->federationCache) { ($this->federationCache->has(self::KEY_REQUEST_OBJECT_JTI, $requestObject->getJwtId()) === false) - || throw OidcServerException::invalidRequest(ParamsEnum::Request->value, 'Request object reused.'); + || throw OidcServerException::invalidRequest(ParamsEnum::Request->value, 'Request Object reused.'); } $clientEntityId = $requestObject->getIssuer(); diff --git a/src/Services/Container.php b/src/Services/Container.php index afe47db8..c3f0d4bb 100644 --- a/src/Services/Container.php +++ b/src/Services/Container.php @@ -96,6 +96,7 @@ use SimpleSAML\Module\oidc\Utils\ClassInstanceBuilder; use SimpleSAML\Module\oidc\Utils\FederationCache; use SimpleSAML\Module\oidc\Utils\JwksResolver; +use SimpleSAML\Module\oidc\Utils\ProtocolCache; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Core; use SimpleSAML\OpenID\Federation; @@ -202,6 +203,8 @@ public function __construct() $this->services[CacheFactory::class] = $cacheFactory; $federationCache = $cacheFactory->forFederation(); $this->services[FederationCache::class] = $federationCache; + $protocolCache = $cacheFactory->forProtocol(); + $this->services[ProtocolCache::class] = $protocolCache; $federationFactory = new FederationFactory($moduleConfig, $loggerService, $federationCache); $this->services[FederationFactory::class] = $federationFactory; $federation = $federationFactory->build(); @@ -279,7 +282,13 @@ public function __construct() new UiLocalesRule($requestParamsResolver), new AcrValuesRule($requestParamsResolver), new ScopeOfflineAccessRule($requestParamsResolver), - new ClientAuthenticationRule($requestParamsResolver, $moduleConfig, $jwksResolver), + new ClientAuthenticationRule( + $requestParamsResolver, + $moduleConfig, + $jwksResolver, + $helpers, + $protocolCache, + ), new CodeVerifierRule($requestParamsResolver), ]; $requestRuleManager = new RequestRulesManager($requestRules, $loggerService); diff --git a/src/Utils/ProtocolCache.php b/src/Utils/ProtocolCache.php new file mode 100644 index 00000000..b449c832 --- /dev/null +++ b/src/Utils/ProtocolCache.php @@ -0,0 +1,11 @@ +