fix!: IdTokenVerifier now throws IOException if any issue obtaining p…

…ublic 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
googleapis · Aug 29, 2022 · 289f139 · 289f139
1 parent a3ea3ab
commit 289f139
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 23 deletions.
diff --git a/...-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java b/...-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<String> 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<String, Map<String, PublicKey>> {
+    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<String, PublicKey> 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<String, PublicKey> 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/...th-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java b/...th-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<String> 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,15 +320,23 @@ 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)
             .setCertificatesLocation(SERVICE_ACCOUNT_CERT_URL)
             .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 {