feat(oauth2): implement IAM impersonation mTLS transport pinning and 401 recovery by macastelaz · Pull Request #14212 · googleapis/google-cloud-java

macastelaz · 2026-08-30T02:28:22Z

🥞 Stacked PRs

👉 This PR (feat: implement IAM impersonation mTLS transport pinning and 401 recovery)
⏳ Stacked PR test(oauth2): add mTLS in-process socket tests and live GCP WIF integration tests #14220 (test: add mTLS in-process socket tests and live GCP WIF integration tests)

Summary

Propagates the cycle-pinned mTLS transport factory into ImpersonatedCredentials during Workload Identity Federation with Service Account Impersonation, ensuring the IAM generateAccessToken call uses the exact same pinned client certificate as the STS token exchange.
Also extends 401 recovery in IdentityPoolCredentials to catch unauthorized responses across both STS and IAM calls, reloading fresh certificates and re-executing the full refresh cycle.

Test Coverage

1,030 unit tests passing across oauth2_http (including transport pinning lifecycle, multi-cycle rotation, 401 retry on IAM, and suppressed error handling).
Google Java Format: 100% compliant (com.spotify.fmt:fmt-maven-plugin:2.25).

Manual Testing

Sample test "app": https://paste.googleplex.com/5495919190081536
Test results: https://paste.googleplex.com/5757145073713152
See b/542238030 for tracking.

gemini-code-assist

Code Review

This pull request introduces support for actor tokens in IdentityPoolCredentials (specifically for file-based JSON credential sources over mTLS endpoints) and adds certificate pinning with automatic certificate reloading on 401 Unauthorized errors. It also updates other credential classes to support passing a custom HttpTransportFactory during token refresh, masks actor tokens in logs, and adds corresponding unit tests. The review feedback suggests persisting the reloaded transport factory back to this.transportFactory after a successful retry to prevent subsequent refreshes from failing first, and preserving programmatically set actorTokenSupplier instances in the Builder copy constructor.

…401 recovery - Pin mTLS HttpTransportFactory across multi-step STS and IAM token exchanges so both requests use the exact same certificate snapshot within a single refresh cycle. - Add 401 Unauthorized recovery with automatic certificate reload from X509Provider and single-retry coordination in IdentityPoolCredentials and ImpersonatedCredentials. - Preserve custom non-default HttpTransportFactory instances when X509Provider is configured. - Add comprehensive unit tests across IdentityPoolCredentialsTest, ImpersonatedCredentialsTest, and OAuth2UtilsTest.

…dentials and address review findings - Explicitly scope inner sourceCredentials to CLOUD_PLATFORM_SCOPE in ExternalAccountCredentials.buildImpersonatedCredentials and ImpersonatedCredentials.refreshAccessToken so STS issues tokens authorized to call IAM generateAccessToken even when downstream target scopes are configured via createScoped. - Ensure public no-arg ImpersonatedCredentials.refreshAccessToken delegates without overriding source credential transport settings. - Preserve custom actorTokenSupplier in IdentityPoolCredentials.Builder copy constructor when credentialSource is present. - Ensure HTTP response is closed in a finally block in ImpersonatedCredentials.refreshAccessToken. - Attach initial 401 exception as suppressed when the 401 retry attempt fails in IdentityPoolCredentials.refreshWithRetry. - Add unit tests in IdentityPoolCredentialsTest and ImpersonatedCredentialsTest covering scoped impersonation, custom actorTokenSupplier preservation, and retry exception chaining.

