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 18 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 |
|---|---|---|
|
|
@@ -42,17 +42,21 @@ | |
| import java.nio.file.Files; | ||
| import java.nio.file.NoSuchFileException; | ||
| import java.nio.file.Paths; | ||
| import java.security.KeyStore; | ||
| import java.security.KeyStoreException; | ||
| import java.security.cert.Certificate; | ||
| import java.security.cert.CertificateEncodingException; | ||
| import java.security.cert.CertificateException; | ||
| import java.security.cert.CertificateFactory; | ||
| import java.security.cert.X509Certificate; | ||
| import java.util.ArrayList; | ||
| import java.util.Base64; | ||
| import java.util.Enumeration; | ||
| import java.util.List; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
| import org.jspecify.annotations.NullMarked; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** | ||
| * Provider for retrieving the subject tokens for {@link IdentityPoolCredentials} by reading an | ||
|
|
@@ -64,6 +68,9 @@ | |
| public class CertificateIdentityPoolSubjectTokenSupplier | ||
| implements IdentityPoolSubjectTokenSupplier { | ||
|
|
||
| // Matches the computed value in released versions so previously serialized instances still load. | ||
| private static final long serialVersionUID = -7726329500407809830L; | ||
|
|
||
| private final IdentityPoolCredentialSource credentialSource; | ||
|
|
||
| private static final Pattern PEM_CERT_PATTERN = | ||
|
|
@@ -96,6 +103,35 @@ private static String loadAndEncodeLeafCertificate(String path) throws IOExcepti | |
| } | ||
| } | ||
|
|
||
| private static @Nullable String extractAndEncodeLeafCertificate(@Nullable KeyStore keyStore) | ||
|
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. note for us in a future PR: looks like parsing through the keystore is a repeated operation in multiple spots (I think I recall seeing this in mtls PR before). Maybe we can extract some common functionality
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. Agreed! Once both PRs land, we can extract the |
||
| throws IOException { | ||
| if (keyStore == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| Enumeration<String> aliases = keyStore.aliases(); | ||
| if (aliases == null) { | ||
| return null; | ||
| } | ||
| while (aliases.hasMoreElements()) { | ||
| String alias = aliases.nextElement(); | ||
| if (keyStore.isKeyEntry(alias)) { | ||
| Certificate[] chain = keyStore.getCertificateChain(alias); | ||
| if (chain != null && chain.length > 0 && chain[0] instanceof X509Certificate) { | ||
| return encodeCert((X509Certificate) chain[0]); | ||
| } | ||
| Certificate cert = keyStore.getCertificate(alias); | ||
| if (cert instanceof X509Certificate) { | ||
| return encodeCert((X509Certificate) cert); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } catch (KeyStoreException | CertificateEncodingException e) { | ||
| throw new IOException("Failed to extract leaf certificate from pinned KeyStore", e); | ||
|
lqiu96 marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| static X509Certificate parseCertificate(byte[] certData) throws CertificateException { | ||
| if (certData == null || certData.length == 0) { | ||
|
|
@@ -135,14 +171,27 @@ private static String encodeCert(X509Certificate certificate) | |
| */ | ||
| @Override | ||
| public String getSubjectToken(ExternalAccountSupplierContext context) throws IOException { | ||
| String leafCertPath = credentialSource.getCredentialLocation(); | ||
| return getSubjectToken(context, null); | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves the X509 subject token, extracting the leaf certificate directly from {@code | ||
| * pinnedKeyStore} when provided so that the TLS client certificate and {@code subject_token} are | ||
| * pinned to the exact same certificate snapshot. | ||
| */ | ||
| String getSubjectToken(ExternalAccountSupplierContext context, @Nullable KeyStore pinnedKeyStore) | ||
|
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. This new non-private overload changes the computed
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. 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. |
||
| throws IOException { | ||
| String trustChainPath = null; | ||
| if (credentialSource.getCertificateConfig() != null) { | ||
| trustChainPath = credentialSource.getCertificateConfig().getTrustChainPath(); | ||
| } | ||
|
|
||
| // Load and encode the leaf certificate. | ||
| String encodedLeafCert = loadAndEncodeLeafCertificate(leafCertPath); | ||
| // Extract the leaf certificate from the pinned KeyStore if present; otherwise read from disk. | ||
| String encodedLeafCert = extractAndEncodeLeafCertificate(pinnedKeyStore); | ||
| if (encodedLeafCert == null) { | ||
| String leafCertPath = credentialSource.getCredentialLocation(); | ||
| encodedLeafCert = loadAndEncodeLeafCertificate(leafCertPath); | ||
| } | ||
|
|
||
| // Initialize the certificate chain for the subject token. The Security Token Service (STS) | ||
| // requires that the leaf certificate (the one used for authenticating this workload) must be | ||
|
|
@@ -222,7 +271,8 @@ private void populateCertChainFromTrustChain( | |
| // elsewhere in the chain. | ||
| if (encodedCurrentCert.equals(encodedLeafCert)) { | ||
| throw new IllegalArgumentException( | ||
| "The leaf certificate should only appear at the beginning of the trust chain file, or be omitted entirely."); | ||
| "The leaf certificate should only appear at the beginning of the trust chain file, or" | ||
| + " be omitted entirely."); | ||
| } | ||
|
|
||
| // Add the current certificate to the chain. | ||
|
|
@@ -241,7 +291,7 @@ private void populateCertChainFromTrustChain( | |
| * @throws CertificateException If an error occurs while parsing a certificate. | ||
| */ | ||
| @VisibleForTesting | ||
| static List<X509Certificate> readTrustChain(String trustChainPath) | ||
| static List<X509Certificate> readTrustChain(@Nullable String trustChainPath) | ||
| throws IOException, CertificateException { | ||
| List<X509Certificate> certificateTrustChain = new ArrayList<>(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ | |
| import java.io.ObjectInputStream; | ||
| import java.math.BigDecimal; | ||
| import java.net.URI; | ||
| import java.security.KeyStore; | ||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
@@ -85,6 +86,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 +95,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 +198,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 +239,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 +290,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 +525,55 @@ 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; | ||
| } | ||
|
|
||
| /** | ||
| * Refreshes the access token using the specified transport factory for per-cycle transport | ||
| * pinning. | ||
| * | ||
| * <p>When {@code serviceAccountImpersonationUrl} is configured, the outer {@link | ||
| * ExternalAccountCredentials} delegates token refresh to its internal {@link | ||
| * ImpersonatedCredentials}, which in turn calls back into its cloned {@code sourceCredentials} | ||
| * (typed as {@link ExternalAccountCredentials} with {@code serviceAccountImpersonationUrl == | ||
| * null}) via {@link #refreshAccessToken(HttpTransportFactory, KeyStore)} so the intermediate STS | ||
| * token exchange and the IAM {@code generateAccessToken} request share the same pinned transport | ||
| * and {@link KeyStore}. | ||
| * | ||
| * <p>{@link AwsCredentials} and {@link PluggableAuthCredentials} delegate {@link | ||
| * #refreshAccessToken()} into this method, while {@link IdentityPoolCredentials} coordinates | ||
| * per-cycle transport pinning and retries directly. Because {@link ExternalAccountCredentials} is | ||
| * a {@code public abstract} class with {@code protected} constructors and these hooks are | ||
| * intentionally package-private, a default implementation delegating to {@link | ||
| * #refreshAccessToken()} is provided so external subclasses outside this package remain | ||
| * compilable. | ||
| * | ||
| * @param cycleTransportFactory the HTTP transport factory to use for this refresh cycle | ||
| * @return the refreshed access token | ||
| * @throws IOException if the token refresh fails | ||
| */ | ||
| AccessToken refreshAccessToken(HttpTransportFactory cycleTransportFactory) throws IOException { | ||
| return refreshAccessToken(); | ||
| } | ||
|
|
||
| AccessToken refreshAccessToken( | ||
| HttpTransportFactory cycleTransportFactory, @Nullable KeyStore pinnedKeyStore) | ||
| throws IOException { | ||
| return refreshAccessToken(cycleTransportFactory); | ||
|
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. qq, it's not super clear to me why we need this override. Is this due to the funky behavior with 3p creds and impersonation. IIRC, it seems to call each other in some weird way that ends up being somewhat of a cycle?
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. One more note: can these methods just be abstract methods that the subclasses override?
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. Yes, spot on β when
On making them
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. This looks primarily really only used/ needed for IdentityPoolCredentials. Can this just be put into that class?
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 β removed |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -552,11 +602,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 +686,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 +838,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 +863,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 +962,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.
I'm not entirely sure I follow what is going on here. This looks to set refreshAccessToken with the existing transportFactory? There would not be any "cycle" here. This also seems to be the case for PluggableAuth and Aws, but IdentityPool is different.
Does it need this overload then for refreshAccessToken(HttpTransportFactory cycleTransportFactory)?
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.
Good point β neither
AwsCredentialsnorPluggableAuthCredentialsuses per-cycle transport pinning, so they don't need therefreshAccessToken(HttpTransportFactory)overload. Removed the overload fromExternalAccountCredentials,AwsCredentials, andPluggableAuthCredentials(keeping it only onIdentityPoolCredentials), while retaining the earlygetImpersonatedCredentials()delegation insiderefreshAccessToken()so the subject token isn't fetched twice when service account impersonation is configured.