Support application-supplied SSLContext in client-v2 and jdbc-v2#2918
Open
polyglotAI-bot wants to merge 1 commit into
Open
Support application-supplied SSLContext in client-v2 and jdbc-v2#2918polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
Adds a way to inject a fully pre-built javax.net.ssl.SSLContext so trust/key material can be assembled in memory and never written to disk - the use-case from #2909 (diskless TLS behind connection pools such as HikariCP). client-v2: - New ClientConfigProperties.SSL_CONTEXT ("ssl_context"). It holds a live object, so it is never parsed from or represented as a string. - Client.Builder.setSSLContext(SSLContext) stores the context and it is injected into the parsed (object) configuration in the Client constructor. - HttpAPIClientHelper.createSSLContext returns the supplied context as is, bypassing the trust/key material builder. ssl_mode still governs server hostname verification at the socket-factory layer. jdbc-v2: - JdbcConfiguration accepts a live SSLContext under the ssl_context key in the connection Properties (scoped relaxation of the string-only validation; any other non-string value still throws) and forwards it to the client builder. Tests: client-v2 ClientBuilderTest (builder plumbing + createSSLContext short-circuit) and jdbc-v2 JdbcConfigurationTest (Properties capture, forwarding to the builder, and that other non-string values are still rejected). Examples (client-v2 and jdbc SSLExamples) and docs/features.md updated. Fixes: #2909 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Repository collaborators can run the JMH benchmark suite against this PR by commenting: Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%): Only one benchmark run per PR is active at a time — issuing a new |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e49a73e. Configure here.
| } | ||
|
|
||
| return new Client(this.endpoints, this.configuration, this.sharedOperationExecutor, | ||
| this.columnToMethodMatchingStrategy, this.metricRegistry, this.queryIdGenerator, cManager); |
There was a problem hiding this comment.
Custom context fails cert store check
Medium Severity
Client.Builder.build() throws when legacy SSL configuration properties (e.g., trust_store, sslcert) are present in the string configuration, even if an SSLContext is supplied via setSSLContext. This contradicts the new API's documented behavior that these properties should be ignored, causing client creation to fail.
Reviewed by Cursor Bugbot for commit e49a73e. Configure here.
| * string and has no textual representation in a configuration map. | ||
| */ | ||
| SSL_CONTEXT("ssl_context", SSLContext.class), | ||
|
|
There was a problem hiding this comment.
String ssl_context silently ignored
Medium Severity
Adding SSL_CONTEXT to ClientConfigProperties lets a string ssl_context value flow through parseConfigMap via parseValue, which has no SSLContext branch and stores null without error. createSSLContext then builds TLS from file/PEM options instead of using a custom context, so JDBC setProperty("ssl_context", …) or URL query params can look configured while behavior silently differs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e49a73e. Configure here.
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2909.
Some deployments assemble their TLS material (certificates + private keys) entirely in memory — fetched and decrypted from a secret store — and are prohibited from writing it to disk, even as temporary files.
client-v2can already build anSSLContextfrom files/PEM, but there was no way to hand it a fully pre-builtjavax.net.ssl.SSLContext, and the JDBC layer rejected any non-StringPropertiesvalue (IllegalArgumentException: Property key and value should be a string), which blocked passing a live context through connection pools such as HikariCP that only exposejava.util.Properties.This PR adds that injection point for client-v2 and jdbc-v2. When a context is supplied it is used as is (the application is responsible for configuring it), and
ssl_modethen only controls server hostname verification — matching the requested behavior.Changes
client-v2
ClientConfigProperties.SSL_CONTEXT("ssl_context"). It carries a live object, so it is never parsed from or represented as a string; it is injected into the parsed (object) configuration directly.Client.Builder.setSSLContext(SSLContext)stores the context; theClientconstructor injects it into the resolved configuration map.HttpAPIClientHelper.createSSLContext(...)returns the supplied context as is and skips building one from trust/key material. Hostname verification is unchanged — still derived fromssl_modeat the connection-socket-factory layer (TRUST/VERIFY_CAskip it,STRICTenforces it).jdbc-v2
JdbcConfigurationcaptures a liveSSLContextsupplied under thessl_contextkey in the connectionProperties(a scoped relaxation of the string-only check — any other non-string value still throws) and forwards it to the underlyingClient.BuilderviasetSSLContext(...).Examples & docs
SSLExamples(both client-v2 and jdbc) gain aconnectWithCustomSSLContextexample that builds the trust material in memory and injects the context (the jdbc one passes it viaProperties.put, since it is not a string).docs/features.mdupdated (client-v2 feature + compatibility notes, jdbc feature + the scopedPropertiesexception).Test
ClientBuilderTest(unit):setSSLContextstores the exact instance in the resolved configuration (and it is absent when not set);createSSLContextreturns the supplied context as is, and still builds its own when none is supplied.JdbcConfigurationTest(unit): a liveSSLContextinPropertiesis accepted (previously threw) and does not leak into the string client properties; it is forwarded to the client builder; and a non-string value under any other key is still rejected.Focused runs:
ClientBuilderTest13/13,JdbcConfigurationTest94/94. Both example projects compile.Pre-PR validation gate
ClientBuilderTest/JdbcConfigurationTeststill passAGENTS.md/docs/changes_checklist.md/docs/features.mddocs/changes_checklist.mdwalkthroughClientConfigProperties.SSL_CONTEXT): keyssl_contextis unique; value typeSSLContext.classis correct; the default isnullandparseValue(null)returnsnull, so it parses to the declared type. It never participates in string config parsing/serialization (injected as an object and skipped byparseConfigMap), so there is no round-trip or older-reader concern. Placed next to the other SSL keys for readability; order is irrelevant for this enum (keyed lookups, no ordinal persistence).Client.Builder.setSSLContext): name/params/return match the siblingsetSSLMode(...); public because it is a user-facing builder option; behavior-focused tests added; reflected indocs/features.md.Client(...)constructor gained a parameter — no public API/binary-compatibility impact.JdbcConfiguration.getSslContext()is new but lives in the internalcom.clickhouse.jdbc.internalpackage.client-v2/jdbc-v2and public API surface →docs/features.mdupdated accordingly. Change is purely additive; default behavior (no context supplied) is unchanged.Notes