fix: add signature verification to IdTokenVerifier by TimurSadykov · Pull Request #861 · googleapis/google-oauth-java-client

TimurSadykov · 2022-03-25T22:41:05Z

Porting the functionality from google-auth-library-java, see https://github.com/googleapis/google-auth-library-java/blob/1434839e704621dc21f902c81cb67e66f7ef1a05/oauth2_http/java/com/google/auth/oauth2/TokenVerifier.java#L81

Fixes #786 ☕️

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

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

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

suztomo · 2022-03-29T19:46:49Z

Lint is failing:

Error:  Found 7 non-complying files, failing build
Error:  To fix formatting errors, run "mvn com.coveo:fmt-maven-plugin:format"
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeResponseUrl.java
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/oauth/OAuthGetAccessToken.java
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/oauth/AbstractOAuthGetToken.java
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/oauth/OAuthAuthorizeTemporaryTokenUrl.java
Error:  Non complying file: /home/runner/work/google-oauth-java-client/google-oauth-java-client/google-oauth-client/src/main/java/com/google/api/client/auth/oauth/OAuthGetTemporaryToken.java

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

suztomo · 2022-03-29T19:50:12Z

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

-    return (issuers == null || idToken.verifyIssuer(issuers))
-        && (audience == null || idToken.verifyAudience(audience))
-        && idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds);
+  public boolean verify(IdToken idToken) throws VerificationException {


Would you improve the Javadoc that explains the newly-added case?

Creating a new function IdToken.verifySignature() will fit better in the implementation.

I'm surprised the breaking change detector doesn't see adding a checked exception as a breaking change.

It also feels a bit weird to have 2 different mechanisms for indicating an invalid token. In the google-auth-library implementation, we only throw VerificationExceptions (to be able to communicate why it failed). This legacy implementation already is hiding why the token in invalid.

Another option, in line with Tomo's suggestion is to add a new verifySignature method that throws VerificationException. verify could call verifySignature and catch the exception and return false (to not change the interface)

Good catch, thanks, especially the exception :)

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

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

suztomo · 2022-03-29T20:05:51Z

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

+        jwks = response.parseAs(JsonWebKeySet.class);
+      } catch (IOException io) {
+        return ImmutableMap.of();


This doesn't show any error or exceptions when the certificate doesn't have the expected format. Is there.a good way to report the problem?

The suggestion definitely makes sense, I left it as is since it looks as intentional. I tried to figure out the background for this and it looks like - it is related to LoadingCache implementation, like unable to throw checked exception from runner or throwing RuntimeExceptions etc. My call is to leave this specific one as is to avoid gotchas because:

it does not seem to be a pain point for other library

we want to error on the side of reliability at the moment, we can improve experience later

library is already officially deprecated

@chingor13 are you ok with that?

How about logging (java.util.logging.Logger) with warn level?

google-oauth-client/pom.xml

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

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

chingor13 · 2022-03-30T16:33:45Z

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

+    this.publicKeyCache =
+        CacheBuilder.newBuilder()
+            .expireAfterWrite(1, TimeUnit.HOURS)
+            .build(new PublicKeyLoader(builder.httpTransportFactory));


httpTransportFactory is now required or you'll get a NPE later. We generally prefer to use preconditions to throw the NPE early for required attributes (using guava's Preconditions.checkNotNull)

Good catch, thanks, I think here we want to set a default ourselves in case of null, otherwise it is going to be a breaking change

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

chingor13 · 2022-03-30T16:41:48Z

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

