diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 9732ed4a2..c0e8028cc 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -3,6 +3,32 @@ 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. +- 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 Claim mappings with type `string`, including mappings without an explicit 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..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 { @@ -55,6 +56,22 @@ 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. 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, 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( + '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 +81,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 +94,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. @@ -92,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) { @@ -103,23 +127,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 +155,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 new file mode 100644 index 000000000..2bfde9199 --- /dev/null +++ b/tests/integration/src/Flows/ConfidentialClientTokenEndpointTest.php @@ -0,0 +1,588 @@ +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->clientEntityFactory = $container->get(ClientEntityFactory::class); + $this->moduleConfig = $container->get(ModuleConfig::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 = $this->createClient(); + $this->clientRepository->add($this->client); + } + + + protected function tearDown(): void + { + $_SERVER = $this->originalServer; + Logger::setCaptureLog(false); + Logger::clearCapturedLog(); + + 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 = self::base64Url(hash('sha256', $codeVerifier, true)); + + $code = $this->requestAuthorizationCode([ + 'code_challenge' => $codeChallenge, + 'code_challenge_method' => 'S256', + ]); + + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, 'code_verifier' => $codeVerifier], + ); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['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_CLIENT_AUTHENTICATION_FAILED, ['code' => $code]); + $this->assertTokenRequestRefused( + self::ERROR_CLIENT_AUTHENTICATION_FAILED, + ['code' => $code, 'client_secret' => 'not-the-secret'], + ); + + $this->assertTokensIssued(['code' => $code, 'client_secret' => self::CLIENT_SECRET]); + } + + + /** + * 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($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())], + ); + $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)]); + } + + + /** + * @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), + ), + ], + ); + + $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->assertForgedLineLoggedAsPartOfTheReason('claim FORGED LINE'); + } + + + 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 + */ + 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 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. + * @return array + */ + protected function clientAssertionParams( + OpenSSLAsymmetricKey $key, + array $claims = [], + string $algorithmHeader = 'RS256', + ): array { + $now = time(); + $signingInput = self::base64Url(json_encode( + ['alg' => $algorithmHeader, '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. + */ + 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, + ModuleConfig::class, + UserRepository::class, + ]; + + foreach ($serviceIds as $serviceId) { + $container->getDefinition($serviceId)->setPublic(true); + } + } + }; + $kernel->boot(); + + return $kernel->getContainer(); + } + + + 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 ($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); + } + + $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); + } + } +}