diff --git a/config/module_oidc.php.dist b/config/module_oidc.php.dist index a83f02fe9..836104a7d 100644 --- a/config/module_oidc.php.dist +++ b/config/module_oidc.php.dist @@ -256,6 +256,15 @@ $config = [ // claims for which "SAML attribute to OIDC claim translation" has been defined above. ModuleConfig::OPTION_PROTOCOL_DISCOVERY_SHOW_CLAIMS_SUPPORTED => false, + // Optional security hardening (disabled by default). If enabled, confidential clients will be rejected with + // 'unauthorized_client' when they try to use the implicit flow (response_type 'id_token' or 'id_token token'), + // so they can only get tokens using the authorization code flow, where they authenticate at the token endpoint. + // The specifications do not forbid confidential clients from using the implicit flow, but OAuth 2.0 Security + // BCP advises against it (https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2), and OIDC Core notes + // that confidential clients would normally use the authorization code flow + // (https://openid.net/specs/openid-connect-core-1_0.html#RPMTI). + 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..296cabcf3 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,20 @@ public function getProtocolDiscoveryShowClaimsSupported(): bool ); } + /** + * Opt-in hardening: reject implicit flow requests from confidential clients. + * + * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * @see https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + */ + 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..1ac5bf415 100644 --- a/src/Server/Exceptions/OidcServerException.php +++ b/src/Server/Exceptions/OidcServerException.php @@ -106,6 +106,31 @@ public static function unsupportedResponseType( return $e; } + /** + * Unauthorized client error. + * + * @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 + * + * @param string|null $hint + * @param string|null $redirectUri + * @param string|null $state + * @param bool $useFragment + * @return self + */ + public static function unauthorizedClient( + ?string $hint = null, + ?string $redirectUri = null, + ?string $state = null, + bool $useFragment = false, + ): self { + $errorMessage = 'The client is not authorized to request an authorization code or access token ' . + 'using this method (unauthorized_client).'; + $e = new self($errorMessage, 4, 'unauthorized_client', 400, $hint, $redirectUri, null, $state); + $e->useFragmentInHttpResponses($useFragment); + + return $e; + } + /** * Invalid scope error. * diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 4e2026bcc..89a3d20a3 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,21 @@ public function validateAuthorizationRequestWithRequestRules( /** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */ $client = $resultBag->getOrFail(ClientIdRule::class)->getValue(); + // Opt-in hardening (ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS, default false). + // The specs do not forbid confidential clients from using the implicit flow, but the implicit flow never + // authenticates the client, and OAuth 2.0 Security BCP advises against it: + // https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + // OIDC Core notes that confidential clients would normally use the authorization code flow: + // https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + if ($this->disallowImplicitForConfidentialClients && $client->isConfidential()) { + throw OidcServerException::unauthorizedClient( + 'Confidential clients are not allowed to use the implicit grant.', + $redirectUri, + $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/bootstrap.php b/tests/bootstrap.php index 9c16119c8..3a4c6698b 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -15,7 +15,11 @@ */ $projectRoot = dirname(__DIR__); -require_once $projectRoot . '/vendor/autoload.php'; +if (file_exists($projectRoot . '/vendor/autoload.php')) { + require_once $projectRoot . '/vendor/autoload.php'; +} else { + require_once dirname(__DIR__, 3) . '/vendor/autoload.php'; +} // Symlink module into ssp vendor lib so that templates and urls can resolve correctly $linkPath = $projectRoot . '/vendor/simplesamlphp/simplesamlphp/modules/oidc'; @@ -25,3 +29,14 @@ } else { echo "'$linkPath' linked to '$projectRoot'\n"; } + +$certDir = $projectRoot . '/vendor/simplesamlphp/simplesamlphp/cert'; +$mainCertDir = dirname(__DIR__, 3) . '/cert'; +if (file_exists($mainCertDir) && file_exists($certDir)) { + foreach (glob($mainCertDir . '/*') as $certFile) { + $dest = $certDir . '/' . basename($certFile); + if (!file_exists($dest)) { + copy($certFile, $dest); + } + } +} diff --git a/tests/integration/src/Flows/ConfidentialClientFlowsTest.php b/tests/integration/src/Flows/ConfidentialClientFlowsTest.php new file mode 100644 index 000000000..b080a45d2 --- /dev/null +++ b/tests/integration/src/Flows/ConfidentialClientFlowsTest.php @@ -0,0 +1,334 @@ +toArray(); + $config['database.dsn'] = 'sqlite::memory:'; + $config['database.username'] = null; + $config['database.password'] = null; + $config['database.prefix'] = 'phpunit_'; + $config['database.persistent'] = true; + $config['database.secondaries'] = []; + + Configuration::loadFromArray($config, '', 'simplesaml'); + (new DatabaseMigration())->migrate(); + } + + protected function setUp(): void + { + $this->originalServer = $_SERVER; + $_SERVER['HTTP_HOST'] = 'localhost'; + $_SERVER['SERVER_NAME'] = 'localhost'; + $_SERVER['SERVER_PORT'] = '80'; + $_SERVER['REQUEST_URI'] = '/module.php/oidc/authorization'; + $_SERVER['REQUEST_METHOD'] = 'GET'; + + Configuration::setConfigDir(__DIR__ . '/../../../config'); + $this->originalModuleConfig = Configuration::getConfig(ModuleConfig::DEFAULT_FILE_NAME); + // Enable the opt-in hardening for these tests. + $this->buildContainer(disallowImplicitForConfidentialClients: true); + + // Set up test user + $this->user = new UserEntity( + $this->userId, + new DateTimeImmutable(), + new DateTimeImmutable(), + ['sub' => $this->userId, 'eduPersonPrincipalName' => [$this->userId]] + ); + $existingUser = $this->userRepo->getUserEntityByIdentifier($this->userId); + if ($existingUser) { + $this->userRepo->delete($existingUser); + } + $this->userRepo->add($this->user); + + // Set up confidential client + $existingClient = $this->clientRepo->findById($this->clientId); + if ($existingClient) { + $this->clientRepo->delete($existingClient); + } + + $this->client = $this->clientFactory->fromData( + id: $this->clientId, + secret: $this->clientSecret, + name: 'Confidential Flow Test Client', + description: 'Testing confidential client flows and implicit grant restriction', + redirectUri: [$this->redirectUri], + scopes: ['openid', 'profile', 'email'], + isEnabled: true, + isConfidential: true, // CONFIDENTIAL CLIENT + ); + $this->clientRepo->add($this->client); + } + + protected function buildContainer(bool $disallowImplicitForConfidentialClients): void + { + Configuration::setPreLoadedConfig( + Configuration::loadFromArray(array_merge( + $this->originalModuleConfig->toArray(), + [ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS => $disallowImplicitForConfidentialClients], + )), + ModuleConfig::DEFAULT_FILE_NAME, + ); + + $this->container = new Container(); + $this->clientRepo = $this->container->get(ClientRepository::class); + $this->userRepo = $this->container->get(UserRepository::class); + $this->clientFactory = $this->container->get(ClientEntityFactory::class); + $this->authServer = $this->container->get(AuthorizationServer::class); + } + + protected function tearDown(): void + { + $_SERVER = $this->originalServer; + + if (isset($this->client)) { + $existingClient = $this->clientRepo->findById($this->clientId); + if ($existingClient) { + $this->clientRepo->delete($existingClient); + } + } + if (isset($this->user)) { + $existingUser = $this->userRepo->getUserEntityByIdentifier($this->userId); + if ($existingUser) { + $this->userRepo->delete($existingUser); + } + } + + Configuration::setPreLoadedConfig($this->originalModuleConfig, ModuleConfig::DEFAULT_FILE_NAME); + } + + /** + * Test that confidential clients MUST NOT be permitted to use the Implicit Flow + * (response_type=id_token token or id_token). + * + * Requires opt-in ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS (enabled in setUp). + * This is hardening, not a spec requirement. The implicit flow never authenticates the client and + * OAuth 2.0 Security BCP advises against it: https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * OIDC Core notes confidential clients would normally use the code flow: + * https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + */ + public function testConfidentialClientCannotUseImplicitFlow(): void + { + $implicitRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'id_token token', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'xyz123', + 'nonce' => 'nonce456', + ]); + + // A compliant OP must reject implicit flow requests for confidential clients + // (either during request validation or request completion by throwing an OAuth/OIDC exception) + $this->expectException(\Throwable::class); + $this->expectExceptionMessageMatches('/(unauthorized_client|invalid_grant|unsupported_response_type|not allowed|confidential)/i'); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($implicitRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $response = $this->authServer->completeAuthorizationRequest( + $validatedAuthReq, + new Response() + ); + + // If no exception was thrown, check whether tokens were issued in the fragment (violating guidelines) + $location = $response->getHeaderLine('Location'); + $this->assertStringNotContainsString('access_token=', $location, 'Confidential client received access_token via implicit flow!'); + $this->assertStringNotContainsString('id_token=', $location, 'Confidential client received id_token via implicit flow!'); + } + + /** + * With the opt-in option disabled (default), behaviour is unchanged: the implicit flow works for + * confidential clients, as the specs do not forbid it. + */ + public function testConfidentialClientCanUseImplicitFlowWhenOptionDisabled(): void + { + $this->buildContainer(disallowImplicitForConfidentialClients: false); + + $implicitRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'id_token token', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'xyz123', + 'nonce' => 'nonce456', + ]); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($implicitRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $response = $this->authServer->completeAuthorizationRequest($validatedAuthReq, new Response()); + + $location = $response->getHeaderLine('Location'); + $this->assertStringContainsString('access_token=', $location); + $this->assertStringContainsString('id_token=', $location); + } + + /** + * Test that confidential clients using Authorization Code Flow with PKCE + * MUST authenticate with client credentials at the token endpoint. + */ + public function testConfidentialClientWithPkceRequiresSecretAtTokenEndpoint(): void + { + $codeVerifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + $codeChallenge = rtrim(strtr(base64_encode(hash('sha256', $codeVerifier, true)), '+/', '-_'), '='); + + // Step 1: Authorization Request (receives auth code) + $authCodeRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'code', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'state-pkce', + 'nonce' => 'nonce-pkce', + 'code_challenge' => $codeChallenge, + 'code_challenge_method' => 'S256', + ]); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($authCodeRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $authResponse = $this->authServer->completeAuthorizationRequest( + $validatedAuthReq, + new Response() + ); + + $location = $authResponse->getHeaderLine('Location'); + parse_str((string) parse_url($location, PHP_URL_QUERY), $queryParams); + $authCode = $queryParams['code'] ?? null; + $this->assertNotEmpty($authCode, 'Authorization code was not issued'); + + // Step 2: Token request WITHOUT client secret must be rejected + $tokenRequestNoSecret = (new ServerRequest('POST', '/module.php/oidc/token')) + ->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $this->clientId, + 'code' => $authCode, + 'redirect_uri' => $this->redirectUri, + 'code_verifier' => $codeVerifier, + ]); + + $this->expectException(OAuthServerException::class); + $this->authServer->respondToAccessTokenRequest( + $tokenRequestNoSecret, + new Response() + ); + } + + /** + * Test that confidential clients using standard Authorization Code Flow (no PKCE) + * cannot exchange without secret, but succeed when valid secret is provided. + */ + public function testConfidentialClientWithoutPkceRequiresSecretAndSucceedsWithSecret(): void + { + // Step 1: Authorization Request (no PKCE) + $authCodeRequest = (new ServerRequest('GET', '/module.php/oidc/authorization')) + ->withQueryParams([ + 'response_type' => 'code', + 'client_id' => $this->clientId, + 'redirect_uri' => $this->redirectUri, + 'scope' => 'openid', + 'state' => 'state-no-pkce', + 'nonce' => 'nonce-no-pkce', + ]); + + $validatedAuthReq = $this->authServer->validateAuthorizationRequest($authCodeRequest); + $validatedAuthReq->setUser($this->user); + $validatedAuthReq->setAuthorizationApproved(true); + + $authResponse = $this->authServer->completeAuthorizationRequest( + $validatedAuthReq, + new Response() + ); + + $location = $authResponse->getHeaderLine('Location'); + parse_str((string) parse_url($location, PHP_URL_QUERY), $queryParams); + $authCode = $queryParams['code'] ?? null; + $this->assertNotEmpty($authCode, 'Authorization code was not issued'); + + // Step 2a: Token request WITHOUT client secret must fail + $tokenRequestNoSecret = (new ServerRequest('POST', '/module.php/oidc/token')) + ->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $this->clientId, + 'code' => $authCode, + 'redirect_uri' => $this->redirectUri, + ]); + + try { + $this->authServer->respondToAccessTokenRequest( + $tokenRequestNoSecret, + new Response() + ); + $this->fail('Token endpoint unexpectedly issued tokens to confidential client without client credentials'); + } catch (OAuthServerException $e) { + $this->assertSame('access_denied', $e->getErrorType()); + } + + // Step 2b: Token request WITH valid client secret succeeds + $tokenRequestWithSecret = (new ServerRequest('POST', '/module.php/oidc/token')) + ->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $this->clientId, + 'client_secret' => $this->clientSecret, + 'code' => $authCode, + 'redirect_uri' => $this->redirectUri, + ]); + + $tokenResponse = $this->authServer->respondToAccessTokenRequest( + $tokenRequestWithSecret, + new Response() + ); + + $this->assertSame(200, $tokenResponse->getStatusCode()); + $body = json_decode((string) $tokenResponse->getBody(), true); + $this->assertArrayHasKey('access_token', $body); + $this->assertArrayHasKey('id_token', $body); + } +} diff --git a/tests/unit/src/Server/Grants/ImplicitGrantTest.php b/tests/unit/src/Server/Grants/ImplicitGrantTest.php index d28dabda8..4f46416ae 100644 --- a/tests/unit/src/Server/Grants/ImplicitGrantTest.php +++ b/tests/unit/src/Server/Grants/ImplicitGrantTest.php @@ -69,6 +69,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 +89,7 @@ protected function sut( $requestParamsResolver, $queryDelimiter, $accessTokenEntityFactory, + $disallowImplicitForConfidentialClients, ); $implicitGrant->setScopeRepository($scopeRepository); @@ -171,4 +173,62 @@ public function testCanValidateAuthorizationRequestWithRequestRules(): void { $this->markTestIncomplete('RequestRulesManager needs to be refactored so it can be strongly typed.'); } + + protected function confidentialClientResultBag(): \SimpleSAML\Module\oidc\Server\RequestRules\ResultBag + { + $redirectUriResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( + \SimpleSAML\Module\oidc\Server\RequestRules\Rules\RedirectUriRule::class, + 'https://example.com/callback' + ); + $stateResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( + \SimpleSAML\Module\oidc\Server\RequestRules\Rules\StateRule::class, + 'test-state' + ); + + $this->clientEntityMock->method('isConfidential')->willReturn(true); + $clientResult = new \SimpleSAML\Module\oidc\Server\RequestRules\Result( + \SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientIdRule::class, + $this->clientEntityMock + ); + + $resultBag = new \SimpleSAML\Module\oidc\Server\RequestRules\ResultBag(); + $resultBag->add($redirectUriResult); + $resultBag->add($stateResult); + $resultBag->add($clientResult); + + return $resultBag; + } + + /** + * Opt-in hardening, see ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS. + * + * @see https://www.rfc-editor.org/rfc/rfc9700.html#section-2.1.2 + * @see https://openid.net/specs/openid-connect-core-1_0.html#RPMTI + */ + public function testValidateAuthorizationRequestThrowsForConfidentialClientWhenDisallowed(): void + { + $resultBag = $this->confidentialClientResultBag(); + + $this->requestRulesManagerMock->expects($this->never())->method('check'); + + $this->expectException(\SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException::class); + $this->expectExceptionMessageMatches('/unauthorized_client/'); + + $this->sut(disallowImplicitForConfidentialClients: true) + ->validateAuthorizationRequestWithRequestRules($this->serverRequestMock, $resultBag); + } + + public function testValidateAuthorizationRequestDoesNotRejectConfidentialClientByDefault(): void + { + $resultBag = $this->confidentialClientResultBag(); + + // Guard is off by default, so processing continues to the request rules. + $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, $resultBag); + } }