Skip to content

feat(gax): support transparent retries during mTLS certificate rotations - #13995

Open
macastelaz wants to merge 6 commits into
googleapis:agentic-identities-bound-tokenfrom
macastelaz:rotation-retries-clean-agentic
Open

feat(gax): support transparent retries during mTLS certificate rotations#13995
macastelaz wants to merge 6 commits into
googleapis:agentic-identities-bound-tokenfrom
macastelaz:rotation-retries-clean-agentic

Conversation

@macastelaz

Copy link
Copy Markdown
Contributor

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

• 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.

- 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
@macastelaz
macastelaz requested review from a team as code owners August 5, 2026 02:06

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +341 to +342
@Override
public void start(Listener<RespT> responseListener, HttpJsonMetadata requestHeaders) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The ReleasingHttpJsonClientCall class does not override the cancel method. If a call is cancelled before start() is called, or if it is cancelled early, the reference count on the ChannelEntry will never be decremented, leading to a permanent resource/memory leak of retired channels. Please override cancel to safely release the entry if it hasn't been released yet, matching the behavior of gRPC's ReleasingClientCall.

    @Override
    public void cancel(String message, Throwable cause) {
      try {
        super.cancel(message, cause);
      } finally {
        if (wasReleased.compareAndSet(false, true)) {
          entry.release();
        }
      }
    }

    @Override
    public void start(Listener<RespT> responseListener, HttpJsonMetadata requestHeaders) {

Comment on lines +96 to +102
t =
new UnauthenticatedException(
causeEx.getMessage(),
causeEx.getCause(),
causeEx.getStatusCode(),
true, // isRetryable = true
causeEx.getErrorDetails());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When wrapping the UnauthenticatedException to mark it as retryable, the original exception's stack trace and suppressed exceptions are lost, and the original exception is not preserved as the cause. Please align this with the robust implementation in AttemptCallable.java by preserving the original exception as the cause and copying its stack trace and suppressed exceptions.

                    UnauthenticatedException newEx =
                        new UnauthenticatedException(
                            causeEx.getMessage(),
                            causeEx,
                            causeEx.getStatusCode(),
                            true, // isRetryable = true
                            causeEx.getErrorDetails());
                    newEx.setStackTrace(causeEx.getStackTrace());
                    for (Throwable suppressed : causeEx.getSuppressed()) {
                      newEx.addSuppressed(suppressed);
                    }
                    t = newEx;

@macastelaz
macastelaz force-pushed the rotation-retries-clean-agentic branch from 5678ad4 to e3c70b5 Compare August 5, 2026 14:42
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
@macastelaz
macastelaz force-pushed the rotation-retries-clean-agentic branch from e3c70b5 to a2210c6 Compare August 5, 2026 17:31
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant