Have the library handle challenge management (#35)

Firehed · web-flow · commit 14bb7739776a · 2023-11-02T14:25:48.000-07:00
The examples so far have all used sessions to manage the active challenges, but not all applications are stateful in this way - namely, most APIs will not be session-based. Instead, this creates a new `ChallengeManagerInterface` that handles this for applications. For now there's a single implementation that's still session-based, though (via #30 which I'm reworking) other implementations will be provided (e.g. a cache pool). The majority of the change here is updating examples and adding tests. Note that this would be a BC break but since the library is still pre-1.0 it's not a concern for practical purposes.
diff --git a/README.md b/README.md
@@ -39,6 +39,15 @@ The protocol is always required; the port must only be present if using a non-st
 $rp = new \Firehed\WebAuthn\RelyingParty('https://www.example.com');
 ```
 
+Also create a `ChallengeManagerInterface`.
+This will store and validate the one-time use challenges that are central to the WebAuthn protocol.
+See the [Challenge Management](#challenge-management) section below for more information.
+
+```php
+session_start();
+$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`.
@@ -49,20 +58,13 @@ $rp = new \Firehed\WebAuthn\RelyingParty('https://www.example.com');
 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
 <?php
 
-use Firehed\WebAuthn\ExpiringChallenge;
-
 // Generate challenge
-$challenge = ExpiringChallenge::withLifetime(120);
-
-// Store server-side; adjust to your app's needs
-session_start();
-$_SESSION['webauthn_challenge'] = $challenge;
+$challenge = $challengeManager->createChallenge();
 
 // Send to user
 header('Content-type: application/json');
@@ -154,11 +156,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');
@@ -205,10 +205,7 @@ This assumes the same schema from the previous Registration example.
 ```php
 <?php
 
-use Firehed\WebAuthn\{
-    Codecs,
-    ExpiringChallenge,
-};
+use Firehed\WebAuthn\Codecs;
 
 session_start();
 
@@ -223,8 +220,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');
@@ -310,13 +306,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');
@@ -440,6 +434,26 @@ 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.
+
+| Implementation | Usage |
+| --- | --- |
+| `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.
+> **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.
 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.
diff --git a/composer-require-checker.json b/composer-require-checker.json
@@ -0,0 +1,6 @@
+{
+    "symbol-whitelist": [
+        "PHP_SESSION_ACTIVE",
+        "session_status"
+    ]
+}
diff --git a/examples/functions.php b/examples/functions.php
@@ -3,9 +3,11 @@
 declare(strict_types=1);
 
 use Firehed\WebAuthn\{
+    ChallengeManagerInterface,
     Codecs,
     CredentialContainer,
     RelyingParty,
+    SessionChallengeManager,
 };
 
 /**
@@ -28,6 +30,11 @@ function createUser(PDO $pdo, string $username): array
     return $response;
 }
 
+function getChallengeManager(): ChallengeManagerInterface
+{
+    return new SessionChallengeManager();
+}
+
 function getCredentialsForUserId(PDO $pdo, string $userId): CredentialContainer
 {
     $stmt = $pdo->prepare('SELECT * FROM user_credentials WHERE user_id = ?');
diff --git a/examples/readmeLoginStep1.php b/examples/readmeLoginStep1.php
@@ -19,8 +19,8 @@
 
 $credentialContainer = getCredentialsForUserId($pdo, $user['id']);
 
-$challenge = ExpiringChallenge::withLifetime(120);
-$_SESSION['webauthn_challenge'] = $challenge;
+$challengeManager = getChallengeManager();
+$challenge = $challengeManager->createChallenge();
 
 // Send to user
 header('Content-type: application/json');
diff --git a/examples/readmeLoginStep3.php b/examples/readmeLoginStep3.php
@@ -20,12 +20,12 @@
 $getResponse = $parser->parseGetResponse($data);
 
 $rp = getRelyingParty();
-$challenge = $_SESSION['webauthn_challenge'];
 
 $credentialContainer = getCredentialsForUserId($pdo, $_SESSION['authenticating_user_id']);
+$challengeManager = getChallengeManager();
 
 try {
-    $updatedCredential = $getResponse->verify($challenge, $rp, $credentialContainer);
+    $updatedCredential = $getResponse->verify($challengeManager, $rp, $credentialContainer);
 } catch (Throwable) {
     // Verification failed. Send an error to the user?
     header('HTTP/1.1 403 Unauthorized');
diff --git a/examples/readmeRegisterStep1.php b/examples/readmeRegisterStep1.php
@@ -13,10 +13,8 @@
 $_SESSION['user_id'] = $user['id'];
 
 // Generate challenge
-$challenge = ExpiringChallenge::withLifetime(120);
-
-// Store server-side; adjust to your app's needs
-$_SESSION['webauthn_challenge'] = $challenge;
+$challengeManager = getChallengeManager();
+$challenge = $challengeManager->createChallenge();
 
 // Send to user
 header('Content-type: application/json');
diff --git a/examples/readmeRegisterStep3.php b/examples/readmeRegisterStep3.php
@@ -18,10 +18,10 @@
 $createResponse = $parser->parseCreateResponse($data);
 
 $rp = getRelyingParty();
-$challenge = $_SESSION['webauthn_challenge'];
+$challengeManager = getChallengeManager();
 
 try {
-    $credential = $createResponse->verify($challenge, $rp);
+    $credential = $createResponse->verify($challengeManager, $rp);
 } catch (Throwable) {
     // Verification failed. Send an error to the user?
     header('HTTP/1.1 403 Unauthorized');
diff --git a/phpunit.xml b/phpunit.xml
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
-         bootstrap="vendor/autoload.php"
+         bootstrap="tests/bootstrap.php"
          executionOrder="depends,defects"
          forceCoversAnnotation="true"
          beStrictAboutCoversAnnotation="false"
diff --git a/src/ChallengeManagerInterface.php b/src/ChallengeManagerInterface.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Firehed\WebAuthn;
+
+interface ChallengeManagerInterface
+{
+    /**
+     * Generates a new Challenge, stores it in the backing mechanism, and
+     * returns it.
+     *
+     * @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;
+}
diff --git 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,
+        ChallengeManagerInterface $challenge,
         RelyingParty $rp,
         UserVerificationRequirement $uv = UserVerificationRequirement::Preferred,
     ): CredentialInterface {
@@ -47,8 +47,14 @@ public function verify(
         }
 
         // 7.1.8
+        $cdjChallenge = $C['challenge'];
+        $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
@@ -15,12 +15,15 @@
  */
 class GetResponse implements Responses\AssertionInterface
 {
+    private AuthenticatorData $authData;
+
     public function __construct(
         private BinaryString $credentialId,
         private BinaryString $rawAuthenticatorData,
         private BinaryString $clientDataJson,
         private BinaryString $signature,
     ) {
+        $this->authData = AuthenticatorData::parse($this->rawAuthenticatorData);
     }
 
     /**
@@ -36,7 +39,7 @@ public function getUsedCredentialId(): BinaryString
      * @link https://www.w3.org/TR/webauthn-2/#sctn-verifying-assertion
      */
     public function verify(
-        ChallengeInterface $challenge,
+        ChallengeManagerInterface $challenge,
         RelyingParty $rp,
         CredentialContainer | CredentialInterface $credential,
         UserVerificationRequirement $uv = UserVerificationRequirement::Preferred,
@@ -71,7 +74,7 @@ public function verify(
 
         // 7.2.8
         $cData = $this->clientDataJson->unwrap();
-        $authData = AuthenticatorData::parse($this->rawAuthenticatorData);
+        $authData = $this->authData;
         $sig = $this->signature->unwrap();
 
         // 7.2.9
@@ -89,8 +92,14 @@ public function verify(
         }
 
         // 7.2.12
+        $cdjChallenge = $C['challenge'];
+        $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');
         }
 
diff --git 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,
+        ChallengeManagerInterface $challenge,
         RelyingParty $rp,
         CredentialContainer | CredentialInterface $credential,
         UserVerificationRequirement $uv = UserVerificationRequirement::Preferred,
diff --git 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,
+        ChallengeManagerInterface $challenge,
         RelyingParty $rp,
         UserVerificationRequirement $uv = UserVerificationRequirement::Preferred,
     ): CredentialInterface;
diff --git a/src/SessionChallengeManager.php b/src/SessionChallengeManager.php
@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Firehed\WebAuthn;
+
+use BadMethodCallException;
+
+use function array_key_exists;
+use function session_status;
+
+use const PHP_SESSION_ACTIVE;
+
+class SessionChallengeManager implements ChallengeManagerInterface
+{
+    private const SESSION_KEY = 'passkey_challenge';
+
+    public function __construct()
+    {
+        // Do this later?
+        if (session_status() !== PHP_SESSION_ACTIVE) {
+            throw new BadMethodCallException('No active session. Call session_start() before using this.');
+        }
+    }
+
+    public function createChallenge(): ChallengeInterface
+    {
+        $c = ExpiringChallenge::withLifetime(120);
+        $_SESSION[self::SESSION_KEY] = $c;
+        return $c;
+    }
+
+    public function useFromClientDataJSON(string $base64Url): ?ChallengeInterface
+    {
+        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;
+    }
+}
diff --git a/tests/ChallengeManagerTestTrait.php b/tests/ChallengeManagerTestTrait.php
diff --git a/tests/CreateResponseTest.php b/tests/CreateResponseTest.php
diff --git a/tests/EndToEndTest.php b/tests/EndToEndTest.php
diff --git a/tests/FixedChallengeManager.php b/tests/FixedChallengeManager.php
diff --git a/tests/GetResponseTest.php b/tests/GetResponseTest.php
diff --git a/tests/SessionChallengeManagerTest.php b/tests/SessionChallengeManagerTest.php
diff --git a/tests/bootstrap.php b/tests/bootstrap.php
