-
Notifications
You must be signed in to change notification settings - Fork 230
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
base: main
Are you sure you want to change the base?
Changes from 15 commits
4974f3a
336a109
689cf68
982e586
546942b
424104b
b5d43c9
4fc863a
b6e6d1a
d81aafe
a8d466f
083557d
9831d5d
2fec328
fb324dc
3dbc8e9
bef8346
40e2f9e
e47e4f9
88caa5e
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 ex) { | ||
// Throwing an IOException would be a breaking change, so wrap it here. | ||
// This should not happen for this credential type. | ||
throw new IllegalStateException("Failed to sign: Error obtaining universe domain", ex); | ||
zhumin8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
/** | ||
|
@@ -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 | ||
|
@@ -775,8 +783,9 @@ public ImpersonatedCredentials build() { | |
return new ImpersonatedCredentials(this); | ||
} catch (IOException e) { | ||
// throwing exception would be breaking change. catching instead. | ||
zhumin8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// this should never happen. | ||
throw new IllegalStateException(e); | ||
// this should never happen because ImpersonatedCredential can only be SA or User | ||
zhumin8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Credentials. | ||
throw new SigningException("Signing failed", 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. Why is the exception type changed here? 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. Thanks for capturing this! This is actually a mistake on my side made in 2fec328 trying to address this feedback The intended change is done in 3dbc8e9. I am reverting this accidental change in 40e2f9e |
||
} | ||
} | ||
} | ||
|
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.