feat(gax): support transparent retries during mTLS certificate rotations - #13995
feat(gax): support transparent retries during mTLS certificate rotations#13995macastelaz wants to merge 6 commits into
Conversation
- 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
There was a problem hiding this comment.
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.
| @Override | ||
| public void start(Listener<RespT> responseListener, HttpJsonMetadata requestHeaders) { |
There was a problem hiding this comment.
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) {| t = | ||
| new UnauthenticatedException( | ||
| causeEx.getMessage(), | ||
| causeEx.getCause(), | ||
| causeEx.getStatusCode(), | ||
| true, // isRetryable = true | ||
| causeEx.getErrorDetails()); |
There was a problem hiding this comment.
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;5678ad4 to
e3c70b5
Compare
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
e3c70b5 to
a2210c6
Compare
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
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.
mTLS Fail-Open Security Fix (CertificateBasedAccess.java):
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):
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.