From de7e02dfc75cce4b98f6e0ab3a18ead96380ff3a Mon Sep 17 00:00:00 2001 From: Sean Fraser Date: Fri, 7 Jun 2024 13:25:15 -0400 Subject: [PATCH] Add support for authorization code grant with PKCE for distributed apps move handlers for code challenge from OIDC Authorization Controller to the base authorization controller, so that non-OIDC flows can leverage it as well Add mechanism to enforce PKCE code challenges for public clients Add mechanism to configure supported code challenge methods change code_verifier comparison to use hash_equals() to avoid timing attacks added tests for all PKCE code challenge flows --- .gitignore | 1 + src/OAuth2/Controller/AuthorizeController.php | 56 +++++++- src/OAuth2/GrantType/AuthorizationCode.php | 2 +- .../OpenID/Controller/AuthorizeController.php | 42 +----- src/OAuth2/Server.php | 4 +- .../Controller/AuthorizeControllerTest.php | 121 ++++++++++++++++++ .../GrantType/AuthorizationCodeTest.php | 105 ++++++++++++++- test/config/storage.json | 30 +++++ 8 files changed, 314 insertions(+), 47 deletions(-) diff --git a/.gitignore b/.gitignore index c43a667d4..96a10370e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ test/config/test.sqlite vendor composer.lock .idea +.phpunit.result.cache diff --git a/src/OAuth2/Controller/AuthorizeController.php b/src/OAuth2/Controller/AuthorizeController.php index b2e12bb5d..36789bd3b 100644 --- a/src/OAuth2/Controller/AuthorizeController.php +++ b/src/OAuth2/Controller/AuthorizeController.php @@ -2,6 +2,7 @@ namespace OAuth2\Controller; +use OAuth2\Storage\ClientCredentialsInterface; use OAuth2\Storage\ClientInterface; use OAuth2\ScopeInterface; use OAuth2\RequestInterface; @@ -61,6 +62,16 @@ class AuthorizeController implements AuthorizeControllerInterface */ protected $scopeUtil; + /** + * @var string|null + */ + protected $code_challenge; + + /** + * @var string|null + */ + protected $code_challenge_method; + /** * Constructor * @@ -88,6 +99,8 @@ public function __construct(ClientInterface $clientStorage, array $responseTypes 'require_exact_redirect_uri' => true, 'redirect_status_code' => 302, 'enforce_pkce' => false, + 'enforce_pkce_for_public_clients' => false, + 'supported_code_challenge_methods' => ['plain', 'S256'], ), $config); if (is_null($scopeUtil)) { @@ -139,7 +152,7 @@ public function handleAuthorizeRequest(RequestInterface $request, ResponseInterf $authResult = $this->responseTypes[$this->response_type]->getAuthorizeResponse($params, $user_id); - list($redirect_uri, $uri_params) = $authResult; + [$redirect_uri, $uri_params] = $authResult; if (empty($redirect_uri) && !empty($registered_redirect_uri)) { $redirect_uri = $registered_redirect_uri; @@ -186,6 +199,8 @@ protected function buildAuthorizeParameters($request, $response, $user_id) 'client_id' => $this->client_id, 'redirect_uri' => $this->redirect_uri, 'response_type' => $this->response_type, + 'code_challenge' => $this->code_challenge, + 'code_challenge_method' => $this->code_challenge_method, ); return $params; @@ -257,7 +272,7 @@ public function validateAuthorizeRequest(RequestInterface $request, ResponseInte $response_type = $request->query('response_type', $request->request('response_type')); // for multiple-valued response types - make them alphabetical - if (false !== strpos($response_type, ' ')) { + if ($response_type && false !== strpos($response_type, ' ')) { $types = explode(' ', $response_type); sort($types); $response_type = ltrim(implode(' ', $types)); @@ -334,6 +349,41 @@ public function validateAuthorizeRequest(RequestInterface $request, ResponseInte return false; } + $code_challenge = $request->query('code_challenge'); + $code_challenge_method = $request->query('code_challenge_method'); + if ($code_challenge) { + if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $code_challenge) !== 1) { + $response->setError(400, 'invalid_code_challenge', 'The PKCE code challenge supplied is invalid'); + + return false; + } + + $supported_challenge_methods = $this->config['supported_code_challenge_methods'] ?? ['plain', 'S256']; + if (!$code_challenge_method) { + $response->setError(400, 'missing_code_challenge_method', 'This application requires you specify a PKCE code challenge method. Supported methods: ' . implode(', ', $supported_challenge_methods) + ); + + return false; + } + if (!in_array($code_challenge_method, $supported_challenge_methods, true)) { + $response->setError(400, 'invalid_code_challenge_method', 'The PKCE code challenge method supplied is invalid. Supported methods: ' . implode(', ', $supported_challenge_methods) + ); + + return false; + } + } else { + $isPublic = false; + if ($this->clientStorage instanceof ClientCredentialsInterface) { + $isPublic = $this->clientStorage->isPublicClient($client_id); + } + + if ($this->config['enforce_pkce'] || ($this->config['enforce_pkce_for_public_clients'] && $isPublic)) { + $response->setError(400, 'missing_code_challenge', 'This application requires you provide a PKCE code challenge'); + + return false; + } + } + // save the input data and return true $this->scope = $requestedScope; $this->state = $state; @@ -341,6 +391,8 @@ public function validateAuthorizeRequest(RequestInterface $request, ResponseInte // Only save the SUPPLIED redirect URI (@see http://tools.ietf.org/html/rfc6749#section-4.1.3) $this->redirect_uri = $supplied_redirect_uri; $this->response_type = $response_type; + $this->code_challenge = $code_challenge; + $this->code_challenge_method = $code_challenge_method; return true; } diff --git a/src/OAuth2/GrantType/AuthorizationCode.php b/src/OAuth2/GrantType/AuthorizationCode.php index 5bcb4f253..6f18ac399 100644 --- a/src/OAuth2/GrantType/AuthorizationCode.php +++ b/src/OAuth2/GrantType/AuthorizationCode.php @@ -112,7 +112,7 @@ public function validateRequest(RequestInterface $request, ResponseInterface $re return FALSE; } - if ($code_verifier_hashed !== $authCode['code_challenge']) { + if (!hash_equals($authCode['code_challenge'], $code_verifier_hashed)) { $response->setError(400, 'code_verifier_mismatch', "The PKCE code verifier parameter does not match the code challenge."); return FALSE; diff --git a/src/OAuth2/OpenID/Controller/AuthorizeController.php b/src/OAuth2/OpenID/Controller/AuthorizeController.php index 52e183bb3..db8a6c2a5 100644 --- a/src/OAuth2/OpenID/Controller/AuthorizeController.php +++ b/src/OAuth2/OpenID/Controller/AuthorizeController.php @@ -7,7 +7,7 @@ use OAuth2\ResponseInterface; /** - * @see OAuth2\Controller\AuthorizeControllerInterface + * @see \OAuth2\Controller\AuthorizeControllerInterface */ class AuthorizeController extends BaseAuthorizeController implements AuthorizeControllerInterface { @@ -16,16 +16,6 @@ class AuthorizeController extends BaseAuthorizeController implements AuthorizeCo */ private $nonce; - /** - * @var mixed - */ - protected $code_challenge; - - /** - * @var mixed - */ - protected $code_challenge_method; - /** * Set not authorized response * @@ -75,10 +65,6 @@ protected function buildAuthorizeParameters($request, $response, $user_id) // add the nonce to return with the redirect URI $params['nonce'] = $this->nonce; - // Add PKCE code challenge. - $params['code_challenge'] = $this->code_challenge; - $params['code_challenge_method'] = $this->code_challenge_method; - return $params; } @@ -104,32 +90,6 @@ public function validateAuthorizeRequest(RequestInterface $request, ResponseInte $this->nonce = $nonce; - $code_challenge = $request->query('code_challenge'); - $code_challenge_method = $request->query('code_challenge_method'); - - if ($this->config['enforce_pkce']) { - if (!$code_challenge) { - $response->setError(400, 'missing_code_challenge', 'This application requires you provide a PKCE code challenge'); - - return false; - } - - if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $code_challenge) !== 1) { - $response->setError(400, 'invalid_code_challenge', 'The PKCE code challenge supplied is invalid'); - - return false; - } - - if (!in_array($code_challenge_method, array('plain', 'S256'), true)) { - $response->setError(400, 'missing_code_challenge_method', 'This application requires you specify a PKCE code challenge method'); - - return false; - } - } - - $this->code_challenge = $code_challenge; - $this->code_challenge_method = $code_challenge_method; - return true; } diff --git a/src/OAuth2/Server.php b/src/OAuth2/Server.php index 1fbc6666d..4da518c8b 100644 --- a/src/OAuth2/Server.php +++ b/src/OAuth2/Server.php @@ -173,6 +173,8 @@ public function __construct($storage = array(), array $config = array(), array $ 'require_exact_redirect_uri' => true, 'allow_implicit' => false, 'enforce_pkce' => false, + 'enforce_pkce_for_public_clients' => false, + 'supported_code_challenge_methods' => array('plain', 'S256'), 'allow_credentials_in_request_body' => true, 'allow_public_clients' => true, 'always_issue_new_refresh_token' => false, @@ -578,7 +580,7 @@ protected function createDefaultAuthorizeController() } } - $config = array_intersect_key($this->config, array_flip(explode(' ', 'allow_implicit enforce_state require_exact_redirect_uri enforce_pkce'))); + $config = array_intersect_key($this->config, array_flip(explode(' ', 'allow_implicit enforce_state require_exact_redirect_uri enforce_pkce enforce_pkce_for_public_clients supported_code_challenge_methods'))); if ($this->config['use_openid_connect']) { return new OpenIDAuthorizeController($this->storages['client'], $this->responseTypes, $config, $this->getScopeUtil()); diff --git a/test/OAuth2/Controller/AuthorizeControllerTest.php b/test/OAuth2/Controller/AuthorizeControllerTest.php index 88f0d0da0..d931b5aa5 100644 --- a/test/OAuth2/Controller/AuthorizeControllerTest.php +++ b/test/OAuth2/Controller/AuthorizeControllerTest.php @@ -342,6 +342,125 @@ public function testUserDeniesAccessResponse() $this->assertEquals($query['error_description'], 'The user denied access to your application'); } + public function testEnforcePkce() + { + $server = $this->getTestServer(array('enforce_pkce' => true)); + $request = new Request(array( + 'client_id' => 'Test Client ID', // valid client id + 'redirect_uri' => 'http://adobe.com', // valid redirect URI + 'response_type' => 'code', + 'state' => 'xyz', + )); + $server->handleAuthorizeRequest($request, $response = new Response(), true); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('missing_code_challenge', $response->getParameter('error')); + $this->assertEquals('This application requires you provide a PKCE code challenge', $response->getParameter('error_description')); + } + + public function testEnforcePkcePublicClient() + { + $server = $this->getTestServer(array('enforce_pkce_for_public_clients' => true)); + $request = new Request(array( + 'client_id' => 'Test Public Client ID', // valid client id + 'redirect_uri' => 'http://adobe.com', // valid redirect URI + 'response_type' => 'code', + 'state' => 'xyz', + )); + $server->handleAuthorizeRequest($request, $response = new Response(), true); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('missing_code_challenge', $response->getParameter('error')); + $this->assertEquals('This application requires you provide a PKCE code challenge', $response->getParameter('error_description')); + } + + public function testInvalidCodeChallenge() + { + $server = $this->getTestServer(array('enforce_pkce' => true)); + $request = new Request(array( + 'client_id' => 'Test Client ID', // valid client id + 'redirect_uri' => 'http://adobe.com', // valid redirect URI + 'response_type' => 'code', + 'state' => 'xyz', + 'code_challenge' => 'invalid!', + 'code_challenge_method' => 'plain', + )); + $server->handleAuthorizeRequest($request, $response = new Response(), true); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('invalid_code_challenge', $response->getParameter('error')); + $this->assertEquals('The PKCE code challenge supplied is invalid', $response->getParameter('error_description')); + } + + public function testMissingCodeChallengeMethod() + { + $server = $this->getTestServer(array('enforce_pkce' => true)); + $request = new Request(array( + 'client_id' => 'Test Client ID', // valid client id + 'redirect_uri' => 'http://adobe.com', // valid redirect URI + 'response_type' => 'code', + 'state' => 'xyz', + 'code_challenge' => 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + )); + $server->handleAuthorizeRequest($request, $response = new Response(), true); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('missing_code_challenge_method', $response->getParameter('error')); + $this->assertEquals('This application requires you specify a PKCE code challenge method. Supported methods: plain, S256', $response->getParameter('error_description')); + } + + public function testInvalidCodeChallengeMethod() + { + $server = $this->getTestServer(array('enforce_pkce' => true)); + $request = new Request(array( + 'client_id' => 'Test Client ID', // valid client id + 'redirect_uri' => 'http://adobe.com', // valid redirect URI + 'response_type' => 'code', + 'state' => 'xyz', + 'code_challenge' => 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + 'code_challenge_method' => 'invalid', + )); + $server->handleAuthorizeRequest($request, $response = new Response(), true); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('invalid_code_challenge_method', $response->getParameter('error')); + $this->assertEquals('The PKCE code challenge method supplied is invalid. Supported methods: plain, S256', $response->getParameter('error_description')); + } + + public function testSuccessfulPkceChallenge() + { + $server = $this->getTestServer(array('enforce_pkce' => true)); + $request = new Request(array( + 'client_id' => 'Test Client ID', // valid client id + 'redirect_uri' => 'http://adobe.com', // valid redirect URI + 'response_type' => 'code', + 'state' => 'xyz', + 'code_challenge' => 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + 'code_challenge_method' => 'S256', + )); + $server->handleAuthorizeRequest($request, $response = new Response(), true); + + $this->assertEquals($response->getStatusCode(), 302); + $location = $response->getHttpHeader('Location'); + $parts = parse_url($location); + parse_str($parts['query'], $query); + + $this->assertEquals('http', $parts['scheme']); // same as passed in to redirect_uri + $this->assertEquals('adobe.com', $parts['host']); // same as passed in to redirect_uri + $this->assertArrayHasKey('query', $parts); + $this->assertFalse(isset($parts['fragment'])); + + // assert fragment is in "application/x-www-form-urlencoded" format + parse_str($parts['query'], $query); + $this->assertNotNull($query); + $this->assertArrayHasKey('code', $query); + + // assert code challenge is stored + $code = $server->getStorage('authorization_code')->getAuthorizationCode($query['code']); + $this->assertEquals($code['code_challenge'], 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'); + $this->assertEquals($code['code_challenge_method'], 'S256'); + } + public function testCodeQueryParamIsSet() { $server = $this->getTestServer(); @@ -478,6 +597,8 @@ public function testCreateController() { $storage = Bootstrap::getInstance()->getMemoryStorage(); $controller = new AuthorizeController($storage); + + $this->expectNotToPerformAssertions(); } private function getTestServer($config = array()) diff --git a/test/OAuth2/GrantType/AuthorizationCodeTest.php b/test/OAuth2/GrantType/AuthorizationCodeTest.php index b2314ffc6..4521b2174 100644 --- a/test/OAuth2/GrantType/AuthorizationCodeTest.php +++ b/test/OAuth2/GrantType/AuthorizationCodeTest.php @@ -213,10 +213,111 @@ public function testValidClientDifferentCode() $this->assertEquals($response->getParameter('error_description'), 'authorization_code doesn\'t exist or is invalid for the client'); } - private function getTestServer() + public function testMissingCodeVerifier() + { + $server = $this->getTestServer(); + $request = TestRequest::createPost(array( + 'grant_type' => 'authorization_code', // valid grant type + 'client_id' => 'Test Client ID', // valid client id + 'client_secret' => 'TestSecret', // valid client secret + 'code' => 'testcode-pkce-challenge-plain', // valid code + )); + $server->grantAccessToken($request, $response = new Response()); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('code_verifier_missing', $response->getParameter('error')); + $this->assertEquals("The PKCE code verifier parameter is required.", $response->getParameter('error_description')); + } + + public function testInvalidCodeVerifier() + { + $server = $this->getTestServer(); + $request = TestRequest::createPost(array( + 'grant_type' => 'authorization_code', // valid grant type + 'client_id' => 'Test Client ID', // valid client id + 'client_secret' => 'TestSecret', // valid client secret + 'code' => 'testcode-pkce-challenge-plain', // valid code + 'code_verifier' => 'invalidcodeverifier', // invalid code verifier + )); + $server->grantAccessToken($request, $response = new Response()); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('code_verifier_invalid', $response->getParameter('error')); + $this->assertEquals("The PKCE code verifier parameter is invalid.", $response->getParameter('error_description')); + } + + public function testInvalidPkceChallengeMethod() + { + $server = $this->getTestServer(); + $request = TestRequest::createPost(array( + 'grant_type' => 'authorization_code', // valid grant type + 'client_id' => 'Test Client ID', // valid client id + 'client_secret' => 'TestSecret', // valid client secret + 'code' => 'testcode-pkce-challenge-invalid-method', // valid code + 'code_verifier' => 'testcodechallengetestcodechallengetestcodechallenge', // valid code verifier + )); + $server->grantAccessToken($request, $response = new Response()); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('code_challenge_method_invalid', $response->getParameter('error')); + $this->assertEquals("Unknown PKCE code challenge method.", $response->getParameter('error_description')); + } + + public function testPkceChallengeMismatch() + { + $server = $this->getTestServer(); + $request = TestRequest::createPost(array( + 'grant_type' => 'authorization_code', // valid grant type + 'client_id' => 'Test Client ID', // valid client id + 'client_secret' => 'TestSecret', // valid client secret + 'code' => 'testcode-pkce-challenge-plain', // valid code + 'code_verifier' => 'invalidcodeverifierinvalidcodeverifierinvalidcodeverifier', // invalid code verifier + )); + $server->grantAccessToken($request, $response = new Response()); + + $this->assertEquals($response->getStatusCode(), 400); + $this->assertEquals('code_verifier_mismatch', $response->getParameter('error')); + $this->assertEquals("The PKCE code verifier parameter does not match the code challenge.", $response->getParameter('error_description')); + } + + public function testSuccessfulPlainPkceTokenRequest() + { + $server = $this->getTestServer(); + $request = TestRequest::createPost(array( + 'grant_type' => 'authorization_code', // valid grant type + 'client_id' => 'Test Client ID', // valid client id + 'client_secret' => 'TestSecret', // valid client secret + 'code' => 'testcode-pkce-challenge-plain', // valid code + 'code_verifier' => 'testcodechallengetestcodechallengetestcodechallenge', // valid code verifier + )); + $token = $server->grantAccessToken($request, $response = new Response()); + + $this->assertEquals($response->getStatusCode(), 200); + $this->assertNotNull($token); + $this->assertArrayHasKey('access_token', $token); + } + + public function testSuccessfulSha256PkceTokenRequest() + { + $server = $this->getTestServer(); + $request = TestRequest::createPost(array( + 'grant_type' => 'authorization_code', // valid grant type + 'client_id' => 'Test Client ID', // valid client id + 'client_secret' => 'TestSecret', // valid client secret + 'code' => 'testcode-pkce-challenge-s256', // valid code + 'code_verifier' => 'testcodechallengetestcodechallengetestcodechallenge', // valid code verifier + )); + $token = $server->grantAccessToken($request, $response = new Response()); + + $this->assertEquals($response->getStatusCode(), 200); + $this->assertNotNull($token); + $this->assertArrayHasKey('access_token', $token); + } + + private function getTestServer($config = []) { $storage = Bootstrap::getInstance()->getMemoryStorage(); - $server = new Server($storage); + $server = new Server($storage, $config); $server->addGrantType(new AuthorizationCode($storage)); return $server; diff --git a/test/config/storage.json b/test/config/storage.json index 52d3f2399..443158943 100644 --- a/test/config/storage.json +++ b/test/config/storage.json @@ -39,12 +39,42 @@ "redirect_uri": "http://brentertainment.com/voil%C3%A0", "expires": "9999999999", "id_token": "IDTOKEN" + }, + "testcode-pkce-challenge-plain": { + "client_id": "Test Client ID", + "user_id": "", + "redirect_uri": "", + "expires": "9999999999", + "id_token": "IDTOKEN", + "code_challenge": "testcodechallengetestcodechallengetestcodechallenge", + "code_challenge_method": "plain" + }, + "testcode-pkce-challenge-s256": { + "client_id": "Test Client ID", + "user_id": "", + "redirect_uri": "", + "expires": "9999999999", + "id_token": "IDTOKEN", + "code_challenge": "F9mz8u-tLgIAl5bWYGpmI0h1g7C5SAszNM85Ra-kd1E", + "code_challenge_method": "S256" + }, + "testcode-pkce-challenge-invalid-method": { + "client_id": "Test Client ID", + "user_id": "", + "redirect_uri": "", + "expires": "9999999999", + "id_token": "IDTOKEN", + "code_challenge": "F9mz8u-tLgIAl5bWYGpmI0h1g7C5SAszNM85Ra-kd1E", + "code_challenge_method": "md5" } }, "client_credentials" : { "Test Client ID": { "client_secret": "TestSecret" }, + "Test Public Client ID": { + "is_public": true + }, "Test Client ID with Redirect Uri": { "client_secret": "TestSecret2", "redirect_uri": "http://brentertainment.com"