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. */