Skip to content

Commit

Permalink
Log every time a session is created
Browse files Browse the repository at this point in the history
Prior to this change there we weren't able to keep track of sessions that got lost.

This change allows us to see every time a session is created and distinguish them by their correlation id.

Log an error on a route that requires an active session when there is none

Prior to this change all routes were able to called, even though the user might not have had an active session

This change will start logging errors when the session wasn't found, or is in an unexpected state

Listen to all routes and log the state of the session

Prior to this change session information got lost. We had no way of tracking down what happened to user sessions in the logs.

This change logs whether a session existed and if it's in a valid state. Log information is enriched with a correlation id to be able to distinguish them.

Enable session requirement check for enrollment

Inject session name into the session check services

That way we always follow the configured session name set in the
framework.yaml

Inject the correlation salt

That way we do not hard code a security measure in the code base. And
allow for manual setting of that SALT

Enable Session constraint testing on Authn routes
  • Loading branch information
mharte-ib authored and johanib committed Nov 21, 2024
1 parent 44e890a commit ff9742a
Show file tree
Hide file tree
Showing 16 changed files with 848 additions and 5 deletions.
5 changes: 5 additions & 0 deletions config/openconext/parameters.yaml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ parameters:
# PCRE as accepted by preg_match (http://php.net/preg_match).
mobile_app_user_agent_pattern: "/^.*$/"

# When logging authentication related messages, having a reference to the session id of the user
# makes auditing the logs much easier. We do not want to log the session_id for obvious reasons, that's why
# a salt is used to hash a part of the session id. Ensuring we do not disclose sensitive data in the logs.
session_correlation_salt: 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'

# Options for the tiqr library
tiqr_library_options:
general:
Expand Down
5 changes: 3 additions & 2 deletions config/packages/framework.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ framework:
# Remove or comment this section to explicitly disable session support.
session:
handler_id: null
cookie_secure: auto
cookie_samesite: none
name: sess_tiqr
cookie_httponly: true
cookie_secure: true
router:
strict_requirements: null
utf8: true
Expand Down
2 changes: 2 additions & 0 deletions config/services.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ services:
$locales: '%locales%'
$tiqrConfiguration: '%tiqr_library_options%'
$appSecret: '%app_secret%'
$sessionOptions: '%session.storage.options%'
$sessionCorrelationSalt: '%session_correlation_salt%'

# makes classes in src/ available to be used as services
# this creates a service per class whose id is the fully-qualified class name
Expand Down
28 changes: 28 additions & 0 deletions src/Attribute/RequiresActiveSession.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

/**
* Copyright 2024 SURFnet B.V.
*
* 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.
*/

declare(strict_types = 1);

namespace Surfnet\Tiqr\Attribute;

use Attribute;

