-
Notifications
You must be signed in to change notification settings - Fork 232
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: ImpersonatedCredentials to support universe domain for idtoken and signblob #1566
Changes from 10 commits
4974f3a
336a109
689cf68
982e586
546942b
424104b
b5d43c9
4fc863a
b6e6d1a
d81aafe
a8d466f
083557d
9831d5d
2fec328
fb324dc
3dbc8e9
bef8346
40e2f9e
e47e4f9
88caa5e
f3c9ae8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -345,12 +345,19 @@ public void setTransportFactory(HttpTransportFactory httpTransportFactory) { | |
*/ | ||
@Override | ||
public byte[] sign(byte[] toSign) { | ||
return IamUtils.sign( | ||
getAccount(), | ||
sourceCredentials, | ||
transportFactory.create(), | ||
toSign, | ||
ImmutableMap.of("delegates", this.delegates)); | ||
try { | ||
return IamUtils.sign( | ||
getAccount(), | ||
sourceCredentials, | ||
getUniverseDomain(), | ||
transportFactory.create(), | ||
toSign, | ||
ImmutableMap.of("delegates", this.delegates)); | ||
} catch (IOException e) { | ||
// Throwing an IOException would be a breaking change, so wrap it here. | ||
// This should not happen for this credential type. | ||
throw new IllegalStateException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we throw a SigningException here for consistency just like ComputeEngineCredentials? If not, can the IllegalStateException have an error message that is the same as the one in ComputeEngineCredentials for consistency? Also, I think adding a new runtime exception might be a behavior breaking change. I think we can justify this addition given that signing needs to support Universe Domains. Perhaps we can add a small sentence in the description about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is my thought on this, I opted for IlligalStateException similar to ExternalAccountCredentials here
Added message in a8d466f There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IIUC (I may be completely off on this as I don't know Impersonation at all), the sourceCredential in the ImpersonatedCredential could be of any type, right? Unless there is a limitation that the underlying sourceCredential for an ImpersonatedCredential can't be of ComputeEngineCredential. I am assuming that if a user tries to impersonate a ComputeEngineCredential, the call
I think SigningException is a RuntimeException which shouldn't require adding it as part of the method signature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is my understanding. sourceCredential can be user or sa (ref), or a couple of external account types (ref). So ComputeEngineCredential cannot be sourceCredential (and I don't know if impersonate a GCE cred have a use case?).
Right, no signature change required, but I was concerned about behavior change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I was looking at this method which seems to allow any type of credential to be passed in. I only see this comment that tries to prevent other Credential types:
Seems a bit odd that certain static methods are checking for Credential types and others aren't. Maybe there is a reason for this... If not, probably something we can backfill and fix in a different PR. I'm assuming in some downstream use case, some functionality will fail when using ImpersonatedCredentials with ComputeEngineCredentials as the source. We probably don't have any users that have this setup (Impersonating a Compute Credentials), I just don't know enough about Impersonation to be sure about that. Would you know if this is the case?
I am just thinking about keeping these consistent. As of now, I think they anything that signs should be either SigningException or IllegalStateException due to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I lean to agree with you on keeping consistent. On the " using ImpersonatedCredentials with ComputeEngineCredentials as the source", I also find it a bit odd that this method you are quoting only specifies allowed source credential types in javadoc but not enforced. @TimurSadykov By any chance you know any context about it? Was this a miss or intentional? Also, in general, is it fair to assume ComputeEngineCredentials should not be source credential for ImpersonatedCredentials? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, let do this if this sounds fine:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} | ||
|
||
/** | ||
|
@@ -525,7 +532,7 @@ public AccessToken refreshAccessToken() throws IOException { | |
this.iamEndpointOverride != null | ||
? this.iamEndpointOverride | ||
: String.format( | ||
OAuth2Utils.IAM_ACCESS_TOKEN_ENDPOINT_FORMAT, | ||
IamUtils.IAM_ACCESS_TOKEN_ENDPOINT_FORMAT, | ||
getUniverseDomain(), | ||
this.targetPrincipal); | ||
|
||
|
@@ -593,7 +600,8 @@ public IdToken idTokenWithAudience(String targetAudience, List<IdTokenProvider.O | |
targetAudience, | ||
includeEmail, | ||
ImmutableMap.of("delegates", this.delegates), | ||
getMetricsCredentialType()); | ||
getMetricsCredentialType(), | ||
getUniverseDomain()); | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the guarantee that IO exception can happen only due to universe domain check?
maybe get universe domain separately so that we can be confident in the error messaging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
IAMUtils.sign()
method catches IOExceptions inside the method and re-throws it asServiceAccountSigner.SigningException
. I believe the only place that can throw IOException is from thegetUniverseDomain()
call, which should happen before we enter thesign()
method.