Navigation Menu

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 impersonation credentials to ADC #613

Merged
merged 33 commits into from May 21, 2021
Merged

feat: add impersonation credentials to ADC #613

merged 33 commits into from May 21, 2021

Conversation

liuchaoren
Copy link
Collaborator

@liuchaoren liuchaoren commented Mar 25, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #614 ☕️

@liuchaoren liuchaoren requested a review from a team as a code owner March 25, 2021 19:42
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2021
@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #613 (55a0c61) into master (292e81a) will increase coverage by 0.38%.
The diff coverage is 89.18%.

❗ Current head 55a0c61 differs from pull request most recent head b5340d0. Consider uploading reports for the commit b5340d0 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master     #613      +/-   ##
============================================
+ Coverage     83.59%   83.98%   +0.38%     
- Complexity      606      622      +16     
============================================
  Files            42       42              
  Lines          2712     2778      +66     
  Branches        289      295       +6     
============================================
+ Hits           2267     2333      +66     
+ Misses          303      300       -3     
- Partials        142      145       +3     
Impacted Files Coverage Δ Complexity Δ
...java/com/google/auth/oauth2/GoogleCredentials.java 63.26% <50.00%> (+1.56%) 12.00 <0.00> (+1.00)
...om/google/auth/oauth2/ImpersonatedCredentials.java 90.67% <90.00%> (+4.27%) 33.00 <17.00> (+17.00)
...google/auth/oauth2/ExternalAccountCredentials.java 98.46% <100.00%> (+0.70%) 27.00 <0.00> (-2.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 3f90c81...b5340d0. Read the comment docs.

@liuchaoren liuchaoren changed the title Add impersonation credentials to ADC. feat: add impersonation credentials to ADC Mar 25, 2021
import java.io.IOException;

/** Indicates that the provided credential has a unsupported type. */
public class CredentialTypeException extends IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This doesn't feel like an IOException.
  2. This is untested. This class is simple but it suggests there's a code path elsewhere that should throw this that isn't tested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this customized exception and adopt the same strategy as

to avoid introducing one more exception in the throw which will propagate to upper level code.

String serviceAccountImpersonationUrl = (String) json.get("service_account_impersonation_url");
String targetPrincipal = extractTargetPrincipal(serviceAccountImpersonationUrl);

List<String> delegates = (List<String>) json.get("delegates");
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like they're a lot of potential ClassCastExceptions here that are not being handled. Where does the json object come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The json object is passed from

and it reads credential json file from disk. The json file is generated by gcloud which should be well-formatted and user shouldn't manually edit the file. We have done something similar in

Copy link
Contributor

Choose a reason for hiding this comment

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

The code should check this input and be prepared to handle a malformed or invalid file. It does not know where the file came from or how it might have been modified since it was generated. Do not assume it has the format you expect. Even if it's right today, the output of gcloud could change in the future.

If other code already makes this mistake, it should be fixed too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fair enough, thanks for pointing this out! I added the exception handling and added a test to cover it. Please take another look.

elharo
elharo previously requested changes Apr 2, 2021
String serviceAccountImpersonationUrl = (String) json.get("service_account_impersonation_url");
String targetPrincipal = extractTargetPrincipal(serviceAccountImpersonationUrl);

List<String> delegates = (List<String>) json.get("delegates");
Copy link
Contributor

Choose a reason for hiding this comment

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

The code should check this input and be prepared to handle a malformed or invalid file. It does not know where the file came from or how it might have been modified since it was generated. Do not assume it has the format you expect. Even if it's right today, the output of gcloud could change in the future.

If other code already makes this mistake, it should be fixed too.

@liuchaoren liuchaoren requested a review from elharo April 2, 2021 19:12
@@ -54,6 +54,7 @@
static final String QUOTA_PROJECT_ID_HEADER_KEY = "x-goog-user-project";
static final String USER_FILE_TYPE = "authorized_user";
static final String SERVICE_ACCOUNT_FILE_TYPE = "service_account";
static final String IMPERSONATED_SERVICE_ACCOUNT = "impersonated_service_account";
Copy link
Contributor

@elharo elharo Apr 2, 2021

Choose a reason for hiding this comment

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

This constant does not improve on the literal string. I suggest just using the string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am debating on these two options. I think even though the constant is not more readable than the string itself. I like the way we define all credential types in one place. So people can see all supported types there. I am not very familiar with Java convention, I trust your judgement on this. Please let me know if you still want me to make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's something that belongs in documentation. Don't rely on the code for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -144,7 +148,52 @@ public static ImpersonatedCredentials create(
}

/**
* @param sourceCredentials the source credential used as to acquire the impersonated credentials
* @param sourceCredentials the source credential used as to acquire the impersonated credentials.
Copy link
Contributor

Choose a reason for hiding this comment

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

delete "as",
delete final period since it's not a complete sentence

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

private final String transportFactoryClassName;

private transient HttpTransportFactory transportFactory;

/**
* @param sourceCredentials the source credential used as to acquire the impersonated credentials
* @param sourceCredentials the source credential used as to acquire the impersonated credentials.
Copy link
Contributor

Choose a reason for hiding this comment

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

delete "as",
delete final period since it's not a complete sentence

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@elharo elharo dismissed their stale review April 2, 2021 22:00

resolved

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.

Please make sure you get approval from @elharo before merging.

elharo
elharo previously requested changes Apr 7, 2021
@@ -54,6 +54,7 @@
static final String QUOTA_PROJECT_ID_HEADER_KEY = "x-goog-user-project";
static final String USER_FILE_TYPE = "authorized_user";
static final String SERVICE_ACCOUNT_FILE_TYPE = "service_account";
static final String IMPERSONATED_SERVICE_ACCOUNT = "impersonated_service_account";
Copy link
Contributor

Choose a reason for hiding this comment

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

That's something that belongs in documentation. Don't rely on the code for that.

assertEquals(QUOTA_PROJECT_ID, credentials.getQuotaProjectId());
assertEquals(DELEGATES, credentials.getDelegates());
assertEquals(new ArrayList<String>(), credentials.getScopes());
assertEquals(credentials.getLifetime(), 3600);
Copy link
Contributor

Choose a reason for hiding this comment

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

expected value comes first; swap the arguments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I made the change in new commits to my branch https://github.com/liuchaoren/google-auth-library-java which should update the PR automatically. Somehow the new commits do not show up in the PR. There might be some lag. I will keep an eye on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New commits show up in the PR now.

assertNull(credentials.getQuotaProjectId());
assertEquals(new ArrayList<String>(), credentials.getDelegates());
assertEquals(new ArrayList<String>(), credentials.getScopes());
assertEquals(credentials.getLifetime(), 3600);
Copy link
Contributor

Choose a reason for hiding this comment

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

expected value comes first; swap the arguments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

assertNull(credentials.getQuotaProjectId());
assertEquals(DELEGATES, credentials.getDelegates());
assertEquals(new ArrayList<String>(), credentials.getScopes());
assertEquals(credentials.getLifetime(), 3600);
Copy link
Contributor

Choose a reason for hiding this comment

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

expected value comes first; swap the arguments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

assertEquals(QUOTA_PROJECT_ID, credentials.getQuotaProjectId());
assertEquals(DELEGATES, credentials.getDelegates());
assertEquals(new ArrayList<String>(), credentials.getScopes());
assertEquals(credentials.getLifetime(), 3600);
Copy link
Contributor

Choose a reason for hiding this comment

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

expected value comes first; swap the arguments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

return this;
}

public String getQuotaProjectId() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is not tested. Is it needed at all? It can probably be removed at no loss.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing it out! It is not used and I removed it.

} catch (ClassCastException | NullPointerException e) {
throw new CredentialFormatException("An invalid input stream was provided.", e);
}
String targetPrincipal = extractTargetPrincipal(serviceAccountImpersonationUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

need to handle the IllegalArgumentException here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, it is handled.

@liuchaoren liuchaoren requested a review from elharo May 5, 2021 14:06
USER_ACCOUNT_CLIENT_ID,
USER_ACCOUNT_CLIENT_SECRET,
REFRESH_TOKEN);
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory because style does not abbreviate in variable names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

USER_ACCOUNT_CLIENT_SECRET,
REFRESH_TOKEN);
json.remove("delegates");
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

public void fromJson_ServiceAccountAsSource() throws IOException {
GenericJson json =
buildImpersonationCredentialsJson(IMPERSONATION_URL, DELEGATES, QUOTA_PROJECT_ID);
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Test()
public void fromJson_InvalidFormat() throws IOException {
GenericJson json = buildInvalidCredentialsJson();
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Test()
public void createScoped() throws IOException {
GoogleCredentials sourceCredentials = getSourceCredentials();
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

public void getRequestMetadata_withQuotaProjectId() throws IOException, IllegalStateException {

GoogleCredentials sourceCredentials = getSourceCredentials();
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Test()
public void getRequestMetadata_withQuotaProjectId() throws IOException, IllegalStateException {

GoogleCredentials sourceCredentials = getSourceCredentials();
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to move these two lines into a fixture. They are repeated a lot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

public void getRequestMetadata_withoutQuotaProjectId() throws IOException, IllegalStateException {

GoogleCredentials sourceCredentials = getSourceCredentials();
MockIAMCredentialsServiceTransportFactory mtransportFactory =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockTransportFactory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -690,7 +908,7 @@ public void serialize() throws IOException, ClassNotFoundException {
assertSame(deserializedCredentials.clock, Clock.SYSTEM);
}

private String getDefaultExpireTime() {
public static String getDefaultExpireTime() {
Date currentDate = new Date();
Copy link
Contributor

Choose a reason for hiding this comment

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

why set the date? Calendar.getInstance is already the current time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@liuchaoren liuchaoren requested a review from elharo May 11, 2021 04:45
GoogleCredentials.fromStream(impersonationCredentialsStream, transportFactoryForSource);
credentials.setTransportFactory(transportFactory);

assertNotNull(credentials);
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 333 will have already thrown a NullPointerException if credentials is null so you can remove this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

GoogleCredentials.fromStream(impersonationCredentialsStream, transportFactoryForSource);
credentials.setTransportFactory(transportFactory);

assertNotNull(credentials);
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 298 will have already thrown a NullPointerException if credentials is null so you can remove this assertNotNull

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}

@Override
public boolean createScopedRequired() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name doesn't quite fit. Create tends t be a factory method. This is more of an is method. Is this new in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is not invented in this PR. It is defined in the base class 'GoogleCredentials' which is overridden by its subclasses. I think it uses "create" in its name because of its relationship with the createScoped method.

* Indicates whether the credentials require scopes to be specified via a call to {@link

return this.quotaProjectId;
}

public List<String> getDelegates() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method doesn't seem used. Can it be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is used in tests to verify the delegates.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case please annotate it as @VisibleForTesting. Also, make it non-public if possible. (It might not be but worth checking.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great, thanks for letting me know the annotation! I added it and removed the "public" to limit the visibility.

@liuchaoren liuchaoren requested a review from elharo May 12, 2021 19:45
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.

Github actions was having trouble yesterday systemwide. Might be fixed now. I pushed the button to rerun.

@liuchaoren liuchaoren merged commit b9823f7 into googleapis:master May 21, 2021
lsirac added a commit that referenced this pull request Jul 22, 2021
…oundaries (#698)

* feat: Adding functional tests for Service Account  (#685)

ServiceAccountCredentials tests for 4110

* feat: allow scopes for self signed jwt (#689)

* feat: self signed jwt support

* update

* address comments

* allow to use uri as audience

* address comments

* chore: release 0.27.0 (#678)

:robot: I have created a release \*beep\* \*boop\*
---
## [0.27.0](https://www.github.com/googleapis/google-auth-library-java/compare/v0.26.0...v0.27.0) (2021-07-14)


### Features

* add Id token support for UserCredentials ([#650](https://www.github.com/googleapis/google-auth-library-java/issues/650)) ([5a8f467](https://www.github.com/googleapis/google-auth-library-java/commit/5a8f4676630854c53aa708a9c8b960770067f858))
* add impersonation credentials to ADC  ([#613](https://www.github.com/googleapis/google-auth-library-java/issues/613)) ([b9823f7](https://www.github.com/googleapis/google-auth-library-java/commit/b9823f70d7f3f7461b7de40bee06f5e7ba0e797c))
* Adding functional tests for Service Account  ([#685](https://www.github.com/googleapis/google-auth-library-java/issues/685)) ([dfe118c](https://www.github.com/googleapis/google-auth-library-java/commit/dfe118c261aadf137a3cf47a7acb9892c7a6db4d))
* allow scopes for self signed jwt ([#689](https://www.github.com/googleapis/google-auth-library-java/issues/689)) ([f4980c7](https://www.github.com/googleapis/google-auth-library-java/commit/f4980c77566bbd5ef4c532acb199d7d484dbcd01))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

* test: adds integration tests for downscoping with credential access boundaries

Co-authored-by: Timur Sadykov <stim@google.com>
Co-authored-by: arithmetic1728 <58957152+arithmetic1728@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
lsirac added a commit that referenced this pull request Jul 26, 2021
…s_in (#699)

* feat: Adding functional tests for Service Account  (#685)

ServiceAccountCredentials tests for 4110

* feat: allow scopes for self signed jwt (#689)

* feat: self signed jwt support

* update

* address comments

* allow to use uri as audience

* address comments

* chore: release 0.27.0 (#678)

:robot: I have created a release \*beep\* \*boop\*
---
## [0.27.0](https://www.github.com/googleapis/google-auth-library-java/compare/v0.26.0...v0.27.0) (2021-07-14)


### Features

* add Id token support for UserCredentials ([#650](https://www.github.com/googleapis/google-auth-library-java/issues/650)) ([5a8f467](https://www.github.com/googleapis/google-auth-library-java/commit/5a8f4676630854c53aa708a9c8b960770067f858))
* add impersonation credentials to ADC  ([#613](https://www.github.com/googleapis/google-auth-library-java/issues/613)) ([b9823f7](https://www.github.com/googleapis/google-auth-library-java/commit/b9823f70d7f3f7461b7de40bee06f5e7ba0e797c))
* Adding functional tests for Service Account  ([#685](https://www.github.com/googleapis/google-auth-library-java/issues/685)) ([dfe118c](https://www.github.com/googleapis/google-auth-library-java/commit/dfe118c261aadf137a3cf47a7acb9892c7a6db4d))
* allow scopes for self signed jwt ([#689](https://www.github.com/googleapis/google-auth-library-java/issues/689)) ([f4980c7](https://www.github.com/googleapis/google-auth-library-java/commit/f4980c77566bbd5ef4c532acb199d7d484dbcd01))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

* test: adds integration tests for downscoping with credential access boundaries

* fix: STS does not always return expires_in, fallback to source credential expiration for DownscopedCredentials

* fix: review

Co-authored-by: Timur Sadykov <stim@google.com>
Co-authored-by: arithmetic1728 <58957152+arithmetic1728@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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.

ADC can load a json of impersonated service account credentials generated by gcloud
3 participants