diff --git a/.env b/.env index c2f125d..0ea63dc 100644 --- a/.env +++ b/.env @@ -49,6 +49,11 @@ MAILING_MAILER_DSN=null://null APP_BASE_URI=https://myep-team.ddev.site +# Regex matched against the Host header; requests for any other host are refused. The +# OAuth2 redirect_uri is generated from the request, so this is what pins it. Production +# sets its own value outside the repository, like APP_BASE_URI above. +APP_TRUSTED_HOSTS=^myep-team\.ddev\.site$ + APP_BPN_USER= APP_BPN_PASSWORD= APP_BPN_IP= diff --git a/.env.test b/.env.test index 9e7162f..e4a4168 100644 --- a/.env.test +++ b/.env.test @@ -4,3 +4,5 @@ APP_SECRET='$ecretf0rt3st' SYMFONY_DEPRECATIONS_HELPER=999999 PANTHER_APP_ENV=panther PANTHER_ERROR_SCREENSHOT_DIR=./var/error-screenshots +# the test client requests http://localhost/ +APP_TRUSTED_HOSTS='^localhost$' diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index e56c29c..72722c1 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -5,6 +5,11 @@ framework: http_method_override: false handle_all_throwables: true + # The OAuth2 redirect_uri handed to MyE&P is generated from the incoming request, so an + # unvalidated Host header would let a crafted request point the authorization code + # somewhere else. Regex patterns, matched against the host without the scheme or port. + trusted_hosts: ['%env(APP_TRUSTED_HOSTS)%'] + # Enables session support. Note that the session will ONLY be started if you read or write from it. # Remove or comment this section to explicitly disable session support. session: diff --git a/src/Controller/Security/OAuth2Controller.php b/src/Controller/Security/OAuth2Controller.php index 07aa543..c1aade7 100644 --- a/src/Controller/Security/OAuth2Controller.php +++ b/src/Controller/Security/OAuth2Controller.php @@ -22,10 +22,9 @@ class OAuth2Controller extends AbstractController { $this->denyUnlessFeatureIsActive(); - $provider = $this->client->getProvider(); - $url = $provider->getAuthorizationUrl(); - $state = $provider->getState(); - $request->getSession()->set('oauth2state', $state); + // the state and the PKCE verifier belong to the client, which is what consumes + // them again on the callback + $url = $this->client->createAuthorizationUrl($request); return $this->redirect($url); } diff --git a/src/Security/MyEpAuthenticator.php b/src/Security/MyEpAuthenticator.php index 6d1109e..8d139e8 100644 --- a/src/Security/MyEpAuthenticator.php +++ b/src/Security/MyEpAuthenticator.php @@ -6,6 +6,7 @@ use App\BusProNet\UserDataHandler; use App\Entity\Teamer; use App\Entity\User; use App\RequiredTeamerCheck\RequiredTeamerCheckRegistry; +use App\Security\OAuth2\AuthorizationDeniedException; use App\Security\OAuth2\AuthorizationRequestException; use App\Security\OAuth2\MyEpClient; use Doctrine\ORM\EntityManagerInterface; @@ -34,6 +35,12 @@ class MyEpAuthenticator extends AbstractAuthenticator * The roles that entitle someone to log in here at all. Anything else MyE&P reports * is dropped rather than stored, so that no role this application assigns a meaning * to can be set from the outside. + * + * This list exists a second time on MyE&P, as the required_roles of this application's + * oauth2_client row: MyE&P refuses the authorization request outright when the account + * holds none of them, and explains why on its own page. The two are one policy written + * twice and must be changed together — widening only one either strands a user at MyE&P + * with no explanation this side can give, or lets one through to be refused here. */ private const ELIGIBLE_ROLES = ['ROLE_TEAM_ADMIN', 'ROLE_TEAMER', 'ROLE_MANAGER', 'ROLE_HOUSE_MANAGER']; @@ -63,11 +70,18 @@ class MyEpAuthenticator extends AbstractAuthenticator { try { $accessToken = $this->client->fetchAccessToken($request); + } catch (AuthorizationDeniedException $e) { + // MyE&P said why it sent no code, so this is not a failure to report as one + $this->logger->info('Login via MyE&P was denied', [ + 'error' => $e->getError(), + 'error_description' => $e->getErrorDescription(), + ]); + throw new CustomUserMessageAuthenticationException('Login via MyE&P wurde abgebrochen'); } catch (AuthorizationRequestException|IdentityProviderException $e) { $this->logger->error('Login via MyE&P failed due to unobtainable access token', [ 'exception' => $e, ]); - throw new CustomUserMessageAuthenticationException('Invalid token'); + throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich'); } try { diff --git a/src/Security/OAuth2/AuthorizationDeniedException.php b/src/Security/OAuth2/AuthorizationDeniedException.php new file mode 100644 index 0000000..923bad7 --- /dev/null +++ b/src/Security/OAuth2/AuthorizationDeniedException.php @@ -0,0 +1,34 @@ +error; + } + + public function getErrorDescription(): ?string + { + return $this->errorDescription; + } +} diff --git a/src/Security/OAuth2/MyEpClient.php b/src/Security/OAuth2/MyEpClient.php index 4b60911..fa6cd39 100644 --- a/src/Security/OAuth2/MyEpClient.php +++ b/src/Security/OAuth2/MyEpClient.php @@ -13,41 +13,104 @@ use Symfony\Component\Routing\Generator\UrlGeneratorInterface; class MyEpClient { + private const SESSION_KEY_STATE = 'oauth2state'; + private const SESSION_KEY_PKCE = 'oauth2pkce'; + private array $config; public function __construct( private readonly UrlGeneratorInterface $urlGenerator, private readonly LoggerInterface $logger, + // the client secret rides in here, marked sensitive so it is redacted from stack + // traces rather than printed in full on an exception page or in a log + #[\SensitiveParameter] array $options, ) { $this->config = $this->resolveConfig($options); } + /** + * Starts the authorization code flow and stores what the callback has to prove. + * + * The state and the PKCE verifier are generated by getAuthorizationUrl() and are only + * readable afterwards, so both are taken from the provider rather than built here. + */ + public function createAuthorizationUrl(Request $request): string + { + $provider = $this->getProvider(); + $url = $provider->getAuthorizationUrl(); + + $session = $request->getSession(); + $session->set(self::SESSION_KEY_STATE, $provider->getState()); + $session->set(self::SESSION_KEY_PKCE, $provider->getPkceCode()); + + return $url; + } + /** * @throws IdentityProviderException + * @throws AuthorizationDeniedException * @throws AuthorizationRequestException */ public function fetchAccessToken(Request $request): AccessTokenInterface { + $session = $request->getSession(); + + // Both are single-use, so they are consumed before anything can fail: a callback + // that throws must not leave a state behind that a second attempt could replay. + $expectedState = $session->remove(self::SESSION_KEY_STATE); + $pkceCode = $session->remove(self::SESSION_KEY_PKCE); + + // Before anything else: MyE&P has said why it is not sending a code, and every + // later check would report the wrong cause. The role gate renders on MyE&P rather + // than redirecting, but a cancelled or refused authorization arrives here. + $error = $request->query->get('error'); + + if (true === is_string($error) && '' !== $error) { + $description = $request->query->get('error_description'); + $description = is_string($description) && '' !== $description ? $description : null; + + $this->logger->error('OAuth2 login request denied', [ + 'error' => $error, + 'error_description' => $description, + ]); + + throw new AuthorizationDeniedException($error, $description, $request); + } + if (null === $code = $request->query->get('code')) { $this->logger->error('OAuth2 login request missing code'); throw new AuthorizationRequestException('Missing code', 400, $request); } - $session = $request->getSession(); + $state = $request->query->get('state'); + // hash_equals() rather than !==, the state being the CSRF secret of the flow. The + // string checks come first: a callback with no state, or a session that never held + // one, must fail here rather than reach a comparison with null. if ( - null === $request->query->get('state') - || $request->query->get('state') !== $session->get('oauth2state') + false === is_string($state) + || false === is_string($expectedState) + || false === hash_equals($expectedState, $state) ) { - $session->remove('oauth2state'); $this->logger->error('OAuth2 login request missing state or mismatch'); throw new AuthorizationRequestException('Missing state or mismatch', 400, $request); } - $session->remove('oauth2state'); + // A challenge went out with the authorization request, so MyE&P rejects an exchange + // without the verifier. Refused here instead, where the reason is still known — the + // usual cause is a session replaced between the two legs of the flow. + if (false === is_string($pkceCode) || '' === $pkceCode) { + $this->logger->error('OAuth2 login request without the PKCE verifier of its session'); + throw new AuthorizationRequestException('Missing PKCE verifier', 400, $request); + } - return $this->getProvider()->getAccessToken('authorization_code', [ + $provider = $this->getProvider(); + + // replays the verifier whose challenge was sent with the authorization request + $provider->setPkceCode($pkceCode); + + return $provider->getAccessToken('authorization_code', [ 'code' => $code, ]); } @@ -68,6 +131,10 @@ class MyEpClient 'urlResourceOwnerDetails' => $this->config['myep_oauth2_url_resource_owner_details'], 'scopes' => $this->config['myep_oauth2_scopes'], 'scopeSeparator' => ' ', + // The redirect URI above is generated from the incoming request, so it is only + // as trustworthy as the Host header. PKCE binds the authorization code to this + // flow by a second, header-independent means. + 'pkceMethod' => AbstractProvider::PKCE_METHOD_S256, ]); } diff --git a/tests/Security/OAuth2/MyEpClientTest.php b/tests/Security/OAuth2/MyEpClientTest.php new file mode 100644 index 0000000..350f6b8 --- /dev/null +++ b/tests/Security/OAuth2/MyEpClientTest.php @@ -0,0 +1,147 @@ +createRequest(); + + $url = $this->createClient()->createAuthorizationUrl($request); + + parse_str((string) parse_url($url, \PHP_URL_QUERY), $query); + + $this->assertSame('S256', $query['code_challenge_method']); + + $session = $request->getSession(); + $verifier = $session->get('oauth2pkce'); + + $this->assertIsString($verifier); + $this->assertSame($session->get('oauth2state'), $query['state']); + + // the challenge is what MyE&P stores; the verifier replayed on the token request + // has to hash to it, or the exchange is refused + $expectedChallenge = trim(strtr(base64_encode(hash('sha256', $verifier, true)), '+/', '-_'), '='); + $this->assertSame($expectedChallenge, $query['code_challenge']); + } + + public function testDeniedCallbackIsReportedAsADenialAndConsumesTheStoredState(): void + { + $request = $this->createRequest([ + 'state' => 'expected-state', + 'error' => 'access_denied', + 'error_description' => 'The user denied the request', + ]); + $request->getSession()->set('oauth2state', 'expected-state'); + $request->getSession()->set('oauth2pkce', 'verifier'); + + try { + $this->createClient()->fetchAccessToken($request); + $this->fail('Expected an AuthorizationDeniedException'); + } catch (AuthorizationDeniedException $e) { + // MyE&P has named the cause; reporting this as a missing code would not + $this->assertSame('access_denied', $e->getError()); + $this->assertSame('The user denied the request', $e->getErrorDescription()); + } + + $this->assertFalse($request->getSession()->has('oauth2state')); + $this->assertFalse($request->getSession()->has('oauth2pkce')); + } + + public function testCallbackIsRefusedWhenTheSessionHoldsNoVerifier(): void + { + $request = $this->createRequest(['code' => 'a-code', 'state' => 'expected-state']); + $request->getSession()->set('oauth2state', 'expected-state'); + + $this->expectException(AuthorizationRequestException::class); + $this->expectExceptionMessage('Missing PKCE verifier'); + + $this->createClient()->fetchAccessToken($request); + } + + public function testCallbackWithoutCodeConsumesTheStoredState(): void + { + $request = $this->createRequest(['state' => 'expected-state']); + $request->getSession()->set('oauth2state', 'expected-state'); + $request->getSession()->set('oauth2pkce', 'verifier'); + + try { + $this->createClient()->fetchAccessToken($request); + $this->fail('Expected an AuthorizationRequestException'); + } catch (AuthorizationRequestException $e) { + $this->assertSame('Missing code', $e->getMessage()); + } + + // the state is single-use: a callback that fails must not leave one behind that a + // later attempt could still replay + $this->assertFalse($request->getSession()->has('oauth2state')); + $this->assertFalse($request->getSession()->has('oauth2pkce')); + } + + public function testCallbackWithAMismatchedStateConsumesTheStoredState(): void + { + $request = $this->createRequest(['code' => 'a-code', 'state' => 'other-state']); + $request->getSession()->set('oauth2state', 'expected-state'); + $request->getSession()->set('oauth2pkce', 'verifier'); + + $this->expectException(AuthorizationRequestException::class); + $this->expectExceptionMessage('Missing state or mismatch'); + + try { + $this->createClient()->fetchAccessToken($request); + } finally { + $this->assertFalse($request->getSession()->has('oauth2state')); + $this->assertFalse($request->getSession()->has('oauth2pkce')); + } + } + + public function testCallbackIsRefusedWhenTheSessionHoldsNoState(): void + { + $request = $this->createRequest(['code' => 'a-code', 'state' => 'any-state']); + + $this->expectException(AuthorizationRequestException::class); + $this->expectExceptionMessage('Missing state or mismatch'); + + $this->createClient()->fetchAccessToken($request); + } + + private function createClient(): MyEpClient + { + $urlGenerator = $this->createMock(UrlGeneratorInterface::class); + $urlGenerator + ->method('generate') + ->willReturn('https://myep-team.example.org/myep-auth/check') + ; + + return new MyEpClient($urlGenerator, $this->createMock(LoggerInterface::class), [ + 'myep_oauth2_client_id' => 'client-id', + 'myep_oauth2_client_secret' => 'client-secret', + 'myep_oauth2_url_authorize' => 'https://my.example.org/authorize', + 'myep_oauth2_url_access_token' => 'https://my.example.org/token', + 'myep_oauth2_url_resource_owner_details' => 'https://my.example.org/api/userinfo', + 'myep_oauth2_scopes' => ['email', 'id', 'roles', 'profile'], + ]); + } + + /** + * @param array $query + */ + private function createRequest(array $query = []): Request + { + $request = new Request($query); + $request->setSession(new Session(new MockArraySessionStorage())); + + return $request; + } +}