Skip to content

Commit

Permalink
Validate the cookie value authentication time
Browse files Browse the repository at this point in the history
The cookie lifetime can be extended by the user. But the explicit
lifetime of the cookie was not yet validated by the cookie service.
Meaning you could extend the SSO lifetime by keeping your cookie alive.

As of this change, the authentication lifetime is checked against the
current timestamp togegher with the cookie expiration time.

See: https://www.pivotaltracker.com/story/show/183402734
 -> the SSO cookie's timestamp is within the configured SSO lifetime
  • Loading branch information
MKodde committed Aug 16, 2023
1 parent 3e96653 commit 5527389
Show file tree
Hide file tree
Showing 11 changed files with 314 additions and 10 deletions.
5 changes: 4 additions & 1 deletion docs/SsoOn2Fa.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ The cookie contains several values, used to ascertain if SSO can be given. These
| `LoA` | The LoA of the second factor |
| `Timestamp` | The timestamp taken during authentication. |

The cookie is used to verify the SSO is issued to the correct identity (user). And to check if the LoA requirement is satisfied by the SSO cookie. The timestamp is kept mainly for audit reasons.
The cookie is used to verify the SSO is issued to the correct identity (user). And to check if the LoA requirement is satisfied by the SSO cookie.
The timestamp is used to verify if the cookie value did not expire. This is determined by adding the cookie expiration time (configured in `sso_cookie_lifetime` param)
to the authentication timestamp found in the cookie. If those two exceed the current timestamp, the cookie is considered to be expired. Even tho the browser cookie itself
might still be alive.

The cookie value contains sensitive data, and its contents are authenticated and encrypted for that reason. We use the Paragonie Halite library for this. Halite uses XSalsa20 for encryption and BLAKE2b for message Authentication (MAC).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ services:
- "@gateway.service.sso_2fa_cookie_config"
public: false

gateway.service.sso_2fa_expiration_helper:
class: Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelper
arguments:
$cookieLifetime: "%sso_cookie_lifetime%"

gateway.service.sso_2fa_cookie_helper:
class: Surfnet\StepupGateway\GatewayBundle\Sso2fa\Http\CookieHelper
arguments:
Expand All @@ -151,10 +156,10 @@ services:
arguments:
- "@gateway.service.sso_2fa_cookie_helper"
- "@gateway.service.institution_configuration"
- "@logger"
- "@gateway.service.second_factor_service"
- "@surfnet_stepup.service.second_factor_type"
- "@gssp.allowed_sps"
- "@gateway.service.sso_2fa_expiration_helper"
- "@logger"

gateway.service.institution_configuration:
class: Surfnet\StepupGateway\GatewayBundle\Service\InstitutionConfigurationService
Expand Down
27 changes: 24 additions & 3 deletions src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/CookieService.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
use Surfnet\StepupGateway\GatewayBundle\Saml\ResponseContext;
use Surfnet\StepupGateway\GatewayBundle\Service\InstitutionConfigurationService;
use Surfnet\StepupGateway\GatewayBundle\Service\SecondFactorService;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelperInterface;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Exception\CookieNotFoundException;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Exception\InvalidAuthenticationTimeException;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Http\CookieHelperInterface;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValue;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValueInterface;
Expand Down Expand Up @@ -61,18 +63,24 @@ class CookieService implements CookieServiceInterface
* @var SecondFactorTypeService
*/
private $secondFactorTypeService;
/**
* @var ExpirationHelperInterface
*/
private $expirationHelper;

public function __construct(
CookieHelperInterface $cookieHelper,
InstitutionConfigurationService $institutionConfigurationService,
LoggerInterface $logger,
SecondFactorService $secondFactorService,
SecondFactorTypeService $secondFactorTypeService
SecondFactorTypeService $secondFactorTypeService,
ExpirationHelperInterface $expirationHelper,
LoggerInterface $logger
) {
$this->cookieHelper = $cookieHelper;
$this->institutionConfigurationService = $institutionConfigurationService;
$this->secondFactorService = $secondFactorService;
$this->secondFactorTypeService = $secondFactorTypeService;
$this->expirationHelper = $expirationHelper;
$this->logger = $logger;
}