lsirac · 2026-09-17T15:39:04Z

    }
    if (this.impersonatedCredentials != null) {
-      return this.impersonatedCredentials.refreshAccessToken();
+      return this.impersonatedCredentials.refreshAccessToken(cycleTransportFactory);


When serviceAccountImpersonationUrl is set, IdentityPoolCredentials.refreshWithRetry fetches the subject and actor tokens to build stsTokenExchangeRequest, calls exchangeExternalCredentialForAccessToken, and then immediately delegates to impersonatedCredentials.refreshAccessToken here while discarding stsTokenExchangeRequest. The inner credential then fetches the subject and actor tokens a second time in the same refresh cycle, or four times on a 401 retry. Can we short-circuit before fetching tokens when serviceAccountImpersonationUrl is present?

Done! Added ImpersonatedCredentials impersonated = getImpersonatedCredentials();
if (impersonated != null) {
return impersonated.refreshAccessToken(cycleTransportFactory);
}

at the start of: IdentityPoolCredentials.refreshWithRetry, AwsCredentials.refreshAccessToken, and
PluggableAuthCredentials.refreshAccessToken before retrieveSubjectToken() / getActorToken() are called.

Subject and actor tokens (and AWS metadata requests) are now fetched only once per refresh attempt (or twice on a 401 retry). Kept the check inside exchangeExternalCredentialForAccessToken as a defensive fallback if called directly.

lsirac · 2026-09-17T15:39:04Z

+   * @return the refreshed access token
+   * @throws IOException if the token refresh fails
+   */
+  public AccessToken refreshAccessToken(HttpTransportFactory transportFactory) throws IOException {


The default refreshAccessToken(HttpTransportFactory) delegates to refreshAccessToken() and silently drops the transport factory. All internal subclasses override it, but a third-party subclass would lose transport pinning without warning. Should we annotate this method or class with @InternalExtensionOnly per the repository versioning guidelines?

Annotated public AccessToken refreshAccessToken(HttpTransportFactory cycleTransportFactory) with @InternalExtensionOnly across ExternalAccountCredentials, IdentityPoolCredentials, AwsCredentials, and PluggableAuthCredentials (and kept ImpersonatedCredentials.refreshAccessToken(HttpTransportFactory) package-private).

lsirac · 2026-09-17T15:39:04Z

  }

+  /**
+   * Refreshes the access token using the specified transport factory. Default implementation


Nit: The Javadoc says the default implementation delegates to refreshAccessToken(), whereas all three subclasses do the reverse by delegating the no-arg method into this overload. Also, naming the parameter transportFactory in AwsCredentials.java:127 and ImpersonatedCredentials.java:601 shadows the class field, whereas IdentityPoolCredentials names it cycleTransportFactory.

Updated the Javadoc on ExternalAccountCredentials.refreshAccessToken(HttpTransportFactory) to clarify that internal subclasses delegate refreshAccessToken() into this method while the default implementation delegates back to refreshAccessToken(), and renamed the parameter to cycleTransportFactory across all classes to avoid shadowing this.transportFactory.

lsirac · 2026-09-17T15:39:04Z

+                  .refreshAccessToken(effectiveTransportFactory);
+      Credentials authCredentials =
+          intermediateAccessToken != null
+              ? OAuth2Credentials.create(intermediateAccessToken)


Wrapping intermediateAccessToken in OAuth2Credentials.create creates a bare credential with a null quotaProjectId, which drops the x-goog-user-project header from the IAM generateAccessToken request. Can we use OAuth2Credentials.newBuilder().setAccessToken(intermediateAccessToken).setQuotaProjectId(this.sourceCredentials.getQuotaProjectId()).build() so quota attribution is preserved?

Replaced OAuth2Credentials.create(intermediateAccessToken) with a GoogleCredentials wrapper initialized via GoogleCredentials.newBuilder().setQuotaProjectId(this.sourceCredentials.getQuotaProjectId()).setUniverseDomain(this.sourceCredentials.getUniverseDomain()) so both quotaProjectId (x-goog-user-project) and universeDomain are preserved on the IAM request.

lsirac · 2026-09-17T15:39:04Z

+      }
+      AccessToken intermediateAccessToken =
+          (transportFactory == null)
+              ? ((ExternalAccountCredentials) this.sourceCredentials).refreshAccessToken()


In the null transport branch, calling refreshAccessToken directly instead of refreshIfExpired bypasses the token cache, lock, and change listeners on sourceCredentials for non-mTLS WIF impersonation. Should line 620 call refreshIfExpired and read getAccessToken, while keeping the uncached refreshAccessToken(effectiveTransportFactory) call on line 621 for pinned mTLS transports?

Done! When cycleTransportFactory == null, we now call this.sourceCredentials.refreshIfExpired() and pass this.sourceCredentials directly to new HttpCredentialsAdapter(this.sourceCredentials) (preserving token caching, locking, and change listeners), while keeping the uncached refreshAccessToken(effectiveTransportFactory) call when cycleTransportFactory != null.

lsirac · 2026-09-17T15:39:04Z

+                    "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/test@project.iam.gserviceaccount.com:generateAccessToken")) {
+          @Override
+          HttpTransportFactory createMtlsTransportFactory(KeyStore keyStore) {
+            usedKeyStores.add(keyStore);


In the five new retry and impersonation tests, createMtlsTransportFactory records usedKeyStores.add(keyStore) and returns () -> mockTransport, which is the exact same HttpTransport instance regardless of which keystore was passed. The tests would still pass even if refreshWithRetry passed the stale transport on retry. Can we return a distinct transport per keystore so the mock verifies that the retry request ran on the transport created from freshKeyStore, and add tests for a persistent 401 failure and a non-401 IAM error?

Updated createMtlsTransportFactory(KeyStore) in the impersonation mTLS tests to return a distinct HttpTransport wrapper per KeyStore that records which KeyStore executed each request (requestKeyStores), and added "refreshAccessToken_impersonation_persistent401OnIam_throwsWithSuppressed" and "refreshAccessToken_impersonation_non401OnIam_doesNotRetry".

lsirac · 2026-09-17T15:39:04Z

  }

+  @Test
+  void builder_actorToken_plainPublicTokenUrl_throws() throws Exception {


Nit: The five actor-token endpoint validation tests added at lines 1821 through 1928 duplicate the existing five tests at lines 2873 through 2981 in the same file. Can we remove the duplicate block?

Removed the 5 duplicate actor-token endpoint validation tests.

lsirac · 2026-09-17T15:39:04Z

+                errorResponse.put("error", "invalid_token");
+                errorResponse.put("error_description", "Invalid or expired client certificate.");
+                return new MockLowLevelHttpResponse()
+                    .setStatusCode(statusCode)


The early return for non-200 status codes exits before reaching assertEquals(EXPECTED_GRANT_TYPE, grantType), so the failed first attempt never has its request form parameters verified. Can we move the grant_type assertion above the status code check?

Moved the grant_type and subject_token assertions above if (statusCode != 200) so request parameters are verified on simulated error responses as well.

lsirac · 2026-09-17T15:39:04Z

+
+  @Test
+  void isUnauthorizedException_null_returnsFalse() {
+    org.junit.jupiter.api.Assertions.assertFalse(OAuth2Utils.isUnauthorizedException(null));


Nit: This file already static-imports assertEquals and assertThrows at lines 35 and 36, but the new tests write out org.junit.jupiter.api.Assertions.assertFalse, org.junit.jupiter.api.Assertions.assertTrue, java.io.IOException, com.google.api.client.http.HttpResponseException, and com.google.api.client.http.HttpHeaders inline. Can we use static imports and top-level imports here?

Replaced all inline fully qualified names with static and top-level imports in OAuth2UtilsTest.

lsirac · 2026-09-17T15:39:04Z

+        .getTransport()
+        .addStatusCodeAndMessage(HttpStatusCodes.STATUS_CODE_OK, "");
+
+    java.util.concurrent.atomic.AtomicReference<HttpTransportFactory> capturedSourceTransport =


Nit: The line-wrapping changes to the existing private key string and universe-domain assertion message are unrelated formatting churn, and java.util.concurrent.atomic.AtomicReference is written out inline. Can we revert the unrelated rewrapping to keep the diff clean and add a top-level import for AtomicReference?

Reverted the line-wrapping changes on SA_PRIVATE_KEY_PKCS8 and the universe-domain assertion message, and replaced inline fully qualified names with top-level imports.

…in IdentityPoolCredentials

…ss and FILE cert pinning

…InternalExtensionOnly

lsirac · 2026-09-19T14:10:23Z

      try {
        KeyStore mtlsKeyStore = this.x509Provider.getKeyStore();
-        this.transportFactory = new MtlsHttpTransportFactory(mtlsKeyStore);
+        if (shouldUseMtlsTransportFactory()) {


After deserialization this only rebuilds the factory when shouldUseMtlsTransportFactory() is true, so an MtlsHttpTransportFactory subclass or a ServiceLoader-provided factory comes back keyless without a client certificate, whereas the old readObject always restored the keystore-backed factory. Should this also replace the factory when transportFactory instanceof MtlsHttpTransportFactory && !((MtlsHttpTransportFactory) transportFactory).hasKeyStore()?

Done - updated readObject to also rebuild this.transportFactory via createMtlsTransportFactory(this.x509Provider.getKeyStore()) when this.transportFactory instanceof MtlsHttpTransportFactory && !((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore().

lsirac · 2026-09-19T14:10:23Z

+   * @return the refreshed access token
+   * @throws IOException if the token refresh fails
+   */
+  @InternalExtensionOnly


Does this overload need to be public? Commit f2d752e compiled cleanly with it package-private, and making it public @InternalExtensionOnly advertises a call target whose default implementation ignores transportFactory and whose IdentityPoolCredentials override skips 401 cert recovery.

Oops - that was over expansive, thanks for catching. Reverted back to package-private

lsirac · 2026-09-19T14:10:24Z

+    iamTransportFactory.getTransport().setExpireTime(getDefaultExpireTime());
+    iamTransportFactory.getTransport().addStatusCodeAndMessage(HttpStatusCodes.STATUS_CODE_OK, "");
+
+    IdentityPoolCredentials sourceCredentials =


The source credential here defaults to cloud-platform, so the new !currentScopes.contains(CLOUD_PLATFORM_SCOPE) branch at ImpersonatedCredentials.java:613 never runs. Can we pass .setScopes(Collections.singletonList("https://www.googleapis.com/auth/devstorage.read_only")) on the source builder so the test covers that branch?

Configured devstorage.read_only on sourceCredentials so the !currentScopes.contains(CLOUD_PLATFORM_SCOPE) branch is exercised.

lsirac · 2026-09-19T14:10:24Z

+            .setTokenUrl("https://sts.mtls.googleapis.com/v1/token")
+            .build();
+    IdentityPoolCredentials deserialized = serializeAndDeserialize(regularCredentials);
+    assertTrue(


Nit: Can this assert ((MtlsHttpTransportFactory) deserialized.getTransportFactory()).hasKeyStore() too? Right now instanceof MtlsHttpTransportFactory passes even when readObject leaves the deserialized subclass with a null KeyStore.

Added assertTrue(((MtlsHttpTransportFactory) deserialized.getTransportFactory()).hasKeyStore()).

lsirac · 2026-09-19T14:10:24Z

+          @Override
+          HttpTransportFactory createMtlsTransportFactory(KeyStore keyStore) {
+            usedKeyStores.add(keyStore);
+            return () -> transport;


Nit: createMtlsTransportFactory returns the same () -> transport instance for both ksA and ksB, so this test passes even if the 401 retry reuses ksA's transport. Could it return a per-keystore recording transport like the impersonation retry tests do?

Good catch - wired separate MockExternalAccountCredentialsTransport instances (transportA returning 401 for ksA, transportB returning 200 for ksB) and asserted each receives 1 request.

lsirac · 2026-09-19T14:10:24Z

      throw new IllegalArgumentException(
          "Actor tokens are only supported for mTLS token exchanges. Please configure a certificate"
-              + " source or MtlsHttpTransportFactory.");
+              + " configuration in the credential source or provide an mTLS-enabled transport.");


Nit: The exception message says "or provide an mTLS-enabled transport", but isMtlsConfigured() at line 230 only accepts x509Provider != null or an MtlsHttpTransportFactory with hasKeyStore(). Should the message say "or provide an MtlsHttpTransportFactory constructed with a KeyStore"?

Updated the exception message (and matching test assertion) to "or provide an MtlsHttpTransportFactory constructed with a KeyStore.".

lsirac · 2026-09-19T14:10:24Z

-                return credential.refreshAccessToken();
-              });
+      Future<AccessToken> futureA = executor.submit(() -> credential.refreshAccessToken());
+      Future<AccessToken> futureB = executor.submit(() -> credential.refreshAccessToken());


