Skip to content

fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext [blocked on #13995] - #14556

Draft
macastelaz wants to merge 18 commits into
googleapis:agentic-identities-bound-tokenfrom
macastelaz:fix-conscrypt-tmf-13995
Draft

macastelaz wants to merge 18 commits into
googleapis:agentic-identities-bound-tokenfrom
macastelaz:fix-conscrypt-tmf-13995

Conversation

@macastelaz

@macastelaz macastelaz commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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 onto agentic-identities-bound-token so the diff is just this fix. This must merge before the feature branch is merged to main.

Problem

When mTLS is active, InstantiatingHttpJsonChannelProvider.createHttpTransport() builds a Conscrypt SSLContext but initializes it with the JDK (SunJSSE) PKIX TrustManagerFactory. 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:

javax.net.ssl.SSLHandshakeException: Unknown authType: GENERIC
Caused by: java.security.cert.CertificateException: Unknown authType: GENERIC

The bug is already on main, but there it only triggers when GOOGLE_API_USE_CLIENT_CERTIFICATE=true is 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 mTLS SSLContext. I called the JDK API directly rather than SslUtils.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.trustStore overrides identically.

Testing

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

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

Comment on lines +185 to +187
File certFile = new File(config.getCertPath());
File keyFile = new File(config.getPrivateKeyPath());
if (!certFile.isFile() || !certFile.canRead() || !keyFile.isFile() || !keyFile.canRead()) {

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

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()) {

Comment on lines +281 to 284
if (!certConfig.isFile() || !certConfig.canRead()) {
throw new CertificateSourceUnavailableException(
"Certificate configuration file does not exist or is not a file: "
+ certConfig.getAbsolutePath());

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

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.

Suggested change
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());

This branch has not been deployed

No deployments
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