Expand Down Expand Up @@ -101,7 +109,7 @@ public function handleSsoOn2faCookieStorage(
if (!$secondFactor) {
throw new RuntimeException(sprintf('Second Factor token not found with ID: %s', $secondFactorId));
}
// Test if the institution of the Idenity this SF belongs to has SSO on 2FA enabled
// Test if the institution of the Identity this SF belongs to has SSO on 2FA enabled
$isEnabled = $this->institutionConfigurationService->ssoOn2faEnabled($secondFactor->institution);
$this->logger->notice(
sprintf(
Expand Down Expand Up @@ -178,6 +186,19 @@ public function shouldSkip2faAuthentication(
);
return false;
}
try {
$isExpired = $this->expirationHelper->isExpired($ssoCookie);
if ($isExpired) {
$this->logger->notice(
'The SSO on 2FA cookie has expired. Meaning [authentication time] + [cookie lifetime] is in the past'
);
return false;
}
} catch (InvalidAuthenticationTimeException $e) {
$this->logger->notice('The SSO on 2FA cookie contained an invalid authentication time', [$e->getMessage()]);
return false;
}

/** @var SecondFactor $secondFactor */
foreach ($secondFactorCollection as $secondFactor) {
$loa = $secondFactor->getLoaLevel($this->secondFactorTypeService);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php declare(strict_types=1);

/**
* Copyright 2023 SURFnet bv
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime;

use DateTime as CoreDateTime;
use Surfnet\StepupBundle\DateTime\DateTime;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Exception\InvalidAuthenticationTimeException;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValueInterface;
use TypeError;

class ExpirationHelper implements ExpirationHelperInterface
{
/**
* @var CoreDateTime
*/
private $now;

/*
* The SSO on 2FA cookie lifetime in seconds
*
* See: config/legacy/parameters.yaml sso_cookie_lifetime
*/
private $cookieLifetime;

public function __construct(int $cookieLifetime, CoreDateTime $now = null)
{
$this->cookieLifetime = $cookieLifetime;
if (!$now) {
$now = DateTime::now();
}
$this->now = $now;
}

public function isExpired(CookieValueInterface $cookieValue): bool
{
try {
$authenticationTimestamp = $cookieValue->authenticationTime();
} catch (TypeError $error) {
throw new InvalidAuthenticationTimeException(
'The authentication time contained a non-int value',
0,
$error
);
}
if ($authenticationTimestamp < 0) {
throw new InvalidAuthenticationTimeException(
'The authentication time is from before the Unix timestamp epoch'
);
}
if ($authenticationTimestamp > $this->now->getTimestamp()) {
throw new InvalidAuthenticationTimeException(
'The authentication time is from the future, which indicates the clock settings ' .
'are incorrect, or the time in the cookie value was tampered with.'
);
}

$expirationTimestamp = $authenticationTimestamp + $this->cookieLifetime;
$currentTimestamp = $this->now->getTimestamp();
// Is the current time greater than the expiration time?
return $currentTimestamp > $expirationTimestamp;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php declare(strict_types=1);

/**
* Copyright 2023 SURFnet bv
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime;

use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValueInterface;

/**
* Used to verify if the authentication time from the CookieValue
* surpasses the current timestamp. Which is determined by adding
* the cookie lifetime to the authentication time. And checking that
* against the current timestamp.
*
* The current timestamp can be set on this helper class in order
* to make testing more predictable. However, if this is not set
* explicitly it will use 'now' as the current timestamp.
*/
interface ExpirationHelperInterface
{
public function isExpired(CookieValueInterface $cookieValue): bool;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php declare(strict_types=1);

/**
* Copyright 2023 SURFnet bv
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace Surfnet\StepupGateway\GatewayBundle\Sso2fa\Exception;

use Surfnet\StepupGateway\GatewayBundle\Exception\InvalidArgumentException;

class InvalidAuthenticationTimeException extends InvalidArgumentException
{

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@

namespace Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject;

use Surfnet\StepupBundle\DateTime\DateTime;
use DateTime;
use function strtolower;
use function strtotime;

class CookieValue implements CookieValueInterface
{
Expand All @@ -41,7 +42,8 @@ public static function from(string $identityId, string $secondFactorId, float $l
$cookieValue->tokenId = $secondFactorId;
$cookieValue->identityId = $identityId;
$cookieValue->loa = $loa;
$cookieValue->authenticationTime = DateTime::now()->format(DATE_ATOM);
$dateTime = new DateTime();
$cookieValue->authenticationTime = $dateTime->format(DATE_ATOM);
return $cookieValue;
}

Expand Down Expand Up @@ -86,4 +88,9 @@ public function issuedTo(string $identityNameId): bool
{
return strtolower($identityNameId) === strtolower($this->identityId);
}

public function authenticationTime(): int
{
return strtotime($this->authenticationTime);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@ public static function deserialize(string $serializedData): CookieValueInterface
public function serialize(): string;

public function meetsRequiredLoa(float $requiredLoa): bool;

public function authenticationTime(): int;
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,9 @@ public function meetsRequiredLoa(float $requiredLoa): bool
{
return false;
}

public function authenticationTime(): int
{
return -1;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\CookieService;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Crypto\CryptoHelper;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Crypto\HaliteCryptoHelper;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelper;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelperInterface;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Http\CookieHelper;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\Configuration;
use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValue;
Expand Down Expand Up @@ -86,6 +88,11 @@ class CookieServiceTest extends TestCase
*/
private $encryptionHelper;

/**
* @var Mockery\LegacyMockInterface|Mockery\MockInterface|ExpirationHelperInterface|(ExpirationHelperInterface&Mockery\LegacyMockInterface)|(ExpirationHelperInterface&Mockery\MockInterface)
*/
private $expirationHelper;

protected function buildService(Configuration $configuration): void
{
// Not all dependencies are included for real, the ones not focussed on crypto and cookie storage are mocked
Expand All @@ -95,13 +102,15 @@ protected function buildService(Configuration $configuration): void
$this->secondFactorTypeService = Mockery::mock(SecondFactorTypeService::class);
$this->configuration = $configuration;
$this->encryptionHelper = new HaliteCryptoHelper($configuration);
$this->expirationHelper = Mockery::mock(ExpirationHelperInterface::class);
$cookieHelper = new CookieHelper($this->configuration, $this->encryptionHelper, $logger);
$this->service = new CookieService(
$cookieHelper,
$this->institutionService,
$logger,
$this->secondFactorService,
$this->secondFactorTypeService
$this->secondFactorTypeService,
$this->expirationHelper,
$logger
);

$this->responseContext = Mockery::mock(ResponseContext::class);
Expand Down Expand Up @@ -349,6 +358,10 @@ public function test_skipping_authentication_succeeds()
->shouldReceive('saveSelectedSecondFactor')
->with($yubikey);

$this->expirationHelper
->shouldReceive('isExpired')
->andReturn(false);

self::assertTrue(
$this->service->shouldSkip2faAuthentication(
$this->responseContext,
Expand Down Expand Up @@ -390,6 +403,10 @@ public function test_skipping_authentication_succeeds_selects_correct_token()
->shouldReceive('saveSelectedSecondFactor')
->with($yubikey);

$this->expirationHelper
->shouldReceive('isExpired')
->andReturn(false);

self::assertTrue(
$this->service->shouldSkip2faAuthentication(
$this->responseContext,
Expand Down
Loading

0 comments on commit 5527389

Please sign in to comment.