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.

Sign up for GitHub

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: service sccount to service account impersonation to support universe domain #1528

Merged
merged 25 commits into from
Oct 18, 2024
Merged

feat: service sccount to service account impersonation to support universe domain #1528

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ea42036
fix: replacing hardcoded id token and access token endpoints for Impe…
zhumin8 Oct 3, 2024
55ccb61
add getUniverseDomain override, add changes for sign. not tested yet.
zhumin8 Oct 3, 2024
600fd68
changes to deal with explicit universe domain at init.
zhumin8 Oct 4, 2024
6b86b3a
clean up and add test.
zhumin8 Oct 9, 2024
2fbf761
fixing refresh flow for TPC case.
zhumin8 Oct 10, 2024
8dfa6ef
minor lint.
zhumin8 Oct 10, 2024
09e80aa
fix test to use iam mock transport for sourceAccount in tests so it c…
zhumin8 Oct 10, 2024
a8bccd1
remove idtoken and sign changes for a separate pr.
zhumin8 Oct 10, 2024
4b43ad8
fix createScoped accesstoken issue introduced by connecting builder t…
zhumin8 Oct 11, 2024
bca6a42
cleanups and add to tests.
zhumin8 Oct 11, 2024
a987ecd
Merge branch 'main' into sa-2-sa
zhumin8 Oct 11, 2024
995209d
remove redudant setters and try/catch.
zhumin8 Oct 11, 2024
4de0b9b
address review feedback. fix isDefaultUniverseDomain in GoogleCredent…
zhumin8 Oct 14, 2024
12ab22e
lint: rm unused import.
zhumin8 Oct 14, 2024
d334e46
update doc/comment.
zhumin8 Oct 15, 2024
ece8884
Merge branch 'main' into sa-2-sa
blakeli0 Oct 15, 2024
78d3a03
refactor GoogleCredentials builder, so that can remove override but a…
zhumin8 Oct 15, 2024
9631acd
Revert "refactor GoogleCredentials builder, so that can remove overri…
zhumin8 Oct 15, 2024
61fd304
change in treating explicit UD: accept universe domain only if it mat…
zhumin8 Oct 17, 2024
fc24a06
fix: skip refreshIfExpired() step for gdu with ssj flow too. Add test.
zhumin8 Oct 17, 2024
56acc23
fix if statement to skip only SA with SSJ flow correctly.
zhumin8 Oct 17, 2024
fdb6582
lint and rename a method.
zhumin8 Oct 17, 2024
00a8cf2
Merge branch 'main' into sa-2-sa
zhumin8 Oct 17, 2024
1313788
lint
zhumin8 Oct 17, 2024
f3a6f23
update exception message.
zhumin8 Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,11 @@ protected boolean isExplicitUniverseDomain() {
/**
* Checks if universe domain equals to {@link Credentials#GOOGLE_DEFAULT_UNIVERSE}.
*
* @return true if universeDomain equals to {@link Credentials#GOOGLE_DEFAULT_UNIVERSE}, false
* @return true if universe domain equals to {@link Credentials#GOOGLE_DEFAULT_UNIVERSE}, false
* otherwise
*/
boolean isDefaultUniverseDomain() {
return this.universeDomain.equals(Credentials.GOOGLE_DEFAULT_UNIVERSE);
boolean isDefaultUniverseDomain() throws IOException {
return getUniverseDomain().equals(Credentials.GOOGLE_DEFAULT_UNIVERSE);
}

/**
Expand Down
79 changes: 60 additions & 19 deletions oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,6 @@ public class ImpersonatedCredentials extends GoogleCredentials
private static final int DEFAULT_LIFETIME_IN_SECONDS = 3600;
private static final String CLOUD_PLATFORM_SCOPE =
"https://www.googleapis.com/auth/cloud-platform";
private static final String IAM_ACCESS_TOKEN_ENDPOINT =
"https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s:generateAccessToken";

private GoogleCredentials sourceCredentials;
private String targetPrincipal;
private List<String> delegates;
Expand Down Expand Up @@ -423,14 +420,7 @@ public boolean createScopedRequired() {

@Override
public GoogleCredentials createScoped(Collection<String> scopes) {
return toBuilder()
.setScopes(new ArrayList<>(scopes))
.setLifetime(this.lifetime)
.setDelegates(this.delegates)
.setHttpTransportFactory(this.transportFactory)
.setQuotaProjectId(this.quotaProjectId)
.setIamEndpointOverride(this.iamEndpointOverride)
.build();
return toBuilder().setScopes(new ArrayList<>(scopes)).setAccessToken(null).build();
}

@Override
Expand Down Expand Up @@ -472,7 +462,7 @@ private ImpersonatedCredentials(Builder builder) {
this.transportFactoryClassName = this.transportFactory.getClass().getName();
this.calendar = builder.getCalendar();
if (this.delegates == null) {
this.delegates = new ArrayList<String>();
this.delegates = new ArrayList<>();
}
if (this.scopes == null) {
throw new IllegalStateException("Scopes cannot be null");
Expand All @@ -482,17 +472,35 @@ private ImpersonatedCredentials(Builder builder) {
}
}

/**
* Gets the universe domain for the credential.
*
* @return An explicit universe domain if it was explicitly provided, use the universe domain from
* source credentials otherwise
*/
@Override
public String getUniverseDomain() throws IOException {
if (isExplicitUniverseDomain()) {
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
return super.getUniverseDomain();
}
return this.sourceCredentials.getUniverseDomain();
}

@Override
public AccessToken refreshAccessToken() throws IOException {
if (this.sourceCredentials.getAccessToken() == null) {
this.sourceCredentials =
this.sourceCredentials.createScoped(Arrays.asList(CLOUD_PLATFORM_SCOPE));
}

try {
this.sourceCredentials.refreshIfExpired();
} catch (IOException e) {
throw new IOException("Unable to refresh sourceCredentials", e);
// skip for nonGDU because it uses self-signed JWT
// and will get refreshed at initialize request step
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not sure why this call is required for GDU but not required for nonGDU, is there any feature that is only supported in GDU?

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 am convinced that this is not needed for nonGDU, reasons in description above.
For GDU, I am not 100% convinced that this is required before initialize step. But out of precautious, I do not want to change the flow for GDU in this pr. I will raise a separate issue to investigate for GDU flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

to answer Blake's question - GDU has token endpoint, nonGDU does not :)

SSJ is a primary flow is GDU now as well. This should be a fallback scenario if SSJ is disabled, execute conditionally if SSJ flag is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated condition to also skip for GDU SSJ flow.

if (isDefaultUniverseDomain()) {
try {
this.sourceCredentials.refreshIfExpired();
} catch (IOException e) {
throw new IOException("Unable to refresh sourceCredentials", e);
}
}

HttpTransport httpTransport = this.transportFactory.create();
Expand All @@ -501,10 +509,16 @@ public AccessToken refreshAccessToken() throws IOException {
HttpCredentialsAdapter adapter = new HttpCredentialsAdapter(sourceCredentials);
HttpRequestFactory requestFactory = httpTransport.createRequestFactory();

String endpointUrl =
String endpointUrl = null;

endpointUrl =
this.iamEndpointOverride != null
? this.iamEndpointOverride
: String.format(IAM_ACCESS_TOKEN_ENDPOINT, this.targetPrincipal);
: String.format(
OAuth2Utils.IAM_ACCESS_TOKEN_ENDPOINT_FORMAT,
getUniverseDomain(),
this.targetPrincipal);

GenericUrl url = new GenericUrl(endpointUrl);

Map<String, Object> body =
Expand Down Expand Up @@ -603,6 +617,9 @@ public boolean equals(Object obj) {
if (!(obj instanceof ImpersonatedCredentials)) {
return false;
}
if (!super.equals(obj)) {
return false;
}
ImpersonatedCredentials other = (ImpersonatedCredentials) obj;
return Objects.equals(this.sourceCredentials, other.sourceCredentials)
&& Objects.equals(this.targetPrincipal, other.targetPrincipal)
Expand All @@ -616,7 +633,7 @@ public boolean equals(Object obj) {

@Override
public Builder toBuilder() {
return new Builder(this.sourceCredentials, this.targetPrincipal);
return new Builder(this);
}

public static Builder newBuilder() {
Expand All @@ -636,11 +653,29 @@ public static class Builder extends GoogleCredentials.Builder {

protected Builder() {}

/**
* @param sourceCredentials The source credentials to use for impersonation.
* @param targetPrincipal The service account to impersonate.
* @deprecated Use {@link #Builder(ImpersonatedCredentials)} instead. This constructor will be
* removed in a future release.
*/
@Deprecated
protected Builder(GoogleCredentials sourceCredentials, String targetPrincipal) {
this.sourceCredentials = sourceCredentials;
this.targetPrincipal = targetPrincipal;
}

protected Builder(ImpersonatedCredentials credentials) {
super(credentials);
this.sourceCredentials = credentials.sourceCredentials;
this.targetPrincipal = credentials.targetPrincipal;
this.delegates = credentials.delegates;
this.scopes = credentials.scopes;
this.lifetime = credentials.lifetime;
this.transportFactory = credentials.transportFactory;
this.iamEndpointOverride = credentials.iamEndpointOverride;
}

@CanIgnoreReturnValue
public Builder setSourceCredentials(GoogleCredentials sourceCredentials) {
this.sourceCredentials = sourceCredentials;
Expand Down Expand Up @@ -724,6 +759,12 @@ public Calendar getCalendar() {
return this.calendar;
}

@Override
public Builder setUniverseDomain(String universeDomain) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have to override this method as it is already public in the parent. I noticed that it seems we are overriding the builder methods in other subclasses of the builder, but I think that was a mistake.

Copy link
Contributor Author

@zhumin8 zhumin8 Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I debated this in my mind. One benefit I see overriding the setter allows for a fluent builder API where method calls can be chained without requiring explicit casting to ImpersonatedCredentials. In this case, I feel it provides a slightly better experience for users setting explicit UD
So user can do

        ImpersonatedCredentials.newBuilder()
            .setUniverseDomain("explicit.domain.com");

instead of casting:

        (ImpersonatedCredentials) ImpersonatedCredentials.newBuilder()
            .setUniverseDomain("explicit.domain.com");

While generally it's best to avoid overriding public methods if possible, override setters in Builders is kind of a special case and more acceptable. .e.g. overriding Builder setters seems to be a pattern in other libraries like Guava (example)

One other concern is removing other overrides in other credential classes will be breaking change to customers code.
That said, I don't feel too strongly about this, and I believe exposing this setter is not a hard requirement for now. If you have any other concerns, I can remove this for now. What do you think?

Copy link
Contributor

@blakeli0 blakeli0 Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think customers have to cast the credential to ImpersonatedCredentials to setUniverseDomain(), I verified in my local that it works. Can you please double check it?
For the Guava example, yes they are doing it because there are some additional logics, but we are not doing anything special other than calling super.setUniverseDomain(). So I think we can delete it at this moment and add it later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirming that explicit overrides were added to avoid casting.

Copy link
Contributor

@blakeli0 blakeli0 Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Min and I confirmed that we do need explicit casting right now. However, the problem is that the parent builder's setters should return the child types. I created a demo PR for a potential fix. Also see how protobuf's GeneratedMessageV3 does it.

That being said, this should be separate problem to solve, so it's OK to override the parent's setters right now.

super.setUniverseDomain(universeDomain);
return this;
}

@Override
public ImpersonatedCredentials build() {
return new ImpersonatedCredentials(this);
Expand Down
5 changes: 5 additions & 0 deletions oauth2_http/java/com/google/auth/oauth2/OAuth2Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ class OAuth2Utils {
static final String IAM_ID_TOKEN_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:generateIdToken";

static final String IAM_ACCESS_TOKEN_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:generateAccessToken";
static final String SIGN_BLOB_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:signBlob";

static final URI TOKEN_SERVER_URI = URI.create("https://oauth2.googleapis.com/token");

static final URI TOKEN_REVOKE_URI = URI.create("https://oauth2.googleapis.com/revoke");
Expand Down
20 changes: 13 additions & 7 deletions oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,13 +992,19 @@ public void getRequestMetadata(
// For default universe Self-signed JWT could be explicitly disabled with
// {@code ServiceAccountCredentials.useJwtAccessWithScope} flag.
// If universe is non-default, it only supports self-signed JWT, and it is always allowed.
if (this.useJwtAccessWithScope || !isDefaultUniverseDomain()) {
// This will call getRequestMetadata(URI uri), which handles self-signed JWT logic.
// Self-signed JWT doesn't use network, so here we do a blocking call to improve
// efficiency. executor will be ignored since it is intended for async operation.
blockingGetToCallback(uri, callback);
} else {
super.getRequestMetadata(uri, executor, callback);
try {
if (this.useJwtAccessWithScope || !isDefaultUniverseDomain()) {
// This will call getRequestMetadata(URI uri), which handles self-signed JWT logic.
// Self-signed JWT doesn't use network, so here we do a blocking call to improve
// efficiency. executor will be ignored since it is intended for async operation.
blockingGetToCallback(uri, callback);
} else {
super.getRequestMetadata(uri, executor, callback);
}
} catch (IOException e) {
// Wrap here because throwing exception would be breaking change.
// This should not happen for this credential type.
throw new IllegalStateException(e);
}
}

Expand Down
10 changes: 5 additions & 5 deletions oauth2_http/javatests/com/google/auth/oauth2/GoogleCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,12 +612,12 @@ public void fromStream_Impersonation_providesToken_WithQuotaProject() throws IOE
.setExpireTime(ImpersonatedCredentialsTest.getDefaultExpireTime());
transportFactory
.getTransport()
.setAccessTokenEndpoint(ImpersonatedCredentialsTest.IMPERSONATION_URL);
.setAccessTokenEndpoint(ImpersonatedCredentialsTest.IMPERSONATION_OVERRIDE_URL);
transportFactory.getTransport().addStatusCodeAndMessage(HttpStatusCodes.STATUS_CODE_OK, "");

InputStream impersonationCredentialsStream =
ImpersonatedCredentialsTest.writeImpersonationCredentialsStream(
ImpersonatedCredentialsTest.IMPERSONATION_URL,
ImpersonatedCredentialsTest.IMPERSONATION_OVERRIDE_URL,
ImpersonatedCredentialsTest.DELEGATES,
ImpersonatedCredentialsTest.QUOTA_PROJECT_ID);

Expand Down Expand Up @@ -647,7 +647,7 @@ public void fromStream_Impersonation_defaultUniverse() throws IOException {

InputStream impersonationCredentialsStream =
ImpersonatedCredentialsTest.writeImpersonationCredentialsStream(
ImpersonatedCredentialsTest.IMPERSONATION_URL,
ImpersonatedCredentialsTest.IMPERSONATION_OVERRIDE_URL,
ImpersonatedCredentialsTest.DELEGATES,
ImpersonatedCredentialsTest.QUOTA_PROJECT_ID);

Expand Down Expand Up @@ -677,12 +677,12 @@ public void fromStream_Impersonation_providesToken_WithoutQuotaProject() throws
.setExpireTime(ImpersonatedCredentialsTest.getDefaultExpireTime());
transportFactory
.getTransport()
.setAccessTokenEndpoint(ImpersonatedCredentialsTest.IMPERSONATION_URL);
.setAccessTokenEndpoint(ImpersonatedCredentialsTest.IMPERSONATION_OVERRIDE_URL);
transportFactory.getTransport().addStatusCodeAndMessage(HttpStatusCodes.STATUS_CODE_OK, "");

InputStream impersonationCredentialsStream =
ImpersonatedCredentialsTest.writeImpersonationCredentialsStream(
ImpersonatedCredentialsTest.IMPERSONATION_URL,
ImpersonatedCredentialsTest.IMPERSONATION_OVERRIDE_URL,
ImpersonatedCredentialsTest.DELEGATES,
null);

Expand Down
Loading