feat(bigquery-jdbc): respect standard JVM trustStore properties by default#13435
feat(bigquery-jdbc): respect standard JVM trustStore properties by default#13435keshavdandeva wants to merge 5 commits into
Conversation
… JVM cacerts with google.p12 fallback
There was a problem hiding this comment.
Code Review
This pull request updates BigQueryJdbcProxyUtility to configure a default NetHttpTransport with a MergedTrustManager (trusting both the JVM's default trust store and Google's bundled certificate store) when no proxy or custom SSL settings are provided. The review feedback focuses on critical performance and robustness improvements: caching the merged SSLContext using a thread-safe lazy initialization holder pattern to avoid high CPU and latency overhead from re-creating it, handling potential null returns from getAcceptedIssuers() to prevent NullPointerException, and preserving debugging context by suppressing the initial CertificateException when falling back to Google's trust manager.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates BigQueryJdbcProxyUtility to return default HttpTransportOptions with a default NetHttpTransport instead of null when no proxy or SSL settings are configured, and updates the corresponding tests. The reviewer pointed out that the default transport factory implementation recreates a new NetHttpTransport on every call, which can cause socket leaks and prevent connection reuse, and also lacks MergedTrustManager integration. It is recommended to instantiate the transport once and reuse it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies BigQueryJdbcProxyUtility to return default HttpTransportOptions configured with a NetHttpTransport when no proxy or SSL settings are specified, rather than returning null. Additionally, it ensures that httpClientBuilder.useSystemProperties() is always called in getHttpTransportFactory. Corresponding unit tests have been updated to reflect these changes. There are no review comments, and I have no additional feedback to provide.
logachev
left a comment
There was a problem hiding this comment.
lgtm, but before approving I'd want us to have an environment where we could validate it
| getHttpTransportFactory( | ||
| proxyProperties, sslTrustStorePath, sslTrustStorePassword, callerClassName)); | ||
| } else { | ||
| final HttpTransport defaultTransport = new NetHttpTransport.Builder().build(); |
b/515129164
Problem
In enterprise corporate networks (e.g. Zscaler), outbound HTTPS traffic is intercepted by transparent proxies doing SSL MITM (man-in-the-middle) decryption. The proxy signs its re-encrypted connections with dynamically generated root CA certificates.
The BigQuery JDBC driver failed to establish TLS connections under these environments because the underlying client library ignored JVM trust stores (the standard Java
cacertsfile or custom system properties set via-Djavax.net.ssl.trustStore).This happened because when direct connections had empty SSL/proxy settings, the driver returned
nullforHttpTransportOptions. This fallback triggered classpath SPI overrides or legacy defaults which invokedGoogleNetHttpTransport.newTrustedTransport(). That convenience constructor hardcodes trust exclusively to a bundledgoogle.p12keystore, completely overriding JVM system properties.Solution
Simplified the driver's transport instantiation to align with the core Google Cloud Java SDK's network defaults:
getHttpTransportOptions(...)to unconditionally return a transport factory configured with a singleNetHttpTransportinstance (new NetHttpTransport.Builder().build()). This allows JSSE to handle TLS certificate validation using standard JVM system properties andcacertsnatively. Bypassing the SPI loader prevents classpath hijacking.getHttpTransportFactory(...)to unconditionally callhttpClientBuilder.useSystemProperties(). This ensures that even when a proxy is set in the JDBC URL, Apache HttpClient still honors system-level properties like-Djavax.net.ssl.trustStore.