From d4c825cc9c91504bb6b6c11fe1787d59e4f5aab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Wed, 4 Nov 2026 16:35:48 +0100 Subject: [PATCH 1/5] Test confidential client authentication at the token endpoint Nothing tested a token request without a secret, or with PKCE, end to end. ClientRepositoryTest covers validateClient() directly and the conformance plans cover successful client_secret_basic and client_secret_post flows, but code_verifier appeared nowhere in tests/. Add two integration tests, written from the ones Ioannis Igoumenos contributed in PR 362 (981ea18) and left out of its merge because they needed a signing key CI does not have. A confidential client, with and without PKCE, is refused without its secret and with a wrong one, and gets tokens with the right one. With PKCE it is also refused with a wrong verifier. Each refusal asserts the error type. The tests generate their signing key pair in a temporary directory and point SimpleSAMLphp's certdir at it, so they need neither vendor/ nor the keys in docker/ssp/, which wip-version-7 no longer tracks. They boot the module Kernel instead of Services\Container, which wip-version-7 no longer has, so the same file runs on both branches. Merging this into wip-version-7 needs one edit, which git will not ask for: ERROR_WITHOUT_CLIENT_AUTHENTICATION must become invalid_client, the answer that branch gives. master answers access_denied. Co-authored-by: Ioannis Igoumenos Assisted-by: Claude Code (claude-opus-5-5, implementation & canary mutations) --- .../ConfidentialClientTokenEndpointTest.php | 330 ++++++++++++++++++ 1 file changed, 330 insertions(+) create mode 100644 tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php diff --git a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php new file mode 100644 index 000000000..6dbad3437 --- /dev/null +++ b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php @@ -0,0 +1,330 @@ +toArray(); + $config['certdir'] = $certDir; + $config['cachedir'] = self::$workDir . '/cache'; + $config['database.dsn'] = 'sqlite::memory:'; + $config['database.username'] = null; + $config['database.password'] = null; + $config['database.prefix'] = 'phpunit_'; + // Not persistent, so that this database is not the one any other test class connects to with the same DSN. + $config['database.persistent'] = false; + $config['database.secondaries'] = []; + + Configuration::loadFromArray($config, '', 'simplesaml'); + (new DatabaseMigration())->migrate(); + } + + + public static function tearDownAfterClass(): void + { + Configuration::clearInternalState(); + + (new Filesystem())->remove(self::$workDir); + } + + + 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'; + + $container = self::bootModuleContainer(); + $this->clientRepository = $container->get(ClientRepository::class); + $this->userRepository = $container->get(UserRepository::class); + $this->authorizationServer = $container->get(AuthorizationServer::class); + + $this->user = new UserEntity( + self::USER_ID, + new DateTimeImmutable(), + new DateTimeImmutable(), + ['sub' => self::USER_ID, 'eduPersonPrincipalName' => [self::USER_ID]], + ); + $this->userRepository->add($this->user); + + $this->client = $container->get(ClientEntityFactory::class)->fromData( + id: self::CLIENT_ID, + secret: self::CLIENT_SECRET, + name: 'Confidential Token Endpoint Client', + description: 'A confidential client at the token endpoint', + redirectUri: [self::REDIRECT_URI], + scopes: ['openid', 'profile', 'email'], + isEnabled: true, + isConfidential: true, + ); + $this->clientRepository->add($this->client); + } + + + protected function tearDown(): void + { + $_SERVER = $this->originalServer; + + if (isset($this->client)) { + $this->clientRepository->delete($this->client); + } + if (isset($this->user)) { + $this->userRepository->delete($this->user); + } + } + + + /** + * The code_verifier proves that the caller started this authorization, not which client the caller is, so it + * does not stand in for the secret of a confidential client. + */ + public function testConfidentialClientWithPkceRequiresSecretAtTokenEndpoint(): void + { + $codeVerifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + $codeChallenge = rtrim(strtr(base64_encode(hash('sha256', $codeVerifier, true)), '+/', '-_'), '='); + + $code = $this->requestAuthorizationCode([ + 'code_challenge' => $codeChallenge, + 'code_challenge_method' => 'S256', + ]); + + $this->assertTokenRequestRefused( + self::ERROR_WITHOUT_CLIENT_AUTHENTICATION, + ['code' => $code, 'code_verifier' => $codeVerifier], + ); + $this->assertTokenRequestRefused( + self::ERROR_WITH_WRONG_SECRET, + ['code' => $code, 'code_verifier' => $codeVerifier, 'client_secret' => 'not-the-secret'], + ); + $this->assertTokenRequestRefused( + 'invalid_grant', + [ + 'code' => $code, + 'code_verifier' => 'not-the-verifier-but-a-well-formed-one-of-43-characters', + 'client_secret' => self::CLIENT_SECRET, + ], + ); + + $this->assertTokensIssued( + ['code' => $code, 'code_verifier' => $codeVerifier, 'client_secret' => self::CLIENT_SECRET], + ); + } + + + public function testConfidentialClientWithoutPkceRequiresSecretAndSucceedsWithSecret(): void + { + $code = $this->requestAuthorizationCode(); + + $this->assertTokenRequestRefused(self::ERROR_WITHOUT_CLIENT_AUTHENTICATION, ['code' => $code]); + $this->assertTokenRequestRefused( + self::ERROR_WITH_WRONG_SECRET, + ['code' => $code, 'client_secret' => 'not-the-secret'], + ); + + $this->assertTokensIssued(['code' => $code, 'client_secret' => self::CLIENT_SECRET]); + } + + + /** + * @param array $additionalParams + */ + protected function requestAuthorizationCode(array $additionalParams = []): string + { + $authorizationRequest = $this->authorizationServer->validateAuthorizationRequest( + (new ServerRequest('GET', '/module.php/oidc/authorization'))->withQueryParams([ + 'response_type' => 'code', + 'client_id' => self::CLIENT_ID, + 'redirect_uri' => self::REDIRECT_URI, + 'scope' => 'openid', + 'state' => 'state-value', + 'nonce' => 'nonce-value', + ...$additionalParams, + ]), + ); + $authorizationRequest->setUser($this->user); + $authorizationRequest->setAuthorizationApproved(true); + + $response = $this->authorizationServer->completeAuthorizationRequest($authorizationRequest, new Response()); + + parse_str((string) parse_url($response->getHeaderLine('Location'), PHP_URL_QUERY), $query); + $this->assertNotEmpty($query['code'] ?? null, 'Authorization code was not issued'); + + return (string) $query['code']; + } + + + /** + * @param array $params + */ + protected function assertTokenRequestRefused(string $expectedErrorType, array $params): void + { + try { + $this->authorizationServer->respondToAccessTokenRequest($this->tokenRequest($params), new Response()); + } catch (OAuthServerException $exception) { + $this->assertSame($expectedErrorType, $exception->getErrorType()); + return; + } + + $this->fail('The token endpoint issued tokens, expected ' . $expectedErrorType); + } + + + /** + * @param array $params + */ + protected function assertTokensIssued(array $params): void + { + $response = $this->authorizationServer->respondToAccessTokenRequest( + $this->tokenRequest($params), + new Response(), + ); + + $this->assertSame(200, $response->getStatusCode()); + $body = json_decode((string) $response->getBody(), true); + $this->assertArrayHasKey('access_token', $body); + $this->assertArrayHasKey('id_token', $body); + } + + + /** + * @param array $params + */ + protected function tokenRequest(array $params): ServerRequest + { + return (new ServerRequest('POST', '/module.php/oidc/token'))->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => self::CLIENT_ID, + 'redirect_uri' => self::REDIRECT_URI, + ...$params, + ]); + } + + + /** + * The container of the module Kernel, the one the module's routes run in. Its services are private, so the ones + * this test fetches are made public before they can be inlined or removed. + */ + protected static function bootModuleContainer(): ContainerInterface + { + $kernel = new class (ModuleConfig::MODULE_NAME) extends Kernel implements CompilerPassInterface { + public function process(ContainerBuilder $container): void + { + $serviceIds = [ + AuthorizationServer::class, + ClientEntityFactory::class, + ClientRepository::class, + UserRepository::class, + ]; + + foreach ($serviceIds as $serviceId) { + $container->getDefinition($serviceId)->setPublic(true); + } + } + }; + $kernel->boot(); + + return $kernel->getContainer(); + } + + + protected static function createSigningKeyPair(string $certDir): void + { + $key = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + + if (!mkdir($certDir, 0700, true) || $key === false || !openssl_pkey_export($key, $privateKey)) { + throw new RuntimeException('Could not create a signing key pair in ' . $certDir); + } + + $files = [ + ModuleConfig::DEFAULT_PKI_PRIVATE_KEY_FILENAME => $privateKey, + ModuleConfig::DEFAULT_PKI_CERTIFICATE_FILENAME => openssl_pkey_get_details($key)['key'], + ]; + + foreach ($files as $name => $contents) { + if (file_put_contents($certDir . '/' . $name, $contents) === false) { + throw new RuntimeException('Could not write ' . $name . ' to ' . $certDir); + } + chmod($certDir . '/' . $name, 0600); + } + } +} From 15fb6b4603810809954729cbec08c2871a5364ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Wed, 4 Nov 2026 17:52:05 +0100 Subject: [PATCH 2/5] Answer invalid_client when token endpoint client authentication fails The token endpoint answered access_denied to a confidential client which presented no authentication, and to one whose client assertion did not hold: no JWKS available, a signature which did not verify, or the wrong Issuer, Subject or Audience. RFC 6749 section 5.2 names invalid_client for the first and RFC 7521 section 4.2.1 requires it for the second. A wrong secret already got it, and wip-version-7 answers it for all of these. The status stays HTTP 401, and League adds its WWW-Authenticate challenge when the request used the Authorization header. The response no longer carries the hint access_denied had, and a client which compares the error member with the old value needs the new one. Since ErrorResponder logs nothing on this branch, the rule logs the reason for each of these refusals at warning level with the client_id. The backstop in AuthCodeGrant answers the same. No request reaches it today, and it runs before the code is decrypted, so it cannot tell an invalid grant from missing authentication as wip-version-7's does. The end-to-end test now has one constant for the answer, and adds the challenge and private_key_jwt, whose checks in the rule had no test on this branch. It passes unedited on wip-version-7. Assisted-by: Claude Code (claude-opus-5-5, implementation & integration tests) Assisted-by: Google Antigravity (gemini-3.8-flash-high, review) --- src/Server/Grants/AuthCodeGrant.php | 5 +- .../Rules/ClientAuthenticationRule.php | 28 ++- .../ConfidentialClientTokenEndpointTest.php | 193 +++++++++++++++--- 3 files changed, 192 insertions(+), 34 deletions(-) diff --git a/src/Server/Grants/AuthCodeGrant.php b/src/Server/Grants/AuthCodeGrant.php index 684764bb1..8b8cf00c1 100644 --- a/src/Server/Grants/AuthCodeGrant.php +++ b/src/Server/Grants/AuthCodeGrant.php @@ -416,8 +416,11 @@ public function respondToAccessTokenRequest( $utilizedClientAuthenticationParams[] = ParamsEnum::CodeVerifier->value; } + // A client which neither authenticated nor sent a code_verifier: a backstop, since CodeVerifierRule refuses a + // client which is not confidential without a code_verifier, and ClientAuthenticationRule a confidential one + // which presents no authentication (RFC 6749 section 5.2). if (empty($utilizedClientAuthenticationParams)) { - throw OidcServerException::accessDenied('Client authentication not performed.'); + throw OidcServerException::invalidClient($request); } // OAuth2 implementation diff --git a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php index edc4cecfa..f73696324 100644 --- a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php +++ b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php @@ -55,6 +55,18 @@ public function checkRule( return new Result($this->getKey(), null); } + // The refusal of a client which does not authenticate (RFC 6749 section 5.2, RFC 7521 section 4.2.1): + // invalid_client, HTTP 401, with a WWW-Authenticate challenge when the request used the Authorization header. + // The answer does not say why, the log does. + $refuse = static function (string $reason) use ($client, $request, $loggerService): OidcServerException { + $loggerService->warning( + 'Client authentication failed: ' . $reason, + ['client_id' => $client->getIdentifier()], + ); + + return OidcServerException::invalidClient($request); + }; + // Let's check if client secret is provided. /** @var ?string $clientSecret */ $clientSecret = $this->requestParamsResolver->getFromRequestBasedOnAllowedMethods( @@ -64,7 +76,7 @@ public function checkRule( ) ?? $request->getServerParams()['PHP_AUTH_PW'] ?? null; if ($clientSecret) { - hash_equals($client->getSecret(), $clientSecret) || throw OidcServerException::invalidClient($request); + hash_equals($client->getSecret(), $clientSecret) || throw $refuse('Client secret does not match.'); return new Result($this->getKey(), ParamsEnum::ClientSecret->value); } @@ -77,7 +89,7 @@ public function checkRule( ); if (is_null($clientAssertionParam)) { - throw OidcServerException::accessDenied('Not a single client authentication method presented.'); + throw $refuse('Not a single client authentication method presented.'); } // private_key_jwt authentication method is used. @@ -103,23 +115,21 @@ public function checkRule( ); } - ($jwks = $this->jwksResolver->forClient($client)) || throw OidcServerException::accessDenied( + ($jwks = $this->jwksResolver->forClient($client)) || throw $refuse( 'Can not validate Client Assertion, client JWKS not available.', ); try { $clientAssertion->verifyWithKeySet($jwks); } catch (\Throwable $exception) { - throw OidcServerException::accessDenied( - 'Client Assertion validation failed: ' . $exception->getMessage(), - ); + throw $refuse('Client Assertion validation failed: ' . $exception->getMessage()); } - ($client->getIdentifier() === $clientAssertion->getIssuer()) || throw OidcServerException::accessDenied( + ($client->getIdentifier() === $clientAssertion->getIssuer()) || throw $refuse( 'Invalid Client Assertion Issuer claim.', ); - ($client->getIdentifier() === $clientAssertion->getSubject()) || throw OidcServerException::accessDenied( + ($client->getIdentifier() === $clientAssertion->getSubject()) || throw $refuse( 'Invalid Client Assertion Subject claim.', ); @@ -133,7 +143,7 @@ public function checkRule( ]; (!empty(array_intersect($expectedAudience, $clientAssertion->getAudience()))) || - throw OidcServerException::accessDenied('Invalid Client Assertion Audience claim.'); + throw $refuse('Invalid Client Assertion Audience claim.'); // Everything seems ok. Save it in cache so we can check for reuse. $this->protocolCache?->set( diff --git a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php index 6dbad3437..7d2135816 100644 --- a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php +++ b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php @@ -8,6 +8,7 @@ use League\OAuth2\Server\Exception\OAuthServerException; use Nyholm\Psr7\Response; use Nyholm\Psr7\ServerRequest; +use OpenSSLAsymmetricKey; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use RuntimeException; @@ -41,17 +42,15 @@ class ConfidentialClientTokenEndpointTest extends TestCase { /** - * The answer to a confidential client which presents no authentication at all. - * - * RFC 6749 section 5.2 names invalid_client for this, and wip-version-7 answers that, so a merge of this file - * into that branch has to change this value. + * The answer to a confidential client which does not authenticate: one which presents nothing or a wrong secret + * (RFC 6749 section 5.2), or an assertion which does not hold (RFC 7521 section 4.2.1). */ - protected const string ERROR_WITHOUT_CLIENT_AUTHENTICATION = 'access_denied'; - - protected const string ERROR_WITH_WRONG_SECRET = 'invalid_client'; + protected const string ERROR_CLIENT_AUTHENTICATION_FAILED = 'invalid_client'; protected const string CLIENT_ID = 'confidential-token-endpoint-client'; + protected const string CLIENT_KEY_ID = 'confidential-token-endpoint-client-key'; + protected const string CLIENT_SECRET = 'my-super-secret-key-12345'; protected const string REDIRECT_URI = 'https://oidcdebugger.com/debug'; @@ -69,6 +68,10 @@ class ConfidentialClientTokenEndpointTest extends TestCase protected AuthorizationServer $authorizationServer; + protected ClientEntityFactory $clientEntityFactory; + + protected ModuleConfig $moduleConfig; + protected ClientEntityInterface $client; protected UserEntity $user; @@ -118,6 +121,8 @@ protected function setUp(): void $this->clientRepository = $container->get(ClientRepository::class); $this->userRepository = $container->get(UserRepository::class); $this->authorizationServer = $container->get(AuthorizationServer::class); + $this->clientEntityFactory = $container->get(ClientEntityFactory::class); + $this->moduleConfig = $container->get(ModuleConfig::class); $this->user = new UserEntity( self::USER_ID, @@ -127,16 +132,7 @@ protected function setUp(): void ); $this->userRepository->add($this->user); - $this->client = $container->get(ClientEntityFactory::class)->fromData( - id: self::CLIENT_ID, - secret: self::CLIENT_SECRET, - name: 'Confidential Token Endpoint Client', - description: 'A confidential client at the token endpoint', - redirectUri: [self::REDIRECT_URI], - scopes: ['openid', 'profile', 'email'], - isEnabled: true, - isConfidential: true, - ); + $this->client = $this->createClient(); $this->clientRepository->add($this->client); } @@ -161,7 +157,7 @@ protected function tearDown(): void public function testConfidentialClientWithPkceRequiresSecretAtTokenEndpoint(): void { $codeVerifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; - $codeChallenge = rtrim(strtr(base64_encode(hash('sha256', $codeVerifier, true)), '+/', '-_'), '='); + $codeChallenge = self::base64Url(hash('sha256', $codeVerifier, true)); $code = $this->requestAuthorizationCode([ 'code_challenge' => $codeChallenge, @@ -169,11 +165,11 @@ public function testConfidentialClientWithPkceRequiresSecretAtTokenEndpoint(): v ]); $this->assertTokenRequestRefused( - self::ERROR_WITHOUT_CLIENT_AUTHENTICATION, + self::ERROR_CLIENT_AUTHENTICATION_FAILED, ['code' => $code, 'code_verifier' => $codeVerifier], ); $this->assertTokenRequestRefused( - self::ERROR_WITH_WRONG_SECRET, + self::ERROR_CLIENT_AUTHENTICATION_FAILED, ['code' => $code, 'code_verifier' => $codeVerifier, 'client_secret' => 'not-the-secret'], ); $this->assertTokenRequestRefused( @@ -195,9 +191,9 @@ public function testConfidentialClientWithoutPkceRequiresSecretAndSucceedsWithSe { $code = $this->requestAuthorizationCode(); - $this->assertTokenRequestRefused(self::ERROR_WITHOUT_CLIENT_AUTHENTICATION, ['code' => $code]); + $this->assertTokenRequestRefused(self::ERROR_CLIENT_AUTHENTICATION_FAILED, ['code' => $code]); $this->assertTokenRequestRefused( - self::ERROR_WITH_WRONG_SECRET, + self::ERROR_CLIENT_AUTHENTICATION_FAILED, ['code' => $code, 'client_secret' => 'not-the-secret'], ); @@ -205,6 +201,81 @@ public function testConfidentialClientWithoutPkceRequiresSecretAndSucceedsWithSe } + /** + * A client which tried the Authorization header gets the challenge RFC 6749 section 5.2 requires. + */ + public function testConfidentialClientWithoutSecretInAuthorizationHeaderIsChallenged(): void + { + $request = $this->tokenRequest(['code' => $this->requestAuthorizationCode()]) + ->withHeader('Authorization', 'Basic ' . base64_encode(self::CLIENT_ID . ':')); + + try { + $this->authorizationServer->respondToAccessTokenRequest($request, new Response()); + } catch (OAuthServerException $exception) { + $this->assertSame(self::ERROR_CLIENT_AUTHENTICATION_FAILED, $exception->getErrorType()); + $this->assertSame(401, $exception->getHttpStatusCode()); + $this->assertSame('Basic realm="OAuth"', $exception->getHttpHeaders()['WWW-Authenticate'] ?? null); + return; + } + + $this->fail('The token endpoint issued tokens.'); + } + + + /** + * private_key_jwt. Each refused request differs from the last one, which is accepted, only in the one thing wrong + * with it, and a refusal does not use up the code. An assertion with a Subject other than its Issuer does not parse + * (ClientAssertion), so the Issuer row is the one which reaches the identity checks. + */ + public function testConfidentialClientWithClientAssertionRequiresOneWhichHolds(): void + { + $code = $this->requestAuthorizationCode(); + $clientKey = self::createRsaKey(); + + // The client has not registered a key yet. + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, ...$this->clientAssertionParams($clientKey)], + ); + + $this->clientRepository->update($this->createClient(self::jwksOf($clientKey))); + + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, ...$this->clientAssertionParams(self::createRsaKey())], + ); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + [ + 'code' => $code, + ...$this->clientAssertionParams($clientKey, ['iss' => 'another-client', 'sub' => 'another-client']), + ], + ); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, ...$this->clientAssertionParams($clientKey, ['aud' => 'https://another.example.org/'])], + ); + + $this->assertTokensIssued(['code' => $code, ...$this->clientAssertionParams($clientKey)]); + } + + + protected function createClient(?array $jwks = null): ClientEntityInterface + { + return $this->clientEntityFactory->fromData( + id: self::CLIENT_ID, + secret: self::CLIENT_SECRET, + name: 'Confidential Token Endpoint Client', + description: 'A confidential client at the token endpoint', + redirectUri: [self::REDIRECT_URI], + scopes: ['openid', 'profile', 'email'], + isEnabled: true, + isConfidential: true, + jwks: $jwks, + ); + } + + /** * @param array $additionalParams */ @@ -280,6 +351,43 @@ protected function tokenRequest(array $params): ServerRequest } + /** + * The parameters of a client assertion signed with the key, built by hand so that what goes over the wire is + * what this test wrote. + * + * @param array $claims Replacing those of an assertion which holds. + * @return array + */ + protected function clientAssertionParams(OpenSSLAsymmetricKey $key, array $claims = []): array + { + $now = time(); + $signingInput = self::base64Url(json_encode( + ['alg' => 'RS256', 'typ' => 'JWT', 'kid' => self::CLIENT_KEY_ID], + JSON_THROW_ON_ERROR, + )) . '.' . self::base64Url(json_encode( + [ + 'iss' => self::CLIENT_ID, + 'sub' => self::CLIENT_ID, + 'aud' => $this->moduleConfig->getIssuer(), + 'jti' => bin2hex(random_bytes(16)), + 'iat' => $now, + 'exp' => $now + 300, + ...$claims, + ], + JSON_THROW_ON_ERROR, + )); + + if (!openssl_sign($signingInput, $signature, $key, OPENSSL_ALGO_SHA256)) { + throw new RuntimeException('Could not sign a client assertion.'); + } + + return [ + 'client_assertion_type' => 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer', + 'client_assertion' => $signingInput . '.' . self::base64Url($signature), + ]; + } + + /** * The container of the module Kernel, the one the module's routes run in. Its services are private, so the ones * this test fetches are made public before they can be inlined or removed. @@ -293,6 +401,7 @@ public function process(ContainerBuilder $container): void AuthorizationServer::class, ClientEntityFactory::class, ClientRepository::class, + ModuleConfig::class, UserRepository::class, ]; @@ -307,11 +416,47 @@ public function process(ContainerBuilder $container): void } - protected static function createSigningKeyPair(string $certDir): void + protected static function base64Url(string $data): string + { + return rtrim(strtr(base64_encode($data), '+/', '-_'), '='); + } + + + protected static function createRsaKey(): OpenSSLAsymmetricKey { $key = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); - if (!mkdir($certDir, 0700, true) || $key === false || !openssl_pkey_export($key, $privateKey)) { + if ($key === false) { + throw new RuntimeException('Could not create an RSA key.'); + } + + return $key; + } + + + /** + * @return array{keys: list>} + */ + protected static function jwksOf(OpenSSLAsymmetricKey $key): array + { + $rsa = openssl_pkey_get_details($key)['rsa']; + + return ['keys' => [[ + 'kty' => 'RSA', + 'use' => 'sig', + 'alg' => 'RS256', + 'kid' => self::CLIENT_KEY_ID, + 'n' => self::base64Url($rsa['n']), + 'e' => self::base64Url($rsa['e']), + ]]]; + } + + + protected static function createSigningKeyPair(string $certDir): void + { + $key = self::createRsaKey(); + + if (!mkdir($certDir, 0700, true) || !openssl_pkey_export($key, $privateKey)) { throw new RuntimeException('Could not create a signing key pair in ' . $certDir); } From b0a5f22be073f6da1d53ebadc4203fe9f0898c49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Wed, 4 Nov 2026 18:10:53 +0100 Subject: [PATCH 3/5] Describe the 6.5 changes in the upgrade guide The release adds the option refusing the implicit flow for confidential clients. The token endpoint now answers invalid_client where it answered access_denied, which a client comparing the error member has to follow, and the reason has moved from the response to the log. Assisted-by: Claude Code (claude-opus-5-5, upgrade note) --- docs/6-oidc-upgrade.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 9732ed4a2..24ab382d2 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -3,6 +3,28 @@ This is an upgrade guide from versions 1 → 6. Review the changes and apply those relevant to your deployment. +## Version 6.4 to 6.5 + +New option, disabled by default: + +- `ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS`: if enabled, +a confidential client requesting the implicit flow is refused with the +`unauthorized_client` error, so it can only get tokens through the authorization +code flow. Public clients are not affected. + +Changes a client can see at the token endpoint, when it exchanges an +authorization code as a confidential client: + +- A client which presents no client authentication, or a client assertion which +is refused for having no JWKS to check it with, a signature which does not +verify, or the wrong Issuer or Audience, now gets `invalid_client` +(RFC 6749 section 5.2, RFC 7521 section 4.2.1). It got `access_denied`. A client +which compares the `error` member with the old value needs the new one. +- These answers stay HTTP 401, with a `WWW-Authenticate` challenge when the +request used the `Authorization` header. They no longer carry a `hint`: the +reason is in the OP log, at `warning` level, with the `client_id`. A wrong client +secret, which already got `invalid_client`, is logged the same way. + ## Version 6.4.3 to 6.4.4 Claim mappings with type `string`, including mappings without an explicit From af7c2aab3f131475dadf404e18d822fc24b84960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Wed, 4 Nov 2026 19:34:37 +0100 Subject: [PATCH 4/5] Keep the logged reason of a client refusal from forging log lines 15fb6b4 logs why client authentication failed. One reason quotes the message of the signature check, and the library's answer to an unknown algorithm quotes the alg header of an assertion nobody has verified yet. SimpleSAMLphp's file log handler writes messages as they are, so a caller with no credentials who knows the client_id of a client with a usable JWKS could write lines of their own into the log. The reason is now cut at 1000 bytes, and every run of bytes outside printable ASCII becomes one space. Stripping control characters alone would leave U+0085 and U+2028, which some consumers break lines on. wip-version-7 logs the same message as it is, in AuthenticatedOAuth2ClientResolver. A merge of this keeps that branch's rule, so the new test fails there until that line gets the same treatment. Assisted-by: Claude Code (claude-opus-5-5, implementation & integration tests) Assisted-by: Google Antigravity (gemini-3.8-flash-high, review) --- .../Rules/ClientAuthenticationRule.php | 5 +- .../ConfidentialClientTokenEndpointTest.php | 66 ++++++++++++++++++- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php index f73696324..265d8fd4c 100644 --- a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php +++ b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php @@ -57,8 +57,11 @@ public function checkRule( // The refusal of a client which does not authenticate (RFC 6749 section 5.2, RFC 7521 section 4.2.1): // invalid_client, HTTP 401, with a WWW-Authenticate challenge when the request used the Authorization header. - // The answer does not say why, the log does. + // The answer does not say why, the log does. A reason can quote what the request sent, before anything about + // the request has been verified: the library's answer to an unknown algorithm quotes the alg header. So the + // reason is kept to printable ASCII of bounded length, which leaves it no line break of any kind. $refuse = static function (string $reason) use ($client, $request, $loggerService): OidcServerException { + $reason = (string)preg_replace('/[^\x20-\x7E]+/', ' ', substr($reason, 0, 1000)); $loggerService->warning( 'Client authentication failed: ' . $reason, ['client_id' => $client->getIdentifier()], diff --git a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php index 7d2135816..30805ec41 100644 --- a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php +++ b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php @@ -10,10 +10,12 @@ use Nyholm\Psr7\ServerRequest; use OpenSSLAsymmetricKey; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use RuntimeException; use SimpleSAML\Configuration; use SimpleSAML\Kernel; +use SimpleSAML\Logger; use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Entities\UserEntity; use SimpleSAML\Module\oidc\Factories\Entities\ClientEntityFactory; @@ -140,6 +142,8 @@ protected function setUp(): void protected function tearDown(): void { $_SERVER = $this->originalServer; + Logger::setCaptureLog(false); + Logger::clearCapturedLog(); if (isset($this->client)) { $this->clientRepository->delete($this->client); @@ -260,6 +264,59 @@ public function testConfidentialClientWithClientAssertionRequiresOneWhichHolds() } + /** + * @return array + */ + public static function lineBreakProvider(): array + { + return [ + 'line feed' => ["\n"], + 'carriage return and line feed' => ["\r\n"], + 'next line' => ["\u{85}"], + 'line separator' => ["\u{2028}"], + 'escape, which starts a terminal sequence' => ["\x1b"], + ]; + } + + + /** + * The library's answer to an unknown algorithm quotes the alg header, which nobody has verified at that point, so a + * caller who knows the client_id must not be able to write lines of their own into the log that way. + */ + #[DataProvider('lineBreakProvider')] + public function testRefusedClientAssertionCanNotForgeLogLines(string $lineBreak): void + { + $code = $this->requestAuthorizationCode(); + $clientKey = self::createRsaKey(); + $this->clientRepository->update($this->createClient(self::jwksOf($clientKey))); + + Logger::setCaptureLog(true); + + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + [ + 'code' => $code, + ...$this->clientAssertionParams( + $clientKey, + [], + 'RS256' . $lineBreak . 'FORGED LINE' . str_repeat('x', 5000), + ), + ], + ); + + $logged = array_filter( + Logger::getCapturedLog(), + static fn(string $entry): bool => str_contains($entry, 'FORGED LINE'), + ); + + $this->assertNotEmpty($logged, 'The reason for the refusal is not in the log.'); + foreach ($logged as $entry) { + $this->assertStringContainsString('RS256 FORGED LINE', $entry); + $this->assertLessThan(1200, strlen($entry)); + } + } + + protected function createClient(?array $jwks = null): ClientEntityInterface { return $this->clientEntityFactory->fromData( @@ -358,11 +415,14 @@ protected function tokenRequest(array $params): ServerRequest * @param array $claims Replacing those of an assertion which holds. * @return array */ - protected function clientAssertionParams(OpenSSLAsymmetricKey $key, array $claims = []): array - { + protected function clientAssertionParams( + OpenSSLAsymmetricKey $key, + array $claims = [], + string $algorithmHeader = 'RS256', + ): array { $now = time(); $signingInput = self::base64Url(json_encode( - ['alg' => 'RS256', 'typ' => 'JWT', 'kid' => self::CLIENT_KEY_ID], + ['alg' => $algorithmHeader, 'typ' => 'JWT', 'kid' => self::CLIENT_KEY_ID], JSON_THROW_ON_ERROR, )) . '.' . self::base64Url(json_encode( [ From 3f0773de6d4c6fd0814cd6d6389af9bbd82264b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 5 Nov 2026 06:04:36 +0100 Subject: [PATCH 5/5] Answer invalid_client to a client assertion which does not parse A client assertion which did not parse left ClientAuthenticationRule as an uncaught JwsException. Its message quotes raw claim values, and SimpleSAMLphp's exception handling logs it as it is, so a caller with no credentials who knows the client_id of a confidential client could write lines of their own into the log, with no JWKS or valid signature involved. The client also got the error page instead of a token error. The rule now catches JwsException around the parse and refuses like any other failed client authentication: invalid_client, with the reason cut and reduced to printable ASCII in the log. This also covers an expired assertion, a missing claim, and a Subject other than the Issuer. The catch stops at JwsException, which the library wraps every failure of the token in, so a parser which can not start stays an error of the OP. The upgrade note also said the old answers carried a hint. They had no such member: OidcServerException appends the hint to error_description, and the note now says that. wip-version-7 refuses these in AuthenticatedOAuth2ClientResolver. Merging this keeps that branch's rule, so both log tests fail there until the line named in af7c2aa gets the same treatment. Assisted-by: Claude Code (claude-opus-5-5, implementation & integration tests) Assisted-by: OpenAI Codex (gpt-6.1-sol, xhigh review) --- docs/6-oidc-upgrade.md | 12 ++-- .../Rules/ClientAuthenticationRule.php | 15 +++- .../ConfidentialClientTokenEndpointTest.php | 71 ++++++++++++++++--- 3 files changed, 82 insertions(+), 16 deletions(-) diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 24ab382d2..c0e8028cc 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -20,10 +20,14 @@ is refused for having no JWKS to check it with, a signature which does not verify, or the wrong Issuer or Audience, now gets `invalid_client` (RFC 6749 section 5.2, RFC 7521 section 4.2.1). It got `access_denied`. A client which compares the `error` member with the old value needs the new one. -- These answers stay HTTP 401, with a `WWW-Authenticate` challenge when the -request used the `Authorization` header. They no longer carry a `hint`: the -reason is in the OP log, at `warning` level, with the `client_id`. A wrong client -secret, which already got `invalid_client`, is logged the same way. +- A client assertion which can not be parsed, which includes one that has expired, +lacks a required claim or has a Subject other than its Issuer, gets +`invalid_client` too. It ended on the SimpleSAMLphp error page, with HTTP 500. +- All of these answers are HTTP 401, with a `WWW-Authenticate` challenge when the +request used the `Authorization` header. Their `error_description` does not say +why the client was refused, which that of `access_denied` did: the reason is in +the OP log, at `warning` level, with the `client_id`. A wrong client secret, which +already got `invalid_client`, is logged the same way. ## Version 6.4.3 to 6.4.4 diff --git a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php index 265d8fd4c..6416ffeb5 100644 --- a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php +++ b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php @@ -19,6 +19,7 @@ use SimpleSAML\OpenID\Codebooks\ClientAssertionTypesEnum; use SimpleSAML\OpenID\Codebooks\HttpMethodsEnum; use SimpleSAML\OpenID\Codebooks\ParamsEnum; +use SimpleSAML\OpenID\Exceptions\JwsException; class ClientAuthenticationRule extends AbstractRule { @@ -58,8 +59,9 @@ public function checkRule( // The refusal of a client which does not authenticate (RFC 6749 section 5.2, RFC 7521 section 4.2.1): // invalid_client, HTTP 401, with a WWW-Authenticate challenge when the request used the Authorization header. // The answer does not say why, the log does. A reason can quote what the request sent, before anything about - // the request has been verified: the library's answer to an unknown algorithm quotes the alg header. So the - // reason is kept to printable ASCII of bounded length, which leaves it no line break of any kind. + // the request has been verified: the library's answer to an unknown algorithm quotes the alg header, and its + // answer to a claim of the wrong type quotes the claim. So the reason is kept to printable ASCII of bounded + // length, which leaves it no line break of any kind. $refuse = static function (string $reason) use ($client, $request, $loggerService): OidcServerException { $reason = (string)preg_replace('/[^\x20-\x7E]+/', ' ', substr($reason, 0, 1000)); $loggerService->warning( @@ -107,7 +109,14 @@ public function checkRule( throw OidcServerException::invalidRequest(ParamsEnum::ClientAssertionType->value); } - $clientAssertion = $this->requestParamsResolver->parseClientAssertionToken($clientAssertionParam); + // The library answers whatever is wrong with the token itself with a JwsException: a token which does not + // parse, a claim which is missing, of the wrong type or expired, or an Issuer other than the Subject. That is + // the client's fault. Anything else, such as the parser failing to start, is the OP's and stays an error. + try { + $clientAssertion = $this->requestParamsResolver->parseClientAssertionToken($clientAssertionParam); + } catch (JwsException $exception) { + throw $refuse('Client Assertion could not be parsed: ' . $exception->getMessage()); + } // Check if the Client Assertion token has already been used. Only applicable if we have cache available. if ($this->protocolCache) { diff --git a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php index 30805ec41..2bfde9199 100644 --- a/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php +++ b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php @@ -244,6 +244,18 @@ public function testConfidentialClientWithClientAssertionRequiresOneWhichHolds() $this->clientRepository->update($this->createClient(self::jwksOf($clientKey))); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, ...$this->clientAssertionParams($clientKey), 'client_assertion' => 'not-a-jws'], + ); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, ...$this->clientAssertionParams($clientKey, ['exp' => time() - 3600])], + ); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, ...$this->clientAssertionParams($clientKey, ['sub' => 'another-client'])], + ); $this->assertTokenRequestRefused( self::ERROR_CLIENT_AUTHENTICATION_FAILED, ['code' => $code, ...$this->clientAssertionParams(self::createRsaKey())], @@ -304,16 +316,33 @@ public function testRefusedClientAssertionCanNotForgeLogLines(string $lineBreak) ], ); - $logged = array_filter( - Logger::getCapturedLog(), - static fn(string $entry): bool => str_contains($entry, 'FORGED LINE'), + $this->assertForgedLineLoggedAsPartOfTheReason('RS256 FORGED LINE'); + } + + + /** + * The library's answer to a claim of the wrong type quotes the claim. This one is the easier way in: the client + * needs no JWKS and the signature does not matter, as parsing comes before both. + */ + #[DataProvider('lineBreakProvider')] + public function testClientAssertionWhichDoesNotParseCanNotForgeLogLines(string $lineBreak): void + { + $code = $this->requestAuthorizationCode(); + + Logger::setCaptureLog(true); + + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + [ + 'code' => $code, + ...$this->clientAssertionParams( + self::createRsaKey(), + ['jti' => ['claim' . $lineBreak . 'FORGED LINE' . str_repeat('x', 5000)]], + ), + ], ); - $this->assertNotEmpty($logged, 'The reason for the refusal is not in the log.'); - foreach ($logged as $entry) { - $this->assertStringContainsString('RS256 FORGED LINE', $entry); - $this->assertLessThan(1200, strlen($entry)); - } + $this->assertForgedLineLoggedAsPartOfTheReason('claim FORGED LINE'); } @@ -408,11 +437,35 @@ protected function tokenRequest(array $params): ServerRequest } + /** + * The reason for the refusal is in the log, and what it quotes did not get a line of its own: every entry holding + * the text is short, has it joined to what came before it, and has no line break or other control character ahead + * of it. The entry may have line breaks after it, as LoggerService writes the context there with var_export(). + */ + protected function assertForgedLineLoggedAsPartOfTheReason(string $expectedQuote): void + { + $logged = array_filter( + Logger::getCapturedLog(), + static fn(string $entry): bool => str_contains($entry, 'FORGED LINE'), + ); + + $this->assertNotEmpty($logged, 'The reason for the refusal is not in the log.'); + foreach ($logged as $entry) { + $this->assertStringContainsString($expectedQuote, $entry); + $this->assertDoesNotMatchRegularExpression( + '/[^\x20-\x7E]/', + (string) strstr($entry, 'FORGED LINE', true), + ); + $this->assertLessThan(1200, strlen($entry)); + } + } + + /** * The parameters of a client assertion signed with the key, built by hand so that what goes over the wire is * what this test wrote. * - * @param array $claims Replacing those of an assertion which holds. + * @param array $claims Replacing those of an assertion which holds. * @return array */ protected function clientAssertionParams(