New server registration validation flow (#30)

CHANGELOG.md

@@ -8,9 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 - ChallengeProviderInterface (will replace ChallengeProvider)
+- Server::validateRegistration(RegisterRequest, RegistrationResponseInterface) (will replace Server::setRegisterRequest + Server::register)
 
 ### Deprecated
 - ChallengeProvider
+- Server::register(RegistrationResponseInterface)
+- Server::setRegisterRequest(RegisterRequest)
 
 
 ## [1.2.0] - 2021-10-26

README.md

@@ -150,8 +150,8 @@ $rawPostBody = trim(file_get_contents('php:https://input'));
 $data = json_decode($rawPostBody, true);
 $response = \Firehed\U2F\WebAuthn\RegistrationResponse::fromDecodedJson($data);
 
-$server->setRegisterRequest($_SESSION['registration_request']);
-$registration = $server->register($response)
+$request = $_SESSION['registration_request'];
+$registration = $server->validateRegistration($request, $response);
 ```
 
 #### Persist the `$registration`

src/Server.php

@@ -38,6 +38,8 @@ class Server
  * Holds a RegisterRequest used by `register()`, which contains the
  * challenge in the signature.
  *
+ * @deprecated
+ *
  * @var ?RegisterRequest
  */
  private $registerRequest;
@@ -69,6 +71,7 @@ public function __construct()
  }
  // @codeCoverageIgnoreEnd
  }
+
  /**
  * This method authenticates a `LoginResponseInterface` against outstanding
  * registrations and their corresponding `SignRequest`s. If the response's
@@ -196,15 +199,11 @@ public function authenticate(LoginResponseInterface $response): RegistrationInte
  * @throws SE if the response cannot be proven authentic
  * @throws BadMethodCallException if a precondition is not met
  */
- public function register(RegistrationResponseInterface $response): RegistrationInterface
- {
- if (!$this->registerRequest) {
- throw new BadMethodCallException(
- 'Before calling register(), provide a RegisterRequest '.
- 'with setRegisterRequest()'
- );
- }
- $this->validateChallenge($this->registerRequest, $response);
+ public function validateRegistration(
+ RegisterRequest $request,
+ RegistrationResponseInterface $response
+ ): RegistrationInterface {
+ $this->validateChallenge($request, $response);
  // Check the Application Parameter
  $this->validateRelyingParty($response->getRpIdHash());
 
@@ -231,6 +230,30 @@ public function register(RegistrationResponseInterface $response): RegistrationI
  ->setPublicKey($response->getPublicKey());
  }
 
+ /**
+ * @deprecated This is being replaced with validateRegistration()
+ *
+ * This method authenticates a RegistrationResponseInterface against its
+ * corresponding RegisterRequest by verifying the certificate and signature.
+ * If valid, it returns a registration; if not, a SE will be thrown and
+ * attempt to register the key must be aborted.
+ *
+ * @param RegistrationResponseInterface $response The response to verify
+ * @return RegistrationInterface if the response is proven authentic
+ * @throws SE if the response cannot be proven authentic
+ * @throws BadMethodCallException if a precondition is not met
+ */
+ public function register(RegistrationResponseInterface $response): RegistrationInterface
+ {
+ if ($this->registerRequest === null) {
+ throw new BadMethodCallException(
+ 'Before calling register(), provide a RegisterRequest '.
+ 'with setRegisterRequest()'
+ );
+ }
+ return $this->validateRegistration($this->registerRequest, $response);
+ }
+
  /**
  * Disables verification of the Attestation Certificate against the list of
  * CA certificates. This lowers overall security, at the benefit of being
@@ -265,6 +288,8 @@ public function setTrustedCAs(array $CAs): self
  }
 
  /**
+ * @deprecated
+ *
  * Provide the previously-generated RegisterRequest to be used when
  * verifying a RegisterResponse during register()
  *

tests/ServerTest.php

@@ -114,6 +114,9 @@ public function testGenerateSignRequests(): void
  }
  }
 
+ /**
+ * @deprecated
+ */
  public function testSetRegisterRequestReturnsSelf(): void
  {
  $req = $this->getDefaultRegisterRequest();
@@ -168,14 +171,53 @@ public function testRegisterThrowsIfNoRegistrationRequestProvided(): void
  $this->server->register($this->getDefaultRegisterResponse());
  }
 
+ /**
+ * @deprecated
+ */
+ public function testLegacyRegistration(): void
+ {
+ $request = $this->getDefaultRegisterRequest();
+ $response = $this->getDefaultRegisterResponse();
+ $registration = $this->server->setRegisterRequest($request)
+ ->register($response);
+
+ $this->assertInstanceOf(
+ RegistrationInterface::class,
+ $registration,
+ 'Server->register did not return a registration'
+ );
+ $this->assertSame(
+ 0,
+ $registration->getCounter(),
+ 'Counter should start at 0'
+ );
+
+ $this->assertSame(
+ $response->getAttestationCertificate()->getBinary(),
+ $registration->getAttestationCertificate()->getBinary(),
+ 'Attestation cert was not copied from response'
+ );
+
+ $this->assertSame(
+ $response->getKeyHandleBinary(),
+ $registration->getKeyHandleBinary(),
+ 'Key handle was not copied from response'
+ );
+
+ $this->assertSame(
+ $response->getPublicKey()->getBinary(),
+ $registration->getPublicKey()->getBinary(),
+ 'Public key was not copied from response'
+ );
+ }
+
  public function testRegistration(): void
  {
  $request = $this->getDefaultRegisterRequest();
  $response = $this->getDefaultRegisterResponse();
 
  $registration = $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ ->validateRegistration($request, $response);
  $this->assertInstanceOf(
  RegistrationInterface::class,
  $registration,
@@ -217,9 +259,7 @@ public function testRegisterDefaultsToTryingEmptyCAList(): void
  // meaning that an exception should be thrown unless either a)
  // a matching CA is provided or b) verification is explicitly disabled
  $server = (new Server())->setAppId(self::APP_ID);
- $server
- ->setRegisterRequest($request)
- ->register($response);
+ $server->validateRegistration($request, $response);
  }
 
  public function testRegisterThrowsIfChallengeDoesNotMatch(): void
@@ -232,9 +272,7 @@ public function testRegisterThrowsIfChallengeDoesNotMatch(): void
 
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::CHALLENGE_MISMATCH);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  public function testRegisterThrowsWithUntrustedDeviceIssuerCertificate(): void
@@ -249,9 +287,7 @@ public function testRegisterThrowsWithUntrustedDeviceIssuerCertificate(): void
  ]);
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::NO_TRUSTED_CA);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  public function testRegisterWorksWithCAList(): void
@@ -267,9 +303,7 @@ public function testRegisterWorksWithCAList(): void
  $this->server->setTrustedCAs($CAs);
 
  try {
- $reg = $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $reg = $this->server->validateRegistration($request, $response);
  } catch (SecurityException $e) {
  if ($e->getCode() === SecurityException::NO_TRUSTED_CA) {
  $this->fail('CA Verification should have succeeded');
@@ -291,9 +325,7 @@ public function testRegisterThrowsWithChangedApplicationParameter(): void
 
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::WRONG_RELYING_PARTY);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  public function testRegisterThrowsWithChangedChallengeParameter(): void
@@ -310,9 +342,7 @@ public function testRegisterThrowsWithChangedChallengeParameter(): void
 
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::SIGNATURE_INVALID);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  public function testRegisterThrowsWithChangedKeyHandle(): void
@@ -327,9 +357,7 @@ public function testRegisterThrowsWithChangedKeyHandle(): void
 
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::SIGNATURE_INVALID);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  public function testRegisterThrowsWithChangedPubkey(): void
@@ -344,9 +372,7 @@ public function testRegisterThrowsWithChangedPubkey(): void
 
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::SIGNATURE_INVALID);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  public function testRegisterThrowsWithBadSignature(): void
@@ -361,9 +387,7 @@ public function testRegisterThrowsWithBadSignature(): void
 
  $this->expectException(SecurityException::class);
  $this->expectExceptionCode(SecurityException::SIGNATURE_INVALID);
- $this->server
- ->setRegisterRequest($request)
- ->register($response);
+ $this->server->validateRegistration($request, $response);
  }
 
  // -( Authentication )-----------------------------------------------------
@@ -614,7 +638,6 @@ public function testRegistrationWithoutCidPubkeyBug14Case2(): void
  $registerRequest = new RegisterRequest();
  $registerRequest->setAppId($server->getAppId())
  ->setChallenge('E23usdC7VkxjN1mwRAeyjg');
- $server->setRegisterRequest($registerRequest);
 
  $json = '{"registrationData":"BQSTffB-e9hdFwhsfb2t-2ppwyxZAltnDf6TYwv4'.
  '1VtleEO4488JwNFGr_bks_4EzA4DoluDBCgfmULGpZpXykTZQMOMz9DfbESHnuBY9'.
@@ -633,7 +656,7 @@ public function testRegistrationWithoutCidPubkeyBug14Case2(): void
  '5maW5pc2hFbnJvbGxtZW50In0"}';
  $registerResponse = RegisterResponse::fromJson($json);
 
- $registration = $server->register($registerResponse);
+ $registration = $server->validateRegistration($registerRequest, $registerResponse);
  $this->assertInstanceOf(Registration::class, $registration);
  }