feat(oauth2): Extract actor tokens for cert-bound OAuth2 STS exchange - #13955
macastelaz merged 28 commits into
Conversation
Implementation of Phase 1-3 of the Cert-Bound Oauth2 Design Document: 1. Extend IdentityPoolCredentialSource to parse actorTokenFieldName. 2. Relax mutual exclusivity to allow BOTH file and certificate configurations. 3. Parse actor_token_type in ExternalAccountCredentials. 4. Refactor FileIdentityPoolTokenSupplier and track file timestamp via volatile CachedFile for the parsed JSON payload. 5. Inject actor_token and actor_token_type into StsTokenExchangeRequest using ActingParty. 6. Enforce that actor token extraction requires an mTLS STS configuration.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for actor tokens in IdentityPoolCredentials by adding actor token types and field names, introducing the IdentityPoolActorTokenSupplier interface, and refactoring the file-based supplier to FileIdentityPoolTokenSupplier with caching. Feedback suggests optimizing disk I/O by sharing a single FileIdentityPoolTokenSupplier instance for both subject and actor tokens, passing the target field name dynamically, and relaxing the mTLS URL validation check to generically look for .mtls. to support custom universes and Private Service Connect endpoints.
Fixes test failures and thread synchronization bugs regarding actor token credentials from https://paste.googleplex.com/5381957298028544
- Mark CachedFile and X509Provider transient to ensure clean serialization. - Add static modifier to FileIdentityPoolTokenSupplier serialVersionUID. - Make IdentityPoolActorTokenSupplier public with @NullMarked annotation. - Preserve actorTokenSupplier in IdentityPoolCredentials Builder copy constructor. - Mask actor_token in Slf4jLoggingHelpers sensitive keys. - Add no-arg constructor to MtlsHttpTransportFactory for serialization support. - Handle Data.isNull in FileIdentityPoolTokenSupplier JSON parsing. - Add comprehensive test coverage for supplier caching, builder, serialization, and log masking.
…uilder copy constructor - Guard actorTokenSupplier assignment with if (this.credentialSource == null) in Builder copy constructor. - Add getIdentityPoolActorTokenSupplier getter for test assertions. - Add createScoped tests for both file-sourced and supplier-sourced credentials with actor tokens.
…s and FileIdentityPoolTokenSupplier - Add builder_actorTokenTypeWithoutSupplier_throws testing missing supplier validation. - Add builder_fileWithCertificateConfig_initializesMtlsTransport testing mTLS initialization for composite file + cert sources. - Add toBuilder_preservesConfiguration testing builder reconstruction. - Add parseToken_textFormat_succeeds and parseToken_jsonFormat_missingFieldName_throws testing static token parsing methods.
|
Before we launch lets make sure to add integration tests for each supported transport and certificate rotation scenario to make sure that bound tokens work e2e. |
Summary of changes: - Comment 1: Fix Javadoc referencing package-private API - Comment 2: Replace instanceof with isMtlsConfigured() check - Comment 3: Per-cycle cert pinning with KeyStore snapshot, 401 retry - Comment 4: Atomic subject+actor token read via readTokens() - Comment 5: Make setActorTokenSupplier/Type package-private - Comment 6: Add Javadocs to builder setter methods - Comment 7: Fix Builder copy constructor (always copy actorTokenType) - Comment 8: Annotate no-arg MtlsHttpTransportFactory with @internalapi - Comment 9: Revert class rename to FileIdentityPoolSubjectTokenSupplier - Comment 10: Remove CachedFile/volatile caching mechanism - Comment 11: Add actorTokenFieldName validation - Comment 12: Integration tests noted for follow-up PR Added overload exchangeExternalCredentialForAccessToken(request, factory) for per-cycle transport factory threading. Added 12 new unit tests covering readTokens(), validation, and mTLS. All 982 existing + new tests pass.
- Fix copyright year (2024 -> 2026) in FileIdentityPoolSubjectTokenSupplier - Add Javadoc explaining class name retained for serialization compatibility - Add hasKeyStore() to MtlsHttpTransportFactory for watertight mTLS validation - Update isMtlsConfigured() to verify KeyStore is non-null via hasKeyStore() - Update no-arg constructor Javadoc to explain serialization requirement - Add comment to Builder copy constructor explaining supplier reconstruction - Add 3 unit tests for hasKeyStore() and no-arg factory validation
|
Note that as a result of this latest iteration, I've identified and captured some additional future work to explore in subsequent PRs which I've captured here: https://paste.googleplex.com/5973231872901120 |
| @InternalApi | ||
| public class MtlsHttpTransportFactory implements HttpTransportFactory { | ||
| private final KeyStore mtlsKeyStore; | ||
| public class MtlsHttpTransportFactory implements HttpTransportFactory, java.io.Serializable { |
There was a problem hiding this comment.
nit: Can we import the what is needed (e.g. Serializable) to avoid needing the fully qualified names?
There was a problem hiding this comment.
Done here and elsewhere where we were using fully qualified names unnecessarily
| /** Functional interface for supplying an actor token for IdentityPool credentials. */ | ||
| @NullMarked | ||
| @FunctionalInterface | ||
| interface IdentityPoolActorTokenSupplier extends java.io.Serializable { |
There was a problem hiding this comment.
Some thoughts about the hierarchy (not a blocker on this PR as the interface is package-private and the methods added are internal), but wanted to get your thoughts:
From reading the docs, I believe this is a deliberate design choice as not all general ExternalAccount types may have the concept of an actor_token. I have a slight worry that we may be tying the interface a little too closely to the class itself. (SubjectToken interface and now a new ActorToken interface). Expanding this in the future may result in something like PluggableActorTokenSupplier and AwsActorTokenSupplier interfaces or new interfaces for a new token type.
What do you think about having something like ExternalAccountTokenSupplier (generalized for subject and actor tokens now and can work for any new token in the future). I see the two token methods below take in ExternalAccountSupplierContext param so it may be a bit harder to generalize for more types.
public interface ExternalAccountTokenSupplier {
String getSubjectToken(ExternalAccountSupplierContext context);
String getActorToken(ExternalAccountSupplierContext context);
}
IdentityPoolSubjectTokenSupplier would extend from ExternalAccountTokenSupplier and throw an exception or return null for getActorToken.
IdentityPoolSubjectTokenSupplier interface would be marked as obsolete and we would use the more general ExternalAccountTokenSupplier. Widening the class type should be fine.
There was a problem hiding this comment.
Thanks for flagging this! I definitely agree we will want to avoid a future where we would have something like "PluggableActorTokenSupplier" and "AwsActorTokenSupplier" interfaces.
With that said, I think we may hit a problem if trying to combine them into a single ExternalAccountTokenSupplier which is that I believe java limits @FunctionalInterface to having only a single abstract method and then whenever a lambda expression is provided it is used for that single abstract method.
I think if/when we expand the actor token support beyond IdentityPoolCredentials we can evaluate other options (which we won't be limited to given the package-private nature of this current solution as you called out), such as having a single ExternalAccountActorTokenSupplier (keeping it separate from the subject token but generalizing beyond cred type).
| URI uri = URI.create(url); | ||
| String host = uri.getHost(); | ||
| if (host != null | ||
| && host.endsWith("googleapis.com") | ||
| && !host.contains(".mtls.") | ||
| && !host.contains(".p.")) { | ||
| throw new IllegalArgumentException( | ||
| "The " | ||
| + fieldName | ||
| + " endpoint (" | ||
| + url | ||
| + ") cannot be used with actor tokens because it is a plain public Google API" | ||
| + " endpoint. Please use an mTLS endpoint (e.g. containing '.mtls.') or Private" | ||
| + " Service Connect (containing '.p.')."); | ||
| } |
There was a problem hiding this comment.
One general question: Tracing through the tokenUrl flow, I see that we have a default TOKEN_URL set to be https://sts.{UNIVERSE_DOMAIN}/v1/token which then gets replaced with the proper universe domain. Can you double check that this wouldn't result in accidental IllegalArgumentException errors from this?
There was a problem hiding this comment.
Double checked this shouldn't lead to any accidental IllegalArgumentException errors. The constructor of the base ExternalAccountCredentials checks if the tokenUrl is null and if so uses the default token URL that replaces the universe domain ("https://sts.{UNIVERSE_DOMAIN}/v1/token"). That means getTokenUrl never contains the unreplaced universe domain. The only time we'd get an IllegalArgumentException is if the user doesn't specify the mtls token url while specifying an actor token requiring the mtls endpoint - but this is WAI. There is a possible world in the future, from what I've heard, where we may be able to do a token exchange on a non-mtls endpoint so leaving that door open for now seems to make the most sense to me (though we still plan to validate the user config has the mtls token url if they include the actor token).
…token suppliers
…nd add @nullable to getActorTokenType
| @Nullable private final IdentityPoolActorTokenSupplier actorTokenSupplier; | ||
| @Nullable private final String actorTokenType; | ||
| // Transient: not serialized directly. Reconstructed in readObject() from the credentialSource | ||
| // certificate config so deserialized credentials remain usable for mTLS and refresh. | ||
| @Nullable private transient X509Provider x509Provider; |
There was a problem hiding this comment.
nit: annotation is type-use so it should be on the type
There was a problem hiding this comment.
Shoot - sorry about that. Fixed!
# Conflicts: # google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
| if (builder.transportFactory == null | ||
| || builder.transportFactory == OAuth2Utils.HTTP_TRANSPORT_FACTORY | ||
| || builder.transportFactory instanceof OAuth2Utils.DefaultHttpTransportFactory) { | ||
| this.transportFactory = new MtlsHttpTransportFactory(mtlsKeyStore); | ||
| } |
There was a problem hiding this comment.
IIUC, since this upgrades the transportFactory to the mtls variant when there is the default transportFactory, what are the consequences when users set a custom HttpTransportFactory?
If it may not work, should we add a debug level statement? Or would it be better to throw an exception for an invalid state
There was a problem hiding this comment.
Added an else block to log at the debug level to inform users that they need to ensure that mtls is supported in their custom transport factory if mtls is required by the token endpoint.
| HttpTransportFactory cycleTransportFactory = this.transportFactory; | ||
| if (this.x509Provider != null) { | ||
| KeyStore pinnedKeyStore = this.x509Provider.getKeyStore(); | ||
| cycleTransportFactory = new MtlsHttpTransportFactory(pinnedKeyStore); |
There was a problem hiding this comment.
qq, do we need the same default transport check here as well to avoid overriding any custom transport?
There was a problem hiding this comment.
Thanks for flagging this gap - this definitely needed the same check to avoid overriding any custom transport and added a test to ensure the custom transport gets preserved across refreshes
| if (host != null | ||
| && host.endsWith("googleapis.com") | ||
| && !host.contains(".mtls.") | ||
| && !host.contains(".p.")) { |
There was a problem hiding this comment.
nit: Can you add a small comment to explain the conditions as it was a bit hard for me to figure out immediately.
IIUC, this checks that GDU cannot work unless it's configured with PSC or MTLS right? We do allow for non-GDU, but don't explicitly check for PSC/ mTLS in those cases.
I'm sure there probably are some weird edge cases that we may not be validating here and I'm assuming that we don't intend to cover every possible case.
There was a problem hiding this comment.
Added a comment clarifying the intent for GDU but not enforcing non-GDU universes.
| } catch (IllegalArgumentException e) { | ||
| throw e; |
There was a problem hiding this comment.
qq, why throw this exception again?
There was a problem hiding this comment.
The original reason for this (keeping the desired IAE that resulted from reasons not related to URL validity) is actually no longer relevant (it's handled in the constructor of ExternalAccountCredentials which already validates the tokenUrl and serviceAccountImpersonationUrl anyways) so I was able to remove the try/catch and simplify this a bit! Thanks for flagging!
| if (actorTokenFieldName != null) { | ||
| if (actorTokenFieldName.trim().isEmpty()) { | ||
| throw new IllegalArgumentException("The actor_token_field_name must not be empty."); | ||
| } | ||
| if (actorTokenFieldName.equals(subjectTokenFieldName)) { | ||
| throw new IllegalArgumentException( | ||
| "The actor_token_field_name must differ from the subject_token_field_name."); | ||
| } | ||
| } |
There was a problem hiding this comment.
just a general question for my curiosity (nothing wrong with validation): How often/ realistic do you think something like this would end up being the case. IIUC, these values will be generated initially by gcloud but I believe a user can simply modify the file themselves if they want. Is this something we've seen customers do in the past?
There was a problem hiding this comment.
This check was added from an earlier review suggestion by @lsirac (comment #13955 (comment)) to fail fast on obvious misconfigurations.
You're right that when users generate the credential config directly via gcloud iam workload-identity-pools create-cred-config, gcloud outputs well-formed field names automatically. However, in practice we do see customers authoring or templating these JSON config files outside of gcloud—for example, via Terraform/IaC templates, Kubernetes ConfigMaps/Helm charts, or by copy-pasting and hand-editing an existing config file when setting up custom token suppliers.
If someone accidentally copy-pastes the same JSON key for both subject_token_field_name and actor_token_field_name (or leaves it blank in a template), failing immediately at config load time with a clear IllegalArgumentException is much easier to diagnose than sending duplicate tokens to STS and getting a generic 400 invalid_grant error during runtime token exchange.
… add debug log, and simplify validateMtlsEndpoint
…d and avoid spurious custom transport log
1fed6d4
into
googleapis:oauth2-bound-tokens
4 participants
Summary
Adds support for actor tokens in
IdentityPoolCredentialsduring certificate-bound OAuth2 STS token exchange, and implements per-cycle mTLS certificate snapshotting and single-retry rotation.Architectural Changes & Review Feedback Resolutions
1. Actor Token Extraction & Atomic Reads
IdentityPoolActorTokenSupplierinterface.FileIdentityPoolSubjectTokenSupplierwithreadTokens()to read and parse JSON credential files in a single atomic I/O operation, returning aTokenPairfor subject and actor tokens.IdentityPoolCredentialSource:actorTokenFieldNamemust be non-empty, distinct fromsubjectTokenFieldName, and is supported only on JSON file sources.2. Per-Cycle Certificate Pinning & 401 Rotation Retry
refreshAccessToken()snapshots theKeyStorefromX509Providerat the start of each refresh cycle, creating a cycle-isolatedMtlsHttpTransportFactorypassed to the STS exchange.refreshAccessToken()re-reads a freshKeyStorefromx509Providerand retries the exchange once (preserving original error context viaaddSuppressedif cert reload fails)..mtls.to support custom universes and PSC endpoints.3. Serialization & Binary Backward Compatibility
FileIdentityPoolSubjectTokenSupplierclass name and declared staticserialVersionUID = 7152208690659890358L;, matching the synthetic SUID generated before this PR to preventInvalidClassExceptionon existing serialized credentials.public NetHttpTransport create()return type onMtlsHttpTransportFactoryto preserve binary compatibility for compiled callers via covariant return typing.readObject()inIdentityPoolCredentialsto reconstruct transientx509ProviderandMtlsHttpTransportFactoryfrom the serializedcredentialSource, ensuringrefreshAccessToken()andcreateScoped()remain fully functional after deserialization.MtlsHttpTransportFactory.hasKeyStore()checksmtlsKeyStore != null && mtlsKeyStore.size() > 0, ensuring uninitialized or empty KeyStores are not treated as mTLS-configured.4. API Visibility
setActorTokenSupplier,setActorTokenType, andsetX509Providerpackage-private onIdentityPoolCredentials.Builderfor internal/testing use.Test Coverage
oauth2_httpmodule).ExternalAccountCredentials.fromStream()and executingrefreshAccessToken().com.spotify.fmt:fmt-maven-plugin:2.25.Manual Testing
readObject()restoration, and multi-cycle refreshes.Next Steps
The next phase (which will follow in a separate PR to keep reviews
scoped) will introduce dynamic mTLS transport rotation natively in
GAX and a 401 connection-draining interceptor required for
downstream service retry logic.
Based on design: https://docs.google.com/document/d/1NIKeJX86ETNAjoQA-lZIE_G_8mHspisBaL8gwNCO3dA/edit?resourcekey=0-z7teBZZk0WFIJEHL2ODZxw&tab=t.0
See b/542238030 for tracking