Repository navigation
feat(oauth2): implement IAM impersonation mTLS transport pinning and 401 recovery #14212
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
Changes from all commits
dadeda6
5cc87e0
73c5a0a
8034525
17aae1f
f2d752e
a5e69ed
13e8be0
44ca6d7
9313161
6e5f7fe
ddd5ea8
b47ab2a
1f062f2
4e06ff0
d5317c2
091a91b
4bf0fdd
fe93d90
a9acfb5
16716d0
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 |
|---|---|---|
|
|
@@ -85,6 +85,7 @@ public abstract class ExternalAccountCredentials extends GoogleCredentials { | |
|
|
||
| private final @Nullable String tokenInfoUrl; | ||
| private final @Nullable String serviceAccountImpersonationUrl; | ||
| private final @Nullable String targetServiceAccountEmail; | ||
| private final @Nullable String clientId; | ||
| private final @Nullable String clientSecret; | ||
|
|
||
|
|
@@ -93,9 +94,9 @@ public abstract class ExternalAccountCredentials extends GoogleCredentials { | |
| // Service. | ||
| private final @Nullable String workforcePoolUserProject; | ||
|
|
||
| protected transient HttpTransportFactory transportFactory; | ||
| protected transient volatile HttpTransportFactory transportFactory; | ||
|
|
||
| protected @Nullable ImpersonatedCredentials impersonatedCredentials; | ||
| protected transient volatile @Nullable ImpersonatedCredentials impersonatedCredentials; | ||
|
|
||
| private final EnvironmentProvider environmentProvider; | ||
| private final PropertyProvider propertyProvider; | ||
|
|
@@ -196,6 +197,7 @@ protected ExternalAccountCredentials( | |
| this.credentialSource = checkNotNull(credentialSource); | ||
| this.tokenInfoUrl = tokenInfoUrl; | ||
| this.serviceAccountImpersonationUrl = serviceAccountImpersonationUrl; | ||
| this.targetServiceAccountEmail = null; | ||
| this.clientId = clientId; | ||
| this.clientSecret = clientSecret; | ||
| this.scopes = | ||
|
|
@@ -236,6 +238,7 @@ protected ExternalAccountCredentials(ExternalAccountCredentials.Builder builder) | |
| this.credentialSource = builder.credentialSource; | ||
| this.tokenInfoUrl = builder.tokenInfoUrl; | ||
| this.serviceAccountImpersonationUrl = builder.serviceAccountImpersonationUrl; | ||
| this.targetServiceAccountEmail = builder.targetServiceAccountEmail; | ||
| this.clientId = builder.clientId; | ||
| this.clientSecret = builder.clientSecret; | ||
|
|
||
|
|
@@ -286,27 +289,26 @@ protected ExternalAccountCredentials(ExternalAccountCredentials.Builder builder) | |
| if (serviceAccountImpersonationUrl == null) { | ||
| return null; | ||
| } | ||
| String targetPrincipal = | ||
| ImpersonatedCredentials.extractTargetPrincipal(serviceAccountImpersonationUrl); | ||
| // Create a copy of this instance without service account impersonation. | ||
| ExternalAccountCredentials sourceCredentials; | ||
| ExternalAccountCredentials.Builder sourceBuilder; | ||
| if (this instanceof AwsCredentials) { | ||
| sourceCredentials = | ||
| AwsCredentials.newBuilder((AwsCredentials) this) | ||
| .setServiceAccountImpersonationUrl(null) | ||
| .build(); | ||
| sourceBuilder = AwsCredentials.newBuilder((AwsCredentials) this); | ||
| } else if (this instanceof PluggableAuthCredentials) { | ||
| sourceCredentials = | ||
| PluggableAuthCredentials.newBuilder((PluggableAuthCredentials) this) | ||
| .setServiceAccountImpersonationUrl(null) | ||
| .build(); | ||
| sourceBuilder = PluggableAuthCredentials.newBuilder((PluggableAuthCredentials) this); | ||
| } else { | ||
| sourceCredentials = | ||
| IdentityPoolCredentials.newBuilder((IdentityPoolCredentials) this) | ||
| .setServiceAccountImpersonationUrl(null) | ||
| .build(); | ||
| sourceBuilder = IdentityPoolCredentials.newBuilder((IdentityPoolCredentials) this); | ||
| } | ||
|
Comment on lines
+295
to
302
Member
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 this just be simplified to sourceBuilder = toBuilder()?
Contributor
Author
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.
|
||
|
|
||
| String targetPrincipal = | ||
| ImpersonatedCredentials.extractTargetPrincipal(serviceAccountImpersonationUrl); | ||
| // The intermediate STS access token on sourceCredentials is used to authenticate to the IAM | ||
| // Credentials generateAccessToken endpoint, which requires the cloud-platform scope (whereas | ||
| // this.scopes are passed to ImpersonatedCredentials below for the final impersonated token). | ||
| sourceBuilder | ||
| .setServiceAccountImpersonationUrl(null) | ||
| .setScopes(Collections.singletonList(OAuth2Utils.CLOUD_PLATFORM_SCOPE)); | ||
|
Member
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 you add a note to explain why we add this scope?
Contributor
Author
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. Done β added a comment explaining that |
||
| sourceBuilder.targetServiceAccountEmail = targetPrincipal; | ||
| sourceBuilder.setAccessToken(null); | ||
| ExternalAccountCredentials sourceCredentials = sourceBuilder.build(); | ||
|
Contributor
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. Should we be clearing the copied access token before building the source credentials? e.g. after serialization, a forced refresh rebuilds the impersonation credentials but copies the cached service account token which IAM will receive, potentially causing issues (e.g. a token with only a Storage scope won't work).
Contributor
Author
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. Good catch. impersonatedCredentials is now transient and rebuilt lazily, so after deserialization newBuilder(this) copied the outer credential's cached service account token into the source. On the non-pinned path the source then looked fresh, skipped STS, and sent that token to IAM. ensureExternalSourceScoped() didn't catch it because the source is always built with cloud-platform scope. buildImpersonatedCredentials() now clears the copied access token before building the source. Added two tests:
Both fail without the fix. |
||
| return ImpersonatedCredentials.newBuilder() | ||
| .setSourceCredentials(sourceCredentials) | ||
| .setHttpTransportFactory(transportFactory) | ||
|
|
@@ -522,8 +524,21 @@ private static boolean isAwsCredential(Map<String, Object> credentialSource) { | |
| && ((String) credentialSource.get("environment_id")).startsWith("aws"); | ||
| } | ||
|
|
||
| private boolean shouldBuildImpersonatedCredential() { | ||
| return this.serviceAccountImpersonationUrl != null && this.impersonatedCredentials == null; | ||
| @Nullable ImpersonatedCredentials getImpersonatedCredentials() { | ||
| if (this.serviceAccountImpersonationUrl == null) { | ||
| return null; | ||
| } | ||
| ImpersonatedCredentials local = this.impersonatedCredentials; | ||
| if (local == null) { | ||
| synchronized (this) { | ||
| local = this.impersonatedCredentials; | ||
| if (local == null) { | ||
| local = this.buildImpersonatedCredentials(); | ||
| this.impersonatedCredentials = local; | ||
| } | ||
| } | ||
| } | ||
| return local; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -552,11 +567,10 @@ protected AccessToken exchangeExternalCredentialForAccessToken( | |
| StsTokenExchangeRequest stsTokenExchangeRequest, HttpTransportFactory cycleTransportFactory) | ||
| throws IOException { | ||
| // Handle service account impersonation if necessary. | ||
| if (this.shouldBuildImpersonatedCredential()) { | ||
| this.impersonatedCredentials = this.buildImpersonatedCredentials(); | ||
| } | ||
| if (this.impersonatedCredentials != null) { | ||
| return this.impersonatedCredentials.refreshAccessToken(); | ||
| ImpersonatedCredentials impersonated = getImpersonatedCredentials(); | ||
| if (impersonated != null) { | ||
| return impersonated.refreshAccessToken( | ||
| cycleTransportFactory == this.transportFactory ? null : cycleTransportFactory); | ||
| } | ||
|
|
||
| StsRequestHandler.Builder requestHandler = | ||
|
|
@@ -637,7 +651,7 @@ private void readObject(ObjectInputStream input) throws IOException, ClassNotFou | |
| */ | ||
| public @Nullable String getServiceAccountEmail() { | ||
| if (serviceAccountImpersonationUrl == null || serviceAccountImpersonationUrl.isEmpty()) { | ||
| return null; | ||
| return targetServiceAccountEmail; | ||
|
lqiu96 marked this conversation as resolved.
|
||
| } | ||
| return ImpersonatedCredentials.extractTargetPrincipal(serviceAccountImpersonationUrl); | ||
| } | ||
|
|
@@ -789,6 +803,7 @@ public abstract static class Builder extends GoogleCredentials.Builder { | |
| protected @Nullable HttpTransportFactory transportFactory; | ||
|
|
||
| protected @Nullable String serviceAccountImpersonationUrl; | ||
| private @Nullable String targetServiceAccountEmail; | ||
| protected @Nullable String clientId; | ||
| protected @Nullable String clientSecret; | ||
| protected @Nullable Collection<String> scopes; | ||
|
|
@@ -813,6 +828,7 @@ protected Builder(ExternalAccountCredentials credentials) { | |
| this.tokenUrl = credentials.tokenUrl; | ||
| this.tokenInfoUrl = credentials.tokenInfoUrl; | ||
| this.serviceAccountImpersonationUrl = credentials.serviceAccountImpersonationUrl; | ||
| this.targetServiceAccountEmail = credentials.targetServiceAccountEmail; | ||
| this.credentialSource = credentials.credentialSource; | ||
| this.clientId = credentials.clientId; | ||
| this.clientSecret = credentials.clientSecret; | ||
|
|
@@ -911,6 +927,7 @@ public Builder setCredentialSource(CredentialSource credentialSource) { | |
| public Builder setServiceAccountImpersonationUrl( | ||
| @Nullable String serviceAccountImpersonationUrl) { | ||
| this.serviceAccountImpersonationUrl = serviceAccountImpersonationUrl; | ||
| this.targetServiceAccountEmail = null; | ||
| return this; | ||
| } | ||
|
|
||
|
|
||
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.
This new non-private overload changes the computed
serialVersionUID, so the previously saved cert credentials fail to load withInvalidClassException.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.
CertificateIdentityPoolSubjectTokenSupplier now declares serialVersionUID = -7726329500407809830L, which is the computed value in the released jars (identical from 1.50.0 through 1.53.0). I verified it end to end: a cert-sourced IdentityPoolCredentials serialized with 1.53.0 now deserializes with this PR's classes, and fails with InvalidClassException without the pin. Added a test that asserts the value.
I checked the other serializable classes this PR touches against 1.53.0. The same issue applied to OAuthException (new constructor and getHttpStatusCode()), so it's now pinned to its released value (-5276727039237496975L) with a test too. PluggableAuthCredentials' computed value also changed, but it isn't serializable in released versions (the handler field), so there's nothing to keep compatible. The rest keep their released values.