From d68140adc61c79e0f8e5deb35f7aae3b2252a4b8 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Thu, 30 Jan 2020 08:51:11 -0800 Subject: [PATCH] Add ability to break cache if token kid is not found --- lib/WP_Auth0_LoginManager.php | 3 +- .../WP_Auth0_AsymmetricVerifier.php | 16 +-- lib/token-verifier/WP_Auth0_JwksFetcher.php | 23 +++- tests/testApiGetJwks.php | 4 +- tests/testJwksFetcher.php | 110 ++++++++++++++++++ tests/testLoginManagerRedirectLogin.php | 71 ++++++++--- tests/testOptionMigrationWs.php | 2 +- tests/testRoutesGetUser.php | 2 +- tests/testRoutesLogin.php | 15 ++- tests/traits/tokenHelper.php | 25 +++- 10 files changed, 233 insertions(+), 38 deletions(-) create mode 100644 tests/testJwksFetcher.php diff --git a/lib/WP_Auth0_LoginManager.php b/lib/WP_Auth0_LoginManager.php index ed77662b..cf9325b1 100755 --- a/lib/WP_Auth0_LoginManager.php +++ b/lib/WP_Auth0_LoginManager.php @@ -523,8 +523,7 @@ private function decode_id_token( $id_token ) { $expectedIss = 'https://' . $this->a0_options->get( 'domain' ) . '/'; $expectedAlg = $this->a0_options->get( 'client_signing_algorithm' ); if ( 'RS256' === $expectedAlg ) { - $jwks = ( new WP_Auth0_JwksFetcher() )->getKeys(); - $sigVerifier = new WP_Auth0_AsymmetricVerifier( $jwks ); + $sigVerifier = new WP_Auth0_AsymmetricVerifier( new WP_Auth0_JwksFetcher() ); } elseif ( 'HS256' === $expectedAlg ) { $sigVerifier = new WP_Auth0_SymmetricVerifier( $this->a0_options->get( 'client_secret' ) ); } else { diff --git a/lib/token-verifier/WP_Auth0_AsymmetricVerifier.php b/lib/token-verifier/WP_Auth0_AsymmetricVerifier.php index 4c8af7d8..fe2344d4 100644 --- a/lib/token-verifier/WP_Auth0_AsymmetricVerifier.php +++ b/lib/token-verifier/WP_Auth0_AsymmetricVerifier.php @@ -18,7 +18,6 @@ */ final class WP_Auth0_AsymmetricVerifier extends WP_Auth0_SignatureVerifier { - /** * JWKS array with kid as keys, PEM cert as values. * @@ -29,9 +28,9 @@ final class WP_Auth0_AsymmetricVerifier extends WP_Auth0_SignatureVerifier { /** * JwksVerifier constructor. * - * @param array $jwks JWKS to use. + * @param WP_Auth0_JwksFetcher $jwks WP_Auth0_JwksFetcher to use. */ - public function __construct( array $jwks ) { + public function __construct( WP_Auth0_JwksFetcher $jwks ) { $this->jwks = $jwks; parent::__construct( 'RS256' ); } @@ -46,11 +45,14 @@ public function __construct( array $jwks ) { * @throws WP_Auth0_InvalidIdTokenException If ID token kid was not found in the JWKS. */ protected function checkSignature( Token $token ) : bool { - $tokenKid = $token->getHeader( 'kid', false ); - if ( ! array_key_exists( $tokenKid, $this->jwks ) ) { - throw new WP_Auth0_InvalidIdTokenException( 'Could not find a public key for Key ID (kid) "' . $tokenKid . '"' ); + $token_kid = $token->getHeader( 'kid', false ); + $signing_key = $this->jwks->getKey( $token_kid ); + if ( ! $signing_key ) { + throw new WP_Auth0_InvalidIdTokenException( + 'Could not find a public key for Key ID (kid) "' . $token_kid . '"' + ); } - return $token->verify( new RsSigner(), new Key( $this->jwks[ $tokenKid ] ) ); + return $token->verify( new RsSigner(), new Key( $signing_key ) ); } } diff --git a/lib/token-verifier/WP_Auth0_JwksFetcher.php b/lib/token-verifier/WP_Auth0_JwksFetcher.php index de570f4d..16c27a65 100644 --- a/lib/token-verifier/WP_Auth0_JwksFetcher.php +++ b/lib/token-verifier/WP_Auth0_JwksFetcher.php @@ -33,13 +33,32 @@ protected function convertCertToPem( string $cert ) : string { return $output; } + /** + * Get a key using a kid. + * + * @param string $kid Key ID to get. + * + * @return string + */ + public function getKey( string $kid ) { + $keys = $this->getKeys(); + + if ( ! empty( $keys ) && empty( $keys[ $kid ] ) ) { + $keys = $this->getKeys( false ); + } + + return $keys[ $kid ] ?? null; + } + /** * Gets an array of keys from the JWKS as kid => x5c. * + * @param bool $use_cache Defaults to true to use a cached value. + * * @return array */ - public function getKeys() : array { - $keys = get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME ); + public function getKeys( $use_cache = true ) : array { + $keys = $use_cache ? get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME ) : []; if ( is_array( $keys ) && ! empty( $keys ) ) { return $keys; } diff --git a/tests/testApiGetJwks.php b/tests/testApiGetJwks.php index 1a60162e..4202796c 100644 --- a/tests/testApiGetJwks.php +++ b/tests/testApiGetJwks.php @@ -57,7 +57,7 @@ public function testThatNetworkErrorIsHandled() { } /** - * Test that a network error (caught by WP and returned as a WP_Error) is handled properly. + * Test that an Auth0 API error (caught by WP and returned as a WP_Error) is handled properly. */ public function testThatApiErrorIsHandled() { $this->startHttpMocking(); @@ -74,7 +74,7 @@ public function testThatApiErrorIsHandled() { } /** - * Test that a network error (caught by WP and returned as a WP_Error) is handled properly. + * Test that a successful call is handled correctly. */ public function testThatSuccessfulCallIsReturned() { $this->startHttpMocking(); diff --git a/tests/testJwksFetcher.php b/tests/testJwksFetcher.php new file mode 100644 index 00000000..69370f17 --- /dev/null +++ b/tests/testJwksFetcher.php @@ -0,0 +1,110 @@ +assertFalse( get_transient( 'WP_Auth0_JWKS_cache' ) ); + } + + public function testThatGetKeysUsesCache() { + set_transient( 'WP_Auth0_JWKS_cache', [ '__test_key__' ], 3600 ); + $jwks = new WP_Auth0_JwksFetcher(); + + $this->assertEquals( [ '__test_key__' ], $jwks->getKeys() ); + } + + public function testThatNotUsingCacheCallsEndpoint() { + $this->startHttpMocking(); + $this->http_request_type = 'success_jwks'; + + set_transient( 'WP_Auth0_JWKS_cache', [ '__test_key__' ], 3600 ); + + $this->assertEquals( [ '__test_key__' ], get_transient( 'WP_Auth0_JWKS_cache' ) ); + + $jwks = new WP_Auth0_JwksFetcher(); + $keys = $jwks->getKeys( false ); + + $this->assertEquals( + [ '__test_kid_1__' => "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n" ], + $keys + ); + + $this->assertEquals( $keys, get_transient( 'WP_Auth0_JWKS_cache' ) ); + } + + public function testThatNonArrayCacheCallsEndpoint() { + $this->startHttpMocking(); + $this->http_request_type = 'success_jwks'; + + set_transient( 'WP_Auth0_JWKS_cache', '__invalid_cache__', 3600 ); + $jwks = new WP_Auth0_JwksFetcher(); + + $this->assertEquals( + [ '__test_kid_1__' => "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n" ], + $jwks->getKeys() + ); + } + + public function testThatInvalidJwksReturnsEmptyArray() { + $this->startHttpMocking(); + $this->http_request_type = 'auth0_api_error'; + + $jwks = new WP_Auth0_JwksFetcher(); + $this->assertEquals( [], $jwks->getKeys() ); + } + + public function testThatGetKeyPullsFromCache() { + $this->startHttpMocking(); + $this->http_request_type = 'halt'; + + set_transient( + 'WP_Auth0_JWKS_cache', + [ '__test_kid_1__' => "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n" ], + 3600 + ); + + $jwks = new WP_Auth0_JwksFetcher(); + + $this->assertEquals( + "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n", + $jwks->getKey( '__test_kid_1__' ) + ); + } + + public function testThatGetKeyNotFoundForcesEndpointCall() { + $this->startHttpMocking(); + $this->http_request_type = 'success_jwks'; + + set_transient( 'WP_Auth0_JWKS_cache', [ '__invalid_kid__' => '__invalid_x5c__' ], 3600 ); + + $jwks = new WP_Auth0_JwksFetcher(); + $found_x5c = $jwks->getKey( '__test_kid_1__' ); + + $this->assertEquals( $found_x5c, $jwks->getKey( '__test_kid_1__' ) ); + $this->assertEquals( [ '__test_kid_1__' => $found_x5c ], get_transient( 'WP_Auth0_JWKS_cache' ) ); + } + + public function testThatNotFoundKidReturnsNull() { + $this->startHttpMocking(); + $this->http_request_type = 'success_jwks'; + + $jwks = new WP_Auth0_JwksFetcher(); + $this->assertNull( $jwks->getKey( '__not_found_kid__' ) ); + } +} diff --git a/tests/testLoginManagerRedirectLogin.php b/tests/testLoginManagerRedirectLogin.php index c7c90e11..3210dcca 100644 --- a/tests/testLoginManagerRedirectLogin.php +++ b/tests/testLoginManagerRedirectLogin.php @@ -89,18 +89,28 @@ public function auth0_get_wp_user_handler( $user, $userinfo ) { * @throws Exception - If set to halt on response. */ public function httpMock( $response_type = null, array $args = null, $url = null ) { - $response_type = $response_type ?: $this->getResponseType(); + $response_type = $response_type ?: $this->getResponseType(); + $id_token_payload = [ + 'sub' => '__test_id_token_sub__', + 'iss' => 'https://test.auth0.com/', + 'aud' => '__test_client_id__', + 'nonce' => '__test_nonce__', + 'exp' => time() + 1000, + 'iat' => time() - 1000, + ]; + switch ( $response_type ) { - case 'success_exchange_code_valid_id_token': - $id_token_payload = [ - 'sub' => '__test_id_token_sub__', - 'iss' => 'https://test.auth0.com/', - 'aud' => '__test_client_id__', - 'nonce' => '__test_nonce__', - 'exp' => time() + 1000, - 'iat' => time() - 1000, + case 'success_exchange_code_valid_HS_id_token': + $id_token = self::makeHsToken( $id_token_payload, '__test_client_secret__' ); + return [ + 'body' => sprintf( + '{"access_token":"__test_access_token__","id_token":"%s"}', + $id_token + ), + 'response' => [ 'code' => 200 ], ]; - $id_token = self::makeToken( $id_token_payload, '__test_client_secret__' ); + case 'success_exchange_code_valid_RS_id_token': + $id_token = self::makeRsToken( $id_token_payload ); return [ 'body' => sprintf( '{"access_token":"__test_access_token__","id_token":"%s"}', @@ -293,7 +303,7 @@ public function testThatGetUserCallIsCorrect() { $this->startHttpMocking(); $this->http_request_type = [ // Mocked successful code exchange with a valid ID token. - 'success_exchange_code_valid_id_token', + 'success_exchange_code_valid_HS_id_token', // Stop the get user call. 'halt', ]; @@ -326,7 +336,7 @@ public function testThatLoginUserIsCalledWithManagementApiUserinfo() { $this->startHttpMocking(); $this->http_request_type = [ // Mocked successful code exchange with a valid ID token. - 'success_exchange_code_valid_id_token', + 'success_exchange_code_valid_HS_id_token', // Mocked successful get user call. 'success_get_user', ]; @@ -362,7 +372,7 @@ public function testThatLoginUserIsCalledWithIdTokenIfNoApiAccess() { $this->startHttpMocking(); $this->http_request_type = [ // Mocked successful code exchange with a valid ID token. - 'success_exchange_code_valid_id_token', + 'success_exchange_code_valid_HS_id_token', // Mocked failed get user call. 'auth0_api_error', ]; @@ -395,7 +405,7 @@ public function testThatLoginUserIsCalledWithIdTokenIfFilterIsSetToFalse() { $this->startHttpMocking(); $this->http_request_type = [ // Mocked successful code exchange with a valid ID token. - 'success_exchange_code_valid_id_token', + 'success_exchange_code_valid_HS_id_token', // Mocked successful get user call. 'success_get_user', ]; @@ -419,4 +429,37 @@ public function testThatLoginUserIsCalledWithIdTokenIfFilterIsSetToFalse() { $this->assertEquals( '__test_id_token_sub__', $user_data['userinfo']->user_id ); } + + /** + * @throws WP_Auth0_BeforeLoginException Should not be thrown in this test. + * @throws WP_Auth0_InvalidIdTokenException Should not be thrown in this test. + * @throws WP_Auth0_LoginFlowValidationException Should not be thrown in this test. + */ + public function testThatGetJwksIsCalledForRs256IdToken() { + $this->startHttpMocking(); + $this->http_request_type = [ + // Mocked successful code exchange with a valid ID token. + 'success_exchange_code_valid_RS_id_token', + // Stop the get user call. + 'success_jwks', + ]; + + self::$opts->set( 'domain', 'test.auth0.com' ); + self::$opts->set( 'client_id', '__test_client_id__' ); + self::$opts->set( 'client_signing_algorithm', 'RS256' ); + self::$opts->set( 'cache_expiration', 999999 ); + $_REQUEST['code'] = uniqid(); + $_COOKIE['auth0_nonce'] = '__test_nonce__'; + try { + // Need to hide error messages here because a cookie is set. + // phpcs:ignore + @$this->login->redirect_login(); + } catch ( InvalidArgumentException $e ) { + // Stop process at next exception. + } + + $cached_jwks = get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME ); + + $this->assertArrayHasKey( '__test_kid_1__', $cached_jwks ); + } } diff --git a/tests/testOptionMigrationWs.php b/tests/testOptionMigrationWs.php index 4b1e6764..f61b3847 100644 --- a/tests/testOptionMigrationWs.php +++ b/tests/testOptionMigrationWs.php @@ -158,7 +158,7 @@ public function testThatChangingMigrationToOnKeepsToken() { */ public function testThatChangingMigrationToOnKeepsWithJwtSetsId() { $client_secret = '__test_client_secret__'; - $migration_token = self::makeToken( [ 'jti' => '__test_token_id__' ], $client_secret ); + $migration_token = self::makeHsToken( [ 'jti' => '__test_token_id__' ], $client_secret ); self::$opts->set( 'migration_token', $migration_token ); $input = [ 'migration_ws' => '1', diff --git a/tests/testRoutesGetUser.php b/tests/testRoutesGetUser.php index 839305a8..c5f67bea 100644 --- a/tests/testRoutesGetUser.php +++ b/tests/testRoutesGetUser.php @@ -98,7 +98,7 @@ public function testThatGetUserRouteIsUnauthorizedIfWrongJti() { self::$opts->set( 'client_secret', $client_secret ); self::$opts->set( 'migration_token_id', '__test_token_id__' ); - $_POST['access_token'] = self::makeToken( [ 'jti' => uniqid() ], $client_secret ); + $_POST['access_token'] = self::makeHsToken( [ 'jti' => uniqid() ], $client_secret ); $output = json_decode( wp_auth0_custom_requests( self::$wp, true ) ); diff --git a/tests/testRoutesLogin.php b/tests/testRoutesLogin.php index 4c20fb38..8bcda22d 100644 --- a/tests/testRoutesLogin.php +++ b/tests/testRoutesLogin.php @@ -103,7 +103,7 @@ public function testThatLoginRouteIsUnauthorizedIfWrongJti() { self::$opts->set( 'migration_token_id', '__test_token_id__' ); self::$wp->query_vars['a0_action'] = 'migration-ws-login'; - $_POST['access_token'] = self::makeToken( [ 'jti' => uniqid() ], $client_secret ); + $_POST['access_token'] = self::makeHsToken( [ 'jti' => uniqid() ], $client_secret ); $output = json_decode( wp_auth0_custom_requests( self::$wp, true ) ); @@ -125,7 +125,7 @@ public function testThatLoginRouteIsUnauthorizedIfMissingJti() { self::$opts->set( 'migration_token_id', '__test_token_id__' ); self::$wp->query_vars['a0_action'] = 'migration-ws-login'; - $_POST['access_token'] = self::makeToken( [ 'iss' => uniqid() ], $client_secret ); + $_POST['access_token'] = self::makeHsToken( [ 'iss' => uniqid() ], $client_secret ); $output = json_decode( wp_auth0_custom_requests( self::$wp, true ) ); @@ -143,7 +143,8 @@ public function testThatLoginRouteIsUnauthorizedIfMissingJti() { public function testThatLoginRouteIsBadRequestIfNoUsername() { $client_secret = '__test_client_secret__'; $token_id = '__test_token_id__'; - $migration_token = self::makeToken( [ 'jti' => $token_id ], $client_secret ); + $migration_token = self::makeHsToken( [ 'jti' => $token_id ], $client_secret ); + self::$opts->set( 'migration_ws', true ); self::$opts->set( 'client_secret', $client_secret ); self::$opts->set( 'migration_token', $migration_token ); @@ -167,7 +168,8 @@ public function testThatLoginRouteIsBadRequestIfNoUsername() { public function testThatLoginRouteIsBadRequestIfNoPassword() { $client_secret = '__test_client_secret__'; $token_id = '__test_token_id__'; - $migration_token = self::makeToken( [ 'jti' => $token_id ], $client_secret ); + $migration_token = self::makeHsToken( [ 'jti' => $token_id ], $client_secret ); + self::$opts->set( 'migration_ws', true ); self::$opts->set( 'client_secret', $client_secret ); self::$opts->set( 'migration_token', $migration_token ); @@ -192,7 +194,8 @@ public function testThatLoginRouteIsBadRequestIfNoPassword() { public function testThatLoginRouteIsUnauthorizedIfNotAuthenticated() { $client_secret = '__test_client_secret__'; $token_id = '__test_token_id__'; - $migration_token = self::makeToken( [ 'jti' => $token_id ], $client_secret ); + $migration_token = self::makeHsToken( [ 'jti' => $token_id ], $client_secret ); + self::$opts->set( 'migration_ws', true ); self::$opts->set( 'client_secret', $client_secret ); self::$opts->set( 'migration_token', $migration_token ); @@ -227,7 +230,7 @@ public function testThatLoginRouteReturnsUserIfSuccessful() { 'user_pass' => $_POST['password'], ] ); - $migration_token = self::makeToken( [ 'jti' => $token_id ], $client_secret ); + $migration_token = self::makeHsToken( [ 'jti' => $token_id ], $client_secret ); self::$opts->set( 'migration_ws', true ); self::$opts->set( 'client_secret', $client_secret ); self::$opts->set( 'migration_token', $migration_token ); diff --git a/tests/traits/tokenHelper.php b/tests/traits/tokenHelper.php index a2c688f2..3f619e1a 100644 --- a/tests/traits/tokenHelper.php +++ b/tests/traits/tokenHelper.php @@ -10,7 +10,7 @@ use Lcobucci\JWT\Builder; use Lcobucci\JWT\Signer\Key; use Lcobucci\JWT\Signer\Hmac\Sha256 as HsSigner; -use Lcobucci\JWT\Token; +use Lcobucci\JWT\Signer\Rsa\Sha256 as RsSigner; /** * Trait TokenHelper. @@ -25,13 +25,32 @@ trait TokenHelper { * * @return string */ - public function makeToken( $claims = [], $secret = '__test_secret__' ) { + public function makeHsToken( $claims = [], $secret = '__test_secret__' ) { + return (string) $this->buildToken( $claims )->getToken( new HsSigner(), new Key( $secret ) ); + } + + public function makeRsToken( $claims = [] ) { + $pkey_resource = openssl_pkey_new( + [ + 'digest_alg' => 'sha256', + 'private_key_type' => OPENSSL_KEYTYPE_RSA, + ] + ); + + openssl_pkey_export( $pkey_resource, $rsa_private_key ); + + return (string) $this->buildToken( $claims ) + ->withHeader( 'kid', '__test_kid_1__' ) + ->getToken( new RsSigner(), new Key( $rsa_private_key ) ); + } + + public function buildToken( $claims ) { $builder = new Builder(); foreach ( $claims as $prop => $claim ) { $builder->withClaim( $prop, $claim ); } - return (string) $builder->getToken( new HsSigner(), new Key( $secret ) ); + return $builder; } }