#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD)]
class RequiresActiveSession
{
}
2 changes: 2 additions & 0 deletions src/Controller/AuthenticationNotificationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use Psr\Log\LoggerInterface;
use Surfnet\GsspBundle\Service\AuthenticationService;
use Surfnet\GsspBundle\Service\StateHandlerInterface;
use Surfnet\Tiqr\Attribute\RequiresActiveSession;
use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException;
use Surfnet\Tiqr\Tiqr\TiqrServiceInterface;
use Surfnet\Tiqr\Tiqr\TiqrUserRepositoryInterface;
Expand Down Expand Up @@ -52,6 +53,7 @@ public function __construct(
* @throws InvalidArgumentException
*/
#[Route(path: '/authentication/notification', name: 'app_identity_authentication_notification', methods: ['POST'])]
#[RequiresActiveSession]
public function __invoke(): Response
{
$nameId = $this->authenticationService->getNameId();
Expand Down
2 changes: 2 additions & 0 deletions src/Controller/AuthenticationQrController.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use Psr\Log\LoggerInterface;
use Surfnet\GsspBundle\Service\AuthenticationService;
use Surfnet\GsspBundle\Service\StateHandlerInterface;
use Surfnet\Tiqr\Attribute\RequiresActiveSession;
use Surfnet\Tiqr\Tiqr\TiqrServiceInterface;
use Surfnet\Tiqr\WithContextLogger;
use Symfony\Component\HttpFoundation\Response;
Expand All @@ -44,6 +45,7 @@ public function __construct(
* @throws InvalidArgumentException
*/
#[Route(path: '/authentication/qr', name: 'app_identity_authentication_qr', methods: ['GET'])]
#[RequiresActiveSession]
public function __invoke(): Response
{
$nameId = $this->authenticationService->getNameId();
Expand Down
3 changes: 2 additions & 1 deletion src/Controller/AuthenticationStatusController.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use Psr\Log\LoggerInterface;
use Surfnet\GsspBundle\Service\AuthenticationService;
use Surfnet\GsspBundle\Service\StateHandlerInterface;
use Surfnet\Tiqr\Attribute\RequiresActiveSession;
use Surfnet\Tiqr\Tiqr\TiqrServiceInterface;
use Surfnet\Tiqr\WithContextLogger;
use Symfony\Component\HttpFoundation\JsonResponse;
Expand All @@ -44,6 +45,7 @@ public function __construct(
* @throws InvalidArgumentException
*/
#[Route(path: '/authentication/status', name: 'app_identity_authentication_status', methods: ['GET'])]
#[RequiresActiveSession]
public function __invoke(): JsonResponse
{
try {
Expand All @@ -57,7 +59,6 @@ public function __invoke(): JsonResponse
return $this->refreshAuthenticationPage();
}


$isAuthenticated = $this->tiqrService->isAuthenticated();

if ($isAuthenticated) {
Expand Down
5 changes: 3 additions & 2 deletions src/Controller/RegistrationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use Psr\Log\LoggerInterface;
use Surfnet\GsspBundle\Service\RegistrationService;
use Surfnet\GsspBundle\Service\StateHandlerInterface;
use Surfnet\Tiqr\Attribute\RequiresActiveSession;
use Surfnet\Tiqr\Exception\NoActiveAuthenrequestException;
use Surfnet\Tiqr\Tiqr\Legacy\TiqrService;
use Surfnet\Tiqr\Tiqr\TiqrServiceInterface;
Expand Down Expand Up @@ -90,9 +91,8 @@ public function registration(Request $request): Response
*
*
* @throws \InvalidArgumentException
*
* Requires session cookie to be set to a valid session.
*/
#[RequiresActiveSession]
#[Route(path: '/registration/status', name: 'app_identity_registration_status', methods: ['GET'])]
public function registrationStatus() : Response
{
Expand Down Expand Up @@ -123,6 +123,7 @@ public function registrationStatus() : Response
*
* @throws \InvalidArgumentException
*/
#[RequiresActiveSession]
#[Route(path: '/registration/qr/{enrollmentKey}', name: 'app_identity_registration_qr', methods: ['GET'])]
public function registrationQr(Request $request, string $enrollmentKey): Response
{
Expand Down
93 changes: 93 additions & 0 deletions src/EventSubscriber/RequiresActiveSessionAttributeListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

/**
* Copyright 2024 SURFnet B.V.
*
* 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.
*/

declare(strict_types = 1);

namespace Surfnet\Tiqr\EventSubscriber;

use Psr\Log\LoggerInterface;
use RuntimeException;
use Surfnet\Tiqr\Attribute\RequiresActiveSession;
use Surfnet\Tiqr\Service\SessionCorrelationIdService;
use Surfnet\Tiqr\WithContextLogger;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException;
use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use function is_array;

/**
* This listener acts when the given route has a #[RequiresActiveSession] attribute.
* When a route is marked as to have a required active session this listener will deny access when there is none.
*/
final readonly class RequiresActiveSessionAttributeListener implements EventSubscriberInterface
{
private string $sessionName;

public function __construct(

Check failure on line 43 in src/EventSubscriber/RequiresActiveSessionAttributeListener.php

View workflow job for this annotation

GitHub Actions / run-qa-tests

Method Surfnet\Tiqr\EventSubscriber\RequiresActiveSessionAttributeListener::__construct() has parameter $sessionOptions with no value type specified in iterable type array.
private LoggerInterface $logger,
private SessionCorrelationIdService $sessionCorrelationIdService,
private array $sessionOptions,
) {
if (!array_key_exists('name', $this->sessionOptions)) {
throw new RuntimeException(
'The session name (PHP session cookie identifier) could not be found in the session configuration.'
);
}
$this->sessionName = $this->sessionOptions['name'];
}

public function onKernelControllerArguments(ControllerArgumentsEvent $event): void
{
if (!is_array($event->getAttributes()[RequiresActiveSession::class] ?? null)) {
return;
}

$logger = WithContextLogger::from($this->logger, [
'correlationId' => $this->sessionCorrelationIdService->generateCorrelationId() ?? '',
'route' => $event->getRequest()->getRequestUri(),
]);

try {
$sessionId = $event->getRequest()->getSession()->getId();
$sessionCookieId = $event->getRequest()->cookies->get($this->sessionName);

if (!$sessionCookieId) {
$logger->error('Route requires active session. Active session wasn\'t found. No session cookie was set.');

throw new AccessDeniedException();
}

if ($sessionId !== $sessionCookieId) {
$logger->error('Route requires active session. Session does not match session cookie.');

throw new AccessDeniedException();
}
} catch (SessionNotFoundException) {
$logger->error('Route requires active session. Active session wasn\'t found.');

throw new AccessDeniedException();
}
}

public static function getSubscribedEvents(): array
{
return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 20]];
}
}
87 changes: 87 additions & 0 deletions src/EventSubscriber/SessionStateListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
<?php

/**
* Copyright 2024 SURFnet B.V.
*
* 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.
*/

declare(strict_types = 1);

namespace Surfnet\Tiqr\EventSubscriber;

use Psr\Log\LoggerInterface;
use RuntimeException;
use Surfnet\Tiqr\Service\SessionCorrelationIdService;
use Surfnet\Tiqr\WithContextLogger;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
* Listen to all incoming requests and log the session state information.
*/
final readonly class SessionStateListener implements EventSubscriberInterface
{
private string $sessionName;

public function __construct(

Check failure on line 39 in src/EventSubscriber/SessionStateListener.php

View workflow job for this annotation

GitHub Actions / run-qa-tests

Method Surfnet\Tiqr\EventSubscriber\SessionStateListener::__construct() has parameter $sessionOptions with no value type specified in iterable type array.
private LoggerInterface $logger,
private SessionCorrelationIdService $sessionCorrelationIdService,
private array $sessionOptions,
) {
if (!array_key_exists('name', $this->sessionOptions)) {
throw new RuntimeException(
'The session name (PHP session cookie identifier) could not be found in the session configuration.'
);
}
$this->sessionName = $this->sessionOptions['name'];
}

public function onKernelRequest(RequestEvent $event): void
{
$logger = WithContextLogger::from($this->logger, [
'correlationId' => $this->sessionCorrelationIdService->generateCorrelationId() ?? '',
'route' => $event->getRequest()->getRequestUri(),
]);

$sessionCookieId = $event->getRequest()->cookies->get($this->sessionName);
if ($sessionCookieId === null) {
$logger->info('User made a request without a session cookie.');
return;
}

$logger->info('User made a request with a session cookie.');

try {
$sessionId = $event->getRequest()->getSession()->getId();
$logger->info('User has a session.');

if ($sessionId !== $sessionCookieId) {
$logger->error('The session cookie does not match the session id.');
return;
}
} catch (SessionNotFoundException) {
$logger->info('Session not found.');
return;
}

$logger->info('User session matches the session cookie.');
}

public static function getSubscribedEvents(): array
{
return [KernelEvents::REQUEST => ['onKernelRequest', 20]];
}
}
57 changes: 57 additions & 0 deletions src/Service/SessionCorrelationIdService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

/**
* Copyright 2024 SURFnet B.V.
*
* 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.
*/

declare(strict_types = 1);

namespace Surfnet\Tiqr\Service;

use RuntimeException;
use Symfony\Component\HttpFoundation\RequestStack;

final readonly class SessionCorrelationIdService
{
private string $sessionName;

public function __construct(

Check failure on line 30 in src/Service/SessionCorrelationIdService.php

View workflow job for this annotation

GitHub Actions / run-qa-tests

Method Surfnet\Tiqr\Service\SessionCorrelationIdService::__construct() has parameter $sessionOptions with no value type specified in iterable type array.
private RequestStack $requestStack,
private array $sessionOptions,
private string $sessionCorrelationSalt,
) {
if (!array_key_exists('name', $this->sessionOptions)) {
throw new RuntimeException(
'The session name (PHP session cookie identifier) could not be found in the session configuration.'
);
}
if (empty($this->sessionCorrelationSalt)) {
throw new RuntimeException('Please configure a non empty session correlation salt.');
}

$this->sessionName = $this->sessionOptions['name'];
}

public function generateCorrelationId(): ?string
{
$sessionCookie = $this->requestStack->getMainRequest()?->cookies->get($this->sessionName);

if ($sessionCookie === null) {
return null;
}

return hash('sha256', $this->sessionCorrelationSalt . substr($sessionCookie, 0, 10));
}
}
Loading

0 comments on commit ff9742a

Please sign in to comment.