From 59b1fc68f371970f50e0e318ca65b845652742a8 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Tue, 23 Apr 2024 13:45:23 +0200 Subject: [PATCH] chore: improve testing and override fetch url to use Guzzle (#21) * chore: improve testing * Update description of getRoute function * Move descriptions to docblock * Add additional test for nonce, state and code challenge * Cleanup * Override fetchUrl and add test for id token logic * Generate JWT in test * Run linter * Improve test with signed id token * Reformat headers * Add additional request tests * Add some description * Use tls_verify config option for ca path --- config/oidc.php | 10 +- .../OpenIDConfigurationLoader.php | 8 +- src/OpenIDConnectClient.php | 104 ++++++- src/OpenIDConnectServiceProvider.php | 11 +- .../LoginControllerResponseTest.php | 265 ++++++++++++++++++ tests/TestFunctions.php | 18 ++ 6 files changed, 402 insertions(+), 14 deletions(-) create mode 100644 tests/Feature/Http/Controllers/LoginControllerResponseTest.php diff --git a/config/oidc.php b/config/oidc.php index 4e6c9c3..12cb5b5 100644 --- a/config/oidc.php +++ b/config/oidc.php @@ -81,9 +81,13 @@ ], /** - * TLS Verify - * Can be disabled for local development. - * Is used in OpenIDConfigurationLoader and in the ServiceProvider for OpenIDConnectClient. + * TLS Verify - Used as the verify option for Guzzle. + * + * Default is true and verifies the certificate and uses the default CA bundle of the system. + * When set to `false` it disables the certificate verification (this is insecure!). + * When set to a path of a CA bundle, the custom certificate is used. + * + * @link https://docs.guzzlephp.org/en/latest/request-options.html#verify */ 'tls_verify' => env('OIDC_TLS_VERIFY', true), ]; diff --git a/src/OpenIDConfiguration/OpenIDConfigurationLoader.php b/src/OpenIDConfiguration/OpenIDConfigurationLoader.php index 0cc75be..d24a2fe 100644 --- a/src/OpenIDConfiguration/OpenIDConfigurationLoader.php +++ b/src/OpenIDConfiguration/OpenIDConfigurationLoader.php @@ -13,7 +13,7 @@ public function __construct( protected string $issuer, protected ?Repository $cacheStore = null, protected int $cacheTtl = 3600, - protected bool $tlsVerify = true, + protected bool|string $tlsVerify = true, ) { } @@ -39,9 +39,9 @@ protected function getConfigurationFromIssuer(): OpenIDConfiguration $url = $this->getOpenIDConfigurationUrl(); $response = Http::baseUrl($this->issuer) - ->when(!$this->tlsVerify, function ($pendingRequest) { - return $pendingRequest->withoutVerifying(); - }) + ->withOptions([ + 'verify' => $this->tlsVerify + ]) ->get($url); if (!$response->successful()) { diff --git a/src/OpenIDConnectClient.php b/src/OpenIDConnectClient.php index 9ea02f2..856117a 100644 --- a/src/OpenIDConnectClient.php +++ b/src/OpenIDConnectClient.php @@ -6,6 +6,7 @@ use Illuminate\Http\Exceptions\HttpResponseException; use Illuminate\Http\RedirectResponse; +use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Session; use Illuminate\Support\Str; use Jumbojett\OpenIDConnectClientException; @@ -19,6 +20,15 @@ class OpenIDConnectClient extends \Jumbojett\OpenIDConnectClient { protected ?JweDecryptInterface $jweDecrypter; protected ?OpenIDConfiguration $openIDConfiguration; + /** + * @var int|null Response code from the server + */ + protected ?int $responseCode; + + /** + * @var string|null Content type from the server + */ + private ?string $responseContentType; public function __construct( ?string $providerUrl = null, @@ -95,7 +105,6 @@ protected function handleJweResponse($jwe): string * @param string|string[]|bool|null $default optional * @throws OpenIDConnectClientException * @return string|string[]|bool - * @psalm-suppress ImplementedReturnTypeMismatch */ protected function getWellKnownConfigValue($param, $default = null): string|array|bool { @@ -156,4 +165,97 @@ protected function getAuthorizationEndpoint(): string return $authorizationEndpoint; } + + /** + * Override the fetchURL method to use Laravel HTTP client. + * This uses Guzzle in the background. + * + * @param string $url + * @param string | null $post_body string If this is set the post type will be POST + * @param array $headers Extra headers to be sent with the request. + * @return string + * @throws OpenIDConnectClientException + */ + protected function fetchURL(string $url, string $post_body = null, array $headers = []): string + { + $pendingRequest = Http::withUserAgent($this->getUserAgent()) + ->timeout($this->timeOut) + ->withOptions([ + 'verify' => $this->getCertPath() ?: $this->getVerifyPeer() + ]); + + if (count($headers) > 0) { + $pendingRequest->withHeaders($this->reformatHeaders($headers)); + } + + if ($post_body === null) { + $request = $pendingRequest->get($url); + } else { + $isJson = is_object(json_decode($post_body, false)); + + $request = $pendingRequest + ->withBody($post_body, $isJson ? 'application/json' : 'application/x-www-form-urlencoded') + ->post($url); + } + + $this->responseCode = $request->status(); + $this->responseContentType = $request->header('Content-Type'); + + if ($request->failed()) { + throw new OpenIDConnectClientException( + 'Request failed with status code ' . $this->responseCode . ' ' . $request->reason() + ); + } + + return $request->body(); + } + + /** + * Get the response code from last action/curl request. + * + * @return int + */ + public function getResponseCode(): int + { + return $this->responseCode ?? 0; + } + + /** + * Get the content type from last action/curl request. + * + * @return string|null + */ + public function getResponseContentType(): ?string + { + return $this->responseContentType; + } + + public function setTlsVerify(bool|string $tlsVerify): void + { + $verify = (bool)$tlsVerify; + $this->setVerifyHost($verify); + $this->setVerifyPeer($verify); + + if (is_string($tlsVerify)) { + $this->setCertPath($tlsVerify); + } + } + + /** + * Reformat the headers from string to array for Guzzle. + * + * @param array $headers + * @return array> + */ + protected function reformatHeaders(array $headers): array + { + $result = []; + + foreach ($headers as $header) { + [$key, $value] = explode(": ", $header, 2); + $result[$key] = [$value]; + } + + return $result; + } } diff --git a/src/OpenIDConnectServiceProvider.php b/src/OpenIDConnectServiceProvider.php index 286b6cd..3c9acda 100644 --- a/src/OpenIDConnectServiceProvider.php +++ b/src/OpenIDConnectServiceProvider.php @@ -87,7 +87,7 @@ protected function registerConfigurationLoader(): void $app['config']->get('oidc.issuer'), $app['cache']->store($app['config']->get('oidc.configuration_cache.store')), $app['config']->get('oidc.configuration_cache.ttl'), - $app['config']->get('oidc.tls_verify') === true, + $app['config']->get('oidc.tls_verify'), ); }); } @@ -104,7 +104,9 @@ protected function registerClient(): void if (!empty($app['config']->get('oidc.client_secret'))) { $oidc->setClientSecret($app['config']->get('oidc.client_secret')); } - $oidc->setCodeChallengeMethod($app['config']->get('oidc.code_challenge_method')); + if (!empty($app['config']->get('oidc.code_challenge_method'))) { + $oidc->setCodeChallengeMethod($app['config']->get('oidc.code_challenge_method')); + } $oidc->setRedirectURL($app['url']->route('oidc.login')); $additionalScopes = $app['config']->get('oidc.additional_scopes'); @@ -112,10 +114,7 @@ protected function registerClient(): void $oidc->addScope($additionalScopes); } - if ($app['config']->get('oidc.tls_verify') !== true) { - $oidc->setVerifyHost(false); - $oidc->setVerifyPeer(false); - } + $oidc->setTlsVerify($app['config']->get('oidc.tls_verify')); return $oidc; }); } diff --git a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php new file mode 100644 index 0000000..5fde09c --- /dev/null +++ b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php @@ -0,0 +1,265 @@ +mockOpenIDConfigurationLoader(); + + $response = $this->getRoute('oidc.login', ['error' => 'test-error']); + + $response + ->assertStatus(400); + } + + public function testNonceAndStateAreSetInCache(): void + { + $this->mockOpenIDConfigurationLoader(); + + // Check if nonce and state are not set in cache. + $this->assertNull(session('openid_connect_nonce')); + $this->assertNull(session('openid_connect_state')); + + // Make request to login route. + $response = $this->getRoute('oidc.login'); + + // Now the nonce and state should be set and should be in the url + $nonce = session('openid_connect_nonce'); + $state = session('openid_connect_state'); + + $response + ->assertStatus(302) + ->assertRedirectContains("https://provider.rdobeheer.nl/authorize") + ->assertRedirectContains('response_type=code') + ->assertRedirectContains('redirect_uri=http%3A%2F%2Flocalhost%2Foidc%2Flogin') + ->assertRedirectContains('client_id=test-client-id') + ->assertRedirectContains('nonce=' . $nonce) + ->assertRedirectContains('state=' . $state) + ->assertRedirectContains('client_id=test-client-id'); + } + + /** + * @dataProvider codeChallengeMethodProvider + */ + public function testCodeChallengeIsSetWhenSupported( + ?string $requestedCodeChallengeMethod, + array $codeChallengesSupportedAtProvider, + bool $codeChallengeShouldBeSet, + ): void { + $this->mockOpenIDConfigurationLoader($codeChallengesSupportedAtProvider); + Config::set('oidc.code_challenge_method', $requestedCodeChallengeMethod); + + // Check if code verified is not set in cache. + $this->assertNull(session('openid_connect_code_verifier')); + + // Make request to login route. + $response = $this->getRoute('oidc.login'); + + $response + ->assertStatus(302) + ->assertRedirectContains("https://provider.rdobeheer.nl/authorize"); + + if ($codeChallengeShouldBeSet) { + $response + ->assertRedirectContains('code_challenge_method=' . $requestedCodeChallengeMethod) + ->assertRedirectContains('code_challenge=') + ->assertSessionHas('openid_connect_code_verifier'); + } else { + $response + ->assertSessionMissing('openid_connect_code_verifier'); + } + } + + public function codeChallengeMethodProvider(): array + { + return [ + 'no code challenge method requested' => [null, [], false], + 'code challenge method requested but not supported' => ['S256', [], false], + 'code challenge method requested and supported' => ['S256', ['S256'], true], + 'code challenge method requested and supported plain' => ['plain', ['plain'], true], + ]; + } + + public function testTokenSignedWithClientSecret(): void + { + $idToken = generateJwt([ + "iss" => "https://provider.rdobeheer.nl", + "aud" => 'test-client-id', + ], 'the-secret-client-secret'); + + Http::fake([ + // Token requested by OpenIDConnectClient::authenticate() function. + 'https://provider.rdobeheer.nl/token' => Http::response([ + 'access_token' => 'access-token-from-token-endpoint', + 'id_token' => $idToken, + 'token_type' => 'Bearer', + 'expires_in' => 3600, + ]), + // User info requested by OpenIDConnectClient::requestUserInfo() function. + 'https://provider.rdobeheer.nl/userinfo?schema=openid' => Http::response([ + 'email' => 'teste@rdobeheer.nl', + ]), + ]); + + // Set OIDC config + $this->mockOpenIDConfigurationLoader(); + + Config::set('oidc.issuer', 'https://provider.rdobeheer.nl'); + Config::set('oidc.client_id', 'test-client-id'); + Config::set('oidc.client_secret', 'the-secret-client-secret'); + + // Set current state, normally this is generated before logging in and send + // to the issuer, when the user is redirected for login. + Session::put('openid_connect_state', 'some-state'); + + // We simulate here that the user now comes back after successful login at issuer. + $response = $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'some-state']); + $response->assertStatus(200); + $response->assertJson([ + 'userInfo' => [ + 'email' => 'teste@rdobeheer.nl', + ] + ]); + + $this->assertEmpty(session('openid_connect_state')); + $this->assertEmpty(session('openid_connect_nonce')); + + Http::assertSentCount(2); + Http::assertSentInOrder([ + 'https://provider.rdobeheer.nl/token', + 'https://provider.rdobeheer.nl/userinfo?schema=openid', + ]); + Http::assertSent(function (Request $request) { + if ($request->url() === 'https://provider.rdobeheer.nl/token') { + $this->assertSame( + expected: 'POST', + actual: $request->method(), + ); + $this->assertSame( + expected: 'grant_type=authorization_code' + . '&code=some-code' + . '&redirect_uri=http%3A%2F%2Flocalhost%2Foidc%2Flogin' + . '&client_id=test-client-id' + . '&client_secret=the-secret-client-secret', + actual: $request->body(), + ); + return true; + } + + if ($request->url() === 'https://provider.rdobeheer.nl/userinfo?schema=openid') { + $this->assertSame( + expected: 'GET', + actual: $request->method(), + ); + $this->assertSame( + expected: [ + 'Bearer access-token-from-token-endpoint' + ], + actual: $request->header('Authorization'), + ); + } + + return true; + }); + } + + protected function mockOpenIDConfigurationLoader(array $codeChallengeMethodsSupported = []): void + { + $mock = Mockery::mock(OpenIDConfigurationLoader::class); + $mock + ->shouldReceive('getConfiguration') + ->andReturn($this->exampleOpenIDConfiguration($codeChallengeMethodsSupported)); + + $this->app->instance(OpenIDConfigurationLoader::class, $mock); + } + + protected function exampleOpenIDConfiguration(array $codeChallengeMethodsSupported = []): OpenIDConfiguration + { + return new OpenIDConfiguration( + version: "3.0", + tokenEndpointAuthMethodsSupported: ["none"], + claimsParameterSupported: true, + requestParameterSupported: false, + requestUriParameterSupported: true, + requireRequestUriRegistration: false, + grantTypesSupported: ["authorization_code"], + frontchannelLogoutSupported: false, + frontchannelLogoutSessionSupported: false, + backchannelLogoutSupported: false, + backchannelLogoutSessionSupported: false, + issuer: "https://provider.rdobeheer.nl", + authorizationEndpoint: "https://provider.rdobeheer.nl/authorize", + jwksUri: "https://provider.rdobeheer.nl/jwks", + tokenEndpoint: "https://provider.rdobeheer.nl/token", + scopesSupported: ["openid"], + responseTypesSupported: ["code"], + responseModesSupported: ["query"], + subjectTypesSupported: ["pairwise"], + idTokenSigningAlgValuesSupported: ["RS256"], + userinfoEndpoint: "https://provider.rdobeheer.nl/userinfo", + codeChallengeMethodsSupported: $codeChallengeMethodsSupported, + ); + } + + /** + * Override the Laravel GET request to put the query parameters in $_REQUEST. + * This is to test the functionality because the tests in Laravel only sets + * $_POST and $_GET and OpenIDConnectClient uses $_REQUEST. + * + * @param string $routeName + * @param array $queryParams + * @return TestResponse + */ + protected function getRoute(string $routeName, array $queryParams = []): TestResponse + { + $_REQUEST = []; + foreach ($queryParams as $key => $value) { + $_REQUEST[$key] = $value; + } + return $this->get(route($routeName, $queryParams)); + } +} diff --git a/tests/TestFunctions.php b/tests/TestFunctions.php index 19c1bf1..4779a78 100644 --- a/tests/TestFunctions.php +++ b/tests/TestFunctions.php @@ -116,3 +116,21 @@ function getJwkFromResource($resource): JWK return JWKFactory::createFromKeyFile(stream_get_meta_data($resource)['uri']); } + +function generateJwt(array $payload, string $signingKey): string +{ + $header = json_encode(['typ' => 'JWT', 'alg' => 'HS256']); + $payload = json_encode($payload); + + $base64UrlHeader = base64UrlEncode($header); + $base64UrlPayload = base64UrlEncode($payload); + + $signature = hash_hmac('sha256', $base64UrlHeader . "." . $base64UrlPayload, $signingKey, true); + + return $base64UrlHeader . "." . $base64UrlPayload . "." . base64UrlEncode($signature); +} + +function base64UrlEncode(string $data): string +{ + return str_replace(['+', '/', '='], ['-', '_', ''], base64_encode($data)); +}