feat(oauth2): Extract actor tokens for cert-bound OAuth2 STS exchange - #13955
feat(oauth2): Extract actor tokens for cert-bound OAuth2 STS exchange#13955macastelaz wants to merge 15 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.
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.
| /** Constructs a default factory for mTLS transports without a custom KeyStore. */ | ||
| public MtlsHttpTransportFactory() { | ||
| this.mtlsKeyStore = null; | ||
| } |
There was a problem hiding this comment.
This is a little confusing to me - why is this being exposed, and when would it be called?
There was a problem hiding this comment.
Added context in the comment but Tl;Dr is that it is not possibly serialized and to support serialization it needs a no-arg constructor.
| * to exchange for GCP access tokens via a local file. | ||
| */ | ||
| @NullMarked | ||
| class FileIdentityPoolTokenSupplier |
There was a problem hiding this comment.
I think renaming will break deserialization for existing creds. Perhaps keep the old name?
There was a problem hiding this comment.
Ack - kept old name and added comment explaining the misnomer (it does more than subject tokens now)
|
|
||
| CachedFile cached = this.cachedFile; | ||
|
|
||
| if (cached == null || cached.lastModified < lastModified) { |
There was a problem hiding this comment.
Hmm, I'm not sure that checking the lastModified timestamp is reliable.
I'm not sure caching is necessary here though -- this isn't really expensive and we don't cache it today. I'd suggest not caching this.
There was a problem hiding this comment.
Removed this caching in favor of atomic read for both properties
| } | ||
| credentialFormatType = CredentialFormatType.JSON; | ||
| subjectTokenFieldName = formatMap.get("subject_token_field_name"); | ||
| actorTokenFieldName = formatMap.get("actor_token_field_name"); |
There was a problem hiding this comment.
Should we validate this and fail early? We don't really do this otherwise but I think there are some edge cases worth checking (actor name == subject token name etc)
There was a problem hiding this comment.
Seems good to try to catch some of the more "obvious" misconfigs (e.g. empty string for actor token field name or the same field name as the subject token field name). Added them here.
| * <p>Note: Actor token extraction is currently restricted to file-based JSON credential sources | ||
| * over mTLS endpoints. When configuring certificate-bound OAuth 2.0 tokens for GAX channel | ||
| * providers, ensure you configure {@code | ||
| * InstantiatingGrpcChannelProvider.newBuilder().setMtlsProvider(...)} in tandem. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Good catch - rewritten to get rid of this reference in the docstring
| X509Provider x509Provider = getX509Provider(builder, credentialSource); | ||
| KeyStore mtlsKeyStore = x509Provider.getKeyStore(); | ||
| this.transportFactory = new MtlsHttpTransportFactory(mtlsKeyStore); |
There was a problem hiding this comment.
Can we refactor our approach so we make sure to use the same cert across all requests for a cred (e.g. STS, IAM)?
I believe in the current implementation, a cert rotation can cause different endpoint to use different cert, when the request would have otherwise gotten through.
There was a problem hiding this comment.
So I think this is something we want to do but I think we can/should defer that from this PR which is mainly focused on actor token extraction - though it does have implications that I've made adjustments for (specifically change the impl here to pin the x509 provider for the refresh cycle so that in a follow-up we have the same cert provider to use in the IAM call).
My understanding (and please correct me if I'm wrong) is that in the event of a cert rotation today, both calls to STS and IAM would fail. With the changes in this PR, the call to STS will succeed on retry but IAM would still fail. Stated another way, this PR doesn't solve the rotation problem, but it also doesn't make it worse and therefore I think keeping this PR focused on the STS exchange call is acceptable. Thoughts?
| .setAudience(getAudience()); | ||
|
|
||
| if (this.actorTokenSupplier != null && this.actorTokenType != null) { | ||
| String actorToken = this.actorTokenSupplier.getActorToken(supplierContext); |
There was a problem hiding this comment.
Similarly here: we should read subject tokens + actor tokens at the same time when possible.
There was a problem hiding this comment.
Updated to do a single read of both (and get rid of the underlying caching mechanism)
| } | ||
|
|
||
| @CanIgnoreReturnValue | ||
| public Builder setActorTokenSupplier(IdentityPoolActorTokenSupplier actorTokenSupplier) { |
There was a problem hiding this comment.
This makes it so they can provide a custom supplier. Is this intentional?
There was a problem hiding this comment.
Good call - I don't think we actually want to support this right now - possibly in the future but I haven't heard anything to suggest this would be an imminent requirement (and certainly isn't for public preview). Made package private.
| } | ||
|
|
||
| @CanIgnoreReturnValue | ||
| public Builder setActorTokenType(String actorTokenType) { |
There was a problem hiding this comment.
Let's add javadocs here and above
| super(credentials); | ||
| if (this.credentialSource == null) { | ||
| this.subjectTokenSupplier = credentials.subjectTokenSupplier; | ||
| this.actorTokenSupplier = credentials.actorTokenSupplier; |
There was a problem hiding this comment.
Since they can pass a custom actor token supplier (see below), I believe this misses that case
There was a problem hiding this comment.
Made the setter for the custom actor token supplier package private which means the only source for this supplier is the credential source and thus this is safe.
If we ever expose the ability to set a custom actor token supplier, we'd need to revisit this but I haven't heard of that being a requirement to date.
|
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 |
Background
This PR is the first milestone in supporting Certificate-Bound
OAuth 2.0 Tokens for workloads calling Google APIs via mTLS.
Currently, ExternalAccountCredentials assumes that a workload
provides either a file credential or a certificate configuration,
but not both. For certificate-bound tokens, we need the mTLS
certificate configuration for the transport layer alongside a disk
file containing the JSON payload representing the identity tokens
(e.g., subject and actor tokens).
What this PR does
This PR updates the OAuth2 configuration extraction and STS
(Security Token Service) wire-up to support fetching bound tokens
with delegation.
Specifically, it includes:
Modified IdentityPoolCredentialSource to allow both a
credential_source (file) and a certificate_config (mTLS) to co-
exist without throwing an IllegalArgumentException.
IdentityPoolCredentialSource to parse actor_token_type and
Extended ExternalAccountCredentials and
actor_token_field_name from the JSON configuration payload.
Refactored FileIdentityPoolSubjectTokenSupplier into a generalized
FileIdentityPoolTokenSupplier capable of extracting both actor and
subject tokens. Introduced a volatile cache so that requests for both tokens
during a single refresh cycle do not result in redundant disk I/O.
Updated IdentityPoolCredentials to take in an
IdentityPoolActorTokenSupplier and actorTokenType. Wired these
directly into the StsTokenExchangeRequest using the existing
ActingParty construct so that the actor token gets properly
injected into the STS token exchange payload.
Manual Testing
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