Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add self signed jwt support #572

Merged
merged 14 commits into from Mar 16, 2021
Merged

feat: add self signed jwt support #572

merged 14 commits into from Mar 16, 2021

Conversation

arithmetic1728
Copy link
Collaborator

@arithmetic1728 arithmetic1728 commented Feb 16, 2021

This PR adds self signed jwt support (https://google.aip.dev/auth/4111). Googlers see this go/yoshi-self-signed-jwt for more details. If user uses service account credentials, and doesn't provide custom endpoint/scopes/audience, then self signed jwt should be used. Since custom endpoint is now supported (cl/356594013) and users cannot provide default audience for ServiceAccountCredentials, we automatically use self signed jwt if scopes are not provided by the users.

Details

  1. Scopes.
    We add a new defaultScopes (for client lib) to GoogleCredentials to distinguish with the existing scopes (for users), and updated the subclasses.
  • For AppEngineCredentials, ComputeEngineCredentials classes, defaultScopes will used if scopes is not provided, there are no other changes.
  • For ServiceAccountCredentials, if scopes is not provided, then a ServiceAccountJWTAccessCredentials instance will be created inside ServiceAccountCredentials, and will be used for self signed jwt.
  • For other type of credentials where scopes are not used, defaultScopes has no effect.
  1. Simplified self signed jwt audience. Googlers see go/simpler-JWT-token-audience
    When self signed jwt is used for service account credentials, in getRequestMetadata(uri), we replace the uri with its http(s)://{host_name}/ prefix. For instance, http client normally passes uri like https://compute.googleapis.com/compute/v1/projects/, but the correct audience should be https://compute.googleapis.com/.

Follow up PRs

It will be trivial changes to gax-java and client libs to integrate this feature.

This PR shows how gax-java integrates the feature. gax-java just needs to pass defaultScopes to the auth lib.

This PR shows how GAPIC client integrates the feature. GAPIC client just needs to pass the default scopes via setDefaultScopes instead of setScopesToApply.

@arithmetic1728 arithmetic1728 requested a review from as a code owner Feb 16, 2021
@google-cla google-cla bot added the cla: yes label Feb 16, 2021
@codecov
Copy link

@codecov codecov bot commented Feb 16, 2021

Codecov Report

Merging #572 (8af2eaa) into master (497d4e7) will increase coverage by 0.12%.
The diff coverage is 86.15%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #572      +/-   ##
============================================
+ Coverage     83.32%   83.45%   +0.12%     
- Complexity      571      594      +23     
============================================
  Files            41       41              
  Lines          2645     2695      +50     
  Branches        274      286      +12     
============================================
+ Hits           2204     2249      +45     
- Misses          301      303       +2     
- Partials        140      143       +3     
Impacted Files Coverage Δ Complexity Δ
...java/com/google/auth/oauth2/GoogleCredentials.java 61.70% <0.00%> (-1.35%) 11.00 <0.00> (ø)
...google/auth/oauth2/DefaultCredentialsProvider.java 81.39% <50.00%> (+0.92%) 34.00 <0.00> (+1.00)
...uth/oauth2/ServiceAccountJwtAccessCredentials.java 75.30% <71.42%> (-0.34%) 42.00 <6.00> (+2.00) ⬇️
.../google/auth/oauth2/ServiceAccountCredentials.java 83.44% <88.88%> (+0.50%) 64.00 <17.00> (+12.00)
...a/com/google/auth/oauth2/AppEngineCredentials.java 86.36% <91.66%> (+1.61%) 19.00 <8.00> (+4.00)
...m/google/auth/oauth2/ComputeEngineCredentials.java 84.86% <100.00%> (+0.97%) 39.00 <7.00> (+4.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 497d4e7...8af2eaa. Read the comment docs.

@arithmetic1728
Copy link
Collaborator Author

@arithmetic1728 arithmetic1728 commented Mar 2, 2021

@bshaffer Hi Brent, could you take a look?

@arithmetic1728 arithmetic1728 added the do not merge label Mar 8, 2021
@arithmetic1728
Copy link
Collaborator Author

@arithmetic1728 arithmetic1728 commented Mar 8, 2021

Don't merge until cl/356594013 is fully rolled out.

@arithmetic1728 arithmetic1728 removed the do not merge label Mar 15, 2021
@arithmetic1728
Copy link
Collaborator Author

@arithmetic1728 arithmetic1728 commented Mar 15, 2021

cl/356594013 is fully rolled out. Tested the change with pubsub regional endpoint and it worked fine.

@arithmetic1728 arithmetic1728 added the kokoro:force-run label Mar 15, 2021
@kokoro-team kokoro-team removed the kokoro:force-run label Mar 15, 2021
@arithmetic1728 arithmetic1728 added the kokoro:force-run label Mar 16, 2021
@kokoro-team kokoro-team removed the kokoro:force-run label Mar 16, 2021
@arithmetic1728 arithmetic1728 merged commit efe103a into master Mar 16, 2021
15 checks passed
@arithmetic1728 arithmetic1728 deleted the self_signed_jwt branch Mar 16, 2021
@suztomo
Copy link
Member

@suztomo suztomo commented Mar 18, 2021

@arithmetic1728 @bshaffer It seems that this commit (the only change between 0.24.1 and 0.25.0) broke the Java sample projects. GoogleCloudPlatform/java-docs-samples#4903 Would you look into the failures?

I've invited both of you to "Yoshi Java" chat room.

@elefeint
Copy link

@elefeint elefeint commented Mar 18, 2021

It also broke Spring Cloud GCP by way of bigquery, config, storage and vision: https://github.com/GoogleCloudPlatform/spring-cloud-gcp/actions/runs/664693361

@ejona86
Copy link
Contributor

@ejona86 ejona86 commented Apr 15, 2021

@arithmetic1728, when merging PRs, could you please use helpful commit messages? You wrote so much good content for this PR, but then it is mostly just garbage on the commit itself: efe103a

@arithmetic1728
Copy link
Collaborator Author

@arithmetic1728 arithmetic1728 commented Apr 15, 2021

@ejona86 Yep, I will edit the commit message in the future when merging PR. Thank you for pointing it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants