diff --git a/config/module_oidc.php.dist b/config/module_oidc.php.dist index a83f02fe9..a3a487b60 100644 --- a/config/module_oidc.php.dist +++ b/config/module_oidc.php.dist @@ -256,6 +256,17 @@ $config = [ // claims for which "SAML attribute to OIDC claim translation" has been defined above. ModuleConfig::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED => false, + // Optional hardening, disabled by default. If enabled, a confidential client requesting the implicit flow + // (response_type 'id_token' or 'id_token token') is refused with the 'unauthorized_client' error, so it can only + // get tokens through the authorization code flow, where it authenticates at the token endpoint. Public clients + // are not affected, and a confidential client which uses the implicit flow today stops working once this is on. + // This goes beyond what the specifications require, as they do not forbid the flow for confidential clients. + // OIDC Core says the authorization code flow may be appropriate for them + // (https://openid.net/specs/openid-connect-core-1_0.html#RPMTI), and the OAuth 2.0 Security BCP advises clients + // against response types which issue access tokens in the authorization response, such as 'id_token token' + // (https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2). + ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS => false, + // Settings regarding Authentication Processing Filters. // Note: OIDC authN state array will not contain all the keys which are available during SAML authN, // like Service Provider metadata, etc. diff --git a/src/Factories/Grant/ImplicitGrantFactory.php b/src/Factories/Grant/ImplicitGrantFactory.php index a22791d84..d5c581761 100644 --- a/src/Factories/Grant/ImplicitGrantFactory.php +++ b/src/Factories/Grant/ImplicitGrantFactory.php @@ -45,6 +45,7 @@ public function build(): ImplicitGrant $this->requestParamsResolver, '#', $this->accessTokenEntityFactory, + $this->moduleConfig->getDisallowImplicitForConfidentialClients(), ); } } diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 2f42d0852..48d36a06a 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -96,6 +96,8 @@ class ModuleConfig final public const string OPTION_PROTOCOL_CLIENT_ENTITY_CACHE_DURATION = 'protocol_client_entity_cache_duration'; final public const string OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED = 'protocol_discover_show_claims_supported'; + final public const string OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS = + 'disallow_implicit_for_confidential_clients'; final public const string OPTION_PKI_NEW_PRIVATE_KEY_PASSPHRASE = 'new_private_key_passphrase'; final public const string OPTION_PKI_NEW_PRIVATE_KEY_FILENAME = 'new_privatekey'; @@ -527,6 +529,18 @@ public function getProtocolDiscoveryShowClaimsSupported(): bool ); } + /** + * Opt-in hardening: refuse implicit flow requests from confidential clients. See the comment on this option in + * config/module_oidc.php.dist. + */ + public function getDisallowImplicitForConfidentialClients(): bool + { + return $this->config()->getOptionalBoolean( + self::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS, + false, + ); + } + /***************************************************************************************************************** * OpenID Federation related config. diff --git a/src/Server/Exceptions/OidcServerException.php b/src/Server/Exceptions/OidcServerException.php index 5a9be60db..e8f9bc220 100644 --- a/src/Server/Exceptions/OidcServerException.php +++ b/src/Server/Exceptions/OidcServerException.php @@ -179,6 +179,31 @@ public static function accessDenied( return $e; } + /** + * The client is not authorized to use this response type (RFC 6749 section 4.2.2.1). + * + * @param string|null $hint + * @param string|null $redirectUri + * @param \Throwable|null $previous + * @param string|null $state + * @param bool $useFragment Use URI fragment to return error parameters + * @return self + */ + public static function unauthorizedClient( + ?string $hint = null, + ?string $redirectUri = null, + ?Throwable $previous = null, + ?string $state = null, + bool $useFragment = false, + ): OidcServerException { + $errorMessage = 'The client is not authorized to request a token using this method.'; + + $e = new self($errorMessage, 10, 'unauthorized_client', 400, $hint, $redirectUri, $previous, $state); + $e->useFragmentInHttpResponses($useFragment); + + return $e; + } + /** * Prompt none requires that user should be authenticated. * diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 4e2026bcc..753dd056e 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/src/Server/Grants/ImplicitGrant.php @@ -64,6 +64,7 @@ public function __construct( protected RequestParamsResolver $requestParamsResolver, protected string $queryDelimiter, AccessTokenEntityFactory $accessTokenEntityFactory, + protected bool $disallowImplicitForConfidentialClients = false, ) { parent::__construct($accessTokenTTL, $queryDelimiter); @@ -144,6 +145,18 @@ public function validateAuthorizationRequestWithRequestRules( /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */ $client = $resultBag->getOrFail(ClientIdRule::class)->getValue(); + // Opt-in hardening, see ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS. The implicit flow + // never authenticates the client, which a confidential client could do at the token endpoint. + if ($this->disallowImplicitForConfidentialClients && $client->isConfidential()) { + throw OidcServerException::unauthorizedClient( + 'Confidential clients are not allowed to use the implicit grant.', + $redirectUri, + null, + $state, + $this->shouldUseFragment(), + ); + } + // Some rules need certain things available in order to work properly... $this->requestRulesManager->setData('default_scope', $this->defaultScope); $this->requestRulesManager->setData('scope_delimiter_string', self::SCOPE_DELIMITER_STRING); diff --git a/src/Server/Grants/RefreshTokenGrant.php b/src/Server/Grants/RefreshTokenGrant.php index ec7b58d1a..5bcfd3f0d 100644 --- a/src/Server/Grants/RefreshTokenGrant.php +++ b/src/Server/Grants/RefreshTokenGrant.php @@ -103,7 +103,7 @@ protected function validateOldRefreshToken(ServerRequestInterface $request, $cli protected function issueRefreshToken( OAuth2AccessTokenEntityInterface $accessToken, - string $authCodeId = null, + ?string $authCodeId = null, ): ?RefreshTokenEntityInterface { if (! is_a($accessToken, AccessTokenEntityInterface::class)) { throw OidcServerException::serverError('Unexpected access token entity type.'); diff --git a/tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php b/tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php new file mode 100644 index 000000000..579962c40 --- /dev/null +++ b/tests/unit/src/Factories/Grant/ImplicitGrantFactoryTest.php @@ -0,0 +1,93 @@ +moduleConfigMock = $this->createMock(ModuleConfig::class); + $this->moduleConfigMock->method('getAccessTokenDuration')->willReturn(new DateInterval('PT1H')); + $this->requestRulesManagerMock = $this->createMock(RequestRulesManager::class); + } + + + protected function buildAndValidateForConfidentialClient(): void + { + $clientEntityMock = $this->createMock(ClientEntity::class); + $clientEntityMock->method('isConfidential')->willReturn(true); + + $resultBag = new ResultBag(); + $resultBag->add(new Result(RedirectUriRule::class, 'https://example.com/callback')); + $resultBag->add(new Result(StateRule::class, 'test-state')); + $resultBag->add(new Result(ClientIdRule::class, $clientEntityMock)); + + $implicitGrant = (new ImplicitGrantFactory( + $this->moduleConfigMock, + $this->createMock(IdTokenBuilder::class), + $this->requestRulesManagerMock, + $this->createMock(AccessTokenRepository::class), + $this->createMock(RequestParamsResolver::class), + $this->createMock(AccessTokenEntityFactory::class), + ))->build(); + + $implicitGrant->validateAuthorizationRequestWithRequestRules( + $this->createMock(ServerRequestInterface::class), + $resultBag, + ); + } + + + public function testBuiltGrantRefusesConfidentialClientWhenOptionIsOn(): void + { + $this->moduleConfigMock->method('getDisallowImplicitForConfidentialClients')->willReturn(true); + $this->requestRulesManagerMock->expects($this->never())->method('check'); + + $this->expectException(OidcServerException::class); + + $this->buildAndValidateForConfidentialClient(); + } + + + public function testBuiltGrantLetsConfidentialClientThroughWhenOptionIsOff(): void + { + $this->moduleConfigMock->method('getDisallowImplicitForConfidentialClients')->willReturn(false); + // The rule check is where the request goes next; failing there stops the test before the rest of the flow. + $this->requestRulesManagerMock->expects($this->once())->method('check') + ->willThrowException(new RuntimeException('rules executed')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('rules executed'); + + $this->buildAndValidateForConfidentialClient(); + } +} diff --git a/tests/unit/src/ModuleConfigTest.php b/tests/unit/src/ModuleConfigTest.php index 94b84f8d4..429df6762 100644 --- a/tests/unit/src/ModuleConfigTest.php +++ b/tests/unit/src/ModuleConfigTest.php @@ -400,6 +400,16 @@ public function testCanGetProtocolDiscoveryShowClaimsSupported(): void ); } + public function testCanGetDisallowImplicitForConfidentialClients(): void + { + $this->assertFalse($this->sut()->getDisallowImplicitForConfidentialClients()); + $this->assertTrue( + $this->sut( + overrides: [ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS => true], + )->getDisallowImplicitForConfidentialClients(), + ); + } + public function testCanGetProtocolNewCertPath(): void { $this->assertNull($this->sut()->getProtocolNewCertPath()); diff --git a/tests/unit/src/Server/Grants/ImplicitGrantTest.php b/tests/unit/src/Server/Grants/ImplicitGrantTest.php index d28dabda8..6377ad98d 100644 --- a/tests/unit/src/Server/Grants/ImplicitGrantTest.php +++ b/tests/unit/src/Server/Grants/ImplicitGrantTest.php @@ -10,15 +10,23 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; +use RuntimeException; use SimpleSAML\Module\oidc\Entities\ClientEntity; use SimpleSAML\Module\oidc\Entities\UserEntity; use SimpleSAML\Module\oidc\Factories\Entities\AccessTokenEntityFactory; use SimpleSAML\Module\oidc\Repositories\AccessTokenRepository; use SimpleSAML\Module\oidc\Repositories\Interfaces\AccessTokenRepositoryInterface; +use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; use SimpleSAML\Module\oidc\Server\Grants\ImplicitGrant; use SimpleSAML\Module\oidc\Server\RequestRules\Interfaces\ResultBagInterface; use SimpleSAML\Module\oidc\Server\RequestRules\RequestRulesManager; +use SimpleSAML\Module\oidc\Server\RequestRules\Result; +use SimpleSAML\Module\oidc\Server\RequestRules\ResultBag; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientIdRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\RedirectUriRule; +use SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule; use SimpleSAML\Module\oidc\Server\RequestTypes\AuthorizationRequest; use SimpleSAML\Module\oidc\Services\IdTokenBuilder; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; @@ -69,6 +77,7 @@ protected function sut( ?string $queryDelimiter = null, ?AccessTokenEntityFactory $accessTokenEntityFactory = null, ?ScopeRepositoryInterface $scopeRepository = null, + bool $disallowImplicitForConfidentialClients = false, ): ImplicitGrant { $idTokenBuilder ??= $this->idTokenBuilderMock; $accessTokenTtl ??= $this->accessTokenTtl1h; @@ -88,6 +97,7 @@ protected function sut( $requestParamsResolver, $queryDelimiter, $accessTokenEntityFactory, + $disallowImplicitForConfidentialClients, ); $implicitGrant->setScopeRepository($scopeRepository); @@ -171,4 +181,68 @@ public function testCanValidateAuthorizationRequestWithRequestRules(): void { $this->markTestIncomplete('RequestRulesManager needs to be refactored so it can be strongly typed.'); } + + protected function resultBagFor(bool $isConfidential): ResultBag + { + $this->clientEntityMock->method('isConfidential')->willReturn($isConfidential); + + $resultBag = new ResultBag(); + $resultBag->add(new Result(RedirectUriRule::class, 'https://example.com/callback')); + $resultBag->add(new Result(StateRule::class, 'test-state')); + $resultBag->add(new Result(ClientIdRule::class, $this->clientEntityMock)); + + return $resultBag; + } + + public function testValidateAuthorizationRequestRefusesConfidentialClientWhenDisallowed(): void + { + $this->requestRulesManagerMock->expects($this->never())->method('check'); + + $responseMock = $this->createMock(ResponseInterface::class); + $responseMock->expects($this->once())->method('withStatus')->with(302)->willReturnSelf(); + $responseMock->expects($this->once())->method('withHeader') + ->with( + 'Location', + 'https://example.com/callback#' . http_build_query([ + 'error' => 'unauthorized_client', + 'error_description' => 'The client is not authorized to request a token using this method. ' . + '(Confidential clients are not allowed to use the implicit grant.)', + 'state' => 'test-state', + ]), + ) + ->willReturnSelf(); + + try { + $this->sut(disallowImplicitForConfidentialClients: true) + ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(true)); + $this->fail('The request should have been refused.'); + } catch (OidcServerException $exception) { + $this->assertSame('unauthorized_client', $exception->getErrorType()); + $exception->generateHttpResponse($responseMock); + } + } + + public function testValidateAuthorizationRequestContinuesForPublicClientWhenDisallowed(): void + { + // The rule check is where the request goes next; failing there stops the test before the rest of the flow. + $this->requestRulesManagerMock->expects($this->once())->method('check') + ->willThrowException(new RuntimeException('rules executed')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('rules executed'); + + $this->sut(disallowImplicitForConfidentialClients: true) + ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(false)); + } + + public function testValidateAuthorizationRequestContinuesForConfidentialClientByDefault(): void + { + $this->requestRulesManagerMock->expects($this->once())->method('check') + ->willThrowException(new RuntimeException('rules executed')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('rules executed'); + + $this->sut()->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $this->resultBagFor(true)); + } }