From d77e73e151b80cd4725042c7882888e89b15dd57 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Fri, 1 Mar 2024 18:19:47 -0800 Subject: [PATCH] Improve support for more complex challenge management (#75) Splits the retrieval and management of challenges. From a good vibes perspective, this a) aligns the main interface name with the functionality it's intended to provide, and b) improves the interface segregation (hi, SOLID). In terms of practical needs, this helps solve the (fairly rare) need of challenges that are managed as part of a broader external system. This could be a data model, microservice, or whatever. By splitting the interfaces and only requiring that the newer `ChallengeLoaderInterface` make it to the verification processes, this becomes both easier and more testable. --- README.md | 10 +++-- examples/getChallenge.php | 5 ++- src/ChallengeLoaderInterface.php | 37 +++++++++++++++++ src/ChallengeLoaderTestTrait.php | 41 +++++++++++++++++++ src/ChallengeManagerInterface.php | 27 +++--------- {tests => src}/ChallengeManagerTestTrait.php | 37 ++++++++++++----- src/CreateResponse.php | 4 +- src/GetResponse.php | 4 +- src/Responses/AssertionInterface.php | 4 +- src/Responses/AttestationInterface.php | 4 +- src/SessionChallengeManager.php | 11 ++--- tests/FixedChallengeManager.php | 2 +- tests/IntegrationTest.php | 2 +- ...ager.php => TestVectorChallengeLoader.php} | 9 +--- .../TestVectorFixedChallenge.php | 2 +- 15 files changed, 140 insertions(+), 59 deletions(-) create mode 100644 src/ChallengeLoaderInterface.php create mode 100644 src/ChallengeLoaderTestTrait.php rename {tests => src}/ChallengeManagerTestTrait.php (69%) rename tests/TestUtilities/{TestVectorChallengeManager.php => TestVectorChallengeLoader.php} (67%) diff --git a/README.md b/README.md index 56c527c..a662259 100644 --- a/README.md +++ b/README.md @@ -69,8 +69,9 @@ Send it to the user as base64. ```php createChallenge(); +// Generate and manage challenge +$challenge = \Firehed\WebAuthn\ExpiringChallenge::withLifetime(300); +$challengeManager->manageChallenge($challenge); // Send to user header('Content-type: application/json'); @@ -223,7 +224,9 @@ $_SESSION['authenticating_user_id'] = $user['id']; // See examples/functions.php for how this works $credentialContainer = getCredentialsForUserId($pdo, $user['id']); -$challenge = $challengeManager->createChallenge(); +// Generate and manage challenge +$challenge = \Firehed\WebAuthn\ExpiringChallenge::withLifetime(300); +$challengeManager->manageChallenge($challenge); // Send to user header('Content-type: application/json'); @@ -595,6 +598,7 @@ In the event you find this necessary, you SHOULD open an Issue and/or Pull Reque 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. +W3C recommends timeouts between 5 and 10 minutes. > [!NOTE] > The W3C specification recommends a timeout in the range of 15-120 seconds. diff --git a/examples/getChallenge.php b/examples/getChallenge.php index 357bc14..3d1ef21 100644 --- a/examples/getChallenge.php +++ b/examples/getChallenge.php @@ -2,11 +2,14 @@ require __DIR__ . '/vendor/autoload.php'; +use Firehed\WebAuthn\ExpiringChallenge; + session_start(); // Generate challenge $challengeManager = getChallengeManager(); -$challenge = $challengeManager->createChallenge(); +$challenge = ExpiringChallenge::withLifetime(300); +$challengeManager->manageChallenge($challenge); // Send to user header('Content-type: application/json'); diff --git a/src/ChallengeLoaderInterface.php b/src/ChallengeLoaderInterface.php new file mode 100644 index 0000000..fbb5662 --- /dev/null +++ b/src/ChallengeLoaderInterface.php @@ -0,0 +1,37 @@ +getChallengeLoaderManagingChallenge($c); + + $result = $cl->useFromClientDataJSON($c->getBase64Url()); + $result2 = $cl->useFromClientDataJSON($c->getBase64Url()); + + self::assertNotNull($result); + self::assertSame($c->getBase64Url(), $result->getBase64Url()); + self::assertNull($result2); + } + + public function testManagerDoesNotReturnUnmanagedChallenge(): void + { + $c = Challenge::random(); + $cl = $this->getChallengeLoaderManagingChallenge($c); + + $c2 = Challenge::random(); + assert($c->getBase64Url() !== $c2->getBase64Url()); + self::assertNull($cl->useFromClientDataJSON($c2->getBase64Url())); + } + + abstract protected function getChallengeLoaderManagingChallenge( + ChallengeInterface $challenge, + ): ChallengeLoaderInterface; +} diff --git a/src/ChallengeManagerInterface.php b/src/ChallengeManagerInterface.php index f6c77cf..f214c34 100644 --- a/src/ChallengeManagerInterface.php +++ b/src/ChallengeManagerInterface.php @@ -4,30 +4,15 @@ namespace Firehed\WebAuthn; -interface ChallengeManagerInterface +/** + * @api + */ +interface ChallengeManagerInterface extends ChallengeLoaderInterface { /** - * Generates a new Challenge, stores it in the backing mechanism, and - * returns it. + * Takes the provided challenge and stores it in the backing mechanism. * * @api */ - public function createChallenge(): ChallengeInterface; - - /** - * Consumes the challenge associated with the ClientDataJSON value from the - * underlying storage mechanism, and returns that challenge if found. - * - * Implementations MUST ensure that subsequent calls to this method with - * the same value return `null`, regardless of whether the initial call - * 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; + public function manageChallenge(ChallengeInterface $challenge): void; } diff --git a/tests/ChallengeManagerTestTrait.php b/src/ChallengeManagerTestTrait.php similarity index 69% rename from tests/ChallengeManagerTestTrait.php rename to src/ChallengeManagerTestTrait.php index b84474e..b6a1302 100644 --- a/tests/ChallengeManagerTestTrait.php +++ b/src/ChallengeManagerTestTrait.php @@ -4,23 +4,30 @@ namespace Firehed\WebAuthn; +/** + * Trait for adding PHPUnit test cases to both packaged and custom + * ChallengeManagerInterface implementations. + * + * @api + */ trait ChallengeManagerTestTrait { - abstract protected function getChallengeManager(): ChallengeManagerInterface; + use ChallengeLoaderTestTrait; - public function testCreateChallengeIsUnique(): void + protected function getChallengeLoaderManagingChallenge(ChallengeInterface $challenge): ChallengeLoaderInterface { $cm = $this->getChallengeManager(); - $c1 = $cm->createChallenge(); - $c2 = $cm->createChallenge(); - self::assertNotSame($c1, $c2); - self::assertNotSame($c1->getBase64(), $c2->getBase64()); + $cm->manageChallenge($challenge); + return $cm; } + abstract protected function getChallengeManager(): ChallengeManagerInterface; + public function testMostRecentChallengeCanBeRetrieved(): void { $cm = $this->getChallengeManager(); - $c = $cm->createChallenge(); + $c = $this->createChallenge(); + $cm->manageChallenge($c); $cdjValue = $c->getBinary()->toBase64Url(); $found = $cm->useFromClientDataJSON($cdjValue); @@ -31,7 +38,8 @@ public function testMostRecentChallengeCanBeRetrieved(): void public function testMostRecentChallengeCanBeRetrievedOnlyOnce(): void { $cm = $this->getChallengeManager(); - $c = $cm->createChallenge(); + $c = $this->createChallenge(); + $cm->manageChallenge($c); $cdjValue = $c->getBinary()->toBase64Url(); $found = $cm->useFromClientDataJSON($cdjValue); @@ -45,7 +53,8 @@ public function testNoChallengeIsReturnedIfManagerIsEmpty(): void { $cm = $this->getChallengeManager(); - $c = Challenge::random(); + $c = $this->createChallenge(); + // Do NOT manage it $cdjValue = $c->getBinary()->toBase64Url(); $found = $cm->useFromClientDataJSON($cdjValue); @@ -56,9 +65,10 @@ public function testNoChallengeIsReturnedIfManagerIsEmpty(): void public function testRetrievalDoesNotCreateChallengeFromUserData(): void { $cm = $this->getChallengeManager(); - $c = $cm->createChallenge(); + $c = $this->createChallenge(); + $cm->manageChallenge($c); - $userChallenge = Challenge::random(); + $userChallenge = $this->createChallenge(); $cdjValue = $userChallenge->getBinary()->toBase64Url(); $retrieved = $cm->useFromClientDataJSON($cdjValue); @@ -67,4 +77,9 @@ public function testRetrievalDoesNotCreateChallengeFromUserData(): void // provided value. self::assertNotSame($userChallenge->getBase64(), $retrieved?->getBase64()); } + + private function createChallenge(): ChallengeInterface + { + return Challenge::random(); + } } diff --git a/src/CreateResponse.php b/src/CreateResponse.php index 36c16dd..88dfe95 100644 --- a/src/CreateResponse.php +++ b/src/CreateResponse.php @@ -40,7 +40,7 @@ public function isUserVerified(): bool * @link https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential */ public function verify( - ChallengeManagerInterface $challenge, + ChallengeLoaderInterface $challengeLoader, RelyingPartyInterface $rp, Enums\UserVerificationRequirement $uv = Enums\UserVerificationRequirement::Preferred, bool $rejectUncertainTrustPaths = true, @@ -62,7 +62,7 @@ public function verify( // 7.1.8 $cdjChallenge = $C['challenge']; - $challenge = $challenge->useFromClientDataJSON($cdjChallenge); + $challenge = $challengeLoader->useFromClientDataJSON($cdjChallenge); if ($challenge === null) { $this->fail('7.1.8', 'C.challenge'); } diff --git a/src/GetResponse.php b/src/GetResponse.php index 4c6e960..2a9fabc 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -50,7 +50,7 @@ public function isUserVerified(): bool * @link https://www.w3.org/TR/webauthn-3/#sctn-verifying-assertion */ public function verify( - ChallengeManagerInterface $challenge, + ChallengeLoaderInterface $challengeLoader, RelyingPartyInterface $rp, CredentialContainer | CredentialInterface $credential, Enums\UserVerificationRequirement $uv = Enums\UserVerificationRequirement::Preferred, @@ -108,7 +108,7 @@ public function verify( // 7.2.13 $cdjChallenge = $C['challenge']; - $challenge = $challenge->useFromClientDataJSON($cdjChallenge); + $challenge = $challengeLoader->useFromClientDataJSON($cdjChallenge); if ($challenge === null) { $this->fail('7.2.12', 'C.challenge'); } diff --git a/src/Responses/AssertionInterface.php b/src/Responses/AssertionInterface.php index 20288ec..0b26fc3 100644 --- a/src/Responses/AssertionInterface.php +++ b/src/Responses/AssertionInterface.php @@ -7,7 +7,7 @@ use Firehed\WebAuthn\{ BinaryString, ChallengeInterface, - ChallengeManagerInterface, + ChallengeLoaderInterface, CredentialContainer, CredentialInterface, RelyingPartyInterface, @@ -57,7 +57,7 @@ public function isUserVerified(): bool; * @api */ public function verify( - ChallengeManagerInterface $challenge, + ChallengeLoaderInterface $challengeLoader, RelyingPartyInterface $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, diff --git a/src/Responses/AttestationInterface.php b/src/Responses/AttestationInterface.php index 09c8e20..0e032ce 100644 --- a/src/Responses/AttestationInterface.php +++ b/src/Responses/AttestationInterface.php @@ -6,7 +6,7 @@ use Firehed\WebAuthn\{ ChallengeInterface, - ChallengeManagerInterface, + ChallengeLoaderInterface, CredentialInterface, RelyingPartyInterface, Enums\UserVerificationRequirement, @@ -26,7 +26,7 @@ interface AttestationInterface public function isUserVerified(): bool; public function verify( - ChallengeManagerInterface $challenge, + ChallengeLoaderInterface $challengeLoader, RelyingPartyInterface $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, bool $rejectUncertainTrustPaths = true, diff --git a/src/SessionChallengeManager.php b/src/SessionChallengeManager.php index 89cff4b..b51cf59 100644 --- a/src/SessionChallengeManager.php +++ b/src/SessionChallengeManager.php @@ -23,11 +23,9 @@ public function __construct() } } - public function createChallenge(): ChallengeInterface + public function manageChallenge(ChallengeInterface $c): void { - $c = ExpiringChallenge::withLifetime(120); $_SESSION[self::SESSION_KEY] = $c; - return $c; } public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface @@ -37,7 +35,10 @@ public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface } $challenge = $_SESSION[self::SESSION_KEY]; unset($_SESSION[self::SESSION_KEY]); - // Validate that the stored challenge matches the CDJ value? - return $challenge; + // Validate that the stored challenge matches the CDJ value + if ($challenge->getBase64Url() === $base64Url) { + return $challenge; + } + return null; } } diff --git a/tests/FixedChallengeManager.php b/tests/FixedChallengeManager.php index a91a982..ba6d60f 100644 --- a/tests/FixedChallengeManager.php +++ b/tests/FixedChallengeManager.php @@ -15,7 +15,7 @@ public function __construct(private ChallengeInterface $challenge) { } - public function createChallenge(): ChallengeInterface + public function manageChallenge(ChallengeInterface $c): void { throw new BadMethodCallException('Should not be used during testing'); } diff --git a/tests/IntegrationTest.php b/tests/IntegrationTest.php index 6a0bfc7..faeea60 100644 --- a/tests/IntegrationTest.php +++ b/tests/IntegrationTest.php @@ -32,7 +32,7 @@ public function testReg(string $dir): void $rp = new SingleOriginRelyingParty($metadata['origin']); $createReq = self::read($dir, 'reg-req'); - $challengeManager = new TestUtilities\TestVectorChallengeManager( + $challengeManager = new TestUtilities\TestVectorChallengeLoader( $createReq['publicKey']['challenge'], // @phpstan-ignore-line ); diff --git a/tests/TestUtilities/TestVectorChallengeManager.php b/tests/TestUtilities/TestVectorChallengeLoader.php similarity index 67% rename from tests/TestUtilities/TestVectorChallengeManager.php rename to tests/TestUtilities/TestVectorChallengeLoader.php index 7103a59..0dc9a43 100644 --- a/tests/TestUtilities/TestVectorChallengeManager.php +++ b/tests/TestUtilities/TestVectorChallengeLoader.php @@ -8,20 +8,15 @@ use Firehed\WebAuthn\{ BinaryString, ChallengeInterface, - ChallengeManagerInterface, + ChallengeLoaderInterface, }; -class TestVectorChallengeManager implements ChallengeManagerInterface +class TestVectorChallengeLoader implements ChallengeLoaderInterface { public function __construct(private string $b64u) { } - public function createChallenge(): ChallengeInterface - { - throw new Exception('Not for use during testing'); - } - public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface { if ($this->b64u === $base64Url) { diff --git a/tests/TestUtilities/TestVectorFixedChallenge.php b/tests/TestUtilities/TestVectorFixedChallenge.php index a721515..5e16742 100644 --- a/tests/TestUtilities/TestVectorFixedChallenge.php +++ b/tests/TestUtilities/TestVectorFixedChallenge.php @@ -12,7 +12,7 @@ }; /** - * Like TestVectorChallengeManager, this would be quite dangerous to use unless + * Like TestVectorChallengeLoader, this would be quite dangerous to use unless * challenges are coming from a known source. DO NOT USE THIS as an example * implementation. */