-    return (issuers == null || idToken.verifyIssuer(issuers))
-        && (audience == null || idToken.verifyAudience(audience))
-        && idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds);
+  public boolean verify(IdToken idToken) throws VerificationException {


I'm surprised the breaking change detector doesn't see adding a checked exception as a breaking change.

It also feels a bit weird to have 2 different mechanisms for indicating an invalid token. In the google-auth-library implementation, we only throw VerificationExceptions (to be able to communicate why it failed). This legacy implementation already is hiding why the token in invalid.

chingor13 · 2022-03-30T16:43:37Z

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

-    return (issuers == null || idToken.verifyIssuer(issuers))
-        && (audience == null || idToken.verifyAudience(audience))
-        && idToken.verifyTime(clock.currentTimeMillis(), acceptableTimeSkewSeconds);
+  public boolean verify(IdToken idToken) throws VerificationException {


Another option, in line with Tomo's suggestion is to add a new verifySignature method that throws VerificationException. verify could call verifySignature and catch the exception and return false (to not change the interface)

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…tory

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

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

suztomo · 2022-03-31T16:34:40Z

The failing lint check is addressed in #863 (merged)

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

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

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

suztomo · 2022-04-12T02:26:38Z

@chingor13 PTAL

suztomo · 2022-04-12T02:31:34Z

Memo: OwlBot reverted the lint fix (#863) in the commit below (right after I merged 863), and now lint is failing in main (So this PR 861 is irrelevant to the lint failure). The lint is not a required check.

b2b2e43

…ogleapis#861) * previously missing signature validation ported from google-auth-library-java * test cases Co-authored-by: Tomo Suzuki <suztomo@google.com> (cherry picked from commit 22419d6)

EmpireMyDev

Nice

Timur Sadykov added 2 commits March 23, 2022 03:16

verifier signature fix port

acb985f

more test cases

1c16f8f

TimurSadykov requested a review from a team as a code owner March 25, 2022 22:41

gcf-owl-bot bot and others added 3 commits March 25, 2022 22:43

🦉 Updates from OwlBot post-processor

872f5f6

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

fix: more test fixes

70ac584

🦉 Updates from OwlBot post-processor

5dde913

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

TimurSadykov requested review from chingor13, lesv and suztomo March 28, 2022 20:17

suztomo reviewed Mar 29, 2022

View reviewed changes

google-oauth-client/src/main/java/com/google/api/client/auth/openidconnect/IdTokenVerifier.java Show resolved Hide resolved

suztomo reviewed Mar 29, 2022

View reviewed changes

TimurSadykov self-assigned this Mar 29, 2022

chingor13 requested changes Mar 30, 2022

View reviewed changes

Timur Sadykov and others added 4 commits March 30, 2022 11:33

Update google-oauth-client/src/main/java/com/google/api/client/auth/o…

5c83d47

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

🦉 Updates from OwlBot post-processor

17affb3

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

fix: restored original interface for verifier, added default http fac…

4d34201

…tory

🦉 Updates from OwlBot post-processor

9611f53

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

suztomo reviewed Mar 31, 2022

View reviewed changes

Timur Sadykov and others added 5 commits March 31, 2022 10:40

Update google-oauth-client/src/main/java/com/google/api/client/auth/o…

8b55b29

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

Update google-oauth-client/src/main/java/com/google/api/client/auth/o…

813bb20

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

doc fixes

e065d1a

fix: better docs and logging of validation exceptions

1af2370

🦉 Updates from OwlBot post-processor

22178ca

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

suztomo reviewed Apr 4, 2022

View reviewed changes

Timur Sadykov and others added 2 commits April 11, 2022 18:05

Update google-oauth-client/src/main/java/com/google/api/client/auth/o…

c8d01d3

…penidconnect/IdTokenVerifier.java Co-authored-by: Tomo Suzuki <suztomo@google.com>

nit and linter fixes

aca11aa

🦉 Updates from OwlBot post-processor

fece412

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

suztomo approved these changes Apr 12, 2022

View reviewed changes

chingor13 approved these changes Apr 12, 2022

View reviewed changes

TimurSadykov merged commit 22419d6 into main Apr 13, 2022

TimurSadykov deleted the stim-signer branch April 13, 2022 18:21

release-please bot mentioned this pull request Apr 13, 2022

chore(main): release 1.33.3 #872

Merged

EmpireMyDev reviewed Dec 29, 2022

View reviewed changes

cx-sam-headrick mentioned this pull request Mar 14, 2023

CVE-2021-22573 @ Maven-com.google.oauth-client:google-oauth-client-1.30.3 cx-sam-headrick/easybuggy4sb#48

Open

Conversation

TimurSadykov commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suztomo commented Mar 29, 2022

Uh oh!

Uh oh!

suztomo Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimurSadykov Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimurSadykov Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suztomo commented Mar 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suztomo commented Apr 12, 2022

Uh oh!

suztomo commented Apr 12, 2022

Uh oh!

EmpireMyDev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TimurSadykov commented Mar 25, 2022 •

edited

Loading

suztomo Mar 29, 2022 •

edited

Loading

TimurSadykov Mar 30, 2022 •

edited

Loading

TimurSadykov Mar 30, 2022 •

edited

Loading