Nit: With the new CyclicBarrier, getKeyStoreCount is deterministically 3, so line 2442 can assert assertEquals(3, ...) instead of >= 3 and verify that credential.getTransportFactory() is still mtlsTransport after the retry.

Tightened to assertEquals(3, getKeyStoreCount.get()) and added assertSame(mtlsTransport, credential.getTransportFactory()).

lsirac · 2026-09-19T14:10:24Z

+  }
+
+  @Test
+  void hasCertificateChanged_nullOrSameReference_returnsFalse() throws Exception {


Nit: hasCertificateChanged_nullOrSameReference_returnsFalse asserts assertTrue at lines 162-163 and only checks identical references. Could we also compare two distinct KeyStore instances loaded with the same cert so oldCerts.equals(newCerts) is tested directly?

Added hasCertificateChanged_distinctKeyStoreInstances_comparesCertificates() comparing two distinct KeyStore instances loaded with the same certificate (assertFalse) and with a rotated certificate (assertTrue).

lsirac · 2026-09-19T14:10:24Z

+            return new AccessToken("intermediate-sts-token-" + count, null);
+          }
+
+          @Override


Nit: The createScoped override here is never called because line 1554 already sets cloud-platform scope, and the 401 retry comes from HttpCredentialsAdapter.handleResponse on the null-transport path. Can we drop the unused createScoped override and rename the test to reflect that?

