diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d58bad..cccd3f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- PKCE (RFC 7636, S256), on by default. The login route generates a verifier, keeps it + in the session under `oauth2pkce_verifier`, and sends the challenge; the + authenticator redeems the code with it. Turn it off per provider with `pkce: false` + for an identity provider that rejects the parameters rather than ignoring them. +- `OpenIdConfigurationProviderManager::isPkceEnabled()`. - `ProviderErrorException`, thrown when the identity provider refuses the authorization request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and @@ -24,14 +29,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Requires `itk-dev/openid-connect` `^5.1`, for its PKCE support. That release also + enforces `allowHttp` on every discovered endpoint, requires `exp` and `iat` on the + ID token, and changes the JWKS cache key — see its changelog before upgrading. +- `getProvider()` returns a fresh provider on every call instead of a memoized one. + `league/oauth2-client` writes the authorization request's `state` onto the provider, + so a held instance carried one request's state into the next — harmless today, but + not under a worker runtime where the process outlives the request. The HTTP client + is now what is kept per provider, so the connection pool still survives. - A refused login is answered with the status that matches its cause: 403 where the user or a policy declined, 503 where the provider reports its own trouble, 500 otherwise. Other callback failures are unchanged and still surface as 500. - `error` and `error_description` are sanitized before they are logged or held — control characters collapsed, invalid UTF-8 dropped, capped at 200 characters — and neither is read at all until the callback's state matches. -- `oauth2provider`, `oauth2state` and `oauth2nonce` are consumed on every callback, - including one carrying a provider error. +- Every one-time session value is consumed on every callback, including one carrying a + provider error: `oauth2provider`, `oauth2state`, `oauth2nonce` and + `oauth2pkce_verifier`. - The stored state is compared with `hash_equals()`, and an empty or missing stored state is rejected explicitly rather than by comparison. - A callback naming a provider that is not configured is now reported as an invalid diff --git a/README.md b/README.md index 96afa7b..76e0b13 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,9 @@ itkdev_openid_connect: # Optional: Cache duration (seconds) for the OIDC discovery document and JWKS # Defaults to 86400 (24 hours) cache_duration: '%env(int:ADMIN_OIDC_CACHE_DURATION)%' + # Optional: Send a PKCE challenge (RFC 7636, S256) with the authorization + # request. Defaults to true. See "PKCE" below. + pkce: true # Optional: Allow (non-secure) http requests (used for mocking a IdP). NOT RECOMMENDED FOR PRODUCTION. # Defaults to false allow_http: '%env(bool:ADMIN_OIDC_ALLOW_HTTP)%' @@ -884,6 +887,30 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator } ``` +### PKCE + +The bundle sends a PKCE challenge (RFC 7636, S256) with every authorization request. +The login route generates a verifier, keeps it in the session, and sends only its +SHA-256 challenge; the authenticator redeems the authorization code with the verifier. +An intercepted code is then useless to whoever intercepted it, because they do not +have the verifier. + +It is on by default and needs no configuration. RFC 6749 §3.1 requires an +authorization server to ignore parameters it does not recognise, so an identity +provider that has never heard of PKCE behaves exactly as it did before. Turn it off +only for one that rejects the parameters outright: + +```yaml +openid_providers: + legacy: + options: + pkce: false +``` + +The verifier lives in the session alongside the state and the nonce, and is consumed +on every callback — success, failure or refusal — so it can never be redeemed against +a code it does not belong to. + ### When the identity provider refuses A provider that will not issue a code redirects back to the callback with an `error` diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md index 10d2e68..cf46e95 100644 --- a/UPGRADE-6.1.md +++ b/UPGRADE-6.1.md @@ -55,5 +55,46 @@ refusal arrives as a plain 500 with the reason only in the message. This was alw the documented shape; 6.1 is the first release where dropping the cause costs you something. -See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning, and -[CHANGELOG.md](CHANGELOG.md) for the rest of the release. +## PKCE is on by default + +Every authorization request now carries a PKCE challenge (RFC 7636, S256). RFC 6749 +§3.1 requires an authorization server to ignore parameters it does not recognise, so a +provider that does not support PKCE behaves as it did before, and one that does gets +the extra protection with no configuration from you. + +If you have an identity provider that rejects unknown parameters rather than ignoring +them, turn it off for that provider: + +```yaml +openid_providers: + legacy: + options: + pkce: false +``` + +The verifier is kept in the session under `oauth2pkce_verifier`. If your application +clears or rewrites the session between the login redirect and the callback, it must +preserve that key alongside `oauth2state` and `oauth2nonce`. + +## The library requires 5.1 + +`itk-dev/openid-connect` `^5.1` comes with this release. Three of its changes affect +running deployments: an identity provider announcing plain-http endpoints now needs +`allow_http`, an ID token without `exp` or `iat` is rejected, and the JWKS cache key +changed so 5.0's entries are not reused. Read its changelog before deploying. + +## `getProvider()` no longer returns the same instance + +`OpenIdConfigurationProviderManager::getProvider()` builds a fresh provider on every +call. `league/oauth2-client` records the authorization request's `state` on the +provider, so a memoized instance carried one request's state into the next — which +matters once a process outlives a request, as under a FrankenPHP worker. The HTTP +client is kept per provider instead, so connections to the identity provider are still +reused. + +Nothing to do unless you held the returned provider and relied on getting the same +object back. + +See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning behind +the error-callback handling, and [CHANGELOG.md](CHANGELOG.md) for the rest of the +release. diff --git a/composer.json b/composer.json index f925474..621365b 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,7 @@ "ext-json": "*", "ext-openssl": "*", "doctrine/orm": "^2.8 || ^3.0", - "itk-dev/openid-connect": "^5.0", + "itk-dev/openid-connect": "^5.1", "psr/log": "^3.0", "symfony/cache": "^6.4 || ^7.0 || ^8.0", "symfony/clock": "^6.4 || ^7.0 || ^8.0", diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 6cac73f..ecd7a05 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -58,6 +58,11 @@ public function login(Request $request, SessionInterface $session, string $provi $nonce = $provider->generateNonce(); $state = $provider->generateState(); + // The verifier is kept, the challenge is sent. Only the holder of the + // verifier can redeem the code, which is what makes a code intercepted in + // transit useless to whoever intercepted it (RFC 7636). + $pkceVerifier = $this->providerManager->isPkceEnabled($providerKey) ? $provider->generatePkceVerifier() : null; + $this->rememberNamedTargetPath($request, $session); // Save to session @@ -65,13 +70,26 @@ public function login(Request $request, SessionInterface $session, string $provi $session->set('oauth2state', $state); $session->set('oauth2nonce', $nonce); + // Written on every login, null included. The key names the verifier for this + // login and nothing else: leaving an abandoned login's verifier in place + // would offer it up against a code it does not belong to. + $session->set('oauth2pkce_verifier', $pkceVerifier); + try { - $authUrl = $provider->getAuthorizationUrl([ + $options = [ 'state' => $state, 'nonce' => $nonce, 'response_type' => 'code', 'scope' => 'openid email profile', - ]); + ]; + + if (null !== $pkceVerifier) { + // Passing a challenge is what turns PKCE on in the library; it adds + // code_challenge_method=S256 alongside. + $options['code_challenge'] = $provider->getPkceChallenge($pkceVerifier); + } + + $authUrl = $provider->getAuthorizationUrl($options); } catch (OpenIdConnectExceptionInterface $e) { // Building the authorization URL fetches the IdP's discovery // document. Surface upstream/transport/cache failures as 503 with diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index bd01dc1..d408470 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -163,6 +163,15 @@ public function getConfigTreeBuilder(): TreeBuilder ->info('Cache duration in seconds for the OIDC discovery document and JWKS (default: 86400 — 24 hours)') ->defaultValue(86400) ->end() + ->booleanNode('pkce') + // On by default: RFC 6749 §3.1 requires an authorization + // server to ignore parameters it does not recognise, so a + // challenge costs nothing at an identity provider that does + // not support PKCE. Turn it off for one that rejects + // unknown parameters outright. + ->info('Send a PKCE challenge (RFC 7636, S256) with the authorization request') + ->defaultTrue() + ->end() ->scalarNode('redirect_uri') ->info('Redirect URI registered at identity provider') ->cannotBeEmpty() diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index d3941ba..bb4b233 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -2,6 +2,7 @@ namespace ItkDev\OpenIdConnectBundle\Security; +use GuzzleHttp\Client as GuzzleClient; use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; @@ -11,10 +12,29 @@ class OpenIdConfigurationProviderManager { - /** @var array */ - private array $providers = []; + /** + * One HTTP client per provider, for the life of the process. + * + * The client owns the connection pool, so sharing it across logins means a token + * exchange reuses an open connection to the identity provider rather than + * renegotiating TLS. Its options come from configuration and never change, and it + * holds nothing belonging to a request, so it is safe to share however long the + * process lives. + * + * @var array + */ + private array $httpClients = []; - /** @var array> */ + /** + * Callback paths, keyed by the routing context's base URL. + * + * Safe to hold across requests: the values are a pure function of configuration + * and that base URL, so two requests sharing a key derive identical paths. The + * key set is bounded by the number of base URLs an application answers on, which + * is one in every deployment shape the bundle documents. + * + * @var array> + */ private array $redirectUriPaths = []; /** @@ -30,6 +50,7 @@ class OpenIdConfigurationProviderManager * callback_path?: string, * leeway?: int, * cache_duration?: int, + * pkce?: bool, * allow_http?: bool, * http_client_options?: array{ * timeout?: float, @@ -55,6 +76,21 @@ public function getProviderKeys(): array return array_keys($this->config['providers']); } + /** + * Whether this provider's authorization request carries a PKCE challenge. + * + * Read from configuration, like the callback paths: the caller needs the answer + * before it has any use for a provider. + * + * An unconfigured key answers true. The config tree makes that unreachable, since + * `pkce` defaults to true there, and matching it here keeps one rule in two places + * from drifting apart. + */ + public function isPkceEnabled(string $providerKey): bool + { + return $this->config['providers'][$providerKey]['pkce'] ?? true; + } + /** * The request path each provider's callback arrives on, keyed by provider. * @@ -166,11 +202,19 @@ private function normalizePath(string $path): string /** * Get a provider by key. * + * A fresh instance every call. A provider belongs to one request: + * `league/oauth2-client` assigns `$this->state` on every `getAuthorizationUrl()` + * call, so an instance shared between requests holds the previous one's state. + * + * Constructing one is cheap. Discovery and JWKS are lazy and cached in the PSR-6 + * pool, and the costly collaborator — the HTTP client, with its connection pool — + * comes from `$httpClients`. + * * @throws OpenIdConnectExceptionInterface */ public function getProvider(string $key): OpenIdConfigurationProvider { - if (!isset($this->providers[$key]) && isset($this->config['providers'][$key])) { + if (isset($this->config['providers'][$key])) { $options = $this->config['providers'][$key]; $providerOptions = [ 'openIDConnectMetadataUrl' => $options['metadata_url'], @@ -204,13 +248,46 @@ public function getProvider(string $key): OpenIdConfigurationProvider $providerOptions += $options['http_client_options']; } - $this->providers[$key] = new OpenIdConfigurationProvider($providerOptions); + return new OpenIdConfigurationProvider( + $providerOptions, + ['httpClient' => $this->httpClient($key, $providerOptions)], + ); } - if (isset($this->providers[$key])) { - return $this->providers[$key]; + throw new InvalidProviderException(sprintf('Invalid provider: %s', $key)); + } + + /** + * The HTTP client for a provider, built once and shared by its providers. + * + * `league/oauth2-client` builds a client per provider instance, which would mean + * a new connection pool per request; building it here gives every provider for a + * key the same one. + * + * The option filter mirrors `AbstractProvider::getAllowedClientOptions()`, so the + * client is configured exactly as league configures its own: `timeout` and + * `proxy` always, `verify` only alongside a proxy — league's rule that TLS + * verification may be relaxed for a proxy and nowhere else. + * + * @param array $providerOptions + */ + private function httpClient(string $key, array $providerOptions): GuzzleClient + { + if (isset($this->httpClients[$key])) { + return $this->httpClients[$key]; } - throw new InvalidProviderException(sprintf('Invalid provider: %s', $key)); + $allowed = ['timeout', 'proxy']; + + // `proxy` is a scalar node, so it arrives as a string or not at all; an empty + // one names no proxy. This matches league's own `empty()` test for every + // value the config tree can produce. + if (isset($providerOptions['proxy']) && '' !== $providerOptions['proxy']) { + $allowed[] = 'verify'; + } + + return $this->httpClients[$key] = new GuzzleClient( + array_intersect_key($providerOptions, array_flip($allowed)) + ); } } diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index 0f22488..d3411d6 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -202,11 +202,13 @@ protected function validateClaims(Request $request): array // Every one-time value is spent here, before anything below can throw. A // callback is used up whether it succeeds, fails validation, or carries the // provider's refusal, and a value left behind is one a later request can - // replay. + // replay. The PKCE verifier belongs to that set: a verifier surviving a + // failed callback could be redeemed against a later code. $providerKey = $session->remove('oauth2provider'); $providerKey = is_string($providerKey) ? $providerKey : ''; $oauth2state = $session->remove('oauth2state'); $oauth2nonce = $session->remove('oauth2nonce'); + $pkceVerifier = $session->remove('oauth2pkce_verifier'); // The session entry is removed above, so carry the provider key on the // request for anything downstream that needs to attribute this login — @@ -280,7 +282,11 @@ protected function validateClaims(Request $request): array throw new ValidationException('Missing or invalid code'); } - $idToken = $provider->getIdToken($code); + // Null where no challenge was sent: the provider has PKCE turned off, or + // the login began under a session that never stored a verifier. The token + // request then carries no code_verifier, which is what an identity + // provider that received no challenge expects. + $idToken = $provider->getIdToken($code, is_string($pkceVerifier) ? $pkceVerifier : null); $claims = $provider->validateIdToken($idToken, $oauth2nonce); // Authentication successful } catch (OpenIdConnectExceptionInterface $exception) { diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 10c6641..2f475d4 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -68,7 +68,7 @@ public function testLogin(): void $request = new Request(query: ['provider' => 'test']); $mockSession = $this->createMock(SessionInterface::class); - $matcher = $this->exactly(3); + $matcher = $this->exactly(4); $mockSession ->expects($matcher) ->method('set')->willReturnCallback(function (...$parameters) use ($matcher) { @@ -84,6 +84,12 @@ public function testLogin(): void $this->assertEquals('oauth2nonce', $parameters[0]); $this->assertEquals('1234', $parameters[1]); } + if (4 === $matcher->numberOfInvocations()) { + // Written even with PKCE off, so a verifier left by an earlier + // login cannot be redeemed against this one's code. + $this->assertEquals('oauth2pkce_verifier', $parameters[0]); + $this->assertNull($parameters[1]); + } }); $response = $controller->login($request, $mockSession, 'test'); @@ -91,6 +97,83 @@ public function testLogin(): void $this->assertSame([], $this->logger->records, 'A successful login must not log a failure.'); } + public function testPkceSendsAChallengeAndKeepsTheVerifier(): void + { + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider->method('generateNonce')->willReturn('1234'); + $mockProvider->method('generateState')->willReturn('abcd'); + $mockProvider + ->expects($this->once()) + ->method('generatePkceVerifier') + ->willReturn('the-verifier'); + $mockProvider + ->expects($this->once()) + ->method('getPkceChallenge') + ->with('the-verifier') + ->willReturn('the-challenge'); + $mockProvider + ->expects($this->once()) + ->method('getAuthorizationUrl') + ->with([ + 'state' => 'abcd', + 'nonce' => '1234', + 'response_type' => 'code', + 'scope' => 'openid email profile', + // The library adds code_challenge_method=S256 alongside this. + 'code_challenge' => 'the-challenge', + ]) + ->willReturn('https://provider.example.org/authorize'); + + $controller = $this->createController($mockProvider, pkce: true); + $session = new Session(new MockArraySessionStorage()); + + $controller->login(new Request(), $session, 'test'); + + // The verifier is kept, never sent. Only the challenge goes over the wire. + $this->assertSame('the-verifier', $session->get('oauth2pkce_verifier')); + } + + public function testPkceCanBeTurnedOffForAProviderThatRejectsIt(): void + { + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider->method('generateNonce')->willReturn('1234'); + $mockProvider->method('generateState')->willReturn('abcd'); + $mockProvider->expects($this->never())->method('generatePkceVerifier'); + $mockProvider->expects($this->never())->method('getPkceChallenge'); + $mockProvider + ->expects($this->once()) + ->method('getAuthorizationUrl') + ->with($this->logicalNot($this->arrayHasKey('code_challenge'))) + ->willReturn('https://provider.example.org/authorize'); + + $controller = $this->createController($mockProvider, pkce: false); + $session = new Session(new MockArraySessionStorage()); + + $controller->login(new Request(), $session, 'test'); + + $this->assertNull($session->get('oauth2pkce_verifier')); + } + + /** + * A verifier from an abandoned login must not be redeemable against the code this + * one is about to receive, so the key is written on every login rather than only + * when PKCE is on. + */ + public function testAStaleVerifierIsOverwrittenWhenPkceIsOff(): void + { + $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); + $stubProvider->method('generateNonce')->willReturn('1234'); + $stubProvider->method('generateState')->willReturn('abcd'); + $stubProvider->method('getAuthorizationUrl')->willReturn('https://provider.example.org/authorize'); + + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2pkce_verifier', 'left-over-from-an-earlier-login'); + + $this->createController($stubProvider, pkce: false)->login(new Request(), $session, 'test'); + + $this->assertNull($session->get('oauth2pkce_verifier')); + } + public function testUnknownProviderKeyMapsTo404(): void { $cause = new InvalidProviderException('Invalid provider: bogus'); @@ -256,7 +339,7 @@ public function testUnknownProviderIsRefusedBeforeTheExpiryCheck(): void $this->fail('Expected NotFoundHttpException'); } - private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null): LoginController + private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null, bool $pkce = false): LoginController { $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); $mockProviderManager @@ -264,6 +347,10 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client ->method('getProvider') ->with('test') ->willReturn($provider); + $mockProviderManager + ->method('isPkceEnabled') + ->with('test') + ->willReturn($pkce); return new LoginController($mockProviderManager, $this->logger, $expiryChecker ?? $this->createExpiryChecker()); } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 031d49b..0694d96 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -526,4 +526,21 @@ public function testCallbackPathAloneSatisfiesTheRequirement(): void $this->assertSame('/auth/callback', $config['openid_providers']['provider1']['options']['callback_path']); } + + public function testPkceDefaultsToOn(): void + { + $config = $this->processor->processConfiguration($this->configuration, [$this->getMinimalConfig()]); + + $this->assertTrue($config['openid_providers']['provider1']['options']['pkce']); + } + + public function testPkceCanBeTurnedOff(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['pkce'] = false; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertFalse($config['openid_providers']['provider1']['options']['pkce']); + } } diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 7660c83..09ab224 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -221,7 +221,11 @@ public function testGetProviderWithoutHttpClientOptionsLeavesGuzzleDefaults(): v $this->assertNull($this->getGuzzleConfig($httpClient, 'timeout')); } - public function testGetProviderCachesInstance(): void + /** + * A provider belongs to one request: `league/oauth2-client` records the + * authorization request's `state` on it, so sharing an instance shares that state. + */ + public function testGetProviderReturnsAFreshInstance(): void { $manager = $this->createManager([ 'test' => $this->getBaseProviderConfig() + [ @@ -229,10 +233,107 @@ public function testGetProviderCachesInstance(): void ], ]); - $provider1 = $manager->getProvider('test'); - $provider2 = $manager->getProvider('test'); + $this->assertNotSame($manager->getProvider('test'), $manager->getProvider('test')); + } - $this->assertSame($provider1, $provider2); + /** + * Nothing one request puts on a provider reaches the next. Asserted through the + * state league records, so the test tracks the library's observable behaviour and + * not the shape of its internals. + */ + public function testNoRequestStateSurvivesOnTheNextProvider(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + ], + ]); + + $first = $manager->getProvider('test'); + $firstState = $first->generateState(); + + $this->assertSame($firstState, $first->getState(), 'The library records state on the provider; if it stops, this test needs rewriting'); + $this->assertNotSame($firstState, $manager->getProvider('test')->getState()); + } + + /** + * The client owns the connection pool: sharing it is what lets a token exchange + * reuse an open connection to the identity provider. + */ + public function testTheHttpClientIsReusedAcrossProviders(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + ], + ]); + + $this->assertSame( + $manager->getProvider('test')->getHttpClient(), + $manager->getProvider('test')->getHttpClient() + ); + } + + /** + * league's rule, kept: TLS verification may be turned off for a proxy and + * nowhere else. Without a proxy, `verify` is not forwarded at all, so Guzzle's + * default — verify — stands. + */ + public function testVerifyIsNotForwardedWithoutAProxy(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + 'http_client_options' => [ + 'timeout' => 1.5, + 'verify' => false, + ], + ], + ]); + + $httpClient = $manager->getProvider('test')->getHttpClient(); + $this->assertInstanceOf(GuzzleClient::class, $httpClient); + + $this->assertSame(1.5, $this->getGuzzleConfig($httpClient, 'timeout')); + $this->assertNotFalse($this->getGuzzleConfig($httpClient, 'verify'), 'Verification must not be disabled without a proxy'); + } + + /** + * Only the three options league forwards reach the client. The rest of a + * provider's configuration — its client secret above all — has no business in + * Guzzle's request options. + */ + public function testProviderCredentialsNeverReachTheHttpClient(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + 'http_client_options' => ['timeout' => 1.5], + ], + ]); + + $httpClient = $manager->getProvider('test')->getHttpClient(); + $this->assertInstanceOf(GuzzleClient::class, $httpClient); + + $this->assertNull($this->getGuzzleConfig($httpClient, 'clientSecret')); + $this->assertNull($this->getGuzzleConfig($httpClient, 'clientId')); + $this->assertNull($this->getGuzzleConfig($httpClient, 'cacheItemPool')); + } + + /** + * Each provider keeps its own, since `http_client_options` is per provider. + */ + public function testEachProviderGetsItsOwnHttpClient(): void + { + $manager = $this->createManager([ + 'one' => $this->getBaseProviderConfig() + ['redirect_uri' => 'https://app.example.org/one'], + 'two' => $this->getBaseProviderConfig() + ['redirect_uri' => 'https://app.example.org/two'], + ]); + + $this->assertNotSame( + $manager->getProvider('one')->getHttpClient(), + $manager->getProvider('two')->getHttpClient() + ); } /** diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 7eece0e..5fe8982 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -671,6 +671,7 @@ public function testEveryOneTimeSessionValueIsConsumed(string $state, array $ext $this->assertFalse($session->has('oauth2provider')); $this->assertFalse($session->has('oauth2state')); $this->assertFalse($session->has('oauth2nonce')); + $this->assertFalse($session->has('oauth2pkce_verifier'), 'A surviving verifier could be redeemed against a later code'); } /** @@ -820,13 +821,78 @@ public function testTheProviderErrorLeavesOnAuthenticationFailureUnwrapped(): vo $this->fail('Expected ProviderErrorException'); } - private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce'): void + public function testTheStoredVerifierIsSentWithTheTokenRequest(): void + { + $claims = new \stdClass(); + $claims->email = 'test@example.org'; + + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider + ->expects($this->once()) + ->method('getIdToken') + ->with('test_code', 'test_verifier') + ->willReturn('an.id.token'); + $mockProvider->method('validateIdToken')->willReturn($claims); + $this->stubProviderManager->method('getProvider')->willReturn($mockProvider); + + $request = new Request(query: ['state' => 'test_state', 'code' => 'test_code']); + $this->setSessionOnRequest($request, pkceVerifier: 'test_verifier'); + + $this->authenticator->authenticate($request); + } + + /** + * @return iterable + */ + public static function absentVerifierProvider(): iterable + { + // PKCE off for this provider, or a callback from a login that began before + // the verifier was ever stored. + yield 'never stored' => [null]; + // Nothing writes a non-string, but the session is shared with the application. + yield 'not a string' => [['test_verifier']]; + } + + /** + * Without a verifier the token request goes out without a `code_verifier`, which + * is what the identity provider expects when it was sent no challenge. + */ + #[DataProvider('absentVerifierProvider')] + public function testNoVerifierMeansNoCodeVerifier(mixed $stored): void + { + $claims = new \stdClass(); + $claims->email = 'test@example.org'; + + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider + ->expects($this->once()) + ->method('getIdToken') + ->with('test_code', null) + ->willReturn('an.id.token'); + $mockProvider->method('validateIdToken')->willReturn($claims); + $this->stubProviderManager->method('getProvider')->willReturn($mockProvider); + + $request = new Request(query: ['state' => 'test_state', 'code' => 'test_code']); + $stubSession = $this->createStub(SessionInterface::class); + $stubSession->method('remove')->willReturnMap([ + ['oauth2provider', 'test_provider_1'], + ['oauth2state', 'test_state'], + ['oauth2nonce', 'test_nonce'], + ['oauth2pkce_verifier', $stored], + ]); + $request->setSession($stubSession); + + $this->authenticator->authenticate($request); + } + + private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce', ?string $pkceVerifier = null): void { $stubSession = $this->createStub(SessionInterface::class); $map = [ ['oauth2provider', 'test_provider_1'], ['oauth2state', 'test_state'], ['oauth2nonce', $nonce], + ['oauth2pkce_verifier', $pkceVerifier], ]; $stubSession->method('remove')->willReturnMap($map); @@ -843,6 +909,7 @@ private function realSessionOnRequest(Request $request): Session $session->set('oauth2provider', 'test_provider_1'); $session->set('oauth2state', 'test_state'); $session->set('oauth2nonce', 'test_nonce'); + $session->set('oauth2pkce_verifier', 'test_verifier'); $request->setSession($session); return $session;