From 2c7ae778c718cd3277ad3f01c73fb2fab63e9816 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 28 Aug 2023 09:13:28 -0700 Subject: [PATCH 01/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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 bacb875836ae7133de25b97b809084eb4fdb2826 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 14:29:20 -0700 Subject: [PATCH 28/30] remove now-useless tests --- ...CreateResponseWithChallengeManagerTest.php | 46 ------------------- tests/GetResponseWithChallengeManagerTest.php | 46 ------------------- 2 files changed, 92 deletions(-) delete mode 100644 tests/CreateResponseWithChallengeManagerTest.php delete mode 100644 tests/GetResponseWithChallengeManagerTest.php 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/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 83170d6f20589f1e9799c2b956b0b31c8107e304 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 16:21:09 -0700 Subject: [PATCH 29/30] Do a bunch of extra work to de-risk race conditions since psr16 is inherently non-atomic --- src/CacheChallengeManager.php | 85 +++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index e0fcc00..6656135 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -5,8 +5,14 @@ namespace Firehed\WebAuthn; use Psr\SimpleCache\CacheInterface; +use RuntimeException; use UnexpectedValueException; +use function assert; +use function bin2hex; +use function hash_equals; +use function is_string; +use function random_bytes; use function sprintf; class CacheChallengeManager implements ChallengeManagerInterface @@ -22,32 +28,75 @@ 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, - Codecs\Base64Url::encode($c->getBinary()->unwrap()), - ); - $this->cache->set($cacheKey, $c, 120); + $key = $this->getKey(Codecs\Base64Url::encode($c->getBinary()->unwrap())); + $this->cache->set($key, $c, 120); return $c; } public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface { - $key = sprintf( + $key = $this->getKey($base64Url); + + // PSR-16 (through the shared definition in PSR-6) designates that + // cache item deletion "MUST NOT be considered an error condition if the + // specified key does not exist". Consequently, there's no way within + // that interface to know if deletion was a no-op or actually removed + // an item. + // + // Since this is used to managed cryptographic nonces and a race + // condition could be exploited, this implementation does some + // additional work (at the expense of some extra round-trips) to block + // race conditions. + // + // First, generate a random value to store in the cache before doing + // anything else. This value will be checked later. + + $raceConditionBlocker = bin2hex(random_bytes(10)); + $raceConditionBlockerKey = $key . '-rcb'; + $this->cache->set($raceConditionBlockerKey, $raceConditionBlocker, 120); + + // Retrieve the original value from the cache that would have been + // stored during createChallenge(). + $challenge = $this->cache->get($key); + + // Remove it from the cache, as it is one-time-use. Always do this, + // even if $challege above is null or invalid: this reduces the + // possibility of other timing attacks. + $deleteResult = $this->cache->delete($key); + + // Finally, read out the value stored above. If a race condition + // occurred and another process or request overwrote the value with + // a different random value, this will be different from the generated + // value above. Look for this and throw an exception if detected. + $raceConditionCheck = $this->cache->get($raceConditionBlockerKey, ''); + assert(is_string($raceConditionCheck)); + if (!hash_equals($raceConditionBlocker, $raceConditionCheck)) { + throw new RuntimeException('Another process or request has used this challenge.'); + } + + // If unable to delete the challenge, abort. This is additional + // insurance to block challenge reuse. + if ($deleteResult === false) { + throw new RuntimeException('Could not remove challenge from pool'); + } + + if ($challenge instanceof ChallengeInterface) { + // Found, happy path + return $challenge; + } elseif ($challenge === null) { + // Not found, either expired or potentially malicious. + return null; + } + // Something interfered with the cache contents. + throw new UnexpectedValueException('Non-challenge found in cache'); + } + + private function getKey(string $base64Url): string + { + return 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'); } } From 849b576d8f23040176c1c05bdea28213782a82d1 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Thu, 2 Nov 2023 16:24:00 -0700 Subject: [PATCH 30/30] add warning --- src/CacheChallengeManager.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/CacheChallengeManager.php b/src/CacheChallengeManager.php index 6656135..1c51b16 100644 --- a/src/CacheChallengeManager.php +++ b/src/CacheChallengeManager.php @@ -15,6 +15,14 @@ use function random_bytes; use function sprintf; +/** + * Uses a PSR-16 cache implementation to manage challenges. + * + * IMPORTANT: The provided cache MUST be a shared pool across the service, + * rather than a server- or process-local version (such as APCu). Providing + * a local cache is highly likely to result in undesired runtime behavior and + * can pose a security risk. + */ class CacheChallengeManager implements ChallengeManagerInterface { public function __construct(