From 289f1397168ac825da68907b050a1ef41827c4ca Mon Sep 17 00:00:00 2001 From: Timur Sadykov Date: Mon, 29 Aug 2022 01:14:03 -0700 Subject: [PATCH] fix!: IdTokenVerifier now throws IOException if any issue obtaining public keys. Adding retries to public key fetch to cover transient network issues. (#934) * fix!: IdtokenVerifier now throws IOException if any issue obtaining public keys. Adding retries to public key fetch to cover transient network issues. * fix: docs nit fixes --- .../auth/openidconnect/IdTokenVerifier.java | 38 ++++++++++++++----- .../openidconnect/IdTokenVerifierTest.java | 36 +++++++++++------- 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java b/google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java index c3650b69e..6cf3eb0ea 100644 --- a/google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java +++ b/google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java @@ -15,6 +15,8 @@ package com.google.api.client.auth.openidconnect; import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; +import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler.BackOffRequired; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpTransport; @@ -25,6 +27,7 @@ import com.google.api.client.util.Base64; import com.google.api.client.util.Beta; import com.google.api.client.util.Clock; +import com.google.api.client.util.ExponentialBackOff; import com.google.api.client.util.Key; import com.google.api.client.util.Preconditions; import com.google.common.annotations.VisibleForTesting; @@ -231,8 +234,10 @@ public final Collection getAudience() { * * @param idToken ID token * @return {@code true} if verified successfully or {@code false} if failed + * @throws IOException if verification fails to run. For example, if it fails to get public keys + * for signature validation. */ - public boolean verify(IdToken idToken) { + public boolean verify(IdToken idToken) throws IOException { boolean payloadValid = verifyPayload(idToken); if (!payloadValid) { @@ -242,11 +247,7 @@ public boolean verify(IdToken idToken) { try { return verifySignature(idToken); } catch (VerificationException ex) { - LOGGER.log( - Level.SEVERE, - "id token signature verification failed. " - + "Please see docs for IdTokenVerifier for default settings and configuration options", - ex); + LOGGER.log(Level.INFO, "Id token signature verification failed. ", ex); return false; } } @@ -281,7 +282,7 @@ protected boolean verifyPayload(IdToken idToken) { } @VisibleForTesting - boolean verifySignature(IdToken idToken) throws VerificationException { + boolean verifySignature(IdToken idToken) throws IOException, VerificationException { if (Boolean.parseBoolean(environment.getVariable(SKIP_SIGNATURE_ENV_VAR))) { return true; } @@ -297,12 +298,12 @@ boolean verifySignature(IdToken idToken) throws VerificationException { String certificateLocation = getCertificateLocation(idToken.getHeader()); publicKeyToUse = publicKeyCache.get(certificateLocation).get(idToken.getHeader().getKeyId()); } catch (ExecutionException | UncheckedExecutionException e) { - throw new VerificationException( + throw new IOException( "Error fetching public key from certificate location " + certificatesLocation, e); } if (publicKeyToUse == null) { - throw new VerificationException( + throw new IOException( "Could not find public key for provided keyId: " + idToken.getHeader().getKeyId()); } @@ -508,6 +509,10 @@ public Builder setHttpTransportFactory(HttpTransportFactory httpTransportFactory /** Custom CacheLoader for mapping certificate urls to the contained public keys. */ static class PublicKeyLoader extends CacheLoader> { + private static final int DEFAULT_NUMBER_OF_RETRIES = 2; + private static final int INITIAL_RETRY_INTERVAL_MILLIS = 1000; + private static final double RETRY_RANDOMIZATION_FACTOR = 0.1; + private static final double RETRY_MULTIPLIER = 2; private final HttpTransportFactory httpTransportFactory; /** @@ -553,6 +558,19 @@ public Map load(String certificateUrl) throws Exception { .createRequestFactory() .buildGetRequest(new GenericUrl(certificateUrl)) .setParser(GsonFactory.getDefaultInstance().createJsonObjectParser()); + request.setNumberOfRetries(DEFAULT_NUMBER_OF_RETRIES); + + ExponentialBackOff backoff = + new ExponentialBackOff.Builder() + .setInitialIntervalMillis(INITIAL_RETRY_INTERVAL_MILLIS) + .setRandomizationFactor(RETRY_RANDOMIZATION_FACTOR) + .setMultiplier(RETRY_MULTIPLIER) + .build(); + + request.setUnsuccessfulResponseHandler( + new HttpBackOffUnsuccessfulResponseHandler(backoff) + .setBackOffRequired(BackOffRequired.ALWAYS)); + HttpResponse response = request.execute(); jwks = response.parseAs(JsonWebKeySet.class); } catch (IOException io) { @@ -589,7 +607,7 @@ public Map load(String certificateUrl) throws Exception { "No valid public key returned by the keystore: " + certificateUrl); } - return keyCacheBuilder.build(); + return keyCache; } private PublicKey buildPublicKey(JsonWebKey key) diff --git a/google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java b/google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java index 75f644061..f27aafa56 100644 --- a/google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java +++ b/google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java @@ -68,9 +68,11 @@ public class IdTokenVerifierTest extends TestCase { "https://www.googleapis.com/oauth2/v1/certs"; private static final String SERVICE_ACCOUNT_RS256_TOKEN = - "eyJhbGciOiJSUzI1NiIsImtpZCI6IjJlZjc3YjM4YTFiMDM3MDQ4NzA0MzkxNmFjYmYyN2Q3NGVkZDA4YjEiLCJ0eXAiOiJKV1QifQ.eyJhdWQiOiJodHRwczovL2V4YW1wbGUuY29tL2F1ZGllbmNlIiwiZXhwIjoxNTg3NjMwNTQzLCJpYXQiOjE1ODc2MjY5NDMsImlzcyI6InNvbWUgaXNzdWVyIiwic3ViIjoic29tZSBzdWJqZWN0In0.gGOQW0qQgs4jGUmCsgRV83RqsJLaEy89-ZOG6p1u0Y26FyY06b6Odgd7xXLsSTiiSnch62dl0Lfi9D0x2ByxvsGOCbovmBl2ZZ0zHr1wpc4N0XS9lMUq5RJQbonDibxXG4nC2zroDfvD0h7i-L8KMXeJb9pYwW7LkmrM_YwYfJnWnZ4bpcsDjojmPeUBlACg7tjjOgBFbyQZvUtaERJwSRlaWibvNjof7eCVfZChE0PwBpZc_cGqSqKXv544L4ttqdCnmONjqrTATXwC4gYxruevkjHfYI5ojcQmXoWDJJ0-_jzfyPE4MFFdCFgzLgnfIOwe5ve0MtquKuv2O0pgvg"; + "eyJhbGciOiJSUzI1NiIsImtpZCI6IjE3MjdiNmI0OTQwMmI5Y2Y5NWJlNGU4ZmQzOGFhN2U3YzExNjQ0YjEiLCJ0eXAiOiJKV1QifQ.eyJhdWQiOiJodHRwczovL2Nsb3VkdGFza3MuZ29vZ2xlYXBpcy5jb20vdjIvcHJvamVjdHMvZ2Nsb3VkLWRldmVsL2xvY2F0aW9ucyIsImF6cCI6InN0aW0tdGVzdEBzdGVsbGFyLWRheS0yNTQyMjIuaWFtLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJlbWFpbCI6InN0aW0tdGVzdEBzdGVsbGFyLWRheS0yNTQyMjIuaWFtLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJlbWFpbF92ZXJpZmllZCI6dHJ1ZSwiZXhwIjoxNjYwODgwNjczLCJpYXQiOjE2NjA4NzcwNzMsImlzcyI6Imh0dHBzOi8vYWNjb3VudHMuZ29vZ2xlLmNvbSIsInN1YiI6IjExMjgxMDY3Mjk2MzcyODM2NjQwNiJ9.Q2tG-hN6UHecbzaCIlg58K9msp58nLZWs03CBGO_D6F3cI4LKQEUzsbcztZqmNGWd0ld4zkrKzIP9cQosa_xold4hEzSX_ORRHYQLimLYaQmP3rKqWPMsbIupPdpnGqBDzAYjc7Pw9pQBzuZJj8e3FEG6a5tblDfMcgeklXZIkwzN7ypWCbFDoDP2STSYJYZ-LQIB0-Zlex7dm2KhyB8QSkMQK60YvpXz4L1OtwG7spk3yUCWxul6hYF76klST0iS6DH03YdaDpt4gRXkTUKyTRfB10h-WhCAKKRzmT6d_IT9ApIyqPhimkgkBHhLNyjK8lgAJdk9CLriSEOgVpsow"; + private static final String SERVICE_ACCOUNT_RS256_TOKEN_BAD_SIGNATURE = + "eyJhbGciOiJSUzI1NiIsImtpZCI6IjE3MjdiNmI0OTQwMmI5Y2Y5NWJlNGU4ZmQzOGFhN2U3YzExNjQ0YjEiLCJ0eXAiOiJKV1QifQ.eyJhdWQiOiJodHRwczovL2Nsb3VkdGFza3MuZ29vZ2xlYXBpcy5jb20vdjIvcHJvamVjdHMvZ2Nsb3VkLWRldmVsL2xvY2F0aW9ucyIsImF6cCI6InN0aW0tdGVzdEBzdGVsbGFyLWRheS0yNTQyMjIuaWFtLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJlbWFpbCI6InN0aW0tdGVzdEBzdGVsbGFyLWRheS0yNTQyMjIuaWFtLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJlbWFpbF92ZXJpZmllZCI6dHJ1ZSwiZXhwIjoxNjYwODgwNjczLCJpYXQiOjE2NjA4NzcwNzMsImlzcyI6Imh0dHBzOi8vYWNjb3VudHMuZ29vZ2xlLmNvbSIsInN1YiI6IjExMjgxMDY3Mjk2MzcyODM2NjQwNiJ9.Q2tG-hN6UHecbzaCIlg58K9msp58nLZWs03CBGO_D6F3cI4LKQEUzsbcztZqmNGWd0ld4zkrKzIP9cQosa_xold4hEzSX_ORRHYQLimLYaQmP3rKqWPMsbIupPdpnGqBDzAYjc7Pw9pQBzuZJj8e3FEG6a5tblDfMcgeklXZIkwzN7ypWCbFDoDP2STSYJYZ-LQIB0-Zlex7dm2KhyB8QSkMQK60YvpXz4L1OtwG7spk3yUCWxul6hYF76klST0iS6DH03YdaDpt4gRXkTUKyTRfB10h-WhCAKKRzmT6d_IT9ApIyqPhimkgkBHhLNyjK8lgAJdk9CLriSEOgVpruy"; private static final String SERVICE_ACCOUNT_CERT_URL = - "https://www.googleapis.com/robot/v1/metadata/x509/integration-tests%40chingor-test.iam.gserviceaccount.com"; + "https://www.googleapis.com/oauth2/v3/certs"; private static final List ALL_TOKENS = Arrays.asList(ES256_TOKEN, FEDERATED_SIGNON_RS256_TOKEN, SERVICE_ACCOUNT_RS256_TOKEN); @@ -184,7 +186,7 @@ public void testBuilderSetNullIssuers() throws Exception { assertNull(verifier.getIssuer()); } - public void testMissingAudience() throws VerificationException { + public void testMissingAudience() throws IOException { IdToken idToken = newIdToken(ISSUER, null); MockClock clock = new MockClock(); @@ -198,7 +200,7 @@ public void testMissingAudience() throws VerificationException { assertFalse(verifier.verify(idToken)); } - public void testVerifyEs256TokenPublicKeyMismatch() throws Exception { + public void testPublicKeyStoreIntermittentError() throws Exception { // Mock HTTP requests MockLowLevelHttpRequest failedRequest = new MockLowLevelHttpRequest() { @@ -245,7 +247,7 @@ public LowLevelHttpResponse execute() throws IOException { }; HttpTransportFactory httpTransportFactory = - mockTransport(failedRequest, badRequest, emptyRequest, goodRequest); + mockTransport(failedRequest, badRequest, badRequest, badRequest, emptyRequest, goodRequest); IdTokenVerifier tokenVerifier = new IdTokenVerifier.Builder() .setClock(FIXED_CLOCK) @@ -255,28 +257,28 @@ public LowLevelHttpResponse execute() throws IOException { try { tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN)); fail("Should have failed verification"); - } catch (VerificationException ex) { + } catch (IOException ex) { assertTrue(ex.getMessage().contains("Error fetching public key")); } try { tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN)); fail("Should have failed verification"); - } catch (VerificationException ex) { + } catch (IOException ex) { assertTrue(ex.getMessage().contains("Error fetching public key")); } try { tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN)); fail("Should have failed verification"); - } catch (VerificationException ex) { + } catch (IOException ex) { assertTrue(ex.getCause().getMessage().contains("No valid public key returned")); } Assert.assertTrue(tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN))); } - public void testVerifyEs256Token() throws VerificationException, IOException { + public void testVerifyEs256Token() throws IOException { HttpTransportFactory httpTransportFactory = mockTransport( "https://www.gstatic.com/iap/verify/public_key-jwk", @@ -289,7 +291,7 @@ public void testVerifyEs256Token() throws VerificationException, IOException { assertTrue(tokenVerifier.verify(IdToken.parse(JSON_FACTORY, ES256_TOKEN))); } - public void testVerifyRs256Token() throws VerificationException, IOException { + public void testVerifyRs256Token() throws IOException { HttpTransportFactory httpTransportFactory = mockTransport( "https://www.googleapis.com/oauth2/v3/certs", @@ -304,7 +306,7 @@ public void testVerifyRs256Token() throws VerificationException, IOException { } public void testVerifyRs256TokenWithLegacyCertificateUrlFormat() - throws VerificationException, IOException { + throws IOException, VerificationException { HttpTransportFactory httpTransportFactory = mockTransport( LEGACY_FEDERATED_SIGNON_CERT_URL, readResourceAsString("legacy_federated_keys.json")); @@ -318,8 +320,8 @@ public void testVerifyRs256TokenWithLegacyCertificateUrlFormat() assertTrue(tokenVerifier.verify(IdToken.parse(JSON_FACTORY, FEDERATED_SIGNON_RS256_TOKEN))); } - public void testVerifyServiceAccountRs256Token() throws VerificationException, IOException { - MockClock clock = new MockClock(1587626643000L); + public void testVerifyServiceAccountRs256Token() throws IOException { + MockClock clock = new MockClock(1660880973000L); IdTokenVerifier tokenVerifier = new IdTokenVerifier.Builder() .setClock(clock) @@ -327,6 +329,14 @@ public void testVerifyServiceAccountRs256Token() throws VerificationException, I .setHttpTransportFactory(new DefaultHttpTransportFactory()) .build(); assertTrue(tokenVerifier.verify(IdToken.parse(JSON_FACTORY, SERVICE_ACCOUNT_RS256_TOKEN))); + + // a token with a bad signature that is expected to fail in verify, but work in verifyPayload + assertFalse( + tokenVerifier.verify( + IdToken.parse(JSON_FACTORY, SERVICE_ACCOUNT_RS256_TOKEN_BAD_SIGNATURE))); + assertTrue( + tokenVerifier.verifyPayload( + IdToken.parse(JSON_FACTORY, SERVICE_ACCOUNT_RS256_TOKEN_BAD_SIGNATURE))); } static String readResourceAsString(String resourceName) throws IOException {