diff --git a/config/services.yaml b/config/services.yaml index 613aebcb..410ec151 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -62,11 +62,6 @@ services: arguments: $errorPageHelper: '@Surfnet\Tiqr\Service\ErrorPageHelper' - Surfnet\Tiqr\Service\TrustedDevice\DateTime\ExpirationHelper: - arguments: - $cookieLifetime: "%trusted_device_cookie_lifetime%" - $gracePeriod: 60 - Surfnet\Tiqr\Service\TrustedDevice\Crypto\CryptoHelperInterface: class: Surfnet\Tiqr\Service\TrustedDevice\Crypto\HaliteCryptoHelper diff --git a/src/Controller/AuthenticationNotificationController.php b/src/Controller/AuthenticationNotificationController.php index c84c6651..a035d01a 100644 --- a/src/Controller/AuthenticationNotificationController.php +++ b/src/Controller/AuthenticationNotificationController.php @@ -108,7 +108,7 @@ public function __invoke(Request $request): Response return $this->generateNotificationResponse('no-trusted-device'); } - if (!$this->trustedDeviceService->isTrustedDevice($cookie, $nameId, $notificationAddress)) { + if ($this->trustedDeviceService->isTrustedDevice($cookie, $nameId, $notificationAddress) === false) { $this->logger->notice( sprintf( 'A trusted device cookie is found for notification address "%s" and user "%s", but has signature mismatch', diff --git a/src/Features/Context/TiqrContext.php b/src/Features/Context/TiqrContext.php index d7a318ab..f503d8c9 100644 --- a/src/Features/Context/TiqrContext.php +++ b/src/Features/Context/TiqrContext.php @@ -40,7 +40,6 @@ use Surfnet\Tiqr\Service\TrustedDevice\TrustedDeviceService; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException; use Surfnet\Tiqr\Tiqr\TiqrConfigurationInterface; use Surfnet\Tiqr\Tiqr\TiqrUserRepositoryInterface; @@ -361,7 +360,7 @@ public function weHaveATrustedDevice(string $notificationAddress): void $request->cookies->set($cookie->getName(), $cookie->getValue()); } $cookieValue = $this->trustedDeviceService->read($request, $userId, $notificationAddress); - Assertion::isInstanceOf($cookieValue, CookieValueInterface::class); + Assertion::isInstanceOf($cookieValue, CookieValue::class); Assertion::true($this->trustedDeviceService->isTrustedDevice($cookieValue, $userId, $notificationAddress)); } @@ -603,8 +602,7 @@ public function aPushNotificationIsSentWithATrustedDevice(string $notificationAd $cryptoHelper = new HaliteCryptoHelper($config); $cookieValue = CookieValue::from($cookieUserId, $notificationAddress); - $helper = new CookieHelper($config, $cryptoHelper, new NullLogger()); - $cookieName = $helper->buildCookieName($userId, $notificationAddress); + $cookieName = 'tiqr-trusted-device' . hash('sha256', $userId . '_' . $notificationAddress); $encryptedValue = $cryptoHelper->encrypt($cookieValue); diff --git a/src/Features/mfaFatigueMitigation.feature b/src/Features/mfaFatigueMitigation.feature index e493caf3..20791e41 100644 --- a/src/Features/mfaFatigueMitigation.feature +++ b/src/Features/mfaFatigueMitigation.feature @@ -20,7 +20,7 @@ Feature: When an user needs to authenticate Then we have a authenticated app And we have a trusted cookie for address: "0000000000111111111122222222223333333333" - Scenario: When a user authenticates without a trusted cookie, a notification should not be sent + Scenario: When a user authenticates without a trusted cookie, a push notification should not be sent Given I am on "/demo/sp" And I fill in "NameID" with my identifier When I press "authenticate" diff --git a/src/Service/TrustedDevice/Crypto/CryptoHelperInterface.php b/src/Service/TrustedDevice/Crypto/CryptoHelperInterface.php index 06e92b00..70c01644 100644 --- a/src/Service/TrustedDevice/Crypto/CryptoHelperInterface.php +++ b/src/Service/TrustedDevice/Crypto/CryptoHelperInterface.php @@ -21,11 +21,10 @@ namespace Surfnet\Tiqr\Service\TrustedDevice\Crypto; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; interface CryptoHelperInterface { - public function encrypt(CookieValueInterface $cookieValue): string; + public function encrypt(CookieValue $cookieValue): string; - public function decrypt(string $cookieData): CookieValueInterface; + public function decrypt(string $cookieData): CookieValue; } diff --git a/src/Service/TrustedDevice/Crypto/HaliteCryptoHelper.php b/src/Service/TrustedDevice/Crypto/HaliteCryptoHelper.php index 31b837fc..6124d47b 100644 --- a/src/Service/TrustedDevice/Crypto/HaliteCryptoHelper.php +++ b/src/Service/TrustedDevice/Crypto/HaliteCryptoHelper.php @@ -21,6 +21,7 @@ namespace Surfnet\Tiqr\Service\TrustedDevice\Crypto; use Exception; +use JsonException; use ParagonIE\Halite\Alerts\InvalidKey; use ParagonIE\Halite\Symmetric\Crypto; use ParagonIE\Halite\Symmetric\EncryptionKey; @@ -29,7 +30,6 @@ use Surfnet\Tiqr\Service\TrustedDevice\Exception\EncryptionFailedException; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; class HaliteCryptoHelper implements CryptoHelperInterface { @@ -53,8 +53,11 @@ public function __construct(Configuration $configuration) * HKDF using a salt This means that learning either derived key cannot lead to learning the other * derived key, or the secret key input in the HKDF. Encrypting many messages using the same * secret key is not a problem in this design. + * + * @throws EncryptionFailedException + * @throws JsonException */ - public function encrypt(CookieValueInterface $cookieValue): string + public function encrypt(CookieValue $cookieValue): string { try { $plainTextCookieValue = new HiddenString($cookieValue->serialize()); @@ -76,8 +79,11 @@ public function encrypt(CookieValueInterface $cookieValue): string * Decrypt the cookie ciphertext back to plain text. * Again using the encryption key, used to encrypt the data. * The decrypt method will return a deserialized CookieValue value object + * + * @throws DecryptionFailedException + * @throws JsonException */ - public function decrypt(string $cookieData): CookieValueInterface + public function decrypt(string $cookieData): CookieValue { try { // Decryption: (we use the default encoding: Halite::DECODE_BASE64URLSAFE) diff --git a/src/Service/TrustedDevice/DateTime/ExpirationHelper.php b/src/Service/TrustedDevice/DateTime/ExpirationHelper.php index a1cb3183..c076b057 100644 --- a/src/Service/TrustedDevice/DateTime/ExpirationHelper.php +++ b/src/Service/TrustedDevice/DateTime/ExpirationHelper.php @@ -23,7 +23,8 @@ use DateTime as CoreDateTime; use Surfnet\StepupBundle\DateTime\DateTime; use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; +use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration; +use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; use TypeError; class ExpirationHelper implements ExpirationHelperInterface @@ -31,17 +32,7 @@ class ExpirationHelper implements ExpirationHelperInterface private CoreDateTime $now; public function __construct( - /** - * The trusted device cookie lifetime in seconds - * See: config/openconext/parameters.yaml trusted_device_cookie_lifetime - */ - readonly private int $cookieLifetime, - /** - * The period in seconds that we still acknowledge the - * cookie even tho the expiration was reached. This accounts - * for server time/sync differences that may occur. - */ - readonly private int $gracePeriod, + private readonly Configuration $configuration, ?CoreDateTime $now = null ) { if ($now === null) { @@ -50,7 +41,7 @@ public function __construct( $this->now = $now; } - public function isExpired(CookieValueInterface $cookieValue): bool + public function isExpired(CookieValue $cookieValue): bool { try { $authenticationTimestamp = $cookieValue->authenticationTime(); @@ -75,7 +66,7 @@ public function isExpired(CookieValueInterface $cookieValue): bool ); } - $expirationTimestamp = $authenticationTimestamp + $this->cookieLifetime + $this->gracePeriod; + $expirationTimestamp = $authenticationTimestamp + $this->configuration->lifetimeInSeconds; $currentTimestamp = $this->now->getTimestamp(); // Is the current time greater than the expiration time? diff --git a/src/Service/TrustedDevice/DateTime/ExpirationHelperInterface.php b/src/Service/TrustedDevice/DateTime/ExpirationHelperInterface.php index bc3cd4c5..b77ef96f 100644 --- a/src/Service/TrustedDevice/DateTime/ExpirationHelperInterface.php +++ b/src/Service/TrustedDevice/DateTime/ExpirationHelperInterface.php @@ -20,7 +20,7 @@ namespace Surfnet\Tiqr\Service\TrustedDevice\DateTime; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; +use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; /** * Used to verify if the authentication time from the CookieValue @@ -34,5 +34,5 @@ */ interface ExpirationHelperInterface { - public function isExpired(CookieValueInterface $cookieValue): bool; + public function isExpired(CookieValue $cookieValue): bool; } diff --git a/src/Service/TrustedDevice/Http/CookieHelper.php b/src/Service/TrustedDevice/Http/CookieHelper.php index 0f3046fd..75a876c6 100644 --- a/src/Service/TrustedDevice/Http/CookieHelper.php +++ b/src/Service/TrustedDevice/Http/CookieHelper.php @@ -25,7 +25,7 @@ use Surfnet\Tiqr\Service\TrustedDevice\Crypto\CryptoHelperInterface; use Surfnet\Tiqr\Service\TrustedDevice\Exception\CookieNotFoundException; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; +use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; use Symfony\Component\HttpFoundation\Cookie; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -40,7 +40,7 @@ public function __construct( ) { } - public function write(Response $response, CookieValueInterface $value): void + public function write(Response $response, CookieValue $value): void { $cookieName = $this->buildCookieName($value->getUserId(), $value->getNotificationAddress()); @@ -57,7 +57,7 @@ public function write(Response $response, CookieValueInterface $value): void /** * Retrieve the current cookie from the Request if it exists. */ - public function read(Request $request, string $userId, string $notificationAddress): CookieValueInterface + public function read(Request $request, string $userId, string $notificationAddress): CookieValue { $cookieName = $this->buildCookieName($userId, $notificationAddress); if (!$request->cookies->has($cookieName)) { @@ -111,7 +111,7 @@ private function getTimestamp(int $expiresInSeconds): int return $currentTimestamp + $expiresInSeconds; } - public function buildCookieName(string $userId, string $notificationAddress): string + private function buildCookieName(string $userId, string $notificationAddress): string { return $this->configuration->prefix . hash('sha256', $userId . '_' . $notificationAddress); } diff --git a/src/Service/TrustedDevice/Http/CookieHelperInterface.php b/src/Service/TrustedDevice/Http/CookieHelperInterface.php index 6f589aee..5f248b85 100644 --- a/src/Service/TrustedDevice/Http/CookieHelperInterface.php +++ b/src/Service/TrustedDevice/Http/CookieHelperInterface.php @@ -20,17 +20,15 @@ namespace Surfnet\Tiqr\Service\TrustedDevice\Http; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; +use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; interface CookieHelperInterface { - public function write(Response $response, CookieValueInterface $value): void; + public function write(Response $response, CookieValue $value): void; - public function read(Request $request, string $userId, string $notificationAddress): CookieValueInterface; + public function read(Request $request, string $userId, string $notificationAddress): CookieValue; public function fingerprint(Request $request, string $userId, string $notificationAddress): string; - - public function buildCookieName(string $userId, string $notificationAddress): string; } diff --git a/src/Service/TrustedDevice/TrustedDeviceService.php b/src/Service/TrustedDevice/TrustedDeviceService.php index 7b337890..dd2e42e7 100644 --- a/src/Service/TrustedDevice/TrustedDeviceService.php +++ b/src/Service/TrustedDevice/TrustedDeviceService.php @@ -28,7 +28,6 @@ use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException; use Surfnet\Tiqr\Service\TrustedDevice\Http\CookieHelperInterface; use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue; -use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -50,7 +49,7 @@ public function registerTrustedDevice(Response $response, string $userId, string } public function isTrustedDevice( - CookieValueInterface $cookie, + CookieValue $cookie, string $userId, string $notificationAddress, ): bool { @@ -58,17 +57,10 @@ public function isTrustedDevice( return false; } - $this->logger->notice('Push-notification MFA is allowed for the device based on the cookie.'); return true; } - - private function store(Response $response, CookieValueInterface $cookieValue): void - { - $this->cookieHelper->write($response, $cookieValue); - } - - public function read(Request $request, string $userId, string $notificationAddress): ?CookieValueInterface + public function read(Request $request, string $userId, string $notificationAddress): ?CookieValue { try { return $this->cookieHelper->read($request, $userId, $notificationAddress); @@ -87,7 +79,12 @@ public function read(Request $request, string $userId, string $notificationAddre } } - private function isCookieValid(CookieValueInterface $cookie, string $userId, string $notificationAddress): bool + private function store(Response $response, CookieValue $cookieValue): void + { + $this->cookieHelper->write($response, $cookieValue); + } + + private function isCookieValid(CookieValue $cookie, string $userId, string $notificationAddress): bool { if ($cookie instanceof CookieValue && ($cookie->getUserId() !== $userId || $cookie->getNotificationAddress() !== $notificationAddress)) { $this->logger->error( diff --git a/src/Service/TrustedDevice/ValueObject/CookieValue.php b/src/Service/TrustedDevice/ValueObject/CookieValue.php index aa631b3a..c159e786 100644 --- a/src/Service/TrustedDevice/ValueObject/CookieValue.php +++ b/src/Service/TrustedDevice/ValueObject/CookieValue.php @@ -23,12 +23,11 @@ use DateTime; use InvalidArgumentException; -use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException; +use JsonException; -use function strtolower; use function strtotime; -readonly class CookieValue implements CookieValueInterface +readonly class CookieValue { private function __construct( private string $userId, @@ -49,9 +48,9 @@ public static function from(string $userId, string $notificationAddress): self } /** - * @throws \JsonException + * @throws JsonException */ - public static function deserialize(string $serializedData): CookieValueInterface + public static function deserialize(string $serializedData): CookieValue { $data = json_decode($serializedData, true, 512, JSON_THROW_ON_ERROR); @@ -75,7 +74,7 @@ public static function deserialize(string $serializedData): CookieValueInterface } /** - * @throws \JsonException + * @throws JsonException */ public function serialize(): string { diff --git a/src/Service/TrustedDevice/ValueObject/CookieValueInterface.php b/src/Service/TrustedDevice/ValueObject/CookieValueInterface.php deleted file mode 100644 index 6922a97e..00000000 --- a/src/Service/TrustedDevice/ValueObject/CookieValueInterface.php +++ /dev/null @@ -1,34 +0,0 @@ -makeExpirationHelper(3600, time(), 5); + // Cookie lifetime 3600 + $helper = $this->makeExpirationHelper(3600, time()); return [ - 'within grace period (outer bound)' => [false, $helper, $this->makeCookieValue(time() - 3605)], - 'within grace period' => [false, $helper, $this->makeCookieValue(time() - 3601)], 'within grace period (lower bound)' => [false, $helper, $this->makeCookieValue(time() - 3600)], - 'outside grace period' => [true, $helper, $this->makeCookieValue(time() - 3606)], + 'outside grace period' => [true, $helper, $this->makeCookieValue(time() - 3601)], ]; } - private function makeExpirationHelper(int $expirationTime, int $now, int $gracePeriod = 0) : ExpirationHelper + private function makeExpirationHelper(int $expirationTime, int $now) : ExpirationHelper { $time = new \DateTime(); $time->setTimestamp($now); - return new ExpirationHelper($expirationTime, $gracePeriod, $time); + + $config = new Configuration( + '', + $expirationTime, + '000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f', + 'none', + ); + + return new ExpirationHelper($config, $time); } - private function makeCookieValue(int $authenticationTime) : CookieValueInterface + private function makeCookieValue(int $authenticationTime) : CookieValue { $dateTime = new \DateTime(); $dateTime->setTimestamp($authenticationTime); @@ -128,7 +134,7 @@ private function makeCookieValue(int $authenticationTime) : CookieValueInterface return CookieValue::deserialize(json_encode($data)); } - private function makeCookieValueUnrestrictedAuthTime($authenticationTime) : CookieValueInterface + private function makeCookieValueUnrestrictedAuthTime($authenticationTime) : CookieValue { $data = [ 'userId' => 'userId', diff --git a/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php b/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php index b0630934..cd21010b 100644 --- a/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php +++ b/tests/Unit/Service/TrustedCookie/TrustedCookieServiceTest.php @@ -52,7 +52,7 @@ protected function buildService(Configuration $configuration, DateTime $now = nu { $this->configuration = $configuration; $encryptionHelper = new HaliteCryptoHelper($configuration); - $expirationHelper = new ExpirationHelper($this->configuration->lifetimeInSeconds, 1, $now); + $expirationHelper = new ExpirationHelper($this->configuration, $now); $cookieHelper = new CookieHelper($this->configuration, $encryptionHelper, $this->logger); $this->service = new TrustedDeviceService( $cookieHelper, @@ -127,7 +127,7 @@ public function test_is_trusted_when_identity_matches(): void '0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f', CookieSameSite::SAMESITE_STRICT->value ), - new DateTime('+2592001 seconds') // lifetime + 1 second grace + new DateTime('+2592000 seconds') ); $cookieValue = CookieValue::from('userId#123', 'notAddr#321'); @@ -148,7 +148,7 @@ public function test_is_untrusted_when_token_expired(): void 2592000, '0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f', CookieSameSite::SAMESITE_STRICT->value - ), new DateTime('+2592002 seconds') // lifetime + 1 second grace + 1 + ), new DateTime('+2592001 seconds') // lifetime + 1 second ); $cookieValue = CookieValue::from('userId#123', 'notAddr#321');