fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext [blocked on #13995] - #14556
macastelaz wants to merge 18 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
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
…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.
…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
…y and channel refresh - Rename MtlsUtils.validateCertAndKeyFiles to checkCertAndKeyFilesReadable. - Move file readability check outside try-catch in MtlsUtils to clearly separate parsing errors from file existence errors. - Remove GKE/GCE placeholder stubs and internal doc references from MtlsUtils. - Simplify MtlsUtils.getCertificateFingerprint using Files.readAllBytes and Guava BaseEncoding. - Defer activeCertFingerprint mutation in ChannelPool until after channel creation succeeds in refreshAll(). - Add unit test in ChannelPoolTest verifying failed refresh attempts do not mutate fingerprint or prevent subsequent retries.
…otation retries - Remove unused FileExistenceProvider/FileContentReader and 3-arg constructor from CertificateBasedAccess. - In ServerStreamingAttemptCallable, BidiStreamingCallable, and ClientStreamingCallable, wrap transportChannel.refresh() in try-catch with warning logging and propagate original exception without marking isRetryable=true. - Add getGeneration() to TransportChannel, ChannelPool, and RefreshingHttpJsonChannel; update AttemptCallable to track attemptGeneration so sibling in-flight requests that failed on the stale connection are retried without redundant channel recreation. - Guard ChannelPool.refresh() and refreshAll() against invocation on shut-down pool and synchronize isShutdown state across shutdown methods. - Add delegating protected constructor in ManagedHttpJsonChannel so RefreshingHttpJsonChannel and ManagedHttpJsonInterceptorChannel do not leak unused parent scheduled executors and default HTTP transports. - Only wrap HTTP/JSON channels with RefreshingHttpJsonChannel when workloadCertPath is not null. - Configure Conscrypt security provider prior to calling NetHttpTransport.Builder.trustCertificates in InstantiatingHttpJsonChannelProvider. - Clear stale transportChannel reference in HttpJsonCallContext.withChannel() and merge() when channel changes. - Add comprehensive unit tests across gax, gax-grpc, and gax-httpjson modules.
…ependency analyzer
…es (googleapis#13995) - CertificateRotationTracker: extract shared mTLS disk fingerprint rotation tracking and 1s positive rotation cache into core gax, using monotonic sequence numbers incremented before disk I/O to coalesce concurrent lock waiters without caching unchanged checks - WorkloadCertificateUtils & MtlsUtils: simplify getCertificateFingerprint(), document return/throw contracts, treat 0-byte truncated certificate files mid-write as empty string, and return true in useMtlsClientCertificate() when workloadCertPath is present while preserving ECP support - ChannelPool: avoid marking pool rotated on partial refresh failure in refreshAll(), clean up newly created entries if any creation fails, guard refreshSafely() against mid-write empty fingerprints, and make getGeneration() package-private - GrpcCallContext & HttpJsonCallContext: allow withChannel(null) to clear the channel - AttemptCallable & ServerStreamingAttemptCallable: check channel.getGeneration() > attemptGeneration after refresh() so failed refreshes do not loop retries on unrotated channels, and mark server-streaming UnauthenticatedException retryable when channel rotates - ApiResultRetryAlgorithm: grant one immediate free retry on retryable UnauthenticatedException even when maxAttempts is 1 or totalTimeout is 0 - ChannelPool & RefreshingHttpJsonChannel: synchronize start() and cancel() on a per-call lock, guard against duplicate start(), only release immediately on cancel() exception if call was not started, catch Throwable in newCall()/start()/cancel(), and re-check outstandingCalls.get() == 0 after shutdownRequested.get() - InstantiatingHttpJsonChannelProvider: gate workloadCertPath on active mTLS without custom HttpTransport, pass initialChannel directly to RefreshingHttpJsonChannel to preserve checked IOException on startup, and guard against leaks and null keystore fallback - InstantiatingGrpcChannelProvider: gate workloadCertPath on !canUseDirectPath() && active mTLS, and fail fast with IOException if mTLS channel credentials cannot be initialized when mTLS is active
… and JDK 8 mock annotations (googleapis#13995)
…eaming exception wrap (googleapis#13995)
…ext (googleapis#13995) The mTLS HttpTransport paired a Conscrypt SSLContext with the JDK (SunJSSE) PKIX TrustManagerFactory. On TLS 1.3 Conscrypt passes authType "GENERIC" to the trust manager, which SunJSSE rejects for CA-issued server certificates with a KeyUsage extension (e.g. Google front ends), so every mTLS HTTP/JSON handshake failed with "Unknown authType: GENERIC". Since this PR enables mTLS automatically when a workload certificate config is present, this broke HTTP/JSON clients by default on Cloud Run. Use Conscrypt's own PKIX TrustManagerFactory, which loads the same default trust store (including javax.net.ssl.trustStore overrides) as the JDK. Adds a TLS 1.3 handshake regression test against a local server presenting a CA-issued leaf with KeyUsage and requiring a client certificate.
There was a problem hiding this comment.
Code Review
This pull request introduces support for dynamic mTLS certificate rotation in both gRPC and HTTP/JSON transports. It adds utility methods in MtlsUtils to resolve and validate certificate configurations, implements a CertificateRotationTracker to monitor file fingerprints on disk, and updates ChannelPool and RefreshingHttpJsonChannel to perform thread-safe, hot-swapping of channels upon rotation. Additionally, it updates ApiResultRetryAlgorithm and AttemptCallable to handle UnauthenticatedException by triggering channel refreshes. I have reviewed the changes and identified two areas where error handling and messaging can be improved for robustness.
| File certFile = new File(config.getCertPath()); | ||
| File keyFile = new File(config.getPrivateKeyPath()); | ||
| if (!certFile.isFile() || !certFile.canRead() || !keyFile.isFile() || !keyFile.canRead()) { |
There was a problem hiding this comment.
If config.getCertPath() or config.getPrivateKeyPath() is null or empty, passing them to new File() will result in a NullPointerException. It is safer to validate that these paths are non-empty before attempting to instantiate File objects.
String certPath = config.getCertPath();
String keyPath = config.getPrivateKeyPath();
if (Strings.isNullOrEmpty(certPath) || Strings.isNullOrEmpty(keyPath)) {
String sourcePrefix =
isDefaultConfig
? "referenced by default configuration '"
: "referenced by configuration '";
throw new IllegalStateException(
"Certificate or private key path is missing in configuration '" + configPath + "'.");
}
File certFile = new File(certPath);
File keyFile = new File(keyPath);
if (!certFile.isFile() || !certFile.canRead() || !keyFile.isFile() || !keyFile.canRead()) {| if (!certConfig.isFile() || !certConfig.canRead()) { | ||
| throw new CertificateSourceUnavailableException( | ||
| "Certificate configuration file does not exist or is not a file: " | ||
| + certConfig.getAbsolutePath()); |
There was a problem hiding this comment.
The exception message only mentions that the file does not exist or is not a file, but the condition also checks !certConfig.canRead(). If the file exists but is unreadable, the message would be misleading. Consider updating the message to include readability.
| if (!certConfig.isFile() || !certConfig.canRead()) { | |
| throw new CertificateSourceUnavailableException( | |
| "Certificate configuration file does not exist or is not a file: " | |
| + certConfig.getAbsolutePath()); | |
| if (!certConfig.isFile() || !certConfig.canRead()) { | |
| throw new CertificateSourceUnavailableException( | |
| "Certificate configuration file does not exist, is not a file, or is not readable: " | |
| + certConfig.getAbsolutePath()); |
Important
Draft, blocked on #13995. This branch is based on #13995's head, so until #13995 merges this PR also shows #13995's commits. Only the last commit (
fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext) belongs to this PR. After #13995 merges I'll rebase ontoagentic-identities-bound-tokenso the diff is just this fix. This must merge before the feature branch is merged tomain.Problem
When mTLS is active,
InstantiatingHttpJsonChannelProvider.createHttpTransport()builds a ConscryptSSLContextbut initializes it with the JDK (SunJSSE) PKIXTrustManagerFactory. On TLS 1.3, Conscrypt passes authType"GENERIC"to the trust manager. SunJSSE's end-entity checks reject that authType for server certificates that are CA-issued and carry a KeyUsage extension, which includes Google front ends. So every mTLS HTTP/JSON handshake fails with:The bug is already on
main, but there it only triggers whenGOOGLE_API_USE_CLIENT_CERTIFICATE=trueis set explicitly. #13995 enables mTLS automatically whenever a workload certificate config is present, so on Cloud Run (agent identity) every HTTP/JSON client fails by default. gRPC isn't affected. The non-mTLS path isn't affected either, because google-http-client already pairs Conscrypt with a provider-matched trust manager there.Fix
Use Conscrypt's own PKIX
TrustManagerFactory(TrustManagerFactory.getInstance("PKIX", conscryptProvider)) for the mTLSSSLContext. I called the JDK API directly rather thanSslUtils.getPkixTrustManagerFactory(Provider), which only exists in google-http-client 2.2.0+.Conscrypt's trust manager loads the same default trust store as the JDK. Verified on JDK 21: 174 anchors in both, the same set, and both honor
-Djavax.net.ssl.trustStoreoverrides identically.Testing
New
InstantiatingHttpJsonChannelProviderTls13Test: a local JDK TLS 1.3 server presents a CA-issued leaf with KeyUsage and requires a client certificate. The test asserts that the transport completes the request and presents its client certificate.Unknown authType: GENERIC.Existing
InstantiatingHttpJsonChannelProviderTest: 14/14 pass.Tested together with feat(gax): support transparent retries during mTLS certificate rotations #13995's latest head, on JDK 21 with Conscrypt active, using feat(gax): support transparent retries during mTLS certificate rotations #13995's end-to-end harness: 22 scenarios with real KMS and BigQuery Storage GAPIC clients against local mTLS servers, where the certificate rotates on disk.
close(). The refreshed transport still uses Conscrypt's socket factory.Unknown authType: GENERIC. Limiting the test server to TLS 1.2 makes the error go away, which confirms that the TLS 1.3 authType is the trigger.Live on Cloud Run (agent identity), combined feat: Enable Bound Token for Agentic Identities #13873 + feat(gax): support transparent retries during mTLS certificate rotations #13995 build, default env:
Unknown authType: GENERIC