diff --git a/CHANGELOG.md b/CHANGELOG.md index dee675c..321f9c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Mutation testing with [Infection](https://infection.github.io/) (`task test:mutation`), run in CI and reported to the Stryker dashboard (mutation score badge in README) +- PKCE support (RFC 7636), S256 only. `generatePkceVerifier()` returns a + 128-character verifier and `getPkceChallenge()` derives its challenge; + `getIdToken()` takes the verifier as an optional second argument and sends + `code_verifier` when given. PKCE is opt-in with no configuration flag — + passing a `code_challenge` to `getAuthorizationUrl()` turns it on, and + `code_challenge_method=S256` is filled in automatically, since omitting it + makes the server assume `plain` (RFC 7636 §4.3) and treat the challenge as a + cleartext value. Neither the verifier nor the challenge is stored on the + provider: `league/oauth2-client` keeps its verifier on the provider object, + which on an instance shared between requests would let one request's verifier + be sent for another request's token exchange, so the verifier is carried by + the caller in the session exactly like the state and the nonce - `phpstan-lowest` CI job and the matching `task analyze:php:lowest`, analysing the declared dependency floor with current dev tooling. Only the packages `require` names are lowered, so findings are about the runtime dependencies diff --git a/README.md b/README.md index b451477..2920c72 100644 --- a/README.md +++ b/README.md @@ -217,6 +217,47 @@ normative sentence names access tokens, so it does not literally cover a bare `id_token` response — but `code` is the flow it points at, and the one [openid-connect-bundle](https://github.com/itk-dev/openid-connect-bundle) uses. +##### PKCE (RFC 7636) + +PKCE is opt-in, and there is no configuration flag: passing a `code_challenge` +turns it on, omitting it changes nothing. + +The verifier is a secret and belongs in the session alongside the state and the +nonce. Only its challenge may reach the authorization request. + +```php +// Authorization request +$verifier = $provider->generatePkceVerifier(); +$session->set('oauth2pkce', $verifier); + +$authUrl = $provider->getAuthorizationUrl([ + 'state' => $state, + 'nonce' => $nonce, + 'response_type' => 'code', + 'code_challenge' => $provider->getPkceChallenge($verifier), +]); +``` + +`code_challenge_method=S256` is filled in for you. Omitting it would make the +server assume `plain` (RFC 7636 §4.3) and treat the challenge as a value sent in +the clear, so it is not left to the caller to remember. + +On the way back, hand the verifier to the code exchange: + +```php +$verifier = $session->get('oauth2pkce'); +$session->remove('oauth2pkce'); + +$idToken = $provider->getIdToken($request->query->get('code'), $verifier); +$claims = $provider->validateIdToken($idToken, $session->get('oauth2nonce')); +``` + +`generatePkceVerifier()` stores nothing on the provider, which is deliberate. +`league/oauth2-client` keeps its own verifier on the provider object; on an +instance shared between requests that would let one request's verifier be sent +for another request's exchange — the same hazard as reading `getState()` back. +The session is the only place the verifier should live. + #### Verify authorized requests The authorization service will redirect the user back to the `redirectUri`. This diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index e7e05b2..7256251 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -54,6 +54,17 @@ class OpenIdConfigurationProvider extends AbstractProvider // decode and cache. private const int MAX_JSON_RESOURCE_BYTES = 1048576; + // PKCE (RFC 7636). S256 is the only challenge method offered: "plain" is a + // downgrade with no reason to exist where sha256 is available. + // + // RFC 7636 §4.1 allows a verifier of 43 to 128 characters and servers must + // support the whole range, so the verifier is always generated at the + // maximum — a shorter one has nothing to recommend it. 96 random bytes + // base64url-encode to exactly 128 characters with no padding, which is why + // that byte count and no other. + private const string PKCE_CHALLENGE_METHOD = 'S256'; + private const int PKCE_VERIFIER_BYTES = 96; + // @see https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout private const string POST_LOGOUT_REDIRECT_URI = 'post_logout_redirect_uri'; private const string ID_TOKEN_HINT = 'id_token_hint'; @@ -168,6 +179,14 @@ public function getAuthorizationUrl(array $options = []): string throw new MissingParameterException('Required parameter "nonce" missing'); } + // PKCE (RFC 7636) is opt-in: passing a code_challenge turns it on. The + // method is filled in here because omitting it makes the server assume + // "plain" (RFC 7636 §4.3), which would silently downgrade an S256 + // challenge to a value sent in the clear. + if (!empty($options['code_challenge'])) { + $options += ['code_challenge_method' => self::PKCE_CHALLENGE_METHOD]; + } + // Add default response_type and response_mode. The `scope` default is // supplied by getDefaultScopes() via league's // getAuthorizationParameters(), so it is not repeated here. @@ -385,18 +404,27 @@ private static function requireStringClaim(object $claims, string $name): string * * @throws OpenIdConnectExceptionInterface */ - public function getIdToken(string $code): string + public function getIdToken(string $code, ?string $codeVerifier = null): string { try { $endpoint = $this->getSecureEndpoint('token_endpoint'); + + $params = [ + 'client_id' => $this->clientId, + 'client_secret' => $this->clientSecret, + 'redirect_uri' => $this->redirectUri, + 'grant_type' => 'authorization_code', + 'code' => $code, + ]; + + // Sent only when the caller holds one. An unsolicited code_verifier + // is an error to a server that issued no challenge. + if (null !== $codeVerifier) { + $params['code_verifier'] = $codeVerifier; + } + $response = $this->getHttpClient()->request('POST', $endpoint, [ - 'form_params' => [ - 'client_id' => $this->clientId, - 'client_secret' => $this->clientSecret, - 'redirect_uri' => $this->redirectUri, - 'grant_type' => 'authorization_code', - 'code' => $code, - ], + 'form_params' => $params, ]); $payload = json_decode((string) $response->getBody(), true, 512, JSON_THROW_ON_ERROR); @@ -466,6 +494,43 @@ public function generateNonce(int $length = 32): string * @throws JsonException * @throws MetadataException */ + /** + * Generate a PKCE code verifier (RFC 7636 §4.1). + * + * Nothing is stored on the provider. As with `generateNonce()`, the caller + * is the only holder — and here that is not merely tidy. league's own PKCE + * support keeps the verifier on the provider instance, which on an instance + * shared between requests (a long-running worker, or a container that + * memoizes the service) would let one request's verifier be sent for + * another's token exchange. Persist this in the session next to the state + * and the nonce, and hand it back to `getIdToken()`. + * + * The length is not configurable: RFC 7636 caps the verifier at 128 + * characters and requires servers to accept that, so there is no reason to + * ask for less entropy. + * + * @return string A 128-character code verifier + */ + public function generatePkceVerifier(): string + { + return self::base64urlEncode(random_bytes(self::PKCE_VERIFIER_BYTES)); + } + + /** + * Derive the S256 code challenge for a verifier (RFC 7636 §4.2). + * + * A pure function of its argument: no provider state is read or written, so + * the verifier never comes to rest on this object. + * + * @param string $verifier The code verifier to derive a challenge for + * + * @return string The S256 challenge, for the `code_challenge` authorization parameter + */ + public function getPkceChallenge(string $verifier): string + { + return self::base64urlEncode(hash('sha256', $verifier, true)); + } + public function getBaseAccessTokenUrl(array $params): string { return $this->getSecureEndpoint('token_endpoint'); @@ -620,6 +685,14 @@ private function getJwksDocument(): array } } + /** + * Encode base 64 url, without padding (RFC 7515 §2). + */ + private static function base64urlEncode(string $input): string + { + return rtrim(strtr(base64_encode($input), '+/', '-_'), '='); + } + /** * Decode base 64 url. * diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index fd7ce31..cb42277 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -238,6 +238,84 @@ public function testGetAuthorizationUrl(): void $this->assertSame($nonce, $query['nonce']); } + /** + * RFC 7636 Appendix B publishes a verifier/challenge pair; deriving anything + * else means the challenge would not match at the token endpoint. + */ + public function testGetPkceChallengeMatchesTheRfcTestVector(): void + { + $this->assertSame( + 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + $this->provider->getPkceChallenge('dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk') + ); + } + + public function testGeneratePkceVerifierProducesAMaximumLengthVerifier(): void + { + $verifier = $this->provider->generatePkceVerifier(); + + // RFC 7636 §4.1 caps the verifier at 128 characters and restricts it to + // the unreserved set. + $this->assertSame(128, strlen($verifier)); + $this->assertMatchesRegularExpression('/^[A-Za-z0-9\-._~]+$/', $verifier); + $this->assertNotSame($verifier, $this->provider->generatePkceVerifier(), 'Each call must produce a fresh verifier'); + } + + /** + * Omitting code_challenge_method makes the server assume "plain" + * (RFC 7636 §4.3), so passing a challenge must carry S256 with it. + */ + public function testGetAuthorizationUrlAddsS256MethodWithAChallenge(): void + { + $verifier = $this->provider->generatePkceVerifier(); + $challenge = $this->provider->getPkceChallenge($verifier); + + $query = $this->authorizationUrlQuery(['code_challenge' => $challenge]); + + $this->assertSame($challenge, $query['code_challenge']); + $this->assertSame('S256', $query['code_challenge_method']); + } + + /** + * The verifier is the secret half of the exchange. It is carried in the + * caller's session and must never reach the authorization request. + */ + public function testGetAuthorizationUrlNeverCarriesTheVerifier(): void + { + $verifier = $this->provider->generatePkceVerifier(); + + $authUrl = $this->provider->getAuthorizationUrl([ + 'state' => '12345678', + 'nonce' => 'abcdefghij', + 'code_challenge' => $this->provider->getPkceChallenge($verifier), + ]); + + $this->assertStringNotContainsString($verifier, $authUrl); + $this->assertStringNotContainsString('code_verifier', $authUrl); + } + + public function testGetAuthorizationUrlOmitsPkceWithoutAChallenge(): void + { + $query = $this->authorizationUrlQuery([]); + + $this->assertArrayNotHasKey('code_challenge', $query); + $this->assertArrayNotHasKey('code_challenge_method', $query); + } + + /** + * A caller who names the method keeps it, so a provider needing "plain" + * is not locked out by the default. + */ + public function testGetAuthorizationUrlKeepsAnExplicitChallengeMethod(): void + { + $query = $this->authorizationUrlQuery([ + 'code_challenge' => 'challenge-value', + 'code_challenge_method' => 'plain', + ]); + + $this->assertSame('plain', $query['code_challenge_method']); + } + public function testGetAuthorizationUrlStateException(): void { $this->expectException(MissingParameterException::class); @@ -917,6 +995,61 @@ public function testGetIdTokenSuccess(): void $this->assertSame('the-id-token', $idToken); } + /** + * With a verifier the exchange carries `code_verifier`, and only then: the + * request map below matches on exact form parameters, so an unconditionally + * added key would fail to match and the stub would return null. That is what + * pins both halves of the condition. + */ + public function testGetIdTokenSendsCodeVerifierWhenGiven(): void + { + $tokenEndpoint = 'https://azure_b2c_test.b2clogin.com/azure_b2c_test.onmicrosoft.com/oauth2/v2.0/token?p=test-policy'; + $openIDConnectMetadataUrl = 'https://provider.example.org/openid-configuration'; + $verifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + + $mockConfigResponse = $this->getMockHttpSuccessResponse('/../MockData/mockOpenIDConfiguration.json'); + + $tokenResponseBody = json_encode(['id_token' => 'the-id-token']); + $mockTokenStream = $this->createStub(StreamInterface::class); + $mockTokenStream->method('getContents')->willReturn($tokenResponseBody); + $mockTokenStream->method('__toString')->willReturn($tokenResponseBody); + + $mockTokenResponse = $this->createStub(ResponseInterface::class); + $mockTokenResponse->method('getStatusCode')->willReturn(200); + $mockTokenResponse->method('getBody')->willReturn($mockTokenStream); + + $mockHttpClient = $this->createStub(ClientInterface::class); + $mockHttpClient->method('request')->willReturnMap([ + ['GET', $openIDConnectMetadataUrl, [], $mockConfigResponse], + ['POST', $tokenEndpoint, ['form_params' => [ + 'client_id' => self::CLIENT_ID, + 'client_secret' => self::CLIENT_SECRET, + 'redirect_uri' => self::REDIRECT_URI, + 'grant_type' => 'authorization_code', + 'code' => 'test-code', + 'code_verifier' => $verifier, + ]], $mockTokenResponse], + ]); + + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(false); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + $provider = new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => $openIDConnectMetadataUrl, + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $mockHttpClient, + ]); + + $this->assertSame('the-id-token', $provider->getIdToken('test-code', $verifier)); + } + public function testGetIdTokenFailure(): void { $openIDConnectMetadataUrl = 'https://provider.example.org/openid-configuration'; @@ -1978,6 +2111,30 @@ private function discoveryDocumentOfExactByteSize(int $bytes): string return $json; } + /** + * Build an authorization URL with state and nonce supplied, and return its + * query parameters. + * + * @param array $options extra authorization parameters + * + * @return array|string> as parse_str() populates it + */ + private function authorizationUrlQuery(array $options): array + { + $authUrl = $this->provider->getAuthorizationUrl($options + [ + 'state' => '12345678', + 'nonce' => 'abcdefghij', + ]); + + $queryString = parse_url($authUrl, PHP_URL_QUERY); + $this->assertIsString($queryString, 'Generated authorization URL must have a query string'); + + $query = []; + parse_str($queryString, $query); + + return $query; + } + private function loadMockFixture(string $filename): array { $path = __DIR__.'/../MockData/'.$filename;