feat(gax): support transparent retries during mTLS certificate rotations by macastelaz · Pull Request #13995 · googleapis/google-cloud-java

macastelaz · 2026-08-05T02:06:31Z

Description

This PR introduces robust dynamic mTLS certificate rotation capabilities for
HTTP/JSON and gRPC transport channels, ensuring that certificates can be
rotated in long-lived environments without prematurely severing active, in-
flight RPCs or streams.

🚀 Core Features & Architectural Updates

• Dynamic Certificate Rotation: Implemented RefreshingHttpJsonChannel and
overhauled the gRPC ChannelPool to support dynamic, thread-safe, hot-swapping
of the underlying active transport channels whenever workload certificates
rotate dynamically on the filesystem.
• Preemptive Drop Mitigation: Refactored the internal channel rotation
pipeline (via refreshAll() and refreshSafely()) so that newly formed
connections are seamlessly brought online while preceding active streams are
cleanly drained and gracefully retired. This mitigates GFE connection drop
errors that previously occurred during hard resource refreshes.
• Core Retry Integration: Aligned streaming algorithm Callables and Retry
mechanisms with the dynamic refresh paradigm to ensure transparent retry
policies are respected, avoiding double-wrapped exceptions when traversing
rotated transports.

🔒 System Hardening & Bug Fixes

During the development of these features, several deep-dive reviews were
conducted over the GAX codebase, resulting in the following critical fixes:

• HTTP/JSON Teardown Thread-Safety: Fixed a race condition in
RefreshingHttpJsonChannel.java where shutdown() was calculating state
dynamically from underlying sub-channels without a lock. This allowed a
concurrent refresh() to spawn completely new channels after teardown began,
permanently leaking the channel pool.
• Outstanding RPC Memory Leak (ChannelPool.java): Fixed an uncontrolled
exception escape hatch in ReleasingClientCall.start(). If a pre-existing
cancellation exception was detected, the method aborted forcefully. This
bypassed onClose and never executed entry.release(), leaving the sub-channel
permanently trapped with an outstanding RPC count and preventing graceful
cleanup during rotations.
• Transport Channel Override Drops: Fixed merge() operations in
GrpcCallContext and HttpJsonCallContext that intentionally dropped custom
outer transportChannel references in favor of strict this.transportChannel
defaults. Context overrides now safely propagate custom overrides.
• Cross-Platform Compatibility: Fixed naively concatenated pathing for
certificates (Windows compatibility) and properly escaped JSON strings inside
CertificateBasedAccess.

⚠️ Behavioral & Security Boundary Changes

mTLS Fail-Open Security Fix (CertificateBasedAccess.java):
- Fix: If an environment strictly mandated mTLS but provided an invalid explicit config via GOOGLE_API_CERTIFICATE_CONFIG (e.g. typos, malformed
  JSON), the system previously swallowed the I/O exception, failed-open to
  a null filepath, and allowed a standard non-mTLS auth connection without
  notifying the developer. The system now correctly fails-closed (crashing
  startup by throwing an IllegalStateException) upon parsing failure,
  preventing unintentional security downgrade rollbacks.
Infinity Timeout Boundary Enforcement (GrpcCallContext & HttpJsonCallContext):
- Fix: Deadlines in GAX strictly prevent expansion (enforcing top-level
  user limits into downstream libraries). However, a logical flaw permitted
  bypassing this if a downstream caller submitted an unconstrained/infinite
  timeout limit (represented as null), quietly erasing strict prior
  deadlines. Override evaluations now properly reject null expansion
  boundaries.

🧪 Testing

Automated Testing

• Added and updated comprehensive unit-tests reflecting the thread-safety
fixes inside ChannelPoolTest.java and RefreshingHttpJsonChannelTest.java.
• Corrected edge case test configurations to leverage realistic mocked X.509
certificates to properly exercise deep WorkloadCertificateUtils.
getCertificateFingerprint() filesystem caching mechanisms.

Manual Testing

