feat(oauth2): support non-mTLS token URLs and unbound actor tokens in IdentityPoolCredentials - #14430
macastelaz wants to merge 5 commits into
Conversation
… IdentityPoolCredentials
There was a problem hiding this comment.
Code Review
This pull request removes the restriction requiring mTLS endpoints and transport configuration for actor token exchanges in IdentityPoolCredentials. Validation checks enforcing mTLS are removed, and unit tests are updated to verify that configuring actor tokens without mTLS now succeeds. Feedback is provided regarding a cross-platform issue in a new test where unescaped backslashes in a file path can cause JSON parsing failures on Windows.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| return this.x509Provider != null | ||
| || (this.transportFactory instanceof MtlsHttpTransportFactory | ||
| && ((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore()); | ||
| } |
There was a problem hiding this comment.
Now that isMtlsConfigured() is removed here, MtlsHttpTransportFactory.hasKeyStore() and checkHasKeyStore(KeyStore) have no production callers left in the repo, but new MtlsHttpTransportFactory(keyStore) still scans keyStore.aliases() and certificate chains on every refresh and 401 retry. Should we remove hasKeyStore() and the duplicate mtlsHttpTransportFactory_hasKeyStore_* tests in IdentityPoolCredentialsTest?
There was a problem hiding this comment.
Removed the 3 duplicate mtlsHttpTransportFactory_hasKeyStore_* tests from IdentityPoolCredentialsTest since they are already covered in MtlsHttpTransportFactoryTest. Note, however, that I kept MtlsHttpTransportFactory.hasKeyStore() itself because PR #14212 actively calls ((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore() in IdentityPoolCredentials.shouldUseMtlsTransportFactory()
and readObject().
| + " source or MtlsHttpTransportFactory.", | ||
| e.getMessage()); | ||
| assertNotNull(credentials); | ||
| assertSame(actorSupplier, credentials.getIdentityPoolActorTokenSupplier()); |
There was a problem hiding this comment.
In Builder(IdentityPoolCredentials) at line 391, this.actorTokenSupplier is only copied when this.credentialSource == null, while this.actorTokenType is copied unconditionally. For a credential built like this test with both credentialSource and .setActorTokenSupplier(actorSupplier), calling credentials.createScoped(...) or credentials.toBuilder().build() drops actorTokenSupplier and throws IllegalArgumentException. Should Builder(IdentityPoolCredentials) preserve credentials.actorTokenSupplier whenever credentials.actorTokenSupplier != credentials.subjectTokenSupplier?
There was a problem hiding this comment.
Great catch! Updated Builder(IdentityPoolCredentials) to preserve credentials.actorTokenSupplier when credentials.actorTokenSupplier != credentials.subjectTokenSupplier (even when credentialSource != null), and added a credentials.createScoped(...) assertion to builder_actorTokenWithNonMtlsTransportFactory_succeeds to verify both actorTokenSupplier and actorTokenType are preserved.
| .build(); | ||
| assertNotNull(cred); | ||
| assertEquals("urn:ietf:params:oauth:token-type:jwt", cred.getActorTokenType()); | ||
| assertEquals(MockExternalAccountCredentialsTransport.STS_MTLS_URL, cred.getTokenUrl()); |
There was a problem hiding this comment.
Can we assert cred.getServiceAccountImpersonationUrl() here? The test configures the impersonation URL and verifies tokenUrl and actorTokenType, but it omits checking the impersonation URL on the built instance.
There was a problem hiding this comment.
Added assertEquals for cred.getServiceAccountImpersonationUrl().
|
|
||
| // Verify Java serialization/deserialization round-trip preserves actor token config | ||
| IdentityPoolCredentials deserialized = serializeAndDeserialize(idp); | ||
| assertEquals("urn:ietf:params:oauth:token-type:jwt", deserialized.getActorTokenType()); |
There was a problem hiding this comment.
Can we add assertNull(deserialized.getX509Provider()) and assertFalse(deserialized.getTransportFactory() instanceof MtlsHttpTransportFactory) to this deserialization check? We should confirm readObject() did not trigger unintended mTLS reconstruction on non-mTLS credentials.
There was a problem hiding this comment.
Added assertNull(deserialized.getX509Provider()) and assertFalse(deserialized.getTransportFactory() instanceof MtlsHttpTransportFactory) after serializeAndDeserialize(idp).
| void builder_actorTokenWithNoArgMtlsFactory_throws() throws Exception { | ||
| // A no-arg MtlsHttpTransportFactory (e.g. from deserialization) has no KeyStore, | ||
| // so isMtlsConfigured() should return false and building should fail. | ||
| void builder_actorTokenWithNoArgMtlsFactory_succeeds() throws Exception { |
There was a problem hiding this comment.
Can we assert assertSame(factory, credentials.getTransportFactory()) in this test and builder_actorTokenWithEmptyMtlsFactory_succeeds to verify the configured factory was preserved? We can also drop throws Exception from the method signature.
There was a problem hiding this comment.
Added assertSame(noArgFactory, credentials.getTransportFactory()) (and dropped throws Exception) in builder_actorTokenWithNoArgMtlsFactory_succeeds, and added assertSame(emptyFactory, credentials.getTransportFactory()) in builder_actorTokenWithEmptyMtlsFactory_succeeds.
| void builder_actorTokenWithNonMtlsTransportFactory_succeeds() { | ||
| IdentityPoolCredentialSource credentialSource = createFileCredentialSource(); | ||
| IdentityPoolActorTokenSupplier actorSupplier = | ||
| new IdentityPoolActorTokenSupplier() { |
There was a problem hiding this comment.
Nit: Can we use a lambda like context -> "token" or testActorSupplier here instead of the anonymous inner class?
There was a problem hiding this comment.
Replaced the anonymous inner class with IdentityPoolActorTokenSupplier actorSupplier = context -> "token";.
| .setHttpTransportFactory(transportFactory); | ||
|
|
||
| TestableIdentityPoolCredentials testable = | ||
| new TestableIdentityPoolCredentials(builder, true, false); |
There was a problem hiding this comment.
Nit: Can we just use new TestableIdentityPoolCredentials(builder, true) here? The two-argument constructor defaults failOnAllExchanges to false already. We can also pass OAuth2Utils.HTTP_TRANSPORT_FACTORY directly instead of instantiating MockExternalAccountCredentialsTransportFactory since TestableIdentityPoolCredentials mocks the exchange.
There was a problem hiding this comment.
Updated refreshAccessToken_401WithActorTokenAndNonMtlsTransport_bubblesUpWithoutRetry to pass OAuth2Utils.HTTP_TRANSPORT_FACTORY directly and use the two-argument new TestableIdentityPoolCredentials(builder, true) constructor.
|
Thanks for the updates I think there are a few issues to think about:
|
…sonation caching, and custom transport deserialization
Thanks for the thorough review! Addressed all three issues:
|
Summary
Allows
actor_tokenandactor_token_typeto be used inIdentityPoolCredentialswith standard (non-mTLS) STS and IAM impersonation endpoints and without requiring client certificate (certificate_config) configuration.Context & Rationale
In #13955, client-side guardrails (
isMtlsConfigured()andvalidateMtlsEndpoint()) were enforced in theIdentityPoolCredentialsconstructor because Google STS initially required actor tokens to be paired with certificate-bound tokens over mTLS. Per the original design discussion, this was intentionally designed as a one-way door that could be loosened in a non-breaking manner once backend support for non-mTLS actor token exchanges was ready.Changes
isMtlsConfigured()check inIdentityPoolCredentials(Builder)so actor tokens can be configured with standardHttpTransportFactoryinstances and without acertificateblock.validateMtlsEndpoint()checks ontokenUrlandserviceAccountImpersonationUrl, allowing standard public endpoints (e.g.,https://sts.googleapis.com/v1/token) to be used with actor tokens.actorTokenSupplierandactorTokenType, as well as JSON format checks for file-based actor token extraction.x509Provider == null(non-mTLS credentials),refreshAccessToken()uses standard transport without snapshotting aKeyStore, and401 Unauthorizederrors propagate immediately without retry.x509Provider != null && transportFactory instanceof MtlsHttpTransportFactory(mTLS credentials), per-cycle certificate pinning and single-retry cert reload on401remain unchanged.IdentityPoolCredentials.*_succeeds).refreshAccessToken_401WithActorTokenAndNonMtlsTransport_bubblesUpWithoutRetryandfromStream_fileCredentialSource_withoutCertificateConfig_andActorToken_withNonMtlsUrl_refreshesSuccessfullyto verify end-to-end non-mTLS actor token exchanges.Verification
oauth2_httpmodule (including all 88 tests inIdentityPoolCredentialsTest).fmt-maven-plugin:2.25:check.