feat: send a PKCE challenge, and stop holding providers between requests

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

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`

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.

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",

src/Controller/LoginController.php

@@ -58,20 +58,38 @@ 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
         $session->set('oauth2provider', $providerKey);
         $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

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()

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<string,OpenIdConfigurationProvider> */
-    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<string, GuzzleClient>
+     */
+    private array $httpClients = [];
 
-    /** @var array<string, array<string, string>> */
+    /**
+     * 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<string, array<string, string>>
+     */
     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<string, mixed> $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))
+        );
     }
 }

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) {