Removed the unused createScoped override and renamed the test to refreshAccessToken_withoutCycleTransportFactory_externalAccountSourceRetriesOn401FromIam.

…1 recovery

lsirac · 2026-09-21T23:03:19Z

+    try {
+      ImpersonatedCredentials impersonated = getImpersonatedCredentials();
+      if (impersonated != null) {
+        return impersonated.refreshAccessToken(cycleTransportFactory);


cycleTransportFactory is never null here because this.transportFactory is always initialized in ExternalAccountCredentials. As a result, non-mTLS pools also take the uncached branch in ImpersonatedCredentials, which skips sourceCredentials.refreshIfExpired() on every refresh and clears HttpCredentialsAdapter's 401 retry handler on the IAM request. Can we pass pinnedKeyStore != null ? cycleTransportFactory : null so only pinned mTLS cycles opt out?

Updated refreshWithRetry() to pass pinnedKeyStore != null ? cycleTransportFactory : null into impersonated.refreshAccessToken(...) so only pinned mTLS cycles opt out of sourceCredentials.refreshIfExpired() caching and HttpCredentialsAdapter's 401 retry handler.

lsirac · 2026-09-21T23:03:19Z

+  AccessToken refreshAccessToken(HttpTransportFactory cycleTransportFactory) throws IOException {
+    ImpersonatedCredentials impersonated = getImpersonatedCredentials();
+    if (impersonated != null) {
+      return impersonated.refreshAccessToken(cycleTransportFactory);


Since this.transportFactory is never null, passing cycleTransportFactory here always forces ImpersonatedCredentials onto the uncached path and disables the 401 retry on IAM. Can this pass null since AWS credentials do not pin a per-cycle mTLS transport?

Updated AwsCredentials to pass cycleTransportFactory == this.transportFactory ? null : cycleTransportFactory into impersonated.refreshAccessToken(...) so standard refreshes retain sourceCredentials.refreshIfExpired() caching and IAM 401 retry handling.

lsirac · 2026-09-21T23:03:19Z

+  AccessToken refreshAccessToken(HttpTransportFactory cycleTransportFactory) throws IOException {
+    ImpersonatedCredentials impersonated = getImpersonatedCredentials();
+    if (impersonated != null) {
+      return impersonated.refreshAccessToken(cycleTransportFactory);


Same as AwsCredentials, cycleTransportFactory is always non-null here even though no per-cycle mTLS transport is pinned. Can we pass null so ImpersonatedCredentials keeps sourceCredentials.refreshIfExpired() caching and the IAM 401 retry?

Updated PluggableAuthCredentials to pass cycleTransportFactory == this.transportFactory ? null : cycleTransportFactory into impersonated.refreshAccessToken(...) so ImpersonatedCredentials retains sourceCredentials.refreshIfExpired() caching and IAM 401 retry handling.

lsirac · 2026-09-21T23:03:19Z


  private final @Nullable String tokenInfoUrl;
  private final @Nullable String serviceAccountImpersonationUrl;
+  private transient @Nullable String targetServiceAccountEmail;


Because targetServiceAccountEmail is marked transient and mutated after construction at line 316 on a sourceCredentials copy whose serviceAccountImpersonationUrl is null, deserializing ImpersonatedCredentials or its sourceCredentials resets targetServiceAccountEmail to null. After deserialization, getServiceAccountEmail() returns null and drops GOOGLE_EXTERNAL_ACCOUNT_IMPERSONATED_EMAIL in PluggableAuthCredentials. Can we drop transient, make this field final, and set it on the Builder before calling .build()?

Dropped transient, made targetServiceAccountEmail final, set it on ExternalAccountCredentials.Builder before calling .build() in buildImpersonatedCredentials(), and added an assertion in AwsCredentialsTest.serialize() verifying getServiceAccountEmail() is preserved across serialization.

lsirac · 2026-09-21T23:03:19Z

            && ((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore());
  }

+  private boolean shouldUseMtlsTransportFactory() {


In readObject() at lines 465 to 472, if x509Provider.getKeyStore() fails transiently during deserialization, the catch comment notes that cert loading will be handled on refreshAccessToken(). However, if this.transportFactory is a deserialized MtlsHttpTransportFactory subclass with hasKeyStore() == false, shouldUseMtlsTransportFactory() returns false because it omits the this.transportFactory instanceof MtlsHttpTransportFactory && !((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore() check from line 466. Should shouldUseMtlsTransportFactory() include that check too?

Added (this.transportFactory instanceof MtlsHttpTransportFactory && !((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore()) directly into shouldUseMtlsTransportFactory() and simplified readObject() to call if (shouldUseMtlsTransportFactory()).

lsirac · 2026-09-21T23:03:19Z

+   * @return the refreshed access token
+   * @throws IOException if the token refresh fails
+   */
+  @InternalExtensionOnly


Nit: Now that refreshAccessToken(HttpTransportFactory) is package-private here and in IdentityPoolCredentials, AwsCredentials, and PluggableAuthCredentials, @InternalExtensionOnly is no longer needed since that annotation is only for public APIs. Can we drop @InternalExtensionOnly and its import from these four classes?

Removed @InternalExtensionOnly from refreshAccessToken(HttpTransportFactory) (and its unused imports) across ExternalAccountCredentials, IdentityPoolCredentials, AwsCredentials, and PluggableAuthCredentials.

lsirac · 2026-09-21T23:03:19Z

    // Properly deserialize the transient transportFactory.
    input.defaultReadObject();
    transportFactory = newInstance(transportFactoryClassName);
+    impersonatedCredentials = null;


Nit: impersonatedCredentials is declared transient at line 100, so it is already null after input.defaultReadObject() and this assignment can be dropped.

Removed the redundant this.impersonatedCredentials = null; assignment in readObject().

lsirac · 2026-09-21T23:03:19Z

+   */
+  AccessToken refreshAccessToken(@Nullable HttpTransportFactory cycleTransportFactory)
+      throws IOException {
+    HttpTransportFactory effectiveTransportFactory =


Nit: this.transportFactory is already guaranteed non-null by firstNonNull in the constructor and readObject(), so firstNonNull(cycleTransportFactory, this.transportFactory) can replace this nested ternary.

Replaced the ternary with firstNonNull(cycleTransportFactory, this.transportFactory).

lsirac · 2026-09-21T23:03:19Z

+        } catch (IOException e) {
+          throw new IOException("Unable to refresh sourceCredentials", e);
+        }
+        Credentials authCredentials =


Nit: Declaring this local as GoogleCredentials authCredentials avoids adding the com.google.auth.Credentials import at line 49.

Declared authCredentials as GoogleCredentials and removed the com.google.auth.Credentials import.

lsirac · 2026-09-21T23:03:19Z

      }
-      // Note: when credentialSource is present, subjectTokenSupplier and actorTokenSupplier
+      // Note: when credentialSource is present, subjectTokenSupplier and file-based
+      // actorTokenSupplier


Nit: // actorTokenSupplier is left orphaned on its own line here. Can we rewrap this comment block cleanly?

Re-wrapped the comment block in Builder(IdentityPoolCredentials).

lsirac · 2026-09-22T14:33:22Z

Found a few issues during cert rotation and impersonation:

Cert rotation fails when certificate_config.json updates cert_path or when cert.pem rotates mid-cycle
In IdentityPoolCredentials, refreshAccessToken() snapshots pinnedKeyStore = this.x509Provider.getKeyStore() at the start of the cycle, but CertificateIdentityPoolSubjectTokenSupplier.getSubjectToken() still reads the leaf certificate from disk a second time instead of reading it from pinnedKeyStore. Also, createCertificateSubjectTokenSupplier sets credentialSource.setCredentialLocation(...) only once at construction time, whereas X509Provider.getKeyStore() re-reads certificate_config.json on every refresh.
- If certificate_config.json updates cert_path and key_path to a new file path, or if cert.pem rotates on disk between x509Provider.getKeyStore() and retrieveSubjectToken(), the TLS handshake presents the new cert while subject_token sends the old cert.
- Live https://sts.mtls.googleapis.com/v1/token rejects this mismatch with HTTP 400 and OAuthException: Error code invalid_grant: Client cert does not match the cert in mTLS handshake., not 401. Because isUnauthorizedException only checks for 401, refreshWithRetry does not retry.
- Can we extract the leaf certificate directly from pinnedKeyStore for CertificateIdentityPoolSubjectTokenSupplier so the TLS client cert and subject_token are always pinned to the same snapshot?
1-hour STS token caching does not work when mTLS is enabled
In ImpersonatedCredentials.refreshAccessToken(HttpTransportFactory), when cycleTransportFactory != null, the else branch calls externalSource.refreshAccessToken(effectiveTransportFactory) directly instead of refreshIfExpired(), and never stores the returned AccessToken on externalSource. As a result, externalSource.getAccessToken() stays null and every ImpersonatedCredentials refresh re-runs the full STS token exchange against sts.mtls.googleapis.com even when pinnedKeyStore has not changed and the 1-hour STS token is still valid. Can we cache the intermediate STS token alongside the KeyStore it was minted with and reuse it while it is unexpired and the KeyStore is unchanged?
Non-atomic cert_path and key_path rotation fails in the TLS handshake and never reaches the 401 retry
When cert_path rotates just before key_path, X509Provider.getKeyStore() loads the new certificate with the old private key into KeyStore without error. Against live sts.mtls.googleapis.com, this fails during the TLS handshake with IOException: Error writing request body to server caused by SSLHandshakeException rather than returning an HTTP 401, so hasCertificateChanged is never reached. Can we validate that the private key matches the certificate public key when loading the KeyStore, or retry once when getKeyStore() or the TLS handshake fails due to a key-cert mismatch?

lsirac · 2026-09-22T15:05:01Z

-      return this.impersonatedCredentials.refreshAccessToken();
+    ImpersonatedCredentials impersonated = getImpersonatedCredentials();
+    if (impersonated != null) {
+      return impersonated.refreshAccessToken(cycleTransportFactory);


Passing cycleTransportFactory here forces ImpersonatedCredentials onto the uncached mTLS path and disables 401 retry on IAM. Can we pass null instead, matching AwsCredentials and PluggableAuthCredentials?

Updated ExternalAccountCredentials.exchangeExternalCredentialForAccessToken to pass cycleTransportFactory == this.transportFactory ? null : cycleTransportFactory (which evaluates to null when no per-cycle transport override is active, matching AwsCredentials and PluggableAuthCredentials).

lsirac · 2026-09-22T15:05:01Z

+                    "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/test@project.iam.gserviceaccount.com:generateAccessToken")) {
+          @Override
+          HttpTransportFactory createMtlsTransportFactory(KeyStore keyStore) {
+            return () -> mockTransport;


Because createMtlsTransportFactory returns () -> mockTransport without capturing the KeyStore, this test still passes if allowRetry is flipped back to true at line 247. Can we record the KeyStore passed into createMtlsTransportFactory and assert the expected keystore sequence?

createMtlsTransportFactory(KeyStore keyStore) in refreshAccessToken_impersonation_stsReturns401_retriesOnceViaOuterCycle now records capturedKeyStores.add(keyStore), returns 401 when keyStore == ks1 and 200 only when keyStore == ks2, and asserts assertEquals(Arrays.asList(ks1, ks2, ks2), capturedKeyStores).

lsirac · 2026-09-22T15:05:01Z

      try {
        KeyStore mtlsKeyStore = this.x509Provider.getKeyStore();
-        this.transportFactory = new MtlsHttpTransportFactory(mtlsKeyStore);
+        if (shouldUseMtlsTransportFactory()) {


For ServiceLoader-registered transport factories, transportFactoryClassName captures the SPI class before the mTLS upgrade. After deserialization, shouldUseMtlsTransportFactory() returns false, so mTLS is not restored. Can we persist a flag so shouldUseMtlsTransportFactory() rebuilds the mTLS transport on deserialization?

Persisted private final boolean useMtlsTransportFactory on IdentityPoolCredentials (and Builder.useMtlsTransportFactory, preserved across toBuilder()), and added deserialization_respectsUseMtlsTransportFactoryFlag in IdentityPoolCredentialsTest to verify that shouldUseMtlsTransportFactory() rebuilds the mTLS transport on deserialization.

lsirac · 2026-09-22T15:05:01Z

    assertEquals(testCredentials.hashCode(), deserializedCredentials.hashCode());
    assertEquals(testCredentials.toString(), deserializedCredentials.toString());
    assertSame(Clock.SYSTEM, deserializedCredentials.clock);
+    assertNotNull(deserializedCredentials.getServiceAccountEmail());


Because serviceAccountImpersonationUrl is set on the fixture, getServiceAccountEmail() reads the email from the URL rather than targetServiceAccountEmail. Can we assert getServiceAccountEmail() on the deserialized inner source copy where the URL was cleared?

Added assertions on deserializedInnerSource ((AwsCredentials) deserializedCredentials.getImpersonatedCredentials().getSourceCredentials()) verifying both assertNull(deserializedInnerSource.getServiceAccountImpersonationUrl()) and assertEquals(testCredentials.getServiceAccountEmail(), deserializedInnerSource.getServiceAccountEmail()).

The new constructor and getHttpStatusCode() change the computed serialVersionUID, so instances serialized by released versions would fail to load. Same change as in PR googleapis#14212.

lqiu96 · 2026-10-13T19:41:21Z

    @CanIgnoreReturnValue
    public Builder setHttpTransportFactory(HttpTransportFactory transportFactory) {
      super.setHttpTransportFactory(transportFactory);
+      this.useMtlsTransportFactory = isDefaultOrMtlsTransportFactory(transportFactory);


can we just simply this to set this only in the constructor?

Builder(IdentityPoolCredentials) needs to preserve this.useMtlsTransportFactory = credentials.useMtlsTransportFactory across clones (toBuilder(), createScoped(), buildImpersonatedCredentials()) because super(credentials) copies credentials.transportFactory into builder.transportFactory — which on an already-constructed credential is no longer null and may have been resolved via ServiceLoader (per #14212 (comment)) or an overridden createMtlsTransportFactory rather than DefaultHttpTransportFactory. If the constructor re-evaluated isDefaultOrMtlsTransportFactory(builder.transportFactory) on the cloned builder without preserving the flag, useMtlsTransportFactory would flip from true to false on the clone. And because Builder(IdentityPoolCredentials) copies that flag, setHttpTransportFactory updates it in case a caller calls credentials.toBuilder().setHttpTransportFactory(customFactory).build(). Added a comment on the builder copy constructor to clarify this!

lqiu96 · 2026-10-13T19:42:35Z

+  @VisibleForTesting
+  boolean shouldUseMtlsTransportFactory() {
+    return this.useMtlsTransportFactory
+        || (this.transportFactory instanceof MtlsHttpTransportFactory
+            && !((MtlsHttpTransportFactory) this.transportFactory).hasKeyStore());
+  }


qq, this isn't immediately clear to me why we can't just use the value of useMtlsTransportFactory? useMtlsTransportFactory seems to be the reuslt of isDefaultOrMtlsTransportFactory which looks like it already checks for this

At construction time, if a caller supplies a custom MtlsHttpTransportFactory subclass that already has a KeyStore (e.g. new CustomMtlsHttpTransportFactory(ks)), isDefaultOrMtlsTransportFactory returns false so useMtlsTransportFactory is false (preserving the custom subclass during construction and normal refreshes).

However, upon deserialization, ExternalAccountCredentials.readObject() instantiates transportFactoryClassName via its no-arg constructor, leaving this.transportFactory as a keyless MtlsHttpTransportFactory (hasKeyStore() == false) while the deserialized useMtlsTransportFactory field is still false. This second check ensures readObject() (and refreshAccessToken() if the cert load in readObject() fails transiently) still rebuilds a KeyStore-backed MtlsHttpTransportFactory in that case (see #14212 (comment)). Added an inline comment here to explain this!

Hmm, would it be possible to use the above isDefaultOrMtlsTransportFactory to simply the checks?

Done — simplified shouldUseMtlsTransportFactory() to return this.useMtlsTransportFactory || isDefaultOrMtlsTransportFactory(this.transportFactory);.

lqiu96 · 2026-10-13T19:48:18Z

+        /* allowRetry= */ false);
+  }

+  private AccessToken refreshWithRetry(


a few quick notes: This method is quite large and it's a bit hard to understand what is going on here (also admittedly I'm not as familiar with IdentityPoolCredentials or 3p creds in general).

Can we split this into a series of helpers. On a high level I think I see a few things

Resolving the transport

Retrieving the subject and actor tokens

Making the request and setting the scopes

I think we can keep the logic regarding retries and all that inside this method

Done — extracted resolveCycleTransportFactory, retrieveSubjectAndActorTokens, and buildAndExchangeStsRequest as private helpers while keeping the pinnedKeyStore snapshot and retry handling inside refreshWithRetry.

Thanks. I still find this a bit hard to follow. I think the cor elogic is in helpers, but the main challenge is with the retry logic.

Is it possible to not use recursion for this? IMO it's easier to follow just seeing the 1 or 2 requests flowing in the code.

I think it may be easier to separate the refresh call from the retry logic.

Perhaps we can create a helper for making the refresh call and invoke it once or twice inside this method depending on whether we are allowed to retry or not.

Done — separated the single-attempt refresh execution into executeRefresh(...) and removed the recursion from refreshWithRetry(...). Non-retryable callers now invoke executeRefresh(...) directly, and refreshWithRetry(...) executes Attempt 1 in the try block and Attempt 2 directly in the catch block.

lqiu96 · 2026-10-13T20:14:00Z

    }
  }

+  private static @Nullable String extractAndEncodeLeafCertificate(@Nullable KeyStore keyStore)


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

Agreed! Once both PRs land, we can extract the KeyStore leaf-certificate lookup into a shared utility method in a follow-up PR.

lqiu96 · 2026-10-13T20:15:48Z

-        ImpersonatedCredentials.extractTargetPrincipal(serviceAccountImpersonationUrl);
+    sourceBuilder
+        .setServiceAccountImpersonationUrl(null)
+        .setScopes(Collections.singletonList(OAuth2Utils.CLOUD_PLATFORM_SCOPE));


can you add a note to explain why we add this scope?

Done — added a comment explaining that sourceCredentials needs CLOUD_PLATFORM_SCOPE to authenticate to the IAM generateAccessToken endpoint, while this.scopes are passed to ImpersonatedCredentials for the final impersonated token.

lqiu96 · 2026-10-13T20:20:56Z

+    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);
    }


can this just be simplified to sourceBuilder = toBuilder()?

ExternalAccountCredentials inherits the concrete public GoogleCredentials.Builder toBuilder() from GoogleCredentials and doesn't override toBuilder() (since ExternalAccountCredentials.Builder is abstract), so toBuilder() here has return type GoogleCredentials.Builder rather than ExternalAccountCredentials.Builder. Re-declaring @Override public abstract Builder toBuilder() on ExternalAccountCredentials would turn an inherited concrete method into an abstract method on a public class (which is a breaking binary/source compatibility change in Clirr), and casting (ExternalAccountCredentials.Builder) toBuilder() would throw ClassCastException at runtime if a custom subclass didn't override toBuilder(), so we kept the explicit instanceof branches here.

lqiu96 · 2026-10-13T20:27:34Z

+  AccessToken refreshAccessToken(HttpTransportFactory cycleTransportFactory) throws IOException {
+    return refreshAccessToken();
+  }
+
+  AccessToken refreshAccessToken(
+      HttpTransportFactory cycleTransportFactory, @Nullable KeyStore pinnedKeyStore)
+      throws IOException {
+    return refreshAccessToken(cycleTransportFactory);


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?

One more note: can these methods just be abstract methods that the subclasses override?

Yes, spot on — when ExternalAccountCredentials is configured with service_account_impersonation_url, it builds an internal ImpersonatedCredentials whose sourceCredentials is a cloned copy of the ExternalAccountCredentials (with serviceAccountImpersonationUrl = null). During a refresh:

The outer IdentityPoolCredentials.refreshAccessToken() snapshots pinnedKeyStore and cycleTransportFactory and delegates to impersonatedCredentials.refreshAccessToken(cycleTransportFactory, pinnedKeyStore).

ImpersonatedCredentials then calls back into its sourceCredentials (typed as ExternalAccountCredentials) via externalSource.refreshAccessToken(effectiveTransportFactory, pinnedKeyStore) to exchange the 3P subject token at STS using that same pinned mTLS transport and KeyStore, before calling IAM generateAccessToken.

On making them abstract: because ExternalAccountCredentials is a public abstract class with protected constructors and these overloads are intentionally package-private (per #14212 (comment)), making a package-private method abstract in Java would prevent any class outside com.google.auth.oauth2 from subclassing ExternalAccountCredentials (and making them protected abstract would be a breaking public API change). Expanded the Javadoc here to explain both the impersonation call flow and why the default fallback is provided.

This looks primarily really only used/ needed for IdentityPoolCredentials. Can this just be put into that class?

Done — removed refreshAccessToken(HttpTransportFactory) and refreshAccessToken(HttpTransportFactory, KeyStore) from ExternalAccountCredentials (and AwsCredentials / PluggableAuthCredentials) so they live solely on IdentityPoolCredentials.

…onation comments

lqiu96 · 2026-10-14T05:26:24Z

+    return refreshAccessToken(this.transportFactory);
+  }
+
+  @Override
+  AccessToken refreshAccessToken(HttpTransportFactory cycleTransportFactory) throws IOException {
+    ImpersonatedCredentials impersonated = getImpersonatedCredentials();
+    if (impersonated != null) {
+      return impersonated.refreshAccessToken(
+          cycleTransportFactory == this.transportFactory ? null : cycleTransportFactory);
+    }


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)?

Good point — neither AwsCredentials nor PluggableAuthCredentials uses per-cycle transport pinning, so they don't need the refreshAccessToken(HttpTransportFactory) overload. Removed the overload from ExternalAccountCredentials, AwsCredentials, and PluggableAuthCredentials (keeping it only on IdentityPoolCredentials), while retaining the early getImpersonatedCredentials() delegation inside refreshAccessToken() so the subject token isn't fetched twice when service account impersonation is configured.

lqiu96 · 2026-10-14T06:07:56Z

+          && this.x509Provider != null
+          && shouldUseMtlsTransportFactory()


in this method would these conditions every be false?

Previously they could be false because non-mTLS refreshes, refreshAccessToken(cycleTransportFactory), and the recursive retry call all went through refreshWithRetry. With executeRefresh(...) separated out and recursion removed, refreshWithRetry is now only called when this.x509Provider != null && shouldUseMtlsTransportFactory(), so those checks are no longer needed and have been removed.

lqiu96 · 2026-10-14T06:08:14Z

+          && (OAuth2Utils.isUnauthorizedException(e)
+              || OAuth2Utils.isInvalidGrantException(e)
+              || isRetryableTransportException(e))) {


can we extract this to a helper to help explain this. isMtlsRetryableExceptions?

Done — extracted isMtlsRetryableException(Throwable) to encapsulate the retryable exception check.

…cope cycle overloads

snippet-bot · 2026-10-14T21:00:58Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

…oped token

macastelaz requested review from a team as code owners August 30, 2026 02:28

macastelaz marked this pull request as draft August 30, 2026 02:28

gemini-code-assist Bot reviewed Aug 30, 2026

View reviewed changes

Comment thread google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java Outdated

Comment thread google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java Outdated

macastelaz force-pushed the cert-bound-oauth-iam-pinning branch from 73a0301 to 4be79cb Compare August 30, 2026 03:19

macastelaz mentioned this pull request Aug 31, 2026

test(oauth2): add mTLS in-process socket tests and live GCP WIF integration tests #14220

Merged

macastelaz force-pushed the cert-bound-oauth-iam-pinning branch from 4be79cb to ce341c7 Compare September 1, 2026 02:09

macastelaz marked this pull request as ready for review September 4, 2026 16:41

macastelaz requested a review from lsirac September 4, 2026 16:41

macastelaz force-pushed the cert-bound-oauth-iam-pinning branch from e09d76e to dadeda6 Compare September 17, 2026 03:17

lsirac requested changes Sep 17, 2026

View reviewed changes

macastelaz added 5 commits September 17, 2026 18:09

fix(oauth2): address PR review feedback for IAM mTLS transport pinning

73c5a0a

fix(oauth2): address post-review audit edge cases for mTLS pinning

8034525

fix(oauth2): preserve exact class check for MtlsHttpTransportFactory …

17aae1f

…in IdentityPoolCredentials

test(oauth2): add coverage for custom MtlsHttpTransportFactory subcla…

f2d752e

…ss and FILE cert pinning

fix(oauth2): annotate refreshAccessToken(HttpTransportFactory) with @…

a5e69ed

…InternalExtensionOnly

macastelaz requested review from blakeli0 and lsirac September 18, 2026 02:50

lsirac requested changes Sep 19, 2026

View reviewed changes

fix(oauth2): address review feedback on mTLS transport pinning and 40…

13e8be0

…1 recovery

macastelaz requested a review from lsirac September 21, 2026 21:12

lsirac reviewed Sep 21, 2026

View reviewed changes

fix(oauth2): address PR googleapis#14212 round 2 review feedback

44ca6d7

macastelaz mentioned this pull request Sep 22, 2026

feat(oauth2): support non-mTLS token URLs and unbound actor tokens in IdentityPoolCredentials #14430

Merged

lsirac reviewed Sep 22, 2026

View reviewed changes

macastelaz requested a review from lqiu96 September 28, 2026 20:34

lqiu96 reviewed Oct 13, 2026

View reviewed changes

Comment thread google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java

lqiu96 reviewed Oct 13, 2026

View reviewed changes

fix(oauth2): extract refreshWithRetry helpers and clarify mTLS/impers…

4bf0fdd

…onation comments

macastelaz requested a review from lqiu96 October 14, 2026 04:00

lqiu96 reviewed Oct 14, 2026

View reviewed changes

chore(oauth2_http): simplify IdentityPoolCredentials retry flow and s…

fe93d90

…cope cycle overloads

macastelaz requested review from a team as code owners October 14, 2026 20:56

macastelaz force-pushed the cert-bound-oauth-iam-pinning branch from 133b9cc to fe93d90 Compare October 15, 2026 01:22

Merge branch 'oauth2-bound-tokens' into cert-bound-oauth-iam-pinning

a9acfb5

macastelaz requested a review from lqiu96 October 15, 2026 02:56

lqiu96 reviewed Oct 15, 2026

View reviewed changes

Comment thread ...le-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java

lqiu96 reviewed Oct 15, 2026

View reviewed changes

Comment thread ...ava/oauth2_http/java/com/google/auth/oauth2/CertificateIdentityPoolSubjectTokenSupplier.java Outdated

lqiu96 reviewed Oct 15, 2026

View reviewed changes

Comment thread google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java Outdated

fix(auth): return null on KeyStore error and unconditionally clear sc…

16716d0

…oped token

macastelaz requested a review from lqiu96 October 15, 2026 15:18

lqiu96 approved these changes Oct 15, 2026

View reviewed changes

macastelaz merged commit 80b8228 into googleapis:oauth2-bound-tokens Oct 15, 2026
290 of 294 checks passed

		&& this.x509Provider != null
		&& shouldUseMtlsTransportFactory()

Conversation

macastelaz commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🥞 Stacked PRs

Summary

Test Coverage

Manual Testing

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macastelaz commented Aug 30, 2026 •

edited

Loading