Improve support for more complex challenge management

README.md

@@ -69,8 +69,9 @@ Send it to the user as base64.
 ```php
 <?php
 
-// Generate challenge
-$challenge = $challengeManager->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.

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');

src/ChallengeLoaderInterface.php

@@ -0,0 +1,37 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Firehed\WebAuthn;
+
+/**
+ * In most cases, you will want to interact with ChallengeManagerInterface,
+ * which extends this. That will let you generate challenges, manage (store)
+ * them, and subsequently verify them if they're found in storage. This is
+ * intended as the primary data flow, and is the recommended path.
+ *
+ * In rare circumstances, you may need to verify externally-managed challenges.
+ * If so, the loading component may opt to only implement this interface. Doing
+ * so is NOT RECOMMENDED at this time.
+ *
+ * @api (with the above caveats)
+ */
+interface ChallengeLoaderInterface
+{
+    /**
+     * 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;
+}

src/ChallengeLoaderTestTrait.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Firehed\WebAuthn;
+
+/**
+ * Trait for adding PHPUnit test cases to both packaged and custom
+ * ChallengeLoaderInterface implementations.
+ *
+ * @api
+ */
+trait ChallengeLoaderTestTrait
+{
+    public function testChallengeCannotBeRetrievedTwice(): void
+    {
+        $c = Challenge::random();
+        $cl = $this->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;
+}

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;
 }

tests/ChallengeManagerTestTrait.php → 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();
+    }
 }

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');
         }

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');
         }

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,

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,

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;
     }
 }

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');
     }

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
         );
 

tests/TestUtilities/TestVectorChallengeManager.php → 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) {

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