From 5abb9832caf304057fb7193bc116c16aaedf27b8 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Fri, 9 Sep 2022 16:23:25 -0700 Subject: [PATCH] Add class for automatically-expiring challenges (#15) This adds a utility class to make best practices around short-lived challenges easier to follow. --- README.md | 14 ++- examples/readmeLoginStep1.php | 4 +- examples/readmeRegisterStep1.php | 4 +- src/Challenge.php | 16 +-- src/ChallengeInterface.php | 18 ++++ src/CreateResponse.php | 4 +- src/Errors/ExpiredChallengeError.php | 11 ++ src/ExpiringChallenge.php | 97 ++++++++++++++++++ src/GetResponse.php | 4 +- src/Responses/AssertionInterface.php | 4 +- src/Responses/AttestationInterface.php | 4 +- tests/ChallengeInterfaceTestTrait.php | 69 +++++++++++++ tests/ChallengeTest.php | 37 ++----- tests/ExpiringChallengeTest.php | 135 +++++++++++++++++++++++++ 14 files changed, 370 insertions(+), 51 deletions(-) create mode 100644 src/ChallengeInterface.php create mode 100644 src/Errors/ExpiredChallengeError.php create mode 100644 src/ExpiringChallenge.php create mode 100644 tests/ChallengeInterfaceTestTrait.php create mode 100644 tests/ExpiringChallengeTest.php diff --git a/README.md b/README.md index 482490a..c1c254d 100644 --- a/README.md +++ b/README.md @@ -48,10 +48,10 @@ Send it to the user as base64. ```php wrapped->unwrap(); + return $this->wrapped; } /** @@ -69,7 +71,7 @@ public function getBase64(): string } /** - * @return array{b64: string} + * @return SerializationFormat */ public function __serialize(): array { @@ -77,7 +79,7 @@ public function __serialize(): array } /** - * @param array{b64: string} $serialized + * @param SerializationFormat $serialized */ public function __unserialize(array $serialized): void { diff --git a/src/ChallengeInterface.php b/src/ChallengeInterface.php new file mode 100644 index 0000000..241f52f --- /dev/null +++ b/src/ChallengeInterface.php @@ -0,0 +1,18 @@ +getUnwrappedBinary()); + $b64u = Codecs\Base64Url::encode($challenge->getBinary()->unwrap()); if (!hash_equals($b64u, $C['challenge'])) { $this->fail('7.1.8', 'C.challenge'); } diff --git a/src/Errors/ExpiredChallengeError.php b/src/Errors/ExpiredChallengeError.php new file mode 100644 index 0000000..fb05bfd --- /dev/null +++ b/src/Errors/ExpiredChallengeError.php @@ -0,0 +1,11 @@ +invert && not in unit tests, throw? + $this->wrapped = Challenge::random(); + $this->expiration = (new DateTimeImmutable())->add($duration); + } + + /** + * @param positive-int $seconds + * + * @api + */ + public static function withLifetime(int $seconds): ChallengeInterface + { + if ($seconds <= 0) { // @phpstan-ignore-line Still need the runtime check here + throw new InvalidArgumentException('Lifetime must be a postive integer'); + } + $duration = sprintf('PT%dS', $seconds); + return new ExpiringChallenge(new DateInterval($duration)); + } + + public function getBase64(): string + { + if ($this->isExpired()) { + throw new Errors\ExpiredChallengeError(); + } + return $this->wrapped->getBase64(); + } + + public function getBinary(): BinaryString + { + if ($this->isExpired()) { + throw new Errors\ExpiredChallengeError(); + } + return $this->wrapped->getBinary(); + } + + private function isExpired(): bool + { + $diff = $this->expiration->diff(new DateTimeImmutable()); + + return $diff->invert === 0; + } + + /** + * @return SerializationFormat + */ + public function __serialize(): array + { + return [ + 'c' => $this->wrapped->getBase64(), + 'e' => $this->expiration->getTimestamp(), + ]; + } + + /** + * @param SerializationFormat $serialized + */ + public function __unserialize(array $serialized): void + { + $bin = base64_decode($serialized['c'], true); + assert($bin !== false); + $this->wrapped = new Challenge(new BinaryString($bin)); + $this->expiration = new DateTimeImmutable('@' . $serialized['e']); + } +} diff --git a/src/GetResponse.php b/src/GetResponse.php index 17897c6..4f2a976 100644 --- a/src/GetResponse.php +++ b/src/GetResponse.php @@ -36,7 +36,7 @@ public function getUsedCredentialId(): BinaryString * @link https://www.w3.org/TR/webauthn-2/#sctn-verifying-assertion */ public function verify( - Challenge $challenge, + ChallengeInterface $challenge, RelyingParty $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, @@ -89,7 +89,7 @@ public function verify( } // 7.2.12 - $b64u = Codecs\Base64Url::encode($challenge->getUnwrappedBinary()); + $b64u = Codecs\Base64Url::encode($challenge->getBinary()->unwrap()); if (!hash_equals($b64u, $C['challenge'])) { $this->fail('7.2.12', 'C.challenge'); } diff --git a/src/Responses/AssertionInterface.php b/src/Responses/AssertionInterface.php index ab46473..358c29d 100644 --- a/src/Responses/AssertionInterface.php +++ b/src/Responses/AssertionInterface.php @@ -6,7 +6,7 @@ use Firehed\WebAuthn\{ BinaryString, - Challenge, + ChallengeInterface, CredentialContainer, CredentialInterface, RelyingParty, @@ -30,7 +30,7 @@ public function getUsedCredentialId(): BinaryString; * @api */ public function verify( - Challenge $challenge, + ChallengeInterface $challenge, RelyingParty $rp, CredentialContainer | CredentialInterface $credential, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, diff --git a/src/Responses/AttestationInterface.php b/src/Responses/AttestationInterface.php index 5681bf5..dbb9460 100644 --- a/src/Responses/AttestationInterface.php +++ b/src/Responses/AttestationInterface.php @@ -5,7 +5,7 @@ namespace Firehed\WebAuthn\Responses; use Firehed\WebAuthn\{ - Challenge, + ChallengeInterface, CredentialInterface, RelyingParty, UserVerificationRequirement, @@ -20,7 +20,7 @@ interface AttestationInterface { public function verify( - Challenge $challenge, + ChallengeInterface $challenge, RelyingParty $rp, UserVerificationRequirement $uv = UserVerificationRequirement::Preferred, ): CredentialInterface; diff --git a/tests/ChallengeInterfaceTestTrait.php b/tests/ChallengeInterfaceTestTrait.php new file mode 100644 index 0000000..5d08c9a --- /dev/null +++ b/tests/ChallengeInterfaceTestTrait.php @@ -0,0 +1,69 @@ +getChallenge(); + + $serialized = serialize($challenge); + $unserialized = unserialize($serialized); + + self::assertInstanceOf(ChallengeInterface::class, $unserialized); + self::assertTrue( + $challenge->getBinary()->equals($unserialized->getBinary()), + 'Wrapped challenge changed', + ); + + self::assertSame( + $challenge->getBase64(), + $unserialized->getBase64(), + 'Base64 changed', + ); + } + + public function testBinaryMatchesBase64(): void + { + $challenge = $this->getChallenge(); + + $binary = $challenge->getBinary(); + $base64 = $challenge->getBase64(); + + self::assertSame( + $base64, + base64_encode($binary->unwrap()), + 'Base64 encoding the unwrapped binary did not match getBase64 result', + ); + } + + /** + * This covers the specific scenario of a challenge being unserialized + * (e.g. from a Session) that was generated from an older version of this + * library. This helps ensure compatibility across versions and reduces the + * risk of a point release breaking any active session data. + */ + public function testInFlightDecode(): void + { + $serialized = $this->getInFlightSerialized(); + $unserialized = unserialize($serialized); + self::assertInstanceOf(ChallengeInterface::class, $unserialized); + + self::assertTrue( + $this->getInFlightChallenge()->equals($unserialized->getBinary()), + 'Decoding resulted in inaccurate challenge', + ); + } + + abstract protected function getInFlightSerialized(): string; + abstract protected function getInFlightChallenge(): BinaryString; +} diff --git a/tests/ChallengeTest.php b/tests/ChallengeTest.php index 716720e..0691a83 100644 --- a/tests/ChallengeTest.php +++ b/tests/ChallengeTest.php @@ -9,37 +9,20 @@ */ class ChallengeTest extends \PHPUnit\Framework\TestCase { - public function testSerializationRoundTrip(): void - { - $challenge = Challenge::random(); - - $serialized = serialize($challenge); - $unserialized = unserialize($serialized); + use ChallengeInterfaceTestTrait; - self::assertInstanceOf(Challenge::class, $unserialized); - self::assertTrue( - hash_equals($challenge->getUnwrappedBinary(), $unserialized->getUnwrappedBinary()), - 'Wrapped challenge changed', - ); + protected function getChallenge(): ChallengeInterface + { + return Challenge::random(); } - /** - * This covers the specific scenario of a challenge being unserialized - * (e.g. from a Session) that was generated from an older version of this - * library. This helps ensure compatibility across versions and reduces the - * risk of a point release breaking any active session data. - */ - public function testInFlightDecode(): void + protected function getInFlightSerialized(): string { - $serialized = 'O:26:"Firehed\WebAuthn\Challenge":1:{s:3:"b64";s:44:"k' . - 'tCbjFzaUuHixxmUFk9G35Yd0EZdWp5+RcHlKdsIK58=";}'; - $unserialized = unserialize($serialized); - self::assertInstanceOf(Challenge::class, $unserialized); + return 'O:26:"Firehed\WebAuthn\Challenge":1:{s:3:"b64";s:44:"ktCbjFzaUuHixxmUFk9G35Yd0EZdWp5+RcHlKdsIK58=";}'; + } - self::assertSame( - base64_decode('ktCbjFzaUuHixxmUFk9G35Yd0EZdWp5+RcHlKdsIK58=', true), - $unserialized->getUnwrappedBinary(), - 'Decoding resulted in inaccurate challenge', - ); + protected function getInFlightChallenge(): BinaryString + { + return BinaryString::fromBase64('ktCbjFzaUuHixxmUFk9G35Yd0EZdWp5+RcHlKdsIK58='); } } diff --git a/tests/ExpiringChallengeTest.php b/tests/ExpiringChallengeTest.php new file mode 100644 index 0000000..b2c40c1 --- /dev/null +++ b/tests/ExpiringChallengeTest.php @@ -0,0 +1,135 @@ +getBase64(); + } + + /** + * @doesNotPerformAssertions This is checking that an exeption is NOT + * thrown when the expiration is in the future. + */ + public function testFutureExpirationOkWhenGettingBinary(): void + { + $ec = new ExpiringChallenge(new DateInterval('PT2S')); + + // This should not throw. + $_ = $ec->getBinary(); + } + + + public function testPastExpirationThrowsWhenGettingBase64(): void + { + $interval = new DateInterval('PT2S'); + $interval->invert = 1; // Negative + $ec = new ExpiringChallenge($interval); + + self::expectException(Errors\ExpiredChallengeError::class); + $ec->getBase64(); + } + + public function testPastExpirationThrowsWhenGettingBinary(): void + { + $interval = new DateInterval('PT2S'); + $interval->invert = 1; // Negative + $ec = new ExpiringChallenge($interval); + + self::expectException(Errors\ExpiredChallengeError::class); + $ec->getBinary(); + } + + /** + * @doesNotPerformAssertions This is checking that an exeption is NOT + * thrown when the expiration is in the future. + */ + public function testFactoryInFuture(): void + { + $ec = ExpiringChallenge::withLifetime(86400); + $_ = $ec->getBase64(); + } + + public function testFactoryRightNow(): void + { + self::expectException(LogicException::class); + // @phpstan-ignore-next-line Validating runtime check + $ec = ExpiringChallenge::withLifetime(0); + } + + public function testFactoryInPast(): void + { + self::expectException(LogicException::class); + // @phpstan-ignore-next-line Validating runtime check + $ec = ExpiringChallenge::withLifetime(-1); + } + + /** + * @doesNotPerformAssertions This indirectly asserts that exceptions are + * thrown + * + * @medium + */ + public function testExpirationByWaiting(): void + { + $ec = new ExpiringChallenge(new DateInterval('PT1S')); + $_ = $ec->getBase64(); + $_ = $ec->getBinary(); + + sleep(1); + try { + $_ = $ec->getBase64(); + self::fail('getBase64 did not throw'); + } catch (Errors\ExpiredChallengeError) { + } + + try { + $_ = $ec->getBinary(); + self::fail('getBinary did not throw'); + } catch (Errors\ExpiredChallengeError) { + } + } + + protected function getInFlightSerialized(): string + { + $nearFuture = time() + 10; + // This is ABSOLUTELY RIDICULOUS, but to bypass the expiration time so + // the wrapped challenge can be read out, this needs to dynamiaclly + // override the expiration in the serialized format. There's no good + // alternative I can think of that doesn't involve a) allowing the + // expiration to change or b) providing some sort of test-only hack + // method to accomplish the same. + return 'O:34:"Firehed\WebAuthn\ExpiringChallenge":2:{s:1:"c";s:44:"Vi' . + 'FgIH5w+B1BzVRWatX+Zjvt2D9JxQCAH6PnJwW+QdQ=";s:1:"e";i:' . + $nearFuture . ';}'; + } + + protected function getInFlightChallenge(): BinaryString + { + return BinaryString::fromBase64('ViFgIH5w+B1BzVRWatX+Zjvt2D9JxQCAH6PnJwW+QdQ='); + } +}