Skip to content

Commit 773b388

Browse files
TimurSadykovgcf-owl-bot[bot]suztomo
authored
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 <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Tomo Suzuki <suztomo@google.com>
1 parent c1b1468 commit 773b388

File tree

2 files changed

+135
-30
lines changed

2 files changed

+135
-30
lines changed

‎google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java

+42-9
Original file line numberDiff line numberDiff line change
@@ -233,12 +233,9 @@ public final Collection<String> getAudience() {
233233
* @return {@code true} if verified successfully or {@code false} if failed
234234
*/
235235
public boolean verify(IdToken idToken) {
236-
boolean tokenFieldsValid =
237-
(issuers == null || idToken.verifyIssuer(issuers))
238-
&& (audience == null || idToken.verifyAudience(audience))
239-
&& idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds);
236+
boolean payloadValid = verifyPayload(idToken);
240237

241-
if (!tokenFieldsValid) {
238+
if (!payloadValid) {
242239
return false;
243240
}
244241

@@ -254,6 +251,35 @@ public boolean verify(IdToken idToken) {
254251
}
255252
}
256253

254+
/**
255+
* Verifies the payload of the given ID token
256+
*
257+
* <p>It verifies:
258+
*
259+
* <ul>
260+
* <li>The issuer is one of {@link #getIssuers()} by calling {@link
261+
* IdToken#verifyIssuer(String)}.
262+
* <li>The audience is one of {@link #getAudience()} by calling {@link
263+
* IdToken#verifyAudience(Collection)}.
264+
* <li>The current time against the issued at and expiration time, using the {@link #getClock()}
265+
* and allowing for a time skew specified in {@link #getAcceptableTimeSkewSeconds()} , by
266+
* calling {@link IdToken#verifyTime(long, long)}.
267+
* </ul>
268+
*
269+
* <p>Overriding is allowed, but it must call the super implementation.
270+
*
271+
* @param idToken ID token
272+
* @return {@code true} if verified successfully or {@code false} if failed
273+
*/
274+
protected boolean verifyPayload(IdToken idToken) {
275+
boolean tokenPayloadValid =
276+
(issuers == null || idToken.verifyIssuer(issuers))
277+
&& (audience == null || idToken.verifyAudience(audience))
278+
&& idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds);
279+
280+
return tokenPayloadValid;
281+
}
282+
257283
@VisibleForTesting
258284
boolean verifySignature(IdToken idToken) throws VerificationException {
259285
if (Boolean.parseBoolean(environment.getVariable(SKIP_SIGNATURE_ENV_VAR))) {
@@ -272,12 +298,12 @@ boolean verifySignature(IdToken idToken) throws VerificationException {
272298
publicKeyToUse = publicKeyCache.get(certificateLocation).get(idToken.getHeader().getKeyId());
273299
} catch (ExecutionException | UncheckedExecutionException e) {
274300
throw new VerificationException(
275-
"Error fetching PublicKey from certificate location " + certificatesLocation, e);
301+
"Error fetching public key from certificate location " + certificatesLocation, e);
276302
}
277303

278304
if (publicKeyToUse == null) {
279305
throw new VerificationException(
280-
"Could not find PublicKey for provided keyId: " + idToken.getHeader().getKeyId());
306+
"Could not find public key for provided keyId: " + idToken.getHeader().getKeyId());
281307
}
282308

283309
try {
@@ -380,7 +406,7 @@ public Builder setIssuer(String issuer) {
380406
}
381407

382408
/**
383-
* Override the location URL that contains published public keys. Defaults to well-known Google
409+
* Overrides the location URL that contains published public keys. Defaults to well-known Google
384410
* locations.
385411
*
386412
* @param certificatesLocation URL to published public keys
@@ -534,7 +560,7 @@ public Map<String, PublicKey> load(String certificateUrl) throws Exception {
534560
Level.WARNING,
535561
"Failed to get a certificate from certificate location " + certificateUrl,
536562
io);
537-
return ImmutableMap.of();
563+
throw io;
538564
}
539565

540566
ImmutableMap.Builder<String, PublicKey> keyCacheBuilder = new ImmutableMap.Builder<>();
@@ -556,6 +582,13 @@ public Map<String, PublicKey> load(String certificateUrl) throws Exception {
556582
}
557583
}
558584

585+
ImmutableMap<String, PublicKey> keyCache = keyCacheBuilder.build();
586+
587+
if (keyCache.isEmpty()) {
588+
throw new VerificationException(
589+
"No valid public key returned by the keystore: " + certificateUrl);
590+
}
591+
559592
return keyCacheBuilder.build();
560593
}
561594

‎google-oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java

+93-21
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,15 @@
3333
import java.io.InputStream;
3434
import java.io.InputStreamReader;
3535
import java.io.Reader;
36+
import java.util.ArrayDeque;
3637
import java.util.Arrays;
3738
import java.util.Collections;
3839
import java.util.HashMap;
3940
import java.util.List;
4041
import java.util.Map;
42+
import java.util.Queue;
4143
import junit.framework.TestCase;
44+
import org.junit.Assert;
4245

4346
/**
4447
* Tests {@link IdTokenVerifier}.
@@ -101,7 +104,7 @@ public void testBuilder() throws Exception {
101104
assertEquals(TRUSTED_CLIENT_IDS, Lists.newArrayList(verifier.getAudience()));
102105
}
103106

104-
public void testVerify() throws Exception {
107+
public void testVerifyPayload() throws Exception {
105108
MockClock clock = new MockClock();
106109
MockEnvironment testEnvironment = new MockEnvironment();
107110
testEnvironment.setVariable(IdTokenVerifier.SKIP_SIGNATURE_ENV_VAR, "true");
@@ -121,21 +124,31 @@ public void testVerify() throws Exception {
121124
clock.timeMillis = 1500000L;
122125
IdToken idToken = newIdToken(ISSUER, CLIENT_ID);
123126
assertTrue(verifier.verify(idToken));
127+
assertTrue(verifier.verifyPayload(idToken));
124128
assertTrue(verifierFlexible.verify(newIdToken(ISSUER2, CLIENT_ID)));
129+
assertTrue(verifierFlexible.verifyPayload(newIdToken(ISSUER2, CLIENT_ID)));
125130
assertFalse(verifier.verify(newIdToken(ISSUER2, CLIENT_ID)));
131+
assertFalse(verifier.verifyPayload(newIdToken(ISSUER2, CLIENT_ID)));
126132
assertTrue(verifier.verify(newIdToken(ISSUER3, CLIENT_ID)));
133+
assertTrue(verifier.verifyPayload(newIdToken(ISSUER3, CLIENT_ID)));
127134
// audience
128135
assertTrue(verifierFlexible.verify(newIdToken(ISSUER, CLIENT_ID2)));
136+
assertTrue(verifierFlexible.verifyPayload(newIdToken(ISSUER, CLIENT_ID2)));
129137
assertFalse(verifier.verify(newIdToken(ISSUER, CLIENT_ID2)));
138+
assertFalse(verifier.verifyPayload(newIdToken(ISSUER, CLIENT_ID2)));
130139
// time
131140
clock.timeMillis = 700000L;
132141
assertTrue(verifier.verify(idToken));
142+
assertTrue(verifier.verifyPayload(idToken));
133143
clock.timeMillis = 2300000L;
134144
assertTrue(verifier.verify(idToken));
145+
assertTrue(verifier.verifyPayload(idToken));
135146
clock.timeMillis = 699999L;
136147
assertFalse(verifier.verify(idToken));
148+
assertFalse(verifier.verifyPayload(idToken));
137149
clock.timeMillis = 2300001L;
138150
assertFalse(verifier.verify(idToken));
151+
assertFalse(verifier.verifyPayload(idToken));
139152
}
140153

141154
public void testEmptyIssuersFails() throws Exception {
@@ -187,28 +200,52 @@ public void testMissingAudience() throws VerificationException {
187200

188201
public void testVerifyEs256TokenPublicKeyMismatch() throws Exception {
189202
// Mock HTTP requests
190-
HttpTransportFactory httpTransportFactory =
191-
new HttpTransportFactory() {
203+
MockLowLevelHttpRequest failedRequest =
204+
new MockLowLevelHttpRequest() {
192205
@Override
193-
public HttpTransport create() {
194-
return new MockHttpTransport() {
195-
@Override
196-
public LowLevelHttpRequest buildRequest(String method, String url)
197-
throws IOException {
198-
return new MockLowLevelHttpRequest() {
199-
@Override
200-
public LowLevelHttpResponse execute() throws IOException {
201-
MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
202-
response.setStatusCode(200);
203-
response.setContentType("application/json");
204-
response.setContent("");
205-
return response;
206-
}
207-
};
208-
}
209-
};
206+
public LowLevelHttpResponse execute() throws IOException {
207+
throw new IOException("test io exception");
208+
}
209+
};
210+
211+
MockLowLevelHttpRequest badRequest =
212+
new MockLowLevelHttpRequest() {
213+
@Override
214+
public LowLevelHttpResponse execute() throws IOException {
215+
MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
216+
response.setStatusCode(404);
217+
response.setContentType("application/json");
218+
response.setContent("");
219+
return response;
220+
}
221+
};
222+
223+
MockLowLevelHttpRequest emptyRequest =
224+
new MockLowLevelHttpRequest() {
225+
@Override
226+
public LowLevelHttpResponse execute() throws IOException {
227+
MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
228+
response.setStatusCode(200);
229+
response.setContentType("application/json");
230+
response.setContent("{\"keys\":[]}");
231+
return response;
232+
}
233+
};
234+
235+
MockLowLevelHttpRequest goodRequest =
236+
new MockLowLevelHttpRequest() {
237+
@Override
238+
public LowLevelHttpResponse execute() throws IOException {
239+
MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
240+
response.setStatusCode(200);
241+
response.setContentType("application/json");
242+
response.setContent(readResourceAsString("iap_keys.json"));
243+
return response;
210244
}
211245
};
246+
247+
HttpTransportFactory httpTransportFactory =
248+
mockTransport(failedRequest, badRequest, emptyRequest, goodRequest);
212249
IdTokenVerifier tokenVerifier =
213250
new IdTokenVerifier.Builder()
214251
.setClock(FIXED_CLOCK)
@@ -219,8 +256,24 @@ public LowLevelHttpResponse execute() throws IOException {
219256
tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN));
220257
fail("Should have failed verification");
221258
} catch (VerificationException ex) {
222-
assertTrue(ex.getMessage().contains("Error fetching PublicKey"));
259+
assertTrue(ex.getMessage().contains("Error fetching public key"));
260+
}
261+
262+
try {
263+
tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN));
264+
fail("Should have failed verification");
265+
} catch (VerificationException ex) {
266+
assertTrue(ex.getMessage().contains("Error fetching public key"));
223267
}
268+
269+
try {
270+
tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN));
271+
fail("Should have failed verification");
272+
} catch (VerificationException ex) {
273+
assertTrue(ex.getCause().getMessage().contains("No valid public key returned"));
274+
}
275+
276+
Assert.assertTrue(tokenVerifier.verifySignature(IdToken.parse(JSON_FACTORY, ES256_TOKEN)));
224277
}
225278

226279
public void testVerifyEs256Token() throws VerificationException, IOException {
@@ -284,6 +337,25 @@ static String readResourceAsString(String resourceName) throws IOException {
284337
}
285338
}
286339

340+
static HttpTransportFactory mockTransport(LowLevelHttpRequest... requests) {
341+
final LowLevelHttpRequest firstRequest = requests[0];
342+
final Queue<LowLevelHttpRequest> requestQueue = new ArrayDeque<>();
343+
for (LowLevelHttpRequest request : requests) {
344+
requestQueue.add(request);
345+
}
346+
return new HttpTransportFactory() {
347+
@Override
348+
public HttpTransport create() {
349+
return new MockHttpTransport() {
350+
@Override
351+
public LowLevelHttpRequest buildRequest(String method, String url) throws IOException {
352+
return requestQueue.poll();
353+
}
354+
};
355+
}
356+
};
357+
}
358+
287359
static HttpTransportFactory mockTransport(String url, String certificates) {
288360
final String certificatesContent = certificates;
289361
final String certificatesUrl = url;

0 commit comments

Comments
 (0)
  翻译：