The following pastes include example test applications which use GAPIC client libraries and force a certificate refresh between calls. The pastes also include the logged application output showing successful detection and handling of the rotation event (including automatic retries).
- gRPC: https://paste.googleplex.com/6340550226739200
- Http/JSON: https://paste.googleplex.com/5043729177116672
- Streaming: https://paste.googleplex.com/5449234957598720
The following paste shows error handling when a cert rotation results in a corrupted certificate on disk - note the underlying java application does not crash and will resume successful calls after a valid cert is written to disk: https://paste.googleplex.com/5066238580621312
The following paste shows disk protection through the use of a 1s TTL on the cached cert: https://paste.googleplex.com/6292261372559360

- Add CertificateBasedAccess and WorkloadCertificateUtils for SPIFFE and custom certificate loading - Implement RefreshingHttpJsonChannel and ChannelPool mTLS certificate fingerprint tracking and rotation - Enable transparent retries for retryable UnauthenticatedExceptions in ApiResultRetryAlgorithm and AttemptCallable - Add override delegation for getEndpoint, getHttpTransport, and getExecutor to preserve SLF4J MDC logging in Showcase tests

gemini-code-assist

Code Review

This pull request introduces support for dynamic mTLS certificate rotation across both gRPC and HTTP/JSON transports by enabling thread-safe channel hot-swapping and automatic refreshing upon encountering an UnauthenticatedException. Key additions include the RefreshingHttpJsonChannel and updates to various callables to intercept and retry unauthenticated errors. However, several critical issues were identified during review: a bug in ChannelPool.refresh() that breaks the GFE channel refresh mechanism for non-mTLS connections; a potential resource leak in RefreshingHttpJsonChannel due to a missing cancel override; regressions caused by the removal of Conscrypt security provider configurations; and incomplete exception wrapping in several streaming callables that results in the loss of the original stack trace, cause, and suppressed exceptions of UnauthenticatedException.

…cle edge cases

Addresses AI code review findings from https://paste.googleplex.com/6563525517508608: - GrpcCallContext: Prevent transportChannel stale inheritance in merge() and withChannel() - RefreshingHttpJsonChannel: Set shutdownRequested and shutdownInitiated in shutdownNow() so newCall() throws IllegalStateException - AttemptCallable / StreamingCallables: Pass getCause() when rethrowing retryable UnauthenticatedException to prevent double-wrapping - CertificateBasedAccess: Enforce fail-closed security boundary when certificate config is malformed or missing required keys, and fix JSON unescaping order - ChannelPool: Update ReleasingClientCall Javadoc contract - Unit tests: Add cache invalidation test helpers to eliminate Thread.sleep() delays and add comprehensive tests for all addressed edge cases

Addresses Gemini code review feedback on ReleasingHttpJsonClientCall and ReleasingClientCall: - Tracks wasStarted atomic flag on client calls to detect if start() has been invoked - If cancel() is invoked before start() (or call is discarded unstarted), cancel() immediately releases the ChannelEntry to decrement the active call reference count - Prevents memory/resource leaks of retired channels that are waiting for outstanding calls to drop to 0 - Adds testCancelBeforeStartReleasesChannelEntry unit tests to both RefreshingHttpJsonChannelTest and ChannelPoolTest

…sensitivity Addresses findings from mTLS security deep-dive code review: - Handle non-workload JSON configs (e.g. PKCS#11 /etc/gcloud/certificate_config.json) gracefully in validateAndResolveConfig without throwing IllegalStateException, preventing initialization failures on Google developer environments - Enforce fail-closed security boundary in getWorkloadCertPath() by validating disk file existence when GOOGLE_API_CERTIFICATE_CONFIG is set and throwing IllegalStateException when mTLS is enabled but no valid cert can be resolved - Make GOOGLE_API_USE_MTLS_ENDPOINT policy comparisons case-insensitive in getMtlsEndpointUsagePolicy()

…nd fail-closed getWorkloadCertPath - Adds testUseMtlsEndpointCaseInsensitive to verify getMtlsEndpointUsagePolicy() handles uppercase 'ALWAYS' and 'NEVER' - Adds assertThrows(IllegalStateException.class, cba::getWorkloadCertPath) in testUseMtlsClientCertificateExplicitTrueNoCredentials to verify getWorkloadCertPath() throws IllegalStateException when mTLS is required but no certificate can be resolved

nbayati

Some feedback on the auth side of things.

@nbayati

…PR 13995 review feedback Address review comments from @nbayati: 1. Make auth library (MtlsUtils) single source of truth for mTLS cert discovery and permission rules. 2. Fix GOOGLE_API_USE_CLIENT_CERTIFICATE flag semantics: true permits mTLS, return null/false cleanly if no certs are found (Row 3). Throw IllegalStateException only when cert config exists but referenced cert/key files are missing (Row 2). 3. Separate GKE and GCE workload certificate resolution paths. 4. Centralize SHA-256 certificate fingerprint calculation in MtlsUtils.

- Separate GKE (credentialbundle.pem) and GCE (certificates.pem + private_key.pem) workload certificate fallback paths in MtlsUtils. - Restore full Javadoc on MtlsUtils.getWorkloadCertificateConfiguration. - Format MtlsUtils and MtlsUtilsTest with google-java-format. - Fix Java 8 Mockito reflection error in GrpcLoggingInterceptorTest by instantiating GrpcLoggingInterceptor directly. - Isolate DirectPath environment tests in InstantiatingGrpcChannelProviderTest from host environment variables.

nbayati · 2026-08-14T20:37:12Z

+    String gkeCertPath = getGkeWorkloadCertPath();
+    if (gkeCertPath != null) {
+      return gkeCertPath;
+    }
+
+    String gceCertPath = getGceWorkloadCertPath();
+    if (gceCertPath != null) {
+      return gceCertPath;
+    }


Thanks for extracting these into helper methods. However, we need to remove them from the fallback chain inside getWorkloadCertPath() because both GKE and GCE are not ready today. GKE lands first and GCE follows ~3 months later. If getWorkloadCertPath() automatically checks both, we cannot phase GKE and GCE separately.
getWorkloadCertPath() should only check GOOGLE_API_CERTIFICATE_CONFIG and the well-known gcloud config file. We can keep these helper methods and not invoke them, or remove them now, and add them back once we implement the bound token support for GKE and GCE.

Ah shoot - I forgot to pull these back! They have both been left in but made package private and directly return null so that we have the structure in place but both are no-ops. If you'd rather me remove them entirely I'm happy to do that too.

nbayati · 2026-08-14T20:50:21Z

+      return null;
+    }
+
+    String certConfigPath = envProvider.getEnv(CERTIFICATE_CONFIGURATION_ENV_VARIABLE);


The env var should be GOOGLE_API_CERTIFICATE_CONFIG.

This is an existing artifact but CERTIFICATE_CONFIGURATION_ENV_VARIABLE is already defined as "GOOGLE_API_CERTIFICATE_CONFIG" - https://github.com/googleapis/google-cloud-java/blame/add1c9872cd459f979614f6927e76a46eaa22e0c/google-auth-library-java/oauth2_http/java/com/google/auth/mtls/MtlsUtils.java#L52

I don't think this PR should be changing this, but we can always go back and clean this up later if we want? The key part, though, is that the value is correct from what I can tell despite the misleading var name.

nbayati · 2026-08-14T21:23:55Z

+        File certFile = new File(config.getCertPath());
+        File keyFile = new File(config.getPrivateKeyPath());
+        if (!certFile.exists() || !keyFile.exists()) {


Instead of checking file.exists(), should we also verify that the files are regular files and readable by the current process using canRead() and isFile() (or Files.isReadable() and Files.isRegularFile())?

exists() returns true for directories, so if we don't have permissions, or if it's not a regular file, trying to open the file stream later will crash with an error.

Great catch and agree that it would make more sense to check for file and readability

nbayati · 2026-08-14T23:14:03Z

+        File keyFile = new File(config.getPrivateKeyPath());
+        if (!certFile.exists() || !keyFile.exists()) {
+          throw new IllegalStateException(
+              "Certificate config points to certificate/key files that do not exist on disk: "


nit: I think it would be helpful to indicate the resolution source of the config file (e.g., whether it came from the GOOGLE_API_CERTIFICATE_CONFIG environment variable or the gcloud default location), so that the user knows what config we are referring to, and how to fix it.

Good call - Agreed that adding specifics here would be helpful - done!

nbayati · 2026-08-15T22:27:02Z

      throw new CertificateSourceUnavailableException(
-          "Certificate configuration loaded successfully, but does not contain a 'certificate_file' path.");
+          "Certificate configuration loaded successfully, but does not contain a 'certificate_file'"
+              + " path.");


Could this break the ECP flow? Do we need to check that "workload" exists but "certificate_file" does not exist?

This method is currently only called from google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java and from what I understand, ECP is not applicable for IdentityPool credentials so throwing here would be acceptable - but let me know if I'm missing something or if you'd like to see other handling here.

nbayati · 2026-08-15T22:29:39Z

when reviewing this file I realized there are some discrepancy between how different libraries treat cert discovery. For instance if GOOGLE_API_CERTIFICATE_CONFIG points to a file that does not exist, I believe right now we fallback on regular TLS, but we should fail because the configuration is broken. Also, each library has its own error messages in different scenarios.
I created this design to align the behavior and the message text between the repos so we can implement a unified behavior: go/sdk-mtls-by-default-cert-discovery

Thanks for putting this together! This PR has been updated to use the states and messages from that proposal.

@nbayati

…th go/sdk-mtls-by-default-cert-discovery Address PR 13995 review feedback from @nbayati: - Align discovery and error behavior with go/sdk-mtls-by-default-cert-discovery: - Fail closed (IllegalStateException) when GOOGLE_API_CERTIFICATE_CONFIG points to a missing, unreadable, malformed, or missing cert/key configuration. - Safe fallback (return null) when implicit default gcloud config is missing or is an ECP-only configuration without a workload block. - Fail closed with clear source identification if default gcloud config is unreadable, malformed, or points to missing cert/key files. - Replace .exists() with .isFile() && .canRead() checks across config, certificate, and key paths. - Make getGkeWorkloadCertPath and getGceWorkloadCertPath package-private stubs returning null with explanatory comments for phased rollout. - Explicitly identify the resolution source (GOOGLE_API_CERTIFICATE_CONFIG vs default gcloud location) in all error messages. - Update getCertificatePath exception message to reference 'cert_configs.workload.cert_path' rather than legacy 'certificate_file'. - Add comprehensive test coverage in MtlsUtilsTest and CertificateBasedAccessTest.

…P flow in getCertificatePath

macastelaz requested review from a team as code owners August 5, 2026 02:06

gemini-code-assist Bot reviewed Aug 5, 2026

View reviewed changes

fix(gax): address mTLS cert config parsing and channel refresh lifecy…

49772f0

…cle edge cases

macastelaz force-pushed the rotation-retries-clean-agentic branch from 5678ad4 to e3c70b5 Compare August 5, 2026 14:42

macastelaz force-pushed the rotation-retries-clean-agentic branch from e3c70b5 to a2210c6 Compare August 5, 2026 17:31

macastelaz added 3 commits August 5, 2026 17:46

nbayati self-requested a review August 7, 2026 19:12

nbayati requested changes Aug 7, 2026

View reviewed changes

macastelaz requested review from a team as code owners August 10, 2026 15:29

macastelaz force-pushed the rotation-retries-clean-agentic branch from 826f766 to 1423299 Compare August 10, 2026 19:31

macastelaz force-pushed the rotation-retries-clean-agentic branch from 1423299 to be0a495 Compare August 10, 2026 19:45

macastelaz requested a review from nbayati August 14, 2026 17:32

nbayati requested changes Aug 15, 2026

View reviewed changes

macastelaz added 2 commits August 18, 2026 14:00

test(auth): add unit test in MtlsUtilsTest to provide coverage for EC…

765eb3b

…P flow in getCertificatePath

Uh oh!

Conversation

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

Uh oh!

Description

🚀 Core Features & Architectural Updates

🔒 System Hardening & Bug Fixes

⚠️ Behavioral & Security Boundary Changes

🧪 Testing

Automated Testing

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nbayati left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

macastelaz commented Aug 5, 2026 •

edited

Loading