From 773b38844cd6a0a72a360cc25692412e9b36b1e7 Mon Sep 17 00:00:00 2001 From: Timur Sadykov Date: Wed, 1 Jun 2022 17:08:39 -0700 Subject: [PATCH] fix: fix IdTokenVerifier so it does not cache empty entries (#892) Couple fixes to the IdTokenVerifier: - Cache will not save empty entry in case of public key fetch failure - Payload verification moved to a separate protected method so child classes can call it directly and avoid duplicate signature verification Co-authored-by: Owl Bot Co-authored-by: Tomo Suzuki --- .../auth/openidconnect/IdTokenVerifier.java | 51 ++++++-- .../openidconnect/IdTokenVerifierTest.java | 114 ++++++++++++++---- 2 files changed, 135 insertions(+), 30 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 9f3c446ad..c3650b69e 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 @@ -233,12 +233,9 @@ public final Collection getAudience() { * @return {@code true} if verified successfully or {@code false} if failed */ public boolean verify(IdToken idToken) { - boolean tokenFieldsValid = - (issuers == null || idToken.verifyIssuer(issuers)) - && (audience == null || idToken.verifyAudience(audience)) - && idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds); + boolean payloadValid = verifyPayload(idToken); - if (!tokenFieldsValid) { + if (!payloadValid) { return false; } @@ -254,6 +251,35 @@ public boolean verify(IdToken idToken) { } } + /** + * Verifies the payload of the given ID token + * + *

It verifies: + * + *

    + *
  • The issuer is one of {@link #getIssuers()} by calling {@link + * IdToken#verifyIssuer(String)}. + *
  • The audience is one of {@link #getAudience()} by calling {@link + * IdToken#verifyAudience(Collection)}. + *
  • The current time against the issued at and expiration time, using the {@link #getClock()} + * and allowing for a time skew specified in {@link #getAcceptableTimeSkewSeconds()} , by + * calling {@link IdToken#verifyTime(long, long)}. + *
+ * + *

Overriding is allowed, but it must call the super implementation. + * + * @param idToken ID token + * @return {@code true} if verified successfully or {@code false} if failed + */ + protected boolean verifyPayload(IdToken idToken) { + boolean tokenPayloadValid = + (issuers == null || idToken.verifyIssuer(issuers)) + && (audience == null || idToken.verifyAudience(audience)) + && idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds); + + return tokenPayloadValid; + } + @VisibleForTesting boolean verifySignature(IdToken idToken) throws VerificationException { if (Boolean.parseBoolean(environment.getVariable(SKIP_SIGNATURE_ENV_VAR))) { @@ -272,12 +298,12 @@ boolean verifySignature(IdToken idToken) throws VerificationException { publicKeyToUse = publicKeyCache.get(certificateLocation).get(idToken.getHeader().getKeyId()); } catch (ExecutionException | UncheckedExecutionException e) { throw new VerificationException( - "Error fetching PublicKey from certificate location " + certificatesLocation, e); + "Error fetching public key from certificate location " + certificatesLocation, e); } if (publicKeyToUse == null) { throw new VerificationException( - "Could not find PublicKey for provided keyId: " + idToken.getHeader().getKeyId()); + "Could not find public key for provided keyId: " + idToken.getHeader().getKeyId()); } try { @@ -380,7 +406,7 @@ public Builder setIssuer(String issuer) { } /** - * Override the location URL that contains published public keys. Defaults to well-known Google + * Overrides the location URL that contains published public keys. Defaults to well-known Google * locations. * * @param certificatesLocation URL to published public keys @@ -534,7 +560,7 @@ public Map load(String certificateUrl) throws Exception { Level.WARNING, "Failed to get a certificate from certificate location " + certificateUrl, io); - return ImmutableMap.of(); + throw io; } ImmutableMap.Builder keyCacheBuilder = new ImmutableMap.Builder<>(); @@ -556,6 +582,13 @@ public Map load(String certificateUrl) throws Exception { } } + ImmutableMap keyCache = keyCacheBuilder.build(); + + if (keyCache.isEmpty()) { + throw new VerificationException( + "No valid public key returned by the keystore: " + certificateUrl); + } + return keyCacheBuilder.build(); } 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 cf32e8ee8..75f644061 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 @@ -33,12 +33,15 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.io.Reader; +import java.util.ArrayDeque; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Queue; import junit.framework.TestCase; +import org.junit.Assert; /** * Tests {@link IdTokenVerifier}. @@ -101,7 +104,7 @@ public void testBuilder() throws Exception { assertEquals(TRUSTED_CLIENT_IDS, Lists.newArrayList(verifier.getAudience())); } - public void testVerify() throws Exception { + public void testVerifyPayload() throws Exception { MockClock clock = new MockClock(); MockEnvironment testEnvironment = new MockEnvironment(); testEnvironment.setVariable(IdTokenVerifier.SKIP_SIGNATURE_ENV_VAR, "true"); @@ -121,21 +124,31 @@ public void testVerify() throws Exception { clock.timeMillis = 1500000L; IdToken idToken = newIdToken(ISSUER, CLIENT_ID); assertTrue(verifier.verify(idToken)); + assertTrue(verifier.verifyPayload(idToken)); assertTrue(verifierFlexible.verify(newIdToken(ISSUER2, CLIENT_ID))); + assertTrue(verifierFlexible.verifyPayload(newIdToken(ISSUER2, CLIENT_ID))); assertFalse(verifier.verify(newIdToken(ISSUER2, CLIENT_ID))); + assertFalse(verifier.verifyPayload(newIdToken(ISSUER2, CLIENT_ID))); assertTrue(verifier.verify(newIdToken(ISSUER3, CLIENT_ID))); + assertTrue(verifier.verifyPayload(newIdToken(ISSUER3, CLIENT_ID))); // audience assertTrue(verifierFlexible.verify(newIdToken(ISSUER, CLIENT_ID2))); + assertTrue(verifierFlexible.verifyPayload(newIdToken(ISSUER, CLIENT_ID2))); assertFalse(verifier.verify(newIdToken(ISSUER, CLIENT_ID2))); + assertFalse(verifier.verifyPayload(newIdToken(ISSUER, CLIENT_ID2))); // time clock.timeMillis = 700000L; assertTrue(verifier.verify(idToken)); + assertTrue(verifier.verifyPayload(idToken)); clock.timeMillis = 2300000L; assertTrue(verifier.verify(idToken)); + assertTrue(verifier.verifyPayload(idToken)); clock.timeMillis = 699999L; assertFalse(verifier.verify(idToken)); + assertFalse(verifier.verifyPayload(idToken)); clock.timeMillis = 2300001L; assertFalse(verifier.verify(idToken)); + assertFalse(verifier.verifyPayload(idToken)); } public void testEmptyIssuersFails() throws Exception { @@ -187,28 +200,52 @@ public void testMissingAudience() throws VerificationException { public void testVerifyEs256TokenPublicKeyMismatch() throws Exception { // Mock HTTP requests - HttpTransportFactory httpTransportFactory = - new HttpTransportFactory() { + MockLowLevelHttpRequest failedRequest = + new MockLowLevelHttpRequest() { @Override - public HttpTransport create() { - return new MockHttpTransport() { - @Override - public LowLevelHttpRequest buildRequest(String method, String url) - throws IOException { - return new MockLowLevelHttpRequest() { - @Override - public LowLevelHttpResponse execute() throws IOException { - MockLowLevelHttpResponse response = new MockLowLevelHttpResponse(); - response.setStatusCode(200); - response.setContentType("application/json"); - response.setContent(""); - return response; - } - }; - } - }; + public LowLevelHttpResponse execute() throws IOException { + throw new IOException("test io exception"); + } + }; + + MockLowLevelHttpRequest badRequest = + new MockLowLevelHttpRequest() { + @Override + public LowLevelHttpResponse execute() throws IOException { + MockLowLevelHttpResponse response = new MockLowLevelHttpResponse(); + response.setStatusCode(404); + response.setContentType("application/json"); + response.setContent(""); + return response; + } + }; + + MockLowLevelHttpRequest emptyRequest = + new MockLowLevelHttpRequest() { + @Override + public LowLevelHttpResponse execute() throws IOException { + MockLowLevelHttpResponse response = new MockLowLevelHttpResponse(); + response.setStatusCode(200); + response.setContentType("application/json"); + response.setContent("{\"keys\":[]}"); + return response; + } + }; + + MockLowLevelHttpRequest goodRequest = + new MockLowLevelHttpRequest() { + @Override + public LowLevelHttpResponse execute() throws IOException { + MockLowLevelHttpResponse response = new MockLowLevelHttpResponse(); + response.setStatusCode(200); + response.setContentType("application/json"); + response.setContent(readResourceAsString("iap_keys.json")); + return response; } }; + + HttpTransportFactory httpTransportFactory = + mockTransport(failedRequest, badRequest, emptyRequest, goodRequest); IdTokenVerifier tokenVerifier = new IdTokenVerifier.Builder() .setClock(FIXED_CLOCK) @@ -219,8 +256,24 @@ public LowLevelHttpResponse execute() throws IOException { tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN)); fail("Should have failed verification"); } catch (VerificationException ex) { - assertTrue(ex.getMessage().contains("Error fetching PublicKey")); + 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) { + 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) { + 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 { @@ -284,6 +337,25 @@ static String readResourceAsString(String resourceName) throws IOException { } } + static HttpTransportFactory mockTransport(LowLevelHttpRequest... requests) { + final LowLevelHttpRequest firstRequest = requests[0]; + final Queue requestQueue = new ArrayDeque<>(); + for (LowLevelHttpRequest request : requests) { + requestQueue.add(request); + } + return new HttpTransportFactory() { + @Override + public HttpTransport create() { + return new MockHttpTransport() { + @Override + public LowLevelHttpRequest buildRequest(String method, String url) throws IOException { + return requestQueue.poll(); + } + }; + } + }; + } + static HttpTransportFactory mockTransport(String url, String certificates) { final String certificatesContent = certificates; final String certificatesUrl = url;