From d1b2ddda3a2d532f559e4688362dcdbc8c5ab939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phan=20Kochen?= Date: Tue, 25 Feb 2025 08:54:42 +0100 Subject: [PATCH] Add signing algorithm selection --- client-tester.php | 10 +-- src/Client.php | 148 ++++++++++++++++++++++++++++++----------- src/JWK.php | 5 +- src/MemoryStore.php | 20 ++++-- src/RedisStore.php | 27 ++++++-- src/StoreInterface.php | 14 ++-- tests/ClientTest.php | 20 ++++-- 7 files changed, 181 insertions(+), 63 deletions(-) diff --git a/client-tester.php b/client-tester.php index 40bcdf6..00ba9e1 100755 --- a/client-tester.php +++ b/client-tester.php @@ -9,10 +9,8 @@ require_once __DIR__.'/vendor/autoload.php'; -$client = new \Portier\Client\Client( - new \Portier\Client\MemoryStore(), - 'http://imaginary-client.test/fake-verify-route' -); +$store = new Portier\Client\MemoryStore(); +$client = new Portier\Client\Client($store, 'http://imaginary-client.test/fake-verify-route'); $client->broker = $argv[1]; $stdin = fopen('php://stdin', 'r'); @@ -40,6 +38,10 @@ echo "err\t{$msg}\n"; } break; + case 'clear-cache': + $store->clearCache(); + echo "ok\n"; + break; default: error_log("invalid command: {$cmd[0]}"); exit(1); diff --git a/src/Client.php b/src/Client.php index 46eaf6f..cc2bb94 100644 --- a/src/Client.php +++ b/src/Client.php @@ -24,7 +24,7 @@ class Client private StoreInterface $store; private string $redirectUri; - private string $clientId; + private string $clientOrigin; /** * The origin of the Portier broker. @@ -51,7 +51,7 @@ public function __construct(StoreInterface $store, string $redirectUri) $this->store = $store; $this->redirectUri = $redirectUri; - $this->clientId = self::getOrigin($this->redirectUri); + $this->clientOrigin = self::getOrigin($this->redirectUri); } /** @@ -103,19 +103,43 @@ public static function normalize(string $email): string */ public function authenticate(string $email, ?string $state = null): string { - $authEndpoint = $this->fetchDiscovery()->authorization_endpoint ?? null; + $discovery = $this->fetchDiscovery(); + + $authEndpoint = $discovery->config->authorization_endpoint ?? null; if (!is_string($authEndpoint)) { throw new \Exception('No authorization_endpoint in discovery document'); } - $nonce = $this->store->createNonce($email); + // Prefer Ed25519. Note that `alg=EdDSA` could also mean Ed448, + // so we must also inspect the key set. + $clientId = $this->clientOrigin; + $supportedAlgs = $discovery->config->id_token_signing_alg_values_supported ?? null; + if (is_array($supportedAlgs) && in_array('EdDSA', $supportedAlgs)) { + $foundEd25519 = false; + $foundOtherEdDSA = false; + foreach ($discovery->jwks as $jwk) { + if (($jwk->use ?? null) === 'sig' && ($jwk->alg ?? null) === 'EdDSA') { + if (($jwk->crv ?? null) === 'Ed25519') { + $foundEd25519 = true; + } else { + $foundOtherEdDSA = true; + break; + } + } + } + if ($foundEd25519 && !$foundOtherEdDSA) { + $clientId .= '?id_token_signed_response_alg=EdDSA'; + } + } + + $nonce = $this->store->createNonce($clientId, $email); $query = [ 'login_hint' => $email, 'scope' => 'openid email', 'nonce' => $nonce, 'response_type' => 'id_token', 'response_mode' => 'form_post', - 'client_id' => $this->clientId, + 'client_id' => $clientId, 'redirect_uri' => $this->redirectUri, ]; if (null !== $state) { @@ -136,7 +160,6 @@ public function verify(string $token): string { assert(!empty($token)); assert(!empty($this->broker)); - assert(!empty($this->clientId)); // Parse the token. $parser = new Parser(new JoseEncoder()); @@ -149,43 +172,56 @@ public function verify(string $token): string throw new \Exception('Token has no "kid" header field'); } - // Fetch broker keys. - $jwksUri = $this->fetchDiscovery()->jwks_uri ?? null; - if (!is_string($jwksUri)) { - throw new \Exception('No jwks_uri in discovery document'); + // Find the matching public key, and verify the signature. + $matchingJwk = null; + foreach ($this->fetchDiscovery()->jwks as $jwk) { + if (($jwk->use ?? null) === 'sig' && ($jwk->kid ?? null) === $kid) { + $matchingJwk = $jwk; + break; + } + } + if (null === $matchingJwk) { + throw new \Exception('Cannot find the JWK used to sign the token'); } - $keysDoc = $this->store->fetchCached('keys', $jwksUri); - if (!isset($keysDoc->keys) || !is_array($keysDoc->keys)) { - throw new \Exception('Keys document incorrectly formatted'); + $alg = $matchingJwk->alg ?? null; + if (!is_string($alg)) { + throw new \Exception('Missing "alg" on JWK'); } + switch ($alg) { + case 'RS256': + $key = JWK::toPem($matchingJwk); + $signer = new JwtSigner\Rsa\Sha256(); + break; - // Find the matching public key, and verify the signature. - $publicKey = ''; - foreach ($keysDoc->keys as $key) { - if ($key instanceof \stdClass - && isset($key->alg) && 'RS256' === $key->alg - && isset($key->kid) && $key->kid === $kid - ) { - try { - $publicKey = JWK::toPem($key); - } catch (\Exception) { + case 'EdDSA': + $crv = $matchingJwk->crv ?? null; + $x = $matchingJwk->x ?? null; + if (!is_string($crv) || !is_string($x)) { + throw new \Exception('Incomplete EdDSA JWK'); + } + if ('Ed25519' !== $crv) { + throw new \Exception('Unsupported EdDSA crv: '.substr($crv, 0, 10)); } + + $key = JWK::decodeBase64Url($x); + $signer = new JwtSigner\Eddsa(); break; - } + + default: + throw new \Exception('Unsupported kty: '.substr($alg, 0, 10)); } - if ('' === $publicKey) { - throw new \Exception('Cannot find the public key used to sign the token'); + if (empty($key)) { + throw new \Exception('Invalid JWK'); } - $publicKey = JwtSigner\Key\InMemory::plainText($publicKey); + $key = JwtSigner\Key\InMemory::plainText($key); // Validate the token claims. $clock = \Lcobucci\Clock\SystemClock::fromUTC(); $leeway = new \DateInterval('PT'.$this->leeway.'S'); $validator = new Validator(); - $validator->assert($token, new JwtConstraint\SignedWith(new JwtSigner\Rsa\Sha256(), $publicKey)); + $validator->assert($token, new JwtConstraint\SignedWith($signer, $key)); $validator->assert($token, new JwtConstraint\IssuedBy($this->broker)); - $validator->assert($token, new JwtConstraint\PermittedFor($this->clientId)); $validator->assert($token, new JwtConstraint\LooseValidAt($clock, $leeway)); // Check that the required token claims are set. @@ -198,11 +234,15 @@ public function verify(string $token): string } $nonce = $claims->get('nonce'); + $aud = $claims->get('aud'); $email = $claims->get('email'); $emailOriginal = $claims->get('email_original', $email); if (!is_string($nonce)) { throw new \Exception(sprintf('Token claim "nonce" is not a string')); } + if (!is_array($aud) || 1 !== count($aud) || !is_string($aud[0])) { + throw new \Exception(sprintf('Token claim "aud" is not a string')); + } if (!is_string($email)) { throw new \Exception(sprintf('Token claim "email" is not a string')); } @@ -211,11 +251,19 @@ public function verify(string $token): string } // Consume the nonce. - $this->store->consumeNonce($nonce, $emailOriginal); - - $state = $claims->get('state'); - if (!is_string($state)) { - $state = null; + $clientId = $aud[0]; + $this->store->consumeNonce($nonce, $clientId, $emailOriginal); + + // Verify the correct signing algorithm was used. + $expectedAlg = 'RS256'; + $sepIdx = strpos($clientId, '?'); + if (false !== $sepIdx) { + $params = []; + parse_str(substr($clientId, $sepIdx + 1), $params); + $expectedAlg = $params['id_token_signed_response_alg'] ?? 'RS256'; + } + if ($alg !== $expectedAlg) { + throw new \Exception(sprintf('Token signed using incorrect algorithm')); } // Return the normalized email. @@ -223,13 +271,37 @@ public function verify(string $token): string } /** - * Fetches the OpenID discovery document from the broker. + * Fetches the OpenID configuration and keys from the broker. + * + * @return object{config: \stdClass, jwks: \stdClass[]} */ - private function fetchDiscovery(): \stdClass + private function fetchDiscovery(): object { - $discoveryUrl = $this->broker.'/.well-known/openid-configuration'; + $configUrl = $this->broker.'/.well-known/openid-configuration'; + $config = $this->store->fetchCached('config', $configUrl); + + $jwksUri = $config->jwks_uri ?? null; + if (!is_string($jwksUri)) { + throw new \Exception('No jwks_uri in openid-configuration'); + } - return $this->store->fetchCached('discovery', $discoveryUrl); + $jwksDoc = $this->store->fetchCached('keys', $jwksUri); + $jwks = $jwksDoc->keys ?? null; + if (!is_array($jwks)) { + throw new \Exception('JWKs document incorrectly formatted'); + } + foreach ($jwks as $jwk) { + if (!($jwk instanceof \stdClass)) { + throw new \Exception('JWKs document incorrectly formatted'); + } + } + /** @var \stdClass[] */ + $jwks = $jwks; + + return (object) [ + 'config' => $config, + 'jwks' => $jwks, + ]; } /** diff --git a/src/JWK.php b/src/JWK.php index 0703143..a83d77f 100644 --- a/src/JWK.php +++ b/src/JWK.php @@ -11,6 +11,9 @@ private function __construct() { } + /** + * Convert a JWK to PEM format. + */ public static function toPem(\stdClass $jwk): string { if (!isset($jwk->kty) || !is_string($jwk->kty)) { @@ -136,7 +139,7 @@ private static function derToPem(string $der): string } /** - * @internal for tests only + * Decode base64url. */ public static function decodeBase64Url(string $input): string { diff --git a/src/MemoryStore.php b/src/MemoryStore.php index 992e645..91ca92c 100644 --- a/src/MemoryStore.php +++ b/src/MemoryStore.php @@ -12,7 +12,7 @@ class MemoryStore extends AbstractStore { /** @var array */ private $cache; - /** @var array */ + /** @var array */ private $nonces; /** @@ -26,6 +26,14 @@ public function __construct() $this->nonces = []; } + /** + * @internal for testing only + */ + public function clearCache(): void + { + $this->cache = []; + } + public function fetchCached(string $cacheId, string $url): \stdClass { $item = $this->cache[$cacheId] ?? null; @@ -43,11 +51,12 @@ public function fetchCached(string $cacheId, string $url): \stdClass return $res->data; } - public function createNonce(string $email): string + public function createNonce(string $clientId, string $email): string { $nonce = $this->generateNonce($email); $this->nonces[$nonce] = (object) [ + 'clientId' => $clientId, 'email' => $email, 'expires' => time() + (int) $this->nonceTtl, ]; @@ -55,13 +64,16 @@ public function createNonce(string $email): string return $nonce; } - public function consumeNonce(string $nonce, string $email): void + public function consumeNonce(string $nonce, string $clientId, string $email): void { $item = $this->nonces[$nonce] ?? null; if (null !== $item) { unset($this->nonces[$nonce]); - if ($item->email === $email && time() < $item->expires) { + if ($item->clientId === $clientId + && $item->email === $email + && time() < $item->expires + ) { return; } } diff --git a/src/RedisStore.php b/src/RedisStore.php index 3474a59..d990b4a 100644 --- a/src/RedisStore.php +++ b/src/RedisStore.php @@ -45,24 +45,41 @@ public function fetchCached(string $cacheId, string $url): \stdClass return $res->data; } - public function createNonce(string $email): string + public function createNonce(string $clientId, string $email): string { $nonce = $this->generateNonce($email); $key = 'nonce:'.$nonce; - $this->redis->setex($key, (int) $this->nonceTtl, $email); + $value = (object) ['clientId' => $clientId, 'email' => $email]; + $value = json_encode($value, flags: JSON_THROW_ON_ERROR); + $this->redis->setex($key, (int) $this->nonceTtl, $value); return $nonce; } - public function consumeNonce(string $nonce, string $email): void + public function consumeNonce(string $nonce, string $clientId, string $email): void { $key = 'nonce:'.$nonce; $this->redis->multi(); $this->redis->get($key); $this->redis->del($key); - $res = $this->redis->exec(); - if ($res[0] !== $email) { + [$value] = $this->redis->exec(); + assert(is_string($value)); + + // Handle old record that didn't include client ID. + if (!str_starts_with($value, '{')) { + if ($value !== $email) { + throw new \Exception('Invalid or expired nonce'); + } + + return; + } + + $value = json_decode($value, flags: JSON_THROW_ON_ERROR); + if (!($value instanceof \stdClass) + || ($value->email ?? null) !== $email + || ($value->clientId ?? null) !== $clientId + ) { throw new \Exception('Invalid or expired nonce'); } } diff --git a/src/StoreInterface.php b/src/StoreInterface.php index 16bc507..f99a669 100644 --- a/src/StoreInterface.php +++ b/src/StoreInterface.php @@ -20,17 +20,19 @@ public function fetchCached(string $cacheId, string $url): \stdClass; /** * Generate and store a nonce. * - * @param string $email email address to associate with the nonce + * @param string $clientId client ID to associate with the nonce + * @param string $email email address to associate with the nonce * * @return string the generated nonce */ - public function createNonce(string $email): string; + public function createNonce(string $clientId, string $email): string; /** - * Consume a nonce, and check if it's valid for the given email address. + * Consume a nonce, and check if it's valid for the given client ID and email address. * - * @param string $nonce the nonce to resolve - * @param string $email the email address that is being verified + * @param string $nonce the nonce to resolve + * @param string $clientId client ID that is being verified + * @param string $email the email address that is being verified */ - public function consumeNonce(string $nonce, string $email): void; + public function consumeNonce(string $nonce, string $clientId, string $email): void; } diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 9d33ae2..bf470ef 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -43,19 +43,29 @@ public function fetchCached(string $cacheId, string $url): \stdClass { $this->fetchCachedCalled = true; - return (object) [ - 'authorization_endpoint' => 'http://imaginary-server.test/auth', - ]; + switch (parse_url($url, PHP_URL_PATH)) { + case '/.well-known/openid-configuration': + return (object) [ + 'authorization_endpoint' => 'http://imaginary-server.test/auth', + 'jwks_uri' => 'http://imaginary-server.test/jwks', + ]; + case '/jwks': + return (object) [ + 'keys' => [], + ]; + default: + throw new \Exception('Unexpected request: '.$url); + } } - public function createNonce(string $email): string + public function createNonce(string $clientId, string $email): string { $this->createNonceCalled = true; return 'foobar'; } - public function consumeNonce(string $nonce, string $email): void + public function consumeNonce(string $nonce, string $clientId, string $email): void { throw new \Exception('Not implemented'); }