From 2c7ae778c718cd3277ad3f01c73fb2fab63e9816 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 09:13:28 -0700 Subject: [PATCH 01/47] Rough pass to read challenge from request --- src/Challenge.php | 18 ++++++++++++++++++ src/CreateResponse.php | 10 ++++++++++ src/GetResponse.php | 9 +++++++++ src/Responses/AssertionInterface.php | 2 ++ src/Responses/AttestationInterface.php | 2 ++ 5 files changed, 41 insertions(+) diff --git a/src/Challenge.php b/src/Challenge.php index 1658f5c..3cc8218 100644 --- a/src/Challenge.php +++ b/src/Challenge.php @@ -37,6 +37,24 @@ public static function random(): Challenge return new Challenge(new BinaryString(random_bytes(32))); } + /** + * This is used to help access the challenges provided by the client, which + * assists the case of having a global stateless challenge pool. + * + * @internal + */ + public static function fromClientDataJSONValue(string $cdjChallenge): ChallengeInterface + { + // cdj contains a base64UrlWeb value + // move to base64url::decode? + $base64 = strtr($cdjChallenge, '-_', '+/'); + $bin = base64_decode($base64, strict: true); + if ($bin === false) { + throw new \Exception(); + } + return new Challenge(new BinaryString($bin)); + } + /** * @internal */ diff --git a/src/CreateResponse.php b/src/CreateResponse.php index 13062e3..edfc1a9 100644 --- a/src/CreateResponse.php +++ b/src/CreateResponse.php @@ -144,6 +144,16 @@ public function verify( // trustworthy" } + public function getChallenge(): ChallengeInterface + { + $cdj = json_decode($this->clientDataJson->unwrap(), true); + assert(is_array($cdj)); + assert(array_key_exists('challenge', $cdj)); + // FIXME: real one + + return Challenge::fromClientDataJSONValue($cdj['challenge']); + } + private function fail(string $section, string $desc): never { throw new Errors\RegistrationError($section, $desc); diff --git a/src/GetResponse.php b/src/GetResponse.php index 4f2a976..3581d77 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -167,6 +167,15 @@ public function verify( return $credential; } + public function getChallenge(): ChallengeInterface + { + $cdj = json_decode($this->clientDataJson->unwrap(), true); + assert(is_array($cdj)); + assert(array_key_exists('challenge', $cdj)); + return Challenge::fromClientDataJSONValue($cdj['challenge']); + } + + private function fail(string $section, string $desc): never { throw new Errors\VerificationError($section, $desc); diff --git a/src/Responses/AssertionInterface.php b/src/Responses/AssertionInterface.php index 358c29d..9a5fd76 100644 --- a/src/Responses/AssertionInterface.php +++ b/src/Responses/AssertionInterface.php @@ -35,4 +35,6 @@ public function verify( CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; + + public function getChallenge(): ChallengeInterface; } diff --git a/src/Responses/AttestationInterface.php b/src/Responses/AttestationInterface.php index dbb9460..3a8cc3d 100644 --- a/src/Responses/AttestationInterface.php +++ b/src/Responses/AttestationInterface.php @@ -24,4 +24,6 @@ public function verify( RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; + + public function getChallenge(): ChallengeInterface; } From 4259a315d641ef71c4229f5e1cd34974a5c94b22 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:18:31 -0700 Subject: [PATCH 02/47] Taking a different approach --- src/Challenge.php | 18 ------------------ src/CreateResponse.php | 10 ---------- src/GetResponse.php | 9 --------- src/Responses/AssertionInterface.php | 2 -- src/Responses/AttestationInterface.php | 2 -- 5 files changed, 41 deletions(-) diff --git a/src/Challenge.php b/src/Challenge.php index 3cc8218..1658f5c 100644 --- a/src/Challenge.php +++ b/src/Challenge.php @@ -37,24 +37,6 @@ public static function random(): Challenge return new Challenge(new BinaryString(random_bytes(32))); } - /** - * This is used to help access the challenges provided by the client, which - * assists the case of having a global stateless challenge pool. - * - * @internal - */ - public static function fromClientDataJSONValue(string $cdjChallenge): ChallengeInterface - { - // cdj contains a base64UrlWeb value - // move to base64url::decode? - $base64 = strtr($cdjChallenge, '-_', '+/'); - $bin = base64_decode($base64, strict: true); - if ($bin === false) { - throw new \Exception(); - } - return new Challenge(new BinaryString($bin)); - } - /** * @internal */ diff --git a/src/CreateResponse.php b/src/CreateResponse.php index edfc1a9..13062e3 100644 --- a/src/CreateResponse.php +++ b/src/CreateResponse.php @@ -144,16 +144,6 @@ public function verify( // trustworthy" } - public function getChallenge(): ChallengeInterface - { - $cdj = json_decode($this->clientDataJson->unwrap(), true); - assert(is_array($cdj)); - assert(array_key_exists('challenge', $cdj)); - // FIXME: real one - - return Challenge::fromClientDataJSONValue($cdj['challenge']); - } - private function fail(string $section, string $desc): never { throw new Errors\RegistrationError($section, $desc); diff --git a/src/GetResponse.php b/src/GetResponse.php index 3581d77..4f2a976 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -167,15 +167,6 @@ public function verify( return $credential; } - public function getChallenge(): ChallengeInterface - { - $cdj = json_decode($this->clientDataJson->unwrap(), true); - assert(is_array($cdj)); - assert(array_key_exists('challenge', $cdj)); - return Challenge::fromClientDataJSONValue($cdj['challenge']); - } - - private function fail(string $section, string $desc): never { throw new Errors\VerificationError($section, $desc); diff --git a/src/Responses/AssertionInterface.php b/src/Responses/AssertionInterface.php index 9a5fd76..358c29d 100644 --- a/src/Responses/AssertionInterface.php +++ b/src/Responses/AssertionInterface.php @@ -35,6 +35,4 @@ public function verify( CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; - - public function getChallenge(): ChallengeInterface; } diff --git a/src/Responses/AttestationInterface.php b/src/Responses/AttestationInterface.php index 3a8cc3d..dbb9460 100644 --- a/src/Responses/AttestationInterface.php +++ b/src/Responses/AttestationInterface.php @@ -24,6 +24,4 @@ public function verify( RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; - - public function getChallenge(): ChallengeInterface; } From eaa502bcd7a94befc487ff88a084f1afe9842ed1 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:31:50 -0700 Subject: [PATCH 03/47] rough interface --- src/ChallengeManagerInterface.php | 101 ++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 src/ChallengeManagerInterface.php diff --git a/src/ChallengeManagerInterface.php b/src/ChallengeManagerInterface.php new file mode 100644 index 0000000..a3defdc --- /dev/null +++ b/src/ChallengeManagerInterface.php @@ -0,0 +1,101 @@ +cacheKeyPrefix, + Codecs\Base64Url::encode($c->getBinary()->unwrap()), + ); + $this->cache->set($cacheKey, $c, 120); + return $c; + } + + public function useFromClientDataJSON(string $base64url): ?ChallengeInterface + { + $key = sprintf( + '%s%s', + $this->cacheKeyPrefix, + $base64url, + ); + $active = $this->cache->get($key); + if ($active === null) { + return $active; + } + $this->cache->delete($key); + return $active; + } + +} + +class SessionChallengeManager +{ + private const SESSION_KEY = 'passkey_challenge'; + + public function __construct() + { + // do later? + if (session_status() !== PHP_SESSION_ACTIVE) { + throw new \BadMethodCallException('Call session_start()'); + } + } + + public function createChallenge(): ChallengeInterface + { + $c = ExpiringChallenge::withLifetime(120); + $_SESSION[self::SESSION_KEY] = $c; + return $c; + } + + /* + public function getActiveChallenge(): ?ChallengeInterface + { + if (!array_key_exists(self::SESSION_KEY, $_SESSION)) { + return null; + } + $challenge = $_SESSION[self::SESSION_KEY]; + unset($_SESSION[self::SESSION_KEY]); + return $challenge; + } + */ +} From 27128396634d3aad8512ca316ad0d4151663acb3 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:32:01 -0700 Subject: [PATCH 04/47] Integrate interface into verify paths --- src/CreateResponse.php | 12 ++++++++++-- src/GetResponse.php | 12 ++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/CreateResponse.php b/src/CreateResponse.php index 13062e3..bd81bef 100644 --- a/src/CreateResponse.php +++ b/src/CreateResponse.php @@ -27,7 +27,7 @@ public function __construct( * @link https://www.w3.org/TR/webauthn-2/#sctn-registering-a-new-credential */ public function verify( - ChallengeInterface $challenge, + ChallengeInterface | ChallengeManagerInterface $challenge, RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface { @@ -47,8 +47,16 @@ public function verify( } // 7.1.8 + $cdjChallenge = $C['challenge']; + if ($challenge instanceof ChallengeManagerInterface) { + $challenge = $challenge->useFromClientDataJSON($cdjChallenge); + if ($challenge === null) { + $this->fail('7.1.8', 'C.challenge'); + } + } + $b64u = Codecs\Base64Url::encode($challenge->getBinary()->unwrap()); - if (!hash_equals($b64u, $C['challenge'])) { + if (!hash_equals($b64u, $cdjChallenge)) { $this->fail('7.1.8', 'C.challenge'); } diff --git a/src/GetResponse.php b/src/GetResponse.php index 4f2a976..781c7c0 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -36,7 +36,7 @@ public function getUsedCredentialId(): BinaryString * @link https://www.w3.org/TR/webauthn-2/#sctn-verifying-assertion */ public function verify( - ChallengeInterface $challenge, + ChallengeInterface|ChallengeManagerInterface $challenge, RelyingParty $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, @@ -89,8 +89,16 @@ public function verify( } // 7.2.12 + $cdjChallenge = $C['challenge']; + if ($challenge instanceof ChallengeManagerInterface) { + $challenge = $challenge->useFromClientDataJSON($cdjChallenge); + if ($challenge === null) { + $this->fail('7.2.12', 'C.challenge'); + } + } + $b64u = Codecs\Base64Url::encode($challenge->getBinary()->unwrap()); - if (!hash_equals($b64u, $C['challenge'])) { + if (!hash_equals($b64u, $cdjChallenge)) { $this->fail('7.2.12', 'C.challenge'); } From 722850f87f195fd18787ba824539ccf0bf51c62e Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:33:14 -0700 Subject: [PATCH 05/47] Update interfaces --- src/Responses/AssertionInterface.php | 3 ++- src/Responses/AttestationInterface.php | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Responses/AssertionInterface.php b/src/Responses/AssertionInterface.php index 358c29d..baf5e7b 100644 --- a/src/Responses/AssertionInterface.php +++ b/src/Responses/AssertionInterface.php @@ -7,6 +7,7 @@ use Firehed\WebAuthn\{ BinaryString, ChallengeInterface, + ChallengeManagerInterface, CredentialContainer, CredentialInterface, RelyingParty, @@ -30,7 +31,7 @@ public function getUsedCredentialId(): BinaryString; * @api */ public function verify( - ChallengeInterface $challenge, + ChallengeInterface | ChallengeManagerInterface $challenge, RelyingParty $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, diff --git a/src/Responses/AttestationInterface.php b/src/Responses/AttestationInterface.php index dbb9460..4ae5d3d 100644 --- a/src/Responses/AttestationInterface.php +++ b/src/Responses/AttestationInterface.php @@ -6,6 +6,7 @@ use Firehed\WebAuthn\{ ChallengeInterface, + ChallengeManagerInterface, CredentialInterface, RelyingParty, UserVerificationRequirement, @@ -20,7 +21,7 @@ interface AttestationInterface { public function verify( - ChallengeInterface $challenge, + ChallengeInterface | ChallengeManagerInterface $challenge, RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; From d9888ae25cfe8c2ab0b1423779aeabf4de84d91e Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:35:04 -0700 Subject: [PATCH 06/47] Extract implementation --- src/CacheChallengeManager.php | 43 +++++++++++++++++++++++++++++++ src/ChallengeManagerInterface.php | 39 ---------------------------- 2 files changed, 43 insertions(+), 39 deletions(-) create mode 100644 src/CacheChallengeManager.php diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php new file mode 100644 index 0000000..107f2a5 --- /dev/null +++ b/src/CacheChallengeManager.php @@ -0,0 +1,43 @@ +cacheKeyPrefix, + Codecs\Base64Url::encode($c->getBinary()->unwrap()), + ); + $this->cache->set($cacheKey, $c, 120); + return $c; + } + + public function useFromClientDataJSON(string $base64url): ?ChallengeInterface + { + $key = sprintf( + '%s%s', + $this->cacheKeyPrefix, + $base64url, + ); + $active = $this->cache->get($key); + if ($active === null) { + return $active; + } + $this->cache->delete($key); + return $active; + } +} diff --git a/src/ChallengeManagerInterface.php b/src/ChallengeManagerInterface.php index a3defdc..29cfe15 100644 --- a/src/ChallengeManagerInterface.php +++ b/src/ChallengeManagerInterface.php @@ -29,45 +29,6 @@ public function useFromClientDataJSON(string $base64url): ?ChallengeInterface; } - - -class CacheChallengeManager -{ - public function __construct( - private \Psr\SimpleCache\Cache $cache, - private string $cacheKeyPrefix = 'webauthn-challenge-', - ) { - } - - public function createChallenge(): ChallengeInterface - { - $c = ExpiringChallenge::withLifetime(120); - $cacheKey = sprintf( - '%s%s', - $this->cacheKeyPrefix, - Codecs\Base64Url::encode($c->getBinary()->unwrap()), - ); - $this->cache->set($cacheKey, $c, 120); - return $c; - } - - public function useFromClientDataJSON(string $base64url): ?ChallengeInterface - { - $key = sprintf( - '%s%s', - $this->cacheKeyPrefix, - $base64url, - ); - $active = $this->cache->get($key); - if ($active === null) { - return $active; - } - $this->cache->delete($key); - return $active; - } - -} - class SessionChallengeManager { private const SESSION_KEY = 'passkey_challenge'; From 735aef91d1b195e5c2ac80ffe2db54912866fac6 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:37:21 -0700 Subject: [PATCH 07/47] Add session handler --- src/CacheChallengeManager.php | 4 +++- src/ChallengeManagerInterface.php | 35 +----------------------------- src/SessionChallengeManager.php | 36 +++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 35 deletions(-) create mode 100644 src/SessionChallengeManager.php diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index 107f2a5..39dbd33 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -26,13 +26,15 @@ public function createChallenge(): ChallengeInterface return $c; } - public function useFromClientDataJSON(string $base64url): ?ChallengeInterface + public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface { $key = sprintf( '%s%s', $this->cacheKeyPrefix, $base64url, ); + // WARNING: race condition. Without at least a CAS guarantee, this + // can't be avoided with SimpleCache. $active = $this->cache->get($key); if ($active === null) { return $active; diff --git a/src/ChallengeManagerInterface.php b/src/ChallengeManagerInterface.php index 29cfe15..e88298f 100644 --- a/src/ChallengeManagerInterface.php +++ b/src/ChallengeManagerInterface.php @@ -25,38 +25,5 @@ public function createChallenge(): ChallengeInterface; * * @internal */ - public function useFromClientDataJSON(string $base64url): ?ChallengeInterface; -} - - -class SessionChallengeManager -{ - private const SESSION_KEY = 'passkey_challenge'; - - public function __construct() - { - // do later? - if (session_status() !== PHP_SESSION_ACTIVE) { - throw new \BadMethodCallException('Call session_start()'); - } - } - - public function createChallenge(): ChallengeInterface - { - $c = ExpiringChallenge::withLifetime(120); - $_SESSION[self::SESSION_KEY] = $c; - return $c; - } - - /* - public function getActiveChallenge(): ?ChallengeInterface - { - if (!array_key_exists(self::SESSION_KEY, $_SESSION)) { - return null; - } - $challenge = $_SESSION[self::SESSION_KEY]; - unset($_SESSION[self::SESSION_KEY]); - return $challenge; - } - */ + public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface; } diff --git a/src/SessionChallengeManager.php b/src/SessionChallengeManager.php new file mode 100644 index 0000000..d8fa78b --- /dev/null +++ b/src/SessionChallengeManager.php @@ -0,0 +1,36 @@ + Date: Mon, 28 Aug 2023 10:43:50 -0700 Subject: [PATCH 08/47] var fix --- src/CacheChallengeManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index 39dbd33..65deffb 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -31,7 +31,7 @@ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface $key = sprintf( '%s%s', $this->cacheKeyPrefix, - $base64url, + $base64Url, ); // WARNING: race condition. Without at least a CAS guarantee, this // can't be avoided with SimpleCache. From 192bf2aabfbb9ce851c6bbcdeddddcaf55f58c95 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:47:58 -0700 Subject: [PATCH 09/47] Suggest simple-cache and use it in dev for testing manager --- composer.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/composer.json b/composer.json index fd47146..5d8c545 100644 --- a/composer.json +++ b/composer.json @@ -41,6 +41,7 @@ "phpstan/phpstan-phpunit": "^1.0", "phpstan/phpstan-strict-rules": "^1.0", "phpunit/phpunit": "^9.3", + "psr/simple-cache": "^3.0", "squizlabs/php_codesniffer": "^3.5" }, "scripts": { @@ -54,5 +55,8 @@ "phpstan": "phpstan analyse", "phpstan-baseline": "phpstan analyse --generate-baseline", "phpcs": "phpcs" + }, + "suggest": { + "psr/simple-cache": "Allows use of CacheChallengeManager" } } From 5e8be2e22cad9335794c630a6b19dcd05e5106da Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:50:13 -0700 Subject: [PATCH 10/47] Ensure cache challenge manager has a challenge --- src/CacheChallengeManager.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index 65deffb..f632ce8 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -5,6 +5,7 @@ namespace Firehed\WebAuthn; use Psr\SimpleCache\CacheInterface; +use UnexpectedValueException; class CacheChallengeManager implements ChallengeManagerInterface { @@ -40,6 +41,9 @@ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface return $active; } $this->cache->delete($key); - return $active; + if ($active instanceof ChallengeInterface) { + return $active; + } + throw new UnexpectedValueException('Non-challenge found in cache'); } } From 06d3d8a93adfe2a0243b8b8dad19f0dc70524c73 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:50:56 -0700 Subject: [PATCH 11/47] notes --- src/CacheChallengeManager.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index f632ce8..f7b4523 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -18,6 +18,8 @@ public function __construct( public function createChallenge(): ChallengeInterface { $c = ExpiringChallenge::withLifetime(120); + // The cache key is designed to mirror the comparison value used in + // the `verify()` methods and `useFromClientDataJSON()` below. $cacheKey = sprintf( '%s%s', $this->cacheKeyPrefix, From c8aabcd7a633bb7dfc4cd7b10c4c5aa5385fe262 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 10:58:53 -0700 Subject: [PATCH 12/47] Update inline examples --- README.md | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index c1c254d..a0e46cf 100644 --- a/README.md +++ b/README.md @@ -33,29 +33,31 @@ The protocol is always required; the port must only be present if using a non-st $rp = new RelyingParty('https://www.example.com'); ``` -Important: WebAuthn will only work in a "secure context". +Also create a `ChallengeManagerInterface`. +There are multiple options available which can suit different applications. + +```php +session_start(); +$challengeManager = new SessionChallengeManager(); +``` + +[!IMPORTANT] +WebAuthn will only work in a "secure context". This means that the domain MUST run over `https`, with a sole exception for `localhost`. -See https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts for more info. +See [https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts]() for more info. ### Registering a WebAuthn credential to a user This step takes place either when a user is first registering, or later on to supplement or replace their password. 1) Create an endpoint that will return a new, random Challenge. -This may be stored in a user's session or equivalent; it needs to be kept statefully server-side. Send it to the user as base64. ```php createChallenge(); // Send to user header('Content-type: application/json'); @@ -147,11 +149,9 @@ $data = json_decode($json, true); $parser = new ResponseParser(); $createResponse = $parser->parseCreateResponse($data); -$rp = $valueFromSetup; // e.g. $psr11Container->get(RelyingParty::class); -$challenge = $_SESSION['webauthn_challenge']; - try { - $credential = $createResponse->verify($challenge, $rp); + // $challengeManager and $rp are the values from the setup step + $credential = $createResponse->verify($challengeManager, $rp); } catch (Throwable) { // Verification failed. Send an error to the user? header('HTTP/1.1 403 Unauthorized'); @@ -190,7 +190,7 @@ header('HTTP/1.1 200 OK'); Note: this workflow may be a little different if supporting [passkeys](https://developer.apple.com/passkeys/). Updated samples will follow. -Before starting, you will need to collect the username or id of the user trying to authenticate, and retreive the user info from storage. +Before starting, you will need to collect the username or id of the user trying to authenticate, and retrieve the user info from storage. This assumes the same schema from the previous Registration example. 1) Create an endpoint that will return a Challenge and any credentials associated with the authenticating user: @@ -216,8 +216,7 @@ $_SESSION['authenticating_user_id'] = $user['id']; // See examples/functions.php for how this works $credentialContainer = getCredentialsForUserId($pdo, $user['id']); -$challenge = ExpiringChallenge::withLifetime(120); -$_SESSION['webauthn_challenge'] = $challenge; +$challenge = $challengeManager->createChallenge(); // Send to user header('Content-type: application/json'); @@ -303,13 +302,11 @@ $data = json_decode($json, true); $parser = new ResponseParser(); $getResponse = $parser->parseGetResponse($data); -$rp = $valueFromSetup; // e.g. $psr11Container->get(RelyingParty::class); -$challenge = $_SESSION['webauthn_challenge']; - $credentialContainer = getCredentialsForUserId($pdo, $_SESSION['authenticating_user_id']); try { - $updatedCredential = $getResponse->verify($challenge, $rp, $credentialContainer); + // $challengeManager and $rp are the values from the setup step + $updatedCredential = $getResponse->verify($challengeManager, $rp, $credentialContainer); } catch (Throwable) { // Verification failed. Send an error to the user? header('HTTP/1.1 403 Unauthorized'); From fd790dd01c10b84df9b3d9cf3bd82839454ab8d2 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 11:10:09 -0700 Subject: [PATCH 13/47] Note new challenge management --- README.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/README.md b/README.md index a0e46cf..ed6453a 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,13 @@ This also means that users do not have to manage passwords for individual websit This will cover the basic workflows for integrating this library to your web application. Classes referenced in the examples may omit the `Firehed\WebAuthn` namespace prefix for brevity. +[!NOTE] +> The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL +> NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", +> "MAY", and "OPTIONAL" in this document are to be interpreted as +> described in BCP 14 [RFC2119] [RFC8174] when, and only when, they +> appear in all capitals, as shown here. + ### Sample Code There's a complete set of working examples in the [`examples`](examples) directory. Application logic is kept to a bare minimum in order to highlight the most important workflow steps. @@ -35,6 +42,7 @@ $rp = new RelyingParty('https://www.example.com'); Also create a `ChallengeManagerInterface`. There are multiple options available which can suit different applications. +See the [Challenge Management](#challenge-management) section below for more information. ```php session_start(); @@ -430,6 +438,19 @@ Those wire formats are covered by semantic versioning and guaranteed to not have Similarly, for data storage, the output of `Codecs\Credential::encode()` are also covered. +#### Challenge management + +Challenges are a [cryptographic nonce](https://en.wikipedia.org/wiki/Cryptographic_nonce) that ensure a login attempt works only once. +Their single-use nature is critical to the security of the WebAuthn protocol. + +Your application SHOULD use one of the library-provided `ChallengeManagerInterface` implementations to ensure the correct behavior. +However, if one of the provided options is not suitable, you MAY implement the interface yourself or manage challenges manually. + +[!WARNING] +You MUST validate that the challenge was generated by your server recently and has not already been used. +**Failing to do so will compromise the security of the protocol!** +The built-in `ChallengeManagerInterface` implementations will handle this for you. + Challenges generated by your server SHOULD expire after a short amount of time. You MAY use the `ExpiringChallenge` class for convenience (e.g. `$challenge = ExpiringChallenge::withLifetime(60);`), which will throw an exception if the specified expiration window has been exceeded. It is RECOMMENDED that your javascript code uses the `timeout` setting (denoted in milliseconds) and matches the server-side challenge expiration, give or take a few seconds. From beaf574e35327449f28e124b7bb733aab07c93a2 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 11:11:28 -0700 Subject: [PATCH 14/47] fix formatting --- README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ed6453a..3a5ee31 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ This also means that users do not have to manage passwords for individual websit This will cover the basic workflows for integrating this library to your web application. Classes referenced in the examples may omit the `Firehed\WebAuthn` namespace prefix for brevity. -[!NOTE] +> [!NOTE] > The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL > NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", > "MAY", and "OPTIONAL" in this document are to be interpreted as @@ -446,10 +446,10 @@ Their single-use nature is critical to the security of the WebAuthn protocol. Your application SHOULD use one of the library-provided `ChallengeManagerInterface` implementations to ensure the correct behavior. However, if one of the provided options is not suitable, you MAY implement the interface yourself or manage challenges manually. -[!WARNING] -You MUST validate that the challenge was generated by your server recently and has not already been used. -**Failing to do so will compromise the security of the protocol!** -The built-in `ChallengeManagerInterface` implementations will handle this for you. +> [!WARNING] +> You MUST validate that the challenge was generated by your server recently and has not already been used. +> **Failing to do so will compromise the security of the protocol!** +> The built-in `ChallengeManagerInterface` implementations will handle this for you. Challenges generated by your server SHOULD expire after a short amount of time. You MAY use the `ExpiringChallenge` class for convenience (e.g. `$challenge = ExpiringChallenge::withLifetime(60);`), which will throw an exception if the specified expiration window has been exceeded. From 240db534b917fb09ea235901b06356bb1ecb9fb5 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 11:21:55 -0700 Subject: [PATCH 15/47] Improve challenge docs --- README.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3a5ee31..9655e05 100644 --- a/README.md +++ b/README.md @@ -438,17 +438,24 @@ Those wire formats are covered by semantic versioning and guaranteed to not have Similarly, for data storage, the output of `Codecs\Credential::encode()` are also covered. -#### Challenge management +### Challenge management Challenges are a [cryptographic nonce](https://en.wikipedia.org/wiki/Cryptographic_nonce) that ensure a login attempt works only once. Their single-use nature is critical to the security of the WebAuthn protocol. Your application SHOULD use one of the library-provided `ChallengeManagerInterface` implementations to ensure the correct behavior. -However, if one of the provided options is not suitable, you MAY implement the interface yourself or manage challenges manually. + +| Implementation | Usage | +| --- | --- | +| `CacheChallengeManager` | Manages challenges in a site-wide pool stored in a [PSR-16](https://www.php-fig.org/psr/psr-16/) SimpleCache implementation. | +| `SessionChallengeManager` | Manages challenges through native PHP [Sessions](https://www.php.net/manual/en/intro.session.php). | + +If one of the provided options is not suitable, you MAY implement the interface yourself or manage challenges manually. > [!WARNING] > You MUST validate that the challenge was generated by your server recently and has not already been used. > **Failing to do so will compromise the security of the protocol!** +> Implementations MUST NOT trust a client-provided value. > The built-in `ChallengeManagerInterface` implementations will handle this for you. Challenges generated by your server SHOULD expire after a short amount of time. From 99c86b9fe0dc13eb7edbed35c063e33f6cefdb85 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 11:47:53 -0700 Subject: [PATCH 16/47] Test the cache manager --- README.md | 1 + tests/CacheChallengeManagerTest.php | 92 +++++++++++++++++++++++++++++ tests/ChallengeManagerTestTrait.php | 55 +++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 tests/CacheChallengeManagerTest.php create mode 100644 tests/ChallengeManagerTestTrait.php diff --git a/README.md b/README.md index 9655e05..871d059 100644 --- a/README.md +++ b/README.md @@ -451,6 +451,7 @@ Your application SHOULD use one of the library-provided `ChallengeManagerInterfa | `SessionChallengeManager` | Manages challenges through native PHP [Sessions](https://www.php.net/manual/en/intro.session.php). | If one of the provided options is not suitable, you MAY implement the interface yourself or manage challenges manually. +In the event you find this necessary, you SHOULD open an Issue and/or Pull Request for the library that indicates the shortcoming. > [!WARNING] > You MUST validate that the challenge was generated by your server recently and has not already been used. diff --git a/tests/CacheChallengeManagerTest.php b/tests/CacheChallengeManagerTest.php new file mode 100644 index 0000000..e8adb98 --- /dev/null +++ b/tests/CacheChallengeManagerTest.php @@ -0,0 +1,92 @@ +has($key)) { + return $default; + } + [$value, $exp] = $this->values[$key]; + if ($exp !== null && $exp < new DateTimeImmutable()) { + return $default; + } + + return $value; + } + + public function set(string $key, mixed $value, null|int|\DateInterval $ttl = null): bool + { + if (is_int($ttl)) { + $itvl = new DateInterval('PT' . $ttl . 'S'); + $exp = (new DateTimeImmutable())->add($itvl); + } elseif ($ttl instanceof DateInterval) { + $exp = (new DateTimeImmutable())->add($ttl); + } else { + $exp = null; + } + $this->values[$key] = [$value, $exp]; + return true; + } + + public function delete(string $key): bool + { + unset($this->values[$key]); + return true; + } + + public function clear(): bool + { + $this->values = []; + return true; + } + + public function getMultiple(iterable $keys, mixed $default = null): iterable + { + throw new BadMethodCallException(); + } + + public function setMultiple(iterable $values, null|int|\DateInterval $ttl = null): bool + { + throw new BadMethodCallException(); + } + + public function deleteMultiple(iterable $keys): bool + { + throw new BadMethodCallException(); + } + + public function has(string $key): bool + { + return array_key_exists($key, $this->values); + } + }; + return new CacheChallengeManager($cache); + } +} diff --git a/tests/ChallengeManagerTestTrait.php b/tests/ChallengeManagerTestTrait.php new file mode 100644 index 0000000..5212c46 --- /dev/null +++ b/tests/ChallengeManagerTestTrait.php @@ -0,0 +1,55 @@ +getChallengeManager(); + $c1 = $cm->createChallenge(); + $c2 = $cm->createChallenge(); + self::assertNotSame($c1, $c2); + self::assertNotSame($c1->getBase64(), $c2->getBase64()); + } + + public function testMostRecentChallengeCanBeRetrieved(): void + { + $cm = $this->getChallengeManager(); + $c = $cm->createChallenge(); + $cdjValue = Codecs\Base64Url::encode($c->getBinary()->unwrap()); + + $found = $cm->useFromClientDataJSON($cdjValue); + self::assertInstanceOf(ChallengeInterface::class, $found); + self::assertSame($c->getBase64(), $found->getBase64()); + } + + public function testMostRecentChallengeCanBeRetrievedOnlyOnce(): void + { + $cm = $this->getChallengeManager(); + $c = $cm->createChallenge(); + $cdjValue = Codecs\Base64Url::encode($c->getBinary()->unwrap()); + + $found = $cm->useFromClientDataJSON($cdjValue); + $again = $cm->useFromClientDataJSON($cdjValue); + + self::assertInstanceOf(ChallengeInterface::class, $found); + self::assertNull($again); + } + + public function testNoChallengeIsReturnedIfManagerIsEmpty(): void + { + $cm = $this->getChallengeManager(); + + $c = Challenge::random(); + $cdjValue = Codecs\Base64Url::encode($c->getBinary()->unwrap()); + + $found = $cm->useFromClientDataJSON($cdjValue); + + self::assertNull($found); + } +} From c9c2ffd13ded80d8e6363bfaf9f6f6e40bc0ce8d Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 11:54:36 -0700 Subject: [PATCH 17/47] Test the session manager --- phpunit.xml | 2 +- tests/SessionChallengeManagerTest.php | 26 ++++++++++++++++++++++++++ tests/bootstrap.php | 3 +++ 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 tests/SessionChallengeManagerTest.php create mode 100644 tests/bootstrap.php diff --git a/phpunit.xml b/phpunit.xml index 3dff4d4..891f16c 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,7 +1,7 @@ Date: Mon, 28 Aug 2023 11:55:48 -0700 Subject: [PATCH 18/47] type fix --- tests/CacheChallengeManagerTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/CacheChallengeManagerTest.php b/tests/CacheChallengeManagerTest.php index e8adb98..f54a3ec 100644 --- a/tests/CacheChallengeManagerTest.php +++ b/tests/CacheChallengeManagerTest.php @@ -72,6 +72,9 @@ public function getMultiple(iterable $keys, mixed $default = null): iterable throw new BadMethodCallException(); } + /** + * @param iterable $values + */ public function setMultiple(iterable $values, null|int|\DateInterval $ttl = null): bool { throw new BadMethodCallException(); From 75dc60cc5e8b9f8cab78355cadb56875dc05ec0c Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:18:12 -0700 Subject: [PATCH 19/47] Explain decisions --- tests/CacheChallengeManagerTest.php | 2 ++ tests/bootstrap.php | 1 + 2 files changed, 3 insertions(+) diff --git a/tests/CacheChallengeManagerTest.php b/tests/CacheChallengeManagerTest.php index f54a3ec..d7496ac 100644 --- a/tests/CacheChallengeManagerTest.php +++ b/tests/CacheChallengeManagerTest.php @@ -21,6 +21,8 @@ class CacheChallengeManagerTest extends TestCase protected function getChallengeManager(): ChallengeManagerInterface { + // Ordinarily, I'd pull in a library for this. Doing so seemed to + // cause an explosion of dependencies so I opted against in this case. $cache = new class implements CacheInterface { /** diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 4b6e0b7..11d9605 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -1,3 +1,4 @@ Date: Mon, 28 Aug 2023 13:34:56 -0700 Subject: [PATCH 20/47] Test both challenge paths in get response --- tests/GetResponseTest.php | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index f1e460b..0eb1302 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -10,7 +10,6 @@ class GetResponseTest extends \PHPUnit\Framework\TestCase { // These hold the values which would be kept server-side. - private Challenge $challenge; private CredentialInterface $credential; private RelyingParty $rp; @@ -32,13 +31,6 @@ public function setUp(): void $this->rp = new RelyingParty('http://localhost:8888'); - $this->challenge = new Challenge(BinaryString::fromBytes([ - 145, 94, 61, 93, 225, 209, 17, 150, - 18, 48, 223, 38, 136, 44, 81, 173, - 233, 248, 232, 46, 211, 200, 99, 52, - 142, 111, 103, 233, 244, 188, 26, 108, - ])); - $this->id = BinaryString::fromBytes([ 116, 216, 28, 85, 64, 195, 24, 125, 129, 100, 47, 13, 163, 166, 205, 188, @@ -102,7 +94,7 @@ public function testCDJTypeMismatchIsError(): void ); $this->expectVerificationError('7.2.11'); - $response->verify($this->challenge, $this->rp, $this->credential); + $response->verify($this->getChallenge(), $this->rp, $this->credential); } // 7.2.12 @@ -121,7 +113,7 @@ public function testCDJChallengeMismatchIsError(): void ); $this->expectVerificationError('7.2.12'); - $response->verify($this->challenge, $this->rp, $this->credential); + $response->verify($this->getChallenge(), $this->rp, $this->credential); } // 7.2.13 @@ -140,7 +132,7 @@ public function testCDJOriginMismatchIsError(): void ); $this->expectVerificationError('7.2.13'); - $response->verify($this->challenge, $this->rp, $this->credential); + $response->verify($this->getChallenge(), $this->rp, $this->credential); } // 7.2.15 @@ -156,7 +148,7 @@ public function testRelyingPartyIdMismatchIsError(): void ); $this->expectVerificationError('7.2.15'); - $response->verify($this->challenge, $rp, $this->credential); + $response->verify($this->getChallenge(), $rp, $this->credential); } // 7.2.16 @@ -178,7 +170,7 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void ); $this->expectVerificationError('7.2.17'); - $response->verify($this->challenge, $this->rp, $this->credential, UserVerificationRequirement::Required); + $response->verify($this->getChallenge(), $this->rp, $this->credential, UserVerificationRequirement::Required); } // 7.2.20 @@ -192,7 +184,7 @@ public function testIncorrectSignatureIsError(): void ); $this->expectVerificationError('7.2.20'); - $response->verify($this->challenge, $this->rp, $this->credential); + $response->verify($this->getChallenge(), $this->rp, $this->credential); } public function testVerifyReturnsCredentialWithUpdatedCounter(): void @@ -208,7 +200,7 @@ public function testVerifyReturnsCredentialWithUpdatedCounter(): void signature: $this->signature, ); - $updatedCredential = $response->verify($this->challenge, $this->rp, $this->credential); + $updatedCredential = $response->verify($this->getChallenge(), $this->rp, $this->credential); self::assertGreaterThan( 0, $updatedCredential->getSignCount(), @@ -243,7 +235,7 @@ public function testCredentialContainerWorks(): void signature: $this->signature, ); - $credential = $response->verify($this->challenge, $this->rp, $container); + $credential = $response->verify($this->getChallenge(), $this->rp, $container); self::assertSame($this->credential->getStorageId(), $credential->getStorageId()); } @@ -259,7 +251,7 @@ public function testEmptyCredentialContainerFails(): void ); $this->expectVerificationError('7.2.7'); - $response->verify($this->challenge, $this->rp, $container); + $response->verify($this->getChallenge(), $this->rp, $container); } public function testCredentialContainerMissingUsedCredentialFails(): void @@ -276,7 +268,7 @@ public function testCredentialContainerMissingUsedCredentialFails(): void ); $this->expectVerificationError('7.2.7'); - $response->verify($this->challenge, $this->rp, $container); + $response->verify($this->getChallenge(), $this->rp, $container); } private function expectVerificationError(string $section): void @@ -284,4 +276,14 @@ private function expectVerificationError(string $section): void $this->expectException(Errors\VerificationError::class); // TODO: how to assert on $section } + + protected function getChallenge(): Challenge|ChallengeManagerInterface + { + return new Challenge(BinaryString::fromBytes([ + 145, 94, 61, 93, 225, 209, 17, 150, + 18, 48, 223, 38, 136, 44, 81, 173, + 233, 248, 232, 46, 211, 200, 99, 52, + 142, 111, 103, 233, 244, 188, 26, 108, + ])); + } } From 31197d169a9c34ffdd80200de1810e66705f101e Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:37:39 -0700 Subject: [PATCH 21/47] Test with manager --- tests/GetResponseWithChallengeManagerTest.php | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 tests/GetResponseWithChallengeManagerTest.php diff --git a/tests/GetResponseWithChallengeManagerTest.php b/tests/GetResponseWithChallengeManagerTest.php new file mode 100644 index 0000000..672ec09 --- /dev/null +++ b/tests/GetResponseWithChallengeManagerTest.php @@ -0,0 +1,41 @@ +challenge; + } + + public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface + { + if ($this->accessed) { + return null; + } + $this->accessed = true; + return $this->challenge; + } + }; + } +} From 1b21fc0b2c9e1e4fd77001642111c324b2a22510 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:39:30 -0700 Subject: [PATCH 22/47] same for create --- tests/CreateResponseTest.php | 32 ++++++++------- ...CreateResponseWithChallengeManagerTest.php | 41 +++++++++++++++++++ 2 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 tests/CreateResponseWithChallengeManagerTest.php diff --git a/tests/CreateResponseTest.php b/tests/CreateResponseTest.php index 19e70a8..1468028 100644 --- a/tests/CreateResponseTest.php +++ b/tests/CreateResponseTest.php @@ -11,7 +11,6 @@ class CreateResponseTest extends \PHPUnit\Framework\TestCase { // These hold the values which would be kept server-side. private RelyingParty $rp; - private Challenge $challenge; // These hold the _default_ values from a sample parsed response. private BinaryString $id; @@ -22,13 +21,6 @@ public function setUp(): void { $this->rp = new RelyingParty('http://localhost:8888'); - $this->challenge = new Challenge(BinaryString::fromBytes([ - 40, 96, 197, 186, 42, 202, 51, 237, - 134, 178, 3, 251, 22, 204, 231, 157, - 47, 77, 43, 123, 3, 245, 57, 77, - 20, 74, 166, 166, 240, 37, 141, 188, - ])); - $this->id = BinaryString::fromBytes([ 236, 58, 219, 22, 123, 115, 98, 124, 11, 0, 207, 244, 106, 41, 249, 202, @@ -188,7 +180,7 @@ public function testCDJTypeMismatchIsError(): void ); $this->expectRegistrationError('7.1.7'); - $response->verify($this->challenge, $this->rp); + $response->verify($this->getChallenge(), $this->rp); } // 7.1.8 @@ -206,7 +198,7 @@ public function testCDJChallengeMismatchIsError(): void ); $this->expectRegistrationError('7.1.8'); - $response->verify($this->challenge, $this->rp); + $response->verify($this->getChallenge(), $this->rp); } // 7.1.9 @@ -224,7 +216,7 @@ public function testCDJOriginMismatchIsError(): void ); $this->expectRegistrationError('7.1.0'); - $response->verify($this->challenge, $this->rp); + $response->verify($this->getChallenge(), $this->rp); } // 7.1.13 @@ -238,7 +230,7 @@ public function testRelyingPartyIdMismatchIsError(): void ); $this->expectRegistrationError('7.1.13'); - $response->verify($this->challenge, $rp); + $response->verify($this->getChallenge(), $rp); } // 7.1.14 @@ -258,7 +250,7 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void ); $this->expectRegistrationError('7.1.15'); - $response->verify($this->challenge, $this->rp, UserVerificationRequirement::Required); + $response->verify($this->getChallenge(), $this->rp, UserVerificationRequirement::Required); } // 7.1.16 @@ -298,7 +290,7 @@ public function testFormatSpecificVerificationOccurs(): void ao: $ao, clientDataJson: $this->clientDataJson, ); - $response->verify($this->challenge, $this->rp); + $response->verify($this->getChallenge(), $this->rp); } public function testSuccess(): void @@ -309,12 +301,22 @@ public function testSuccess(): void clientDataJson: $this->clientDataJson, ); - $cred = $response->verify($this->challenge, $this->rp); + $cred = $response->verify($this->getChallenge(), $this->rp); self::assertSame(0, $cred->getSignCount()); // Look for a specific id and public key? } + protected function getChallenge(): Challenge|ChallengeManagerInterface + { + return new Challenge(BinaryString::fromBytes([ + 40, 96, 197, 186, 42, 202, 51, 237, + 134, 178, 3, 251, 22, 204, 231, 157, + 47, 77, 43, 123, 3, 245, 57, 77, + 20, 74, 166, 166, 240, 37, 141, 188, + ])); + } + private function expectRegistrationError(string $section): void { $this->expectException(Errors\RegistrationError::class); diff --git a/tests/CreateResponseWithChallengeManagerTest.php b/tests/CreateResponseWithChallengeManagerTest.php new file mode 100644 index 0000000..dfaf116 --- /dev/null +++ b/tests/CreateResponseWithChallengeManagerTest.php @@ -0,0 +1,41 @@ +challenge; + } + + public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface + { + if ($this->accessed) { + return null; + } + $this->accessed = true; + return $this->challenge; + } + }; + } +} From 58bebb79b08d5e576dbc295dc31e95c8bb7c1b93 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:41:55 -0700 Subject: [PATCH 23/47] note to handle inactive --- tests/CreateResponseWithChallengeManagerTest.php | 5 +++++ tests/GetResponseWithChallengeManagerTest.php | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/CreateResponseWithChallengeManagerTest.php b/tests/CreateResponseWithChallengeManagerTest.php index dfaf116..1b2aff4 100644 --- a/tests/CreateResponseWithChallengeManagerTest.php +++ b/tests/CreateResponseWithChallengeManagerTest.php @@ -38,4 +38,9 @@ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface } }; } + + public function testFailIfNoActiveChallenge(): void + { + self::markTestIncomplete(); + } } diff --git a/tests/GetResponseWithChallengeManagerTest.php b/tests/GetResponseWithChallengeManagerTest.php index 672ec09..3cb08f5 100644 --- a/tests/GetResponseWithChallengeManagerTest.php +++ b/tests/GetResponseWithChallengeManagerTest.php @@ -38,4 +38,9 @@ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface } }; } + + public function testFailIfNoActiveChallenge(): void + { + self::markTestIncomplete(); + } } From a81086815d9967a530b1b49137bea5cd7f00f158 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:48:25 -0700 Subject: [PATCH 24/47] Ignore dev-only dependencies, this is by design --- composer-require-checker.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 composer-require-checker.json diff --git a/composer-require-checker.json b/composer-require-checker.json new file mode 100644 index 0000000..8c93a13 --- /dev/null +++ b/composer-require-checker.json @@ -0,0 +1,7 @@ +{ + "symbol-whitelist": [ + "PHP_SESSION_ACTIVE", + "Psr\\SimpleCache\\CacheInterface", + "session_status" + ] +} From a7f1edd051047d1e0268b9a4b9beed8e8ab240a3 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:49:37 -0700 Subject: [PATCH 25/47] Tidy imports --- src/CacheChallengeManager.php | 2 ++ src/SessionChallengeManager.php | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index f7b4523..e0fcc00 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -7,6 +7,8 @@ use Psr\SimpleCache\CacheInterface; use UnexpectedValueException; +use function sprintf; + class CacheChallengeManager implements ChallengeManagerInterface { public function __construct( diff --git a/src/SessionChallengeManager.php b/src/SessionChallengeManager.php index d8fa78b..07116d8 100644 --- a/src/SessionChallengeManager.php +++ b/src/SessionChallengeManager.php @@ -4,6 +4,11 @@ namespace Firehed\WebAuthn; +use function array_key_exists; +use function session_status; + +use const PHP_SESSION_ACTIVE; + class SessionChallengeManager implements ChallengeManagerInterface { private const SESSION_KEY = 'passkey_challenge'; From bb619e8427cc862c868eedd9d4d20877421f3839 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 13:59:46 -0700 Subject: [PATCH 26/47] Fix up the readme a bit --- README.md | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 871d059..58f2a72 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,6 @@ This also means that users do not have to manage passwords for individual websit ## Using this library: A Crash Course This will cover the basic workflows for integrating this library to your web application. -Classes referenced in the examples may omit the `Firehed\WebAuthn` namespace prefix for brevity. > [!NOTE] > The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL @@ -37,7 +36,7 @@ This **MUST** match the complete origin that users will interact with; e.g. `htt The protocol is always required; the port must only be present if using a non-standard port and must be excluded for standard ports. ```php -$rp = new RelyingParty('https://www.example.com'); +$rp = new \Firehed\WebAuthn\RelyingParty('https://www.example.com'); ``` Also create a `ChallengeManagerInterface`. @@ -46,13 +45,13 @@ See the [Challenge Management](#challenge-management) section below for more inf ```php session_start(); -$challengeManager = new SessionChallengeManager(); +$challengeManager = new \Firehed\WebAuthn\SessionChallengeManager(); ``` -[!IMPORTANT] -WebAuthn will only work in a "secure context". -This means that the domain MUST run over `https`, with a sole exception for `localhost`. -See [https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts]() for more info. +> [!IMPORTANT] +> WebAuthn will only work in a "secure context". +> This means that the domain MUST run over `https`, with a sole exception for `localhost`. +> See [https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts]() for more info. ### Registering a WebAuthn credential to a user @@ -206,10 +205,7 @@ This assumes the same schema from the previous Registration example. ```php [!NOTE] +> The W3C specification recommends a timeout in the range of 15-120 seconds. ### Error Handling From 0f2ed7fead78e43ff853882cc90f48380a9a6151 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 14:06:50 -0700 Subject: [PATCH 27/47] Clean up session internals --- src/SessionChallengeManager.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/SessionChallengeManager.php b/src/SessionChallengeManager.php index 07116d8..89cff4b 100644 --- a/src/SessionChallengeManager.php +++ b/src/SessionChallengeManager.php @@ -4,6 +4,8 @@ namespace Firehed\WebAuthn; +use BadMethodCallException; + use function array_key_exists; use function session_status; @@ -15,9 +17,9 @@ class SessionChallengeManager implements ChallengeManagerInterface public function __construct() { - // do later? + // Do this later? if (session_status() !== PHP_SESSION_ACTIVE) { - throw new \BadMethodCallException('Call session_start()'); + throw new BadMethodCallException('No active session. Call session_start() before using this.'); } } @@ -30,12 +32,12 @@ public function createChallenge(): ChallengeInterface public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface { - // TODO: match url? if (!array_key_exists(self::SESSION_KEY, $_SESSION)) { return null; } $challenge = $_SESSION[self::SESSION_KEY]; unset($_SESSION[self::SESSION_KEY]); + // Validate that the stored challenge matches the CDJ value? return $challenge; } } From d3c7659561f9f1078a457dd97c5d352b39970570 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 11:59:52 -0700 Subject: [PATCH 28/47] remove cache manager for now --- README.md | 2 - src/CacheChallengeManager.php | 53 ---------------- tests/CacheChallengeManagerTest.php | 97 ----------------------------- 3 files changed, 152 deletions(-) delete mode 100644 src/CacheChallengeManager.php delete mode 100644 tests/CacheChallengeManagerTest.php diff --git a/README.md b/README.md index 58f2a72..44a4eed 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,6 @@ $rp = new \Firehed\WebAuthn\RelyingParty('https://www.example.com'); ``` Also create a `ChallengeManagerInterface`. -There are multiple options available which can suit different applications. See the [Challenge Management](#challenge-management) section below for more information. ```php @@ -443,7 +442,6 @@ Your application SHOULD use one of the library-provided `ChallengeManagerInterfa | Implementation | Usage | | --- | --- | -| `CacheChallengeManager` | Manages challenges in a site-wide pool stored in a [PSR-16](https://www.php-fig.org/psr/psr-16/) SimpleCache implementation. | | `SessionChallengeManager` | Manages challenges through native PHP [Sessions](https://www.php.net/manual/en/intro.session.php). | If one of the provided options is not suitable, you MAY implement the interface yourself or manage challenges manually. diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php deleted file mode 100644 index e0fcc00..0000000 --- a/src/CacheChallengeManager.php +++ /dev/null @@ -1,53 +0,0 @@ -cacheKeyPrefix, - Codecs\Base64Url::encode($c->getBinary()->unwrap()), - ); - $this->cache->set($cacheKey, $c, 120); - return $c; - } - - public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface - { - $key = sprintf( - '%s%s', - $this->cacheKeyPrefix, - $base64Url, - ); - // WARNING: race condition. Without at least a CAS guarantee, this - // can't be avoided with SimpleCache. - $active = $this->cache->get($key); - if ($active === null) { - return $active; - } - $this->cache->delete($key); - if ($active instanceof ChallengeInterface) { - return $active; - } - throw new UnexpectedValueException('Non-challenge found in cache'); - } -} diff --git a/tests/CacheChallengeManagerTest.php b/tests/CacheChallengeManagerTest.php deleted file mode 100644 index d7496ac..0000000 --- a/tests/CacheChallengeManagerTest.php +++ /dev/null @@ -1,97 +0,0 @@ -has($key)) { - return $default; - } - [$value, $exp] = $this->values[$key]; - if ($exp !== null && $exp < new DateTimeImmutable()) { - return $default; - } - - return $value; - } - - public function set(string $key, mixed $value, null|int|\DateInterval $ttl = null): bool - { - if (is_int($ttl)) { - $itvl = new DateInterval('PT' . $ttl . 'S'); - $exp = (new DateTimeImmutable())->add($itvl); - } elseif ($ttl instanceof DateInterval) { - $exp = (new DateTimeImmutable())->add($ttl); - } else { - $exp = null; - } - $this->values[$key] = [$value, $exp]; - return true; - } - - public function delete(string $key): bool - { - unset($this->values[$key]); - return true; - } - - public function clear(): bool - { - $this->values = []; - return true; - } - - public function getMultiple(iterable $keys, mixed $default = null): iterable - { - throw new BadMethodCallException(); - } - - /** - * @param iterable $values - */ - public function setMultiple(iterable $values, null|int|\DateInterval $ttl = null): bool - { - throw new BadMethodCallException(); - } - - public function deleteMultiple(iterable $keys): bool - { - throw new BadMethodCallException(); - } - - public function has(string $key): bool - { - return array_key_exists($key, $this->values); - } - }; - return new CacheChallengeManager($cache); - } -} From 292c2d57908b62be73c102ed1aa8a494a40ad6b6 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:01:28 -0700 Subject: [PATCH 29/47] more de-integration of other cache cm --- composer-require-checker.json | 1 - composer.json | 4 ---- 2 files changed, 5 deletions(-) diff --git a/composer-require-checker.json b/composer-require-checker.json index 8c93a13..81d083c 100644 --- a/composer-require-checker.json +++ b/composer-require-checker.json @@ -1,7 +1,6 @@ { "symbol-whitelist": [ "PHP_SESSION_ACTIVE", - "Psr\\SimpleCache\\CacheInterface", "session_status" ] } diff --git a/composer.json b/composer.json index 5d8c545..fd47146 100644 --- a/composer.json +++ b/composer.json @@ -41,7 +41,6 @@ "phpstan/phpstan-phpunit": "^1.0", "phpstan/phpstan-strict-rules": "^1.0", "phpunit/phpunit": "^9.3", - "psr/simple-cache": "^3.0", "squizlabs/php_codesniffer": "^3.5" }, "scripts": { @@ -55,8 +54,5 @@ "phpstan": "phpstan analyse", "phpstan-baseline": "phpstan analyse --generate-baseline", "phpcs": "phpcs" - }, - "suggest": { - "psr/simple-cache": "Allows use of CacheChallengeManager" } } From d72734c3b27ad55af847d88acd5782e8e197dd20 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:04:20 -0700 Subject: [PATCH 30/47] require challenge manager for create --- src/CreateResponse.php | 10 ++++------ src/Responses/AttestationInterface.php | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/CreateResponse.php b/src/CreateResponse.php index 20d0cc1..6d27fc1 100644 --- a/src/CreateResponse.php +++ b/src/CreateResponse.php @@ -27,7 +27,7 @@ public function __construct( * @link https://www.w3.org/TR/webauthn-2/#sctn-registering-a-new-credential */ public function verify( - ChallengeInterface | ChallengeManagerInterface $challenge, + ChallengeManagerInterface $challenge, RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface { @@ -48,11 +48,9 @@ public function verify( // 7.1.8 $cdjChallenge = $C['challenge']; - if ($challenge instanceof ChallengeManagerInterface) { - $challenge = $challenge->useFromClientDataJSON($cdjChallenge); - if ($challenge === null) { - $this->fail('7.1.8', 'C.challenge'); - } + $challenge = $challenge->useFromClientDataJSON($cdjChallenge); + if ($challenge === null) { + $this->fail('7.1.8', 'C.challenge'); } $b64u = Codecs\Base64Url::encode($challenge->getBinary()->unwrap()); diff --git a/src/Responses/AttestationInterface.php b/src/Responses/AttestationInterface.php index 4ae5d3d..a3ee042 100644 --- a/src/Responses/AttestationInterface.php +++ b/src/Responses/AttestationInterface.php @@ -21,7 +21,7 @@ interface AttestationInterface { public function verify( - ChallengeInterface | ChallengeManagerInterface $challenge, + ChallengeManagerInterface $challenge, RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; From 918633d4e3db23e49559fc8121418f6d70599942 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:04:58 -0700 Subject: [PATCH 31/47] require challenge manager for get --- src/GetResponse.php | 10 ++++------ src/Responses/AssertionInterface.php | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/GetResponse.php b/src/GetResponse.php index fa5f9b2..1bb0521 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -36,7 +36,7 @@ public function getUsedCredentialId(): BinaryString * @link https://www.w3.org/TR/webauthn-2/#sctn-verifying-assertion */ public function verify( - ChallengeInterface|ChallengeManagerInterface $challenge, + ChallengeManagerInterface $challenge, RelyingParty $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, @@ -90,11 +90,9 @@ public function verify( // 7.2.12 $cdjChallenge = $C['challenge']; - if ($challenge instanceof ChallengeManagerInterface) { - $challenge = $challenge->useFromClientDataJSON($cdjChallenge); - if ($challenge === null) { - $this->fail('7.2.12', 'C.challenge'); - } + $challenge = $challenge->useFromClientDataJSON($cdjChallenge); + if ($challenge === null) { + $this->fail('7.2.12', 'C.challenge'); } $b64u = Codecs\Base64Url::encode($challenge->getBinary()->unwrap()); diff --git a/src/Responses/AssertionInterface.php b/src/Responses/AssertionInterface.php index baf5e7b..beb87d8 100644 --- a/src/Responses/AssertionInterface.php +++ b/src/Responses/AssertionInterface.php @@ -31,7 +31,7 @@ public function getUsedCredentialId(): BinaryString; * @api */ public function verify( - ChallengeInterface | ChallengeManagerInterface $challenge, + ChallengeManagerInterface $challenge, RelyingParty $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, From 978da8159817b496f0368093224c3d6b1ed66077 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:10:33 -0700 Subject: [PATCH 32/47] Adjust test buildout --- tests/CreateResponseTest.php | 17 ++++--- ...CreateResponseWithChallengeManagerTest.php | 46 ------------------- tests/GetResponseTest.php | 17 ++++--- tests/GetResponseWithChallengeManagerTest.php | 46 ------------------- 4 files changed, 20 insertions(+), 106 deletions(-) delete mode 100644 tests/CreateResponseWithChallengeManagerTest.php delete mode 100644 tests/GetResponseWithChallengeManagerTest.php diff --git a/tests/CreateResponseTest.php b/tests/CreateResponseTest.php index 1468028..3e251e1 100644 --- a/tests/CreateResponseTest.php +++ b/tests/CreateResponseTest.php @@ -307,14 +307,17 @@ public function testSuccess(): void // Look for a specific id and public key? } - protected function getChallenge(): Challenge|ChallengeManagerInterface + protected function getChallenge(): ChallengeManagerInterface { - return new Challenge(BinaryString::fromBytes([ - 40, 96, 197, 186, 42, 202, 51, 237, - 134, 178, 3, 251, 22, 204, 231, 157, - 47, 77, 43, 123, 3, 245, 57, 77, - 20, 74, 166, 166, 240, 37, 141, 188, - ])); + $mock = self::createMock(ChallengeManagerInterface::class); + $mock->method('useFromClientDataJSON') + ->willReturn(new Challenge(BinaryString::fromBytes([ + 40, 96, 197, 186, 42, 202, 51, 237, + 134, 178, 3, 251, 22, 204, 231, 157, + 47, 77, 43, 123, 3, 245, 57, 77, + 20, 74, 166, 166, 240, 37, 141, 188, + ]))); + return $mock; } private function expectRegistrationError(string $section): void diff --git a/tests/CreateResponseWithChallengeManagerTest.php b/tests/CreateResponseWithChallengeManagerTest.php deleted file mode 100644 index 1b2aff4..0000000 --- a/tests/CreateResponseWithChallengeManagerTest.php +++ /dev/null @@ -1,46 +0,0 @@ -challenge; - } - - public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface - { - if ($this->accessed) { - return null; - } - $this->accessed = true; - return $this->challenge; - } - }; - } - - public function testFailIfNoActiveChallenge(): void - { - self::markTestIncomplete(); - } -} diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index 0eb1302..62cbec2 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -277,13 +277,16 @@ private function expectVerificationError(string $section): void // TODO: how to assert on $section } - protected function getChallenge(): Challenge|ChallengeManagerInterface + protected function getChallenge(): ChallengeManagerInterface { - return new Challenge(BinaryString::fromBytes([ - 145, 94, 61, 93, 225, 209, 17, 150, - 18, 48, 223, 38, 136, 44, 81, 173, - 233, 248, 232, 46, 211, 200, 99, 52, - 142, 111, 103, 233, 244, 188, 26, 108, - ])); + $mock = self::createMock(ChallengeManagerInterface::class); + $mock->method('useFromClientDataJSON') + ->willReturn(new Challenge(BinaryString::fromBytes([ + 145, 94, 61, 93, 225, 209, 17, 150, + 18, 48, 223, 38, 136, 44, 81, 173, + 233, 248, 232, 46, 211, 200, 99, 52, + 142, 111, 103, 233, 244, 188, 26, 108, + ]))); + return $mock; } } diff --git a/tests/GetResponseWithChallengeManagerTest.php b/tests/GetResponseWithChallengeManagerTest.php deleted file mode 100644 index 3cb08f5..0000000 --- a/tests/GetResponseWithChallengeManagerTest.php +++ /dev/null @@ -1,46 +0,0 @@ -challenge; - } - - public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface - { - if ($this->accessed) { - return null; - } - $this->accessed = true; - return $this->challenge; - } - }; - } - - public function testFailIfNoActiveChallenge(): void - { - self::markTestIncomplete(); - } -} From af76732fc5f96738876f49c7c2d0627fdf1df3ad Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:11:39 -0700 Subject: [PATCH 33/47] rename --- tests/CreateResponseTest.php | 16 ++++++++-------- tests/GetResponseTest.php | 22 +++++++++++----------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/CreateResponseTest.php b/tests/CreateResponseTest.php index 3e251e1..957f580 100644 --- a/tests/CreateResponseTest.php +++ b/tests/CreateResponseTest.php @@ -180,7 +180,7 @@ public function testCDJTypeMismatchIsError(): void ); $this->expectRegistrationError('7.1.7'); - $response->verify($this->getChallenge(), $this->rp); + $response->verify($this->getChallengeManager(), $this->rp); } // 7.1.8 @@ -198,7 +198,7 @@ public function testCDJChallengeMismatchIsError(): void ); $this->expectRegistrationError('7.1.8'); - $response->verify($this->getChallenge(), $this->rp); + $response->verify($this->getChallengeManager(), $this->rp); } // 7.1.9 @@ -216,7 +216,7 @@ public function testCDJOriginMismatchIsError(): void ); $this->expectRegistrationError('7.1.0'); - $response->verify($this->getChallenge(), $this->rp); + $response->verify($this->getChallengeManager(), $this->rp); } // 7.1.13 @@ -230,7 +230,7 @@ public function testRelyingPartyIdMismatchIsError(): void ); $this->expectRegistrationError('7.1.13'); - $response->verify($this->getChallenge(), $rp); + $response->verify($this->getChallengeManager(), $rp); } // 7.1.14 @@ -250,7 +250,7 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void ); $this->expectRegistrationError('7.1.15'); - $response->verify($this->getChallenge(), $this->rp, UserVerificationRequirement::Required); + $response->verify($this->getChallengeManager(), $this->rp, UserVerificationRequirement::Required); } // 7.1.16 @@ -290,7 +290,7 @@ public function testFormatSpecificVerificationOccurs(): void ao: $ao, clientDataJson: $this->clientDataJson, ); - $response->verify($this->getChallenge(), $this->rp); + $response->verify($this->getChallengeManager(), $this->rp); } public function testSuccess(): void @@ -301,13 +301,13 @@ public function testSuccess(): void clientDataJson: $this->clientDataJson, ); - $cred = $response->verify($this->getChallenge(), $this->rp); + $cred = $response->verify($this->getChallengeManager(), $this->rp); self::assertSame(0, $cred->getSignCount()); // Look for a specific id and public key? } - protected function getChallenge(): ChallengeManagerInterface + protected function getChallengeManager(): ChallengeManagerInterface { $mock = self::createMock(ChallengeManagerInterface::class); $mock->method('useFromClientDataJSON') diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index 62cbec2..a4baa59 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -94,7 +94,7 @@ public function testCDJTypeMismatchIsError(): void ); $this->expectVerificationError('7.2.11'); - $response->verify($this->getChallenge(), $this->rp, $this->credential); + $response->verify($this->getChallengeManager(), $this->rp, $this->credential); } // 7.2.12 @@ -113,7 +113,7 @@ public function testCDJChallengeMismatchIsError(): void ); $this->expectVerificationError('7.2.12'); - $response->verify($this->getChallenge(), $this->rp, $this->credential); + $response->verify($this->getChallengeManager(), $this->rp, $this->credential); } // 7.2.13 @@ -132,7 +132,7 @@ public function testCDJOriginMismatchIsError(): void ); $this->expectVerificationError('7.2.13'); - $response->verify($this->getChallenge(), $this->rp, $this->credential); + $response->verify($this->getChallengeManager(), $this->rp, $this->credential); } // 7.2.15 @@ -148,7 +148,7 @@ public function testRelyingPartyIdMismatchIsError(): void ); $this->expectVerificationError('7.2.15'); - $response->verify($this->getChallenge(), $rp, $this->credential); + $response->verify($this->getChallengeManager(), $rp, $this->credential); } // 7.2.16 @@ -170,7 +170,7 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void ); $this->expectVerificationError('7.2.17'); - $response->verify($this->getChallenge(), $this->rp, $this->credential, UserVerificationRequirement::Required); + $response->verify($this->getChallengeManager(), $this->rp, $this->credential, UserVerificationRequirement::Required); } // 7.2.20 @@ -184,7 +184,7 @@ public function testIncorrectSignatureIsError(): void ); $this->expectVerificationError('7.2.20'); - $response->verify($this->getChallenge(), $this->rp, $this->credential); + $response->verify($this->getChallengeManager(), $this->rp, $this->credential); } public function testVerifyReturnsCredentialWithUpdatedCounter(): void @@ -200,7 +200,7 @@ public function testVerifyReturnsCredentialWithUpdatedCounter(): void signature: $this->signature, ); - $updatedCredential = $response->verify($this->getChallenge(), $this->rp, $this->credential); + $updatedCredential = $response->verify($this->getChallengeManager(), $this->rp, $this->credential); self::assertGreaterThan( 0, $updatedCredential->getSignCount(), @@ -235,7 +235,7 @@ public function testCredentialContainerWorks(): void signature: $this->signature, ); - $credential = $response->verify($this->getChallenge(), $this->rp, $container); + $credential = $response->verify($this->getChallengeManager(), $this->rp, $container); self::assertSame($this->credential->getStorageId(), $credential->getStorageId()); } @@ -251,7 +251,7 @@ public function testEmptyCredentialContainerFails(): void ); $this->expectVerificationError('7.2.7'); - $response->verify($this->getChallenge(), $this->rp, $container); + $response->verify($this->getChallengeManager(), $this->rp, $container); } public function testCredentialContainerMissingUsedCredentialFails(): void @@ -268,7 +268,7 @@ public function testCredentialContainerMissingUsedCredentialFails(): void ); $this->expectVerificationError('7.2.7'); - $response->verify($this->getChallenge(), $this->rp, $container); + $response->verify($this->getChallengeManager(), $this->rp, $container); } private function expectVerificationError(string $section): void @@ -277,7 +277,7 @@ private function expectVerificationError(string $section): void // TODO: how to assert on $section } - protected function getChallenge(): ChallengeManagerInterface + protected function getChallengeManager(): ChallengeManagerInterface { $mock = self::createMock(ChallengeManagerInterface::class); $mock->method('useFromClientDataJSON') From 3ea5f0def903c70e4590d76d8b64a04b5a598887 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:14:32 -0700 Subject: [PATCH 34/47] wrap in e2e test --- tests/EndToEndTest.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/EndToEndTest.php b/tests/EndToEndTest.php index 77818d6..4503bd8 100644 --- a/tests/EndToEndTest.php +++ b/tests/EndToEndTest.php @@ -30,7 +30,7 @@ public function testRegisterAndLogin(string $directory): void $registerRequest = $this->safeReadJsonFile("$directory/register.json"); $attestation = $parser->parseCreateResponse($registerRequest); - $credential = $attestation->verify($registerChallenge, $this->rp); + $credential = $attestation->verify($this->wrapChallenge($registerChallenge), $this->rp); $loginInfo = $this->safeReadJsonFile("$directory/loginInfo.json"); $loginChallenge = new Challenge( @@ -40,7 +40,7 @@ public function testRegisterAndLogin(string $directory): void $assertion = $parser->parseGetResponse($loginRequest); $updatedCredential = $assertion->verify( - $loginChallenge, + $this->wrapChallenge($loginChallenge), $this->rp, $credential, ); @@ -88,4 +88,12 @@ private function safeReadJsonFile(string $path): array assert(is_array($data)); return $data; } + + private function wrapChallenge(ChallengeInterface $challenge): ChallengeManagerInterface + { + $mock = $this->createMock(ChallengeManagerInterface::class); + $mock->method('useFromClientDataJSON') + ->willReturn($challenge); + return $mock; + } } From 4806de97dd8a0895fa0fd0e40147f58675501f90 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:17:28 -0700 Subject: [PATCH 35/47] update register examples --- examples/readmeRegisterStep1.php | 7 +++---- examples/readmeRegisterStep3.php | 5 +++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/examples/readmeRegisterStep1.php b/examples/readmeRegisterStep1.php index 14761f4..f268d54 100644 --- a/examples/readmeRegisterStep1.php +++ b/examples/readmeRegisterStep1.php @@ -3,6 +3,7 @@ require __DIR__ . '/vendor/autoload.php'; use Firehed\WebAuthn\ExpiringChallenge; +use Firehed\WebAuthn\SessionChallengeManager; session_start(); @@ -13,10 +14,8 @@ $_SESSION['user_id'] = $user['id']; // Generate challenge -$challenge = ExpiringChallenge::withLifetime(120); - -// Store server-side; adjust to your app's needs -$_SESSION['webauthn_challenge'] = $challenge; +$challengeManager = new SessionChallengeManager(); +$challenge = $challengeManager->createChallenge(); // Send to user header('Content-type: application/json'); diff --git a/examples/readmeRegisterStep3.php b/examples/readmeRegisterStep3.php index b33f851..5a52521 100644 --- a/examples/readmeRegisterStep3.php +++ b/examples/readmeRegisterStep3.php @@ -5,6 +5,7 @@ use Firehed\WebAuthn\{ Codecs, ResponseParser, + SessionChallengeManager, }; session_start(); @@ -18,10 +19,10 @@ $createResponse = $parser->parseCreateResponse($data); $rp = getRelyingParty(); -$challenge = $_SESSION['webauthn_challenge']; +$challengeManager = new SessionChallengeManager(); try { - $credential = $createResponse->verify($challenge, $rp); + $credential = $createResponse->verify($challengeManager, $rp); } catch (Throwable) { // Verification failed. Send an error to the user? header('HTTP/1.1 403 Unauthorized'); From 530e659fce558c19255c92d65f565c78ab04c066 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:20:39 -0700 Subject: [PATCH 36/47] update examples rest of the way through --- examples/functions.php | 7 +++++++ examples/readmeLoginStep1.php | 5 +++-- examples/readmeLoginStep3.php | 5 +++-- examples/readmeRegisterStep1.php | 2 +- examples/readmeRegisterStep3.php | 2 +- 5 files changed, 15 insertions(+), 6 deletions(-) diff --git a/examples/functions.php b/examples/functions.php index 3a8e3f9..366f5f8 100644 --- a/examples/functions.php +++ b/examples/functions.php @@ -3,9 +3,11 @@ declare(strict_types=1); use Firehed\WebAuthn\{ + ChallengeManagerInterface, Codecs, CredentialContainer, RelyingParty, + SessionChallengeManager, }; /** @@ -28,6 +30,11 @@ function createUser(PDO $pdo, string $username): array return $response; } +function getChallengeManager(): ChallengeManagerInterface +{ + return new SessionChallengeManager(); +} + function getCredentialsForUserId(PDO $pdo, string $userId): CredentialContainer { $stmt = $pdo->prepare('SELECT * FROM user_credentials WHERE user_id = ?'); diff --git a/examples/readmeLoginStep1.php b/examples/readmeLoginStep1.php index 8a36c26..8d7927d 100644 --- a/examples/readmeLoginStep1.php +++ b/examples/readmeLoginStep1.php @@ -5,6 +5,7 @@ use Firehed\WebAuthn\{ Codecs, ExpiringChallenge, + SessionChallengeManager, }; session_start(); @@ -19,8 +20,8 @@ $credentialContainer = getCredentialsForUserId($pdo, $user['id']); -$challenge = ExpiringChallenge::withLifetime(120); -$_SESSION['webauthn_challenge'] = $challenge; +$challengeManager = getChallengeManager(); +$challenge = $challengeManager->createChallenge(); // Send to user header('Content-type: application/json'); diff --git a/examples/readmeLoginStep3.php b/examples/readmeLoginStep3.php index 6dee223..4260b58 100644 --- a/examples/readmeLoginStep3.php +++ b/examples/readmeLoginStep3.php @@ -5,6 +5,7 @@ use Firehed\WebAuthn\{ Codecs, ResponseParser, + SessionChallengeManager, }; session_start(); @@ -20,12 +21,12 @@ $getResponse = $parser->parseGetResponse($data); $rp = getRelyingParty(); -$challenge = $_SESSION['webauthn_challenge']; $credentialContainer = getCredentialsForUserId($pdo, $_SESSION['authenticating_user_id']); +$challengeManager = getChallengeManager(); try { - $updatedCredential = $getResponse->verify($challenge, $rp, $credentialContainer); + $updatedCredential = $getResponse->verify($challengeManager, $rp, $credentialContainer); } catch (Throwable) { // Verification failed. Send an error to the user? header('HTTP/1.1 403 Unauthorized'); diff --git a/examples/readmeRegisterStep1.php b/examples/readmeRegisterStep1.php index f268d54..aa5bcb7 100644 --- a/examples/readmeRegisterStep1.php +++ b/examples/readmeRegisterStep1.php @@ -14,7 +14,7 @@ $_SESSION['user_id'] = $user['id']; // Generate challenge -$challengeManager = new SessionChallengeManager(); +$challengeManager = getChallengeManager(); $challenge = $challengeManager->createChallenge(); // Send to user diff --git a/examples/readmeRegisterStep3.php b/examples/readmeRegisterStep3.php index 5a52521..35d9942 100644 --- a/examples/readmeRegisterStep3.php +++ b/examples/readmeRegisterStep3.php @@ -19,7 +19,7 @@ $createResponse = $parser->parseCreateResponse($data); $rp = getRelyingParty(); -$challengeManager = new SessionChallengeManager(); +$challengeManager = getChallengeManager(); try { $credential = $createResponse->verify($challengeManager, $rp); From 49d748c1cbed6c5c6f6bd8bbe7d89bde27b80808 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 12:21:06 -0700 Subject: [PATCH 37/47] Format --- tests/GetResponseTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index a4baa59..ab0ba8f 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -170,7 +170,12 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void ); $this->expectVerificationError('7.2.17'); - $response->verify($this->getChallengeManager(), $this->rp, $this->credential, UserVerificationRequirement::Required); + $response->verify( + $this->getChallengeManager(), + $this->rp, + $this->credential, + UserVerificationRequirement::Required, + ); } // 7.2.20 From 3ebcc8104d7fe65c4c2df3d077deaacc7c1cc20f Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 13:51:28 -0700 Subject: [PATCH 38/47] refactor to test replays --- tests/FixedChallengeManager.php | 26 +++++++++++++++++++++ tests/GetResponseTest.php | 41 +++++++++++++++------------------ 2 files changed, 44 insertions(+), 23 deletions(-) create mode 100644 tests/FixedChallengeManager.php diff --git a/tests/FixedChallengeManager.php b/tests/FixedChallengeManager.php new file mode 100644 index 0000000..8d14b03 --- /dev/null +++ b/tests/FixedChallengeManager.php @@ -0,0 +1,26 @@ +challenge = $challenge; + } + public function createChallenge(): ChallengeInterface + { + assert($this->challenge !== null); + return $this->challenge; + } + public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface + { + $challenge = $this->challenge; + // "consume" it after the first use + $this->challenge = null; + return $challenge; + } +} diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index ab0ba8f..55a5060 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -12,6 +12,7 @@ class GetResponseTest extends \PHPUnit\Framework\TestCase // These hold the values which would be kept server-side. private CredentialInterface $credential; private RelyingParty $rp; + private ChallengeManagerInterface $cm; // These hold the _default_ values from a sample parsed response. private BinaryString $id; @@ -76,6 +77,13 @@ public function setUp(): void 48, 31, 150, 139, 30, 239, 98, 204, 1, 90, 103, 114, 23, 105, ]); + + $this->cm = new FixedChallengeManager(new Challenge(BinaryString::fromBytes([ + 145, 94, 61, 93, 225, 209, 17, 150, + 18, 48, 223, 38, 136, 44, 81, 173, + 233, 248, 232, 46, 211, 200, 99, 52, + 142, 111, 103, 233, 244, 188, 26, 108, + ]))); } // 7.2.11 @@ -94,7 +102,7 @@ public function testCDJTypeMismatchIsError(): void ); $this->expectVerificationError('7.2.11'); - $response->verify($this->getChallengeManager(), $this->rp, $this->credential); + $response->verify($this->cm, $this->rp, $this->credential); } // 7.2.12 @@ -113,7 +121,7 @@ public function testCDJChallengeMismatchIsError(): void ); $this->expectVerificationError('7.2.12'); - $response->verify($this->getChallengeManager(), $this->rp, $this->credential); + $response->verify($this->cm, $this->rp, $this->credential); } // 7.2.13 @@ -132,7 +140,7 @@ public function testCDJOriginMismatchIsError(): void ); $this->expectVerificationError('7.2.13'); - $response->verify($this->getChallengeManager(), $this->rp, $this->credential); + $response->verify($this->cm, $this->rp, $this->credential); } // 7.2.15 @@ -148,7 +156,7 @@ public function testRelyingPartyIdMismatchIsError(): void ); $this->expectVerificationError('7.2.15'); - $response->verify($this->getChallengeManager(), $rp, $this->credential); + $response->verify($this->cm, $rp, $this->credential); } // 7.2.16 @@ -171,7 +179,7 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void $this->expectVerificationError('7.2.17'); $response->verify( - $this->getChallengeManager(), + $this->cm, $this->rp, $this->credential, UserVerificationRequirement::Required, @@ -189,7 +197,7 @@ public function testIncorrectSignatureIsError(): void ); $this->expectVerificationError('7.2.20'); - $response->verify($this->getChallengeManager(), $this->rp, $this->credential); + $response->verify($this->cm, $this->rp, $this->credential); } public function testVerifyReturnsCredentialWithUpdatedCounter(): void @@ -205,7 +213,7 @@ public function testVerifyReturnsCredentialWithUpdatedCounter(): void signature: $this->signature, ); - $updatedCredential = $response->verify($this->getChallengeManager(), $this->rp, $this->credential); + $updatedCredential = $response->verify($this->cm, $this->rp, $this->credential); self::assertGreaterThan( 0, $updatedCredential->getSignCount(), @@ -240,7 +248,7 @@ public function testCredentialContainerWorks(): void signature: $this->signature, ); - $credential = $response->verify($this->getChallengeManager(), $this->rp, $container); + $credential = $response->verify($this->cm, $this->rp, $container); self::assertSame($this->credential->getStorageId(), $credential->getStorageId()); } @@ -256,7 +264,7 @@ public function testEmptyCredentialContainerFails(): void ); $this->expectVerificationError('7.2.7'); - $response->verify($this->getChallengeManager(), $this->rp, $container); + $response->verify($this->cm, $this->rp, $container); } public function testCredentialContainerMissingUsedCredentialFails(): void @@ -273,7 +281,7 @@ public function testCredentialContainerMissingUsedCredentialFails(): void ); $this->expectVerificationError('7.2.7'); - $response->verify($this->getChallengeManager(), $this->rp, $container); + $response->verify($this->cm, $this->rp, $container); } private function expectVerificationError(string $section): void @@ -281,17 +289,4 @@ private function expectVerificationError(string $section): void $this->expectException(Errors\VerificationError::class); // TODO: how to assert on $section } - - protected function getChallengeManager(): ChallengeManagerInterface - { - $mock = self::createMock(ChallengeManagerInterface::class); - $mock->method('useFromClientDataJSON') - ->willReturn(new Challenge(BinaryString::fromBytes([ - 145, 94, 61, 93, 225, 209, 17, 150, - 18, 48, 223, 38, 136, 44, 81, 173, - 233, 248, 232, 46, 211, 200, 99, 52, - 142, 111, 103, 233, 244, 188, 26, 108, - ]))); - return $mock; - } } From 1c4ecf84e3997542c9f798e1bd870bab8d4f54be Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 13:53:23 -0700 Subject: [PATCH 39/47] add test --- tests/GetResponseTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index 55a5060..301f4e8 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -105,6 +105,24 @@ public function testCDJTypeMismatchIsError(): void $response->verify($this->cm, $this->rp, $this->credential); } + // 7.2.12 + public function testUsedChallengeIsError(): void + { + $container = new CredentialContainer([$this->credential]); + + $response = new GetResponse( + credentialId: $this->id, + rawAuthenticatorData: $this->rawAuthenticatorData, + clientDataJson: $this->clientDataJson, + signature: $this->signature, + ); + + $credential = $response->verify($this->cm, $this->rp, $container); + + $this->expectVerificationError('7.2.12'); + $response->verify($this->cm, $this->rp, $container); + } + // 7.2.12 public function testCDJChallengeMismatchIsError(): void { From a0f3fffde644e2438c80e22c1a85ee8efd63dfeb Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 13:54:20 -0700 Subject: [PATCH 40/47] adjust parsing --- src/GetResponse.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/GetResponse.php b/src/GetResponse.php index 1bb0521..635f2d1 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -15,12 +15,15 @@ */ class GetResponse implements Responses\AssertionInterface { + private AuthenticatorData $authData; + public function __construct( private BinaryString $credentialId, private BinaryString $rawAuthenticatorData, private BinaryString $clientDataJson, private BinaryString $signature, ) { + $this->authData = AuthenticatorData::parse($this->rawAuthenticatorData); } /** @@ -71,7 +74,7 @@ public function verify( // 7.2.8 $cData = $this->clientDataJson->unwrap(); - $authData = AuthenticatorData::parse($this->rawAuthenticatorData); + $authData = $this->authData; $sig = $this->signature->unwrap(); // 7.2.9 From 411b387c4732ba87865aed85a6dc5f143331f9f2 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 13:57:51 -0700 Subject: [PATCH 41/47] remove imports no longer needed --- examples/readmeLoginStep1.php | 1 - examples/readmeLoginStep3.php | 1 - examples/readmeRegisterStep1.php | 1 - examples/readmeRegisterStep3.php | 1 - 4 files changed, 4 deletions(-) diff --git a/examples/readmeLoginStep1.php b/examples/readmeLoginStep1.php index 8d7927d..14dfa5d 100644 --- a/examples/readmeLoginStep1.php +++ b/examples/readmeLoginStep1.php @@ -5,7 +5,6 @@ use Firehed\WebAuthn\{ Codecs, ExpiringChallenge, - SessionChallengeManager, }; session_start(); diff --git a/examples/readmeLoginStep3.php b/examples/readmeLoginStep3.php index 4260b58..0ad7b75 100644 --- a/examples/readmeLoginStep3.php +++ b/examples/readmeLoginStep3.php @@ -5,7 +5,6 @@ use Firehed\WebAuthn\{ Codecs, ResponseParser, - SessionChallengeManager, }; session_start(); diff --git a/examples/readmeRegisterStep1.php b/examples/readmeRegisterStep1.php index aa5bcb7..94fcf21 100644 --- a/examples/readmeRegisterStep1.php +++ b/examples/readmeRegisterStep1.php @@ -3,7 +3,6 @@ require __DIR__ . '/vendor/autoload.php'; use Firehed\WebAuthn\ExpiringChallenge; -use Firehed\WebAuthn\SessionChallengeManager; session_start(); diff --git a/examples/readmeRegisterStep3.php b/examples/readmeRegisterStep3.php index 35d9942..b60aa04 100644 --- a/examples/readmeRegisterStep3.php +++ b/examples/readmeRegisterStep3.php @@ -5,7 +5,6 @@ use Firehed\WebAuthn\{ Codecs, ResponseParser, - SessionChallengeManager, }; session_start(); From fa1abf8ba30e64851ea8d8a8f2e6ad186b6c5a81 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 13:59:19 -0700 Subject: [PATCH 42/47] adjust create side --- tests/CreateResponseTest.php | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/tests/CreateResponseTest.php b/tests/CreateResponseTest.php index 957f580..6883e30 100644 --- a/tests/CreateResponseTest.php +++ b/tests/CreateResponseTest.php @@ -11,6 +11,7 @@ class CreateResponseTest extends \PHPUnit\Framework\TestCase { // These hold the values which would be kept server-side. private RelyingParty $rp; + private ChallengeManagerInterface $cm; // These hold the _default_ values from a sample parsed response. private BinaryString $id; @@ -163,6 +164,13 @@ public function setUp(): void 97, 108, 104, 111, 115, 116, 58, 56, 56, 56, 56, 34, 125, ]); + + $this->cm = new FixedChallengeManager(new Challenge(BinaryString::fromBytes([ + 40, 96, 197, 186, 42, 202, 51, 237, + 134, 178, 3, 251, 22, 204, 231, 157, + 47, 77, 43, 123, 3, 245, 57, 77, + 20, 74, 166, 166, 240, 37, 141, 188, + ]))); } // 7.1.7 @@ -180,7 +188,7 @@ public function testCDJTypeMismatchIsError(): void ); $this->expectRegistrationError('7.1.7'); - $response->verify($this->getChallengeManager(), $this->rp); + $response->verify($this->cm, $this->rp); } // 7.1.8 @@ -198,7 +206,7 @@ public function testCDJChallengeMismatchIsError(): void ); $this->expectRegistrationError('7.1.8'); - $response->verify($this->getChallengeManager(), $this->rp); + $response->verify($this->cm, $this->rp); } // 7.1.9 @@ -216,7 +224,7 @@ public function testCDJOriginMismatchIsError(): void ); $this->expectRegistrationError('7.1.0'); - $response->verify($this->getChallengeManager(), $this->rp); + $response->verify($this->cm, $this->rp); } // 7.1.13 @@ -230,7 +238,7 @@ public function testRelyingPartyIdMismatchIsError(): void ); $this->expectRegistrationError('7.1.13'); - $response->verify($this->getChallengeManager(), $rp); + $response->verify($this->cm, $rp); } // 7.1.14 @@ -250,7 +258,7 @@ public function testUserVerifiedNotPresentWhenRequiredIsError(): void ); $this->expectRegistrationError('7.1.15'); - $response->verify($this->getChallengeManager(), $this->rp, UserVerificationRequirement::Required); + $response->verify($this->cm, $this->rp, UserVerificationRequirement::Required); } // 7.1.16 @@ -290,7 +298,7 @@ public function testFormatSpecificVerificationOccurs(): void ao: $ao, clientDataJson: $this->clientDataJson, ); - $response->verify($this->getChallengeManager(), $this->rp); + $response->verify($this->cm, $this->rp); } public function testSuccess(): void @@ -301,25 +309,12 @@ public function testSuccess(): void clientDataJson: $this->clientDataJson, ); - $cred = $response->verify($this->getChallengeManager(), $this->rp); + $cred = $response->verify($this->cm, $this->rp); self::assertSame(0, $cred->getSignCount()); // Look for a specific id and public key? } - protected function getChallengeManager(): ChallengeManagerInterface - { - $mock = self::createMock(ChallengeManagerInterface::class); - $mock->method('useFromClientDataJSON') - ->willReturn(new Challenge(BinaryString::fromBytes([ - 40, 96, 197, 186, 42, 202, 51, 237, - 134, 178, 3, 251, 22, 204, 231, 157, - 47, 77, 43, 123, 3, 245, 57, 77, - 20, 74, 166, 166, 240, 37, 141, 188, - ]))); - return $mock; - } - private function expectRegistrationError(string $section): void { $this->expectException(Errors\RegistrationError::class); From 09f40d652d1a6993e02e66f8c8eb0ab17095b485 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 14:04:10 -0700 Subject: [PATCH 43/47] test/notes --- tests/CreateResponseTest.php | 15 +++++++++++++++ tests/GetResponseTest.php | 1 + 2 files changed, 16 insertions(+) diff --git a/tests/CreateResponseTest.php b/tests/CreateResponseTest.php index 6883e30..c727671 100644 --- a/tests/CreateResponseTest.php +++ b/tests/CreateResponseTest.php @@ -191,6 +191,21 @@ public function testCDJTypeMismatchIsError(): void $response->verify($this->cm, $this->rp); } + public function testUsedChallengeIsError(): void + { + $response = new CreateResponse( + id: $this->id, + ao: $this->attestationObject, + clientDataJson: $this->clientDataJson, + ); + + $cred = $response->verify($this->cm, $this->rp); + + // Simulate replay. ChallengeManager no longer recognizes this one. + $this->expectRegistrationError('7.1.8'); + $response->verify($this->cm, $this->rp); + } + // 7.1.8 public function testCDJChallengeMismatchIsError(): void { diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php index 301f4e8..74d66ad 100644 --- a/tests/GetResponseTest.php +++ b/tests/GetResponseTest.php @@ -138,6 +138,7 @@ public function testCDJChallengeMismatchIsError(): void signature: $this->signature, ); + // Simulate replay. ChallengeManager no longer recognizes this one. $this->expectVerificationError('7.2.12'); $response->verify($this->cm, $this->rp, $this->credential); } From aa74b26ad46b946d765b8bb7ab2989ddab9f1393 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 14:07:58 -0700 Subject: [PATCH 44/47] make internals of test helper clearer and more precise --- tests/FixedChallengeManager.php | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/FixedChallengeManager.php b/tests/FixedChallengeManager.php index 8d14b03..a91a982 100644 --- a/tests/FixedChallengeManager.php +++ b/tests/FixedChallengeManager.php @@ -4,23 +4,28 @@ namespace Firehed\WebAuthn; +use BadMethodCallException; + class FixedChallengeManager implements ChallengeManagerInterface { - private ?ChallengeInterface $challenge; - public function __construct(ChallengeInterface $challenge) + /** @var array */ + private array $seen = []; + + public function __construct(private ChallengeInterface $challenge) { - $this->challenge = $challenge; } + public function createChallenge(): ChallengeInterface { - assert($this->challenge !== null); - return $this->challenge; + throw new BadMethodCallException('Should not be used during testing'); } + public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface { - $challenge = $this->challenge; - // "consume" it after the first use - $this->challenge = null; - return $challenge; + if (array_key_exists($base64Url, $this->seen)) { + return null; + } + $this->seen[$base64Url] = true; + return $this->challenge; } } From afa59d500a30c9504036d5027b65d9127baa73f1 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 14:12:55 -0700 Subject: [PATCH 45/47] explainer --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 44a4eed..79dcad4 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,7 @@ $rp = new \Firehed\WebAuthn\RelyingParty('https://www.example.com'); ``` Also create a `ChallengeManagerInterface`. +This will store and validate the one-time use challenges that are central to the WebAuthn protocol. See the [Challenge Management](#challenge-management) section below for more information. ```php From a18249bd47e504bdb363697e8f27931a69958efc Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 14:16:41 -0700 Subject: [PATCH 46/47] further clarify required behavior for challenge manager --- src/ChallengeManagerInterface.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ChallengeManagerInterface.php b/src/ChallengeManagerInterface.php index e88298f..f6c77cf 100644 --- a/src/ChallengeManagerInterface.php +++ b/src/ChallengeManagerInterface.php @@ -23,6 +23,10 @@ public function createChallenge(): ChallengeInterface; * returned a value or null. Failure to do so will compromise the security * of the webauthn protocol. * + * Implementations MUST NOT use the ClientDataJSON value to construct + * a challenge. They MUST return a previously-stored value if one is found, + * and MAY use $base64Url to search the storage mechanism. + * * @internal */ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface; From 75a59865869d4a254f5f5f10de40932f374f8f1d Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 14:22:49 -0700 Subject: [PATCH 47/47] test new requirement --- tests/ChallengeManagerTestTrait.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/ChallengeManagerTestTrait.php b/tests/ChallengeManagerTestTrait.php index 5212c46..9cbcafc 100644 --- a/tests/ChallengeManagerTestTrait.php +++ b/tests/ChallengeManagerTestTrait.php @@ -52,4 +52,19 @@ public function testNoChallengeIsReturnedIfManagerIsEmpty(): void self::assertNull($found); } + + public function testRetrievalDoesNotCreateChallengeFromUserData(): void + { + $cm = $this->getChallengeManager(); + $c = $cm->createChallenge(); + + $userChallenge = Challenge::random(); + $cdjValue = Codecs\Base64Url::encode($userChallenge->getBinary()->unwrap()); + + $retrieved = $cm->useFromClientDataJSON($cdjValue); + // The implmentation may return the previously-stored value or null, + // but MUST NOT attempt to reconstruct the challenge from the user- + // provided value. + self::assertNotSame($userChallenge->getBase64(), $retrieved?->getBase64()); + } }