fix: fix IdTokenVerifier so it does not cache empty entries #892
Conversation
…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <[email protected]>
…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <[email protected]>
…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <[email protected]>
…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <[email protected]>
TimurSadykov
changed the title
...oauth-client/src/test/java/com/google/api/client/auth/openidconnect/IdTokenVerifierTest.java
Outdated
Show resolved
Hide resolved
| @@ -534,7 +560,7 @@ public Map<String, PublicKey> load(String certificateUrl) throws Exception { | |||
| Level.WARNING, | |||
| "Failed to get a certificate from certificate location " + certificateUrl, | |||
| io); | |||
| return ImmutableMap.of(); | |||
| throw io; | |||
There was a problem hiding this comment.
Memo for myself.
Now this throws an exception. With Guava's LoadingCache, the exception surfaces as ExecutionException in publicKeyCache.get in verifySignature method. This verifySignature method wraps it as VerificationException. The verify method catches VerificationException, logs the SEVERE-level message, and returns false.
With this change the LoadingCache now stops caching failure result. A potential problem would be the library users calling the verify method repeatedly in a situation where the certificate location is inaccessible. From the caller's perspective, the result (getting false) remains the same. In that case IdTokenVerifier$PublicKeyLoader logs the warning message. The user can easily tell the problem. This should be fine.
There was a problem hiding this comment.
Yep, had similar thoughts on this
|
linter failure is expected, known owl-bot issue |
🤖 I have created a release *beep* *boop* --- ## [1.34.0](v1.33.3...v1.34.0) (2022-06-02) ### Features * add build scripts for native image testing in Java 17 ([#1440](https://github.com/googleapis/google-oauth-java-client/issues/1440)) ([#890](#890)) ([373891e](373891e)) * next release from main branch is 1.34.0 ([#875](#875)) ([187651e](187651e)) ### Bug Fixes * fix IdTokenVerifier so it does not cache empty entries ([#892](#892)) ([773b388](773b388)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes idtoken verifier so that it will not cache empty entries in cases of intermittent problems accessing the keystore
Also, it makes the payload verification protected so child classes can use it separately in case they do signature verification themselves
Fixes #891 ☕️