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 workload identity federation support #547

Merged
merged 30 commits into from
Feb 18, 2021
Merged

feat: add workload identity federation support #547

merged 30 commits into from
Feb 18, 2021

Conversation

lsirac
Copy link
Contributor

@lsirac lsirac commented Jan 28, 2021

See go/guac-3pi-java.

  • Add documentation/snippets

lsirac and others added 16 commits August 5, 2020 16:10
* feat: adds an internal token exchange utility based on https://tools.ietf.org/html/rfc8693

* fix: add copyright and address other comments

* fix: fix formatting

* fix: fixes copyright again

* fix: revert pom changes

* fix: revert auto-value changes

* fix: remove gson and address other comments

* fix: fixes to StsRequestHandlerTest
* feat: adds base external account credentials class and support for file/url based external credentials

* fix: javadoc changes

* fix: address review comments

* fix: nits

* fix: fix broken test

* fix: format
* feat: implements AWS signature version 4 for signing requests

* fix: fix javadoc

* fix: address review comments

* fix: changes to visibility and addresses other review comments

* fix: removes sortedHeaderNames from AwsRequestSignature, uses MessageDigest, and misc changes

* feat: generate authorization header in AwsRequestSigner

* fix: address more review comments

* fix: use RuntimeExceptions instead of invalid state/argument

* fix: javadoc

* fix: handle invalid input in Builder & misc fixes

* fix: get dates at construction and no longer catch ParseException in AwsDates

* fix: refactor AwsDates
* feat: adds text/json credential source support to IdentityPoolCredentials

* fix: format

* fix: format

* fix: add missing changes to MockExternalAccountCredentialsTransport

* fix: change parseToken to take an InputStream

* fix: charsets

* fix: broken build

* fix: type null check
* feat: adds support for AWS credentials

* fix: address nits

* fix: remove Truth lib use in AwsCredentialsTest

* fix: address more review comments

* fix: assertEquals param order

* feat: retrieve region from environment variable for AWS Lambda
…for 3pi (#501)

* chore: use ImpersonatedCredentials for service account impersonation in ExternalAccountCredentials

* chore: add test for invalid service account impersonation url
* fix: update AwsCredentials credential source logic

* fix: remove TODOs

* fix: cleanup

* fix: code review nits

* fix: fix broken test

* fix: lint
@lsirac lsirac requested a review from a team as a code owner January 28, 2021 23:42
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 28, 2021
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #547 (c7f5730) into master (2142db3) will increase coverage by 3.39%.
The diff coverage is 93.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #547      +/-   ##
============================================
+ Coverage     79.92%   83.32%   +3.39%     
- Complexity      421      571     +150     
============================================
  Files            28       41      +13     
  Lines          1978     2645     +667     
  Branches        215      274      +59     
============================================
+ Hits           1581     2204     +623     
- Misses          284      301      +17     
- Partials        113      140      +27     
Impacted Files Coverage Δ Complexity Δ
...java/com/google/auth/oauth2/GoogleCredentials.java 63.04% <50.00%> (+1.67%) 11.00 <0.00> (+1.00)
...om/google/auth/oauth2/IdentityPoolCredentials.java 86.02% <86.02%> (ø) 12.00 <12.00> (?)
...om/google/auth/oauth2/StsTokenExchangeRequest.java 86.84% <86.84%> (ø) 19.00 <19.00> (?)
.../java/com/google/auth/oauth2/AwsRequestSigner.java 88.67% <88.67%> (ø) 19.00 <19.00> (?)
...m/google/auth/oauth2/StsTokenExchangeResponse.java 90.32% <90.32%> (ø) 8.00 <8.00> (?)
...tp/java/com/google/auth/oauth2/AwsCredentials.java 94.17% <94.17%> (ø) 16.00 <16.00> (?)
...va/com/google/auth/oauth2/AwsRequestSignature.java 97.50% <97.50%> (ø) 9.00 <9.00> (?)
...google/auth/oauth2/ExternalAccountCredentials.java 97.61% <97.61%> (ø) 26.00 <26.00> (?)
...java/com/google/auth/oauth2/StsRequestHandler.java 98.68% <98.68%> (ø) 17.00 <17.00> (?)
..._http/java/com/google/auth/oauth2/ActingParty.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
... and 17 more

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 2142db3...c7f5730. Read the comment docs.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot easier to review if you keep it on the same branch. Earlier comments apply.

@elharo elharo requested a review from silvolu January 29, 2021 12:46
@lsirac lsirac requested a review from elharo February 2, 2021 01:34
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge master

@lsirac lsirac requested a review from elharo February 3, 2021 00:31
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge in master

@lsirac lsirac changed the title feat: adds support for 3p identities feat: workload identity federation support Feb 10, 2021
Copy link

@silvolu silvolu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auth bits look good, please resolve comments from @elharo

@lsirac lsirac requested a review from elharo February 12, 2021 04:10
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge in master

@lsirac lsirac requested a review from elharo February 12, 2021 20:13
@lsirac lsirac requested a review from elharo February 13, 2021 00:14
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth considering if any of the public classes and methods should be marked @Beta. In the near future it's going to be getting a lot harder to change something once we've shipped it than it has been in the past.

@lsirac lsirac requested a review from elharo February 13, 2021 00:59
*
* <p>By default, attempts to exchange the external credential for a GCP access token.
*/
public class AwsCredentials extends ExternalAccountCredentials {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this class need to be public? It looks like the only public methods are overridden or visible for testing, so I'm guessing no. The less API we have to publish and support ever after the better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep this public. Developers can initialize an AwsCredential and use it directly, and other credentials follow that pattern. They also need to be able to create an AwsCredential with specific scopes.

implements QuotaProjectIdProvider {

/** Base credential source class. Dictates the retrieval method of the external credential. */
abstract static class CredentialSource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could/should this be an outer class? ExternalAccountCredentials is so large github won't even display it by default, which suggests it might help to break it up.

Copy link
Contributor Author

@lsirac lsirac Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this makes sense as an outer class. It's required for all external credentials and not used in any other context. It's also confusing if you come across it outside of this class.

@lsirac lsirac requested a review from elharo February 17, 2021 20:51
throw new IllegalArgumentException("Invalid AWS environment ID.");
}

int environmentVersion = Integer.parseInt(matcher.group(2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more simply, this looks like the string simply equals the literal "aws1"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm we have to parse the version anyway for the error message

@chingor13 chingor13 changed the title feat: workload identity federation support feat: add workload identity federation support Feb 18, 2021
@chingor13 chingor13 merged commit b8dde1e into googleapis:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants