Skip to content

Commit a1f8b04

Browse files
polyglotAI-botpolyglotAI-bot
committed
Address @chernser review on custom SSLContext (client-v2/jdbc-v2)
- Reject trust/key material options when a custom SSLContext is supplied (except ssl_mode) instead of silently ignoring them - Move the custom-vs-built context selection from createSSLContext into createHttpClient; createSSLContext now purely builds from material - Reject a textual ssl_context in Builder.setOption as well as build() - Trim obvious comments, shorten setSSLContext javadoc, drop the duplicate enum-constant javadoc - Update tests (per-property rejection via DataProvider, ssl_mode-allowed case, jdbc material-rejection), docs/features.md, examples, and CHANGELOG Implements: #2909
1 parent 73ff7da commit a1f8b04

9 files changed

Lines changed: 114 additions & 85 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,17 @@
7373

7474
### New Features
7575

76+
- **[client-v2, jdbc-v2]** Added support for an application-supplied `javax.net.ssl.SSLContext`. In client-v2,
77+
`Client.Builder.setSSLContext(SSLContext)` hands the client a fully pre-built context that is used as is; in
78+
jdbc-v2 the same context may be passed as a live object in the connection `Properties` under the `ssl_context`
79+
key (added with `Properties.put`, since it is not a string). Trust/key material options cannot be combined with
80+
a custom context and are rejected; `ssl_mode` still applies but only to server hostname verification. This
81+
supports in-memory TLS material that must never be written to disk, including behind connection pools that only
82+
expose `java.util.Properties`.
83+
- Examples for client-v2 https://github.com/ClickHouse/clickhouse-java/blob/main/examples/client-v2/src/main/java/com/clickhouse/examples/client_v2/SSLExamples.java
84+
- Examples for jdbc-v2 https://github.com/ClickHouse/clickhouse-java/blob/main/examples/jdbc/src/main/java/com/clickhouse/examples/jdbc/SSLExamples.java
85+
(https://github.com/ClickHouse/clickhouse-java/pull/2918, https://github.com/ClickHouse/clickhouse-java/issues/2909)
86+
7687
- **[jdbc-v2, client-v2]** Implemented SSL modes configuration. Now it is possible to set `ssl_mode` to `DISABLED`,
7788
`TRUST`, `VERIFY_CA` and `STRICT`. Note for V1 users: `NONE` is supported only by JDBC driver and mapped to `TRUST`.
7889
Please migrate to the new naming.

‎client-v2/src/main/java/com/clickhouse/client/api/Client.java‎

Lines changed: 32 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,6 @@ private Client(Collection<Endpoint> endpoints, Map<String,String> configuration,
152152
Object metricsRegistry, Supplier<String> queryIdGenerator, CredentialsManager cManager,
153153
SSLContext sslContext) {
154154
Map<String, Object> parsedConfiguration = new ConcurrentHashMap<>(ClientConfigProperties.parseConfigMap(configuration));
155-
// A pre-built SSLContext is a live object and cannot travel through the string-based configuration
156-
// map, so it is injected into the parsed (object) configuration directly.
157155
if (sslContext != null) {
158156
parsedConfiguration.put(ClientConfigProperties.SSL_CONTEXT.getKey(), sslContext);
159157
}
@@ -280,6 +278,17 @@ public static class Builder {
280278
private Supplier<String> queryIdGenerator;
281279
private SSLContext sslContext = null;
282280

281+
// Trust/key material options that feed a context the client would otherwise build; none of them
282+
// may be combined with an application-supplied SSLContext (see build()).
283+
private static final ClientConfigProperties[] SSL_MATERIAL_PROPERTIES = {
284+
ClientConfigProperties.SSL_TRUST_STORE,
285+
ClientConfigProperties.SSL_KEYSTORE_TYPE,
286+
ClientConfigProperties.SSL_KEY_STORE_PASSWORD,
287+
ClientConfigProperties.SSL_KEY,
288+
ClientConfigProperties.CA_CERTIFICATE,
289+
ClientConfigProperties.SSL_CERTIFICATE,
290+
};
291+
283292
public Builder() {
284293
this.endpoints = new HashSet<>();
285294
this.configuration = new HashMap<>();
@@ -353,6 +362,11 @@ private Builder addEndpoint(Endpoint endpoint) {
353362
* @param value - configuration option value
354363
*/
355364
public Builder setOption(String key, String value) {
365+
if (key.equals(ClientConfigProperties.SSL_CONTEXT.getKey())) {
366+
throw new ClientMisconfigurationException("'" + ClientConfigProperties.SSL_CONTEXT.getKey()
367+
+ "' cannot be set as a string; supply a javax.net.ssl.SSLContext object via "
368+
+ "Client.Builder.setSSLContext(...)");
369+
}
356370
this.configuration.put(key, value);
357371
if (key.equals(ClientConfigProperties.PRODUCT_NAME.getKey())) {
358372
setClientName(value);
@@ -795,19 +809,10 @@ public Builder setSSLMode(SSLMode sslMode) {
795809
}
796810

797811
/**
798-
* Supplies a pre-built {@link SSLContext} to be used for secure connections instead of one built
799-
* from the configured trust/key material (trust store, CA certificate, client certificate/key).
800-
*
801-
* <p>When a context is set, the client uses it as is - it is the application's responsibility to
802-
* configure it correctly. Trust- and key-material options ({@link Builder#setSSLTrustStore(String)},
803-
* {@link Builder#setRootCertificate(String)}, {@link Builder#setClientCertificate(String)}, ...)
804-
* are then ignored because they only feed the context the client would otherwise build.
805-
* {@link SSLMode} still applies, but only to server hostname verification: {@link SSLMode#TRUST}
806-
* and {@link SSLMode#VERIFY_CA} skip the hostname check while {@link SSLMode#STRICT} (default)
807-
* enforces it.</p>
808-
*
809-
* <p>This is primarily useful when certificates and keys are held in memory (for example, loaded
810-
* from a secret store) and must never be written to disk.</p>
812+
* Supplies a pre-built {@link SSLContext}. When set, it is used as is instead of a context built
813+
* from the configured trust/key material (which then cannot be set alongside it). {@link SSLMode}
814+
* still applies, but only to server hostname verification: {@link SSLMode#STRICT} (default) enforces
815+
* it while {@link SSLMode#TRUST} and {@link SSLMode#VERIFY_CA} skip it.
811816
*
812817
* @param sslContext a fully configured SSL context; {@code null} clears any previously set context
813818
* @return same instance of the builder
@@ -1197,28 +1202,28 @@ public Client build() {
11971202

11981203
CredentialsManager cManager = new CredentialsManager(this.configuration);
11991204

1200-
// A pre-built SSLContext is a live object and can only be supplied programmatically via
1201-
// setSSLContext(SSLContext). A textual 'ssl_context' value (e.g. from setOption(...), a JDBC
1202-
// property, or a URL query parameter) can never represent a real context, so it is rejected
1203-
// here instead of being silently ignored when the SSL context is created.
1205+
// A textual 'ssl_context' can never be a live context (also rejected in setOption).
12041206
if (configuration.containsKey(ClientConfigProperties.SSL_CONTEXT.getKey())) {
12051207
throw new ClientMisconfigurationException("'" + ClientConfigProperties.SSL_CONTEXT.getKey()
12061208
+ "' cannot be set as a string; supply a javax.net.ssl.SSLContext object via "
12071209
+ "Client.Builder.setSSLContext(...)");
12081210
}
12091211

1210-
// Trust- and key-material options only feed a context the client would otherwise build. When
1211-
// the application supplies its own SSLContext they are ignored (see createSSLContext), so this
1212-
// conflict cannot arise and must not be reported.
1213-
if (this.sslContext == null &&
1214-
configuration.containsKey(ClientConfigProperties.SSL_TRUST_STORE.getKey()) &&
1212+
if (this.sslContext != null) {
1213+
// A custom SSLContext replaces any context the client would build, so trust/key material
1214+
// cannot be set alongside it. SSL_MODE (hostname verification) is still allowed.
1215+
for (ClientConfigProperties material : SSL_MATERIAL_PROPERTIES) {
1216+
if (configuration.containsKey(material.getKey())) {
1217+
throw new ClientMisconfigurationException("'" + material.getKey() + "' cannot be combined"
1218+
+ " with a custom SSLContext; the supplied context is used as is. Only 'ssl_mode'"
1219+
+ " (hostname verification) may be set alongside it.");
1220+
}
1221+
}
1222+
} else if (configuration.containsKey(ClientConfigProperties.SSL_TRUST_STORE.getKey()) &&
12151223
configuration.containsKey(ClientConfigProperties.SSL_CERTIFICATE.getKey())) {
12161224
throw new ClientMisconfigurationException("Trust store and certificates cannot be used together");
12171225
}
12181226

1219-
// A trust store and a CA certificate are not rejected here: for VERIFY_CA/STRICT the trust
1220-
// store takes precedence and the CA certificate is ignored with a warning (see createSSLContext).
1221-
12221227
// Resolve ssl_mode case-insensitively and normalize it to the canonical enum name so that
12231228
// downstream parsing is consistent and an unknown value is reported as a misconfiguration
12241229
// here instead of failing later with a generic enum-parsing error.

‎client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,6 @@ public enum ClientConfigProperties {
120120

121121
SSL_MODE("ssl_mode", SSLMode.class, SSLMode.STRICT.name()),
122122

123-
/**
124-
* A pre-built {@link javax.net.ssl.SSLContext} supplied by the application. When set, the client uses
125-
* it as is instead of building one from the configured trust/key material, and {@link #SSL_MODE} then
126-
* only controls server hostname verification. The value is a live object, so it can only be provided
127-
* programmatically (for example via {@code Client.Builder#setSSLContext}); it is never parsed from a
128-
* string and has no textual representation in a configuration map.
129-
*/
130123
SSL_CONTEXT("ssl_context", SSLContext.class),
131124

132125
RETRY_ON_FAILURE("retry", Integer.class, "3"),

‎client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -154,20 +154,11 @@ public HttpAPIClientHelper(Map<String, Object> configuration, Object metricsRegi
154154
}
155155

156156
/**
157-
* Creates or returns default SSL context.
157+
* Creates an SSL context from the configured trust/key material.
158158
*
159159
* @return SSLContext
160160
*/
161161
public SSLContext createSSLContext(Map<String, Object> configuration) {
162-
// A pre-built SSLContext supplied by the application is used as is; the client does not build one
163-
// from the configured trust/key material. Server hostname verification is still governed by the
164-
// SSL mode where the connection socket factory is created (see createHttpClient).
165-
final Object customSSLContext = configuration.get(ClientConfigProperties.SSL_CONTEXT.getKey());
166-
if (customSSLContext instanceof SSLContext) {
167-
LOG.debug("Using application-supplied SSLContext; trust/key material options are ignored.");
168-
return (SSLContext) customSSLContext;
169-
}
170-
171162
final SSLMode sslMode = ClientConfigProperties.SSL_MODE.getOrDefault(configuration);
172163
final String trustStorePath = (String) configuration.get(ClientConfigProperties.SSL_TRUST_STORE.getKey());
173164
final String caCertificate = (String) configuration.get(ClientConfigProperties.CA_CERTIFICATE.getKey());
@@ -289,7 +280,15 @@ private HttpClientConnectionManager poolConnectionManager(LayeredConnectionSocke
289280
public CloseableHttpClient createHttpClient(boolean initSslContext, Map<String, Object> configuration) {
290281
// Top Level builders
291282
HttpClientBuilder clientBuilder = HttpClientBuilder.create();
292-
SSLContext sslContext = initSslContext ? createSSLContext(configuration) : null;
283+
// An application-supplied SSLContext is used as is; otherwise one is built from the configured
284+
// trust/key material. Server hostname verification below still applies via the SSL mode.
285+
SSLContext sslContext = null;
286+
if (initSslContext) {
287+
Object customSSLContext = configuration.get(ClientConfigProperties.SSL_CONTEXT.getKey());
288+
sslContext = customSSLContext instanceof SSLContext
289+
? (SSLContext) customSSLContext
290+
: createSSLContext(configuration);
291+
}
293292
LayeredConnectionSocketFactory sslConnectionSocketFactory;
294293
if (sslContext != null) {
295294
String socketSNI = (String)configuration.get(ClientConfigProperties.SSL_SOCKET_SNI.getKey());

‎client-v2/src/test/java/com/clickhouse/client/api/ClientBuilderTest.java‎

Lines changed: 34 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -91,41 +91,59 @@ public void testSslModeInvalidValueRejected() {
9191
}
9292

9393
@Test
94-
public void testStringSSLContextRejected() {
95-
// ssl_context is an object-only property; a textual value can never be a real context, so the
96-
// builder must reject it instead of silently ignoring it (previously parsed to null and dropped).
94+
public void testStringSSLContextRejectedBySetOption() {
95+
Assert.expectThrows(ClientMisconfigurationException.class,
96+
() -> new Client.Builder().setOption(ClientConfigProperties.SSL_CONTEXT.getKey(), "not-a-context"));
97+
}
98+
99+
@DataProvider(name = "sslMaterialWithCustomContext_DP")
100+
public static Object[][] sslMaterialWithCustomContext_DP() {
101+
return new Object[][] {
102+
{ ClientConfigProperties.SSL_TRUST_STORE.getKey(), "/path/to/truststore.jks" },
103+
{ ClientConfigProperties.SSL_KEYSTORE_TYPE.getKey(), "JKS" },
104+
{ ClientConfigProperties.SSL_KEY_STORE_PASSWORD.getKey(), "secret" },
105+
{ ClientConfigProperties.SSL_KEY.getKey(), "/path/to/client.key" },
106+
{ ClientConfigProperties.CA_CERTIFICATE.getKey(), "/path/to/ca.crt" },
107+
{ ClientConfigProperties.SSL_CERTIFICATE.getKey(), "/path/to/client.crt" },
108+
};
109+
}
110+
111+
@Test(dataProvider = "sslMaterialWithCustomContext_DP")
112+
public void testCustomSSLContextRejectsOtherSslMaterial(String materialKey, String materialValue) throws Exception {
113+
SSLContext customContext = SSLContext.getInstance("TLS");
114+
customContext.init(null, null, null);
115+
97116
Assert.expectThrows(ClientMisconfigurationException.class, () -> new Client.Builder()
98117
.addEndpoint("https://localhost:8443")
99118
.setUsername("default")
100119
.setPassword("")
101-
.setOption(ClientConfigProperties.SSL_CONTEXT.getKey(), "not-a-context")
120+
.setOption(materialKey, materialValue)
121+
.setSSLContext(customContext)
102122
.build());
103123
}
104124

105125
@Test
106-
public void testCustomSSLContextIgnoresTrustAndKeyMaterialConflict() throws Exception {
126+
public void testCustomSSLContextAllowsSslMode() throws Exception {
107127
SSLContext customContext = SSLContext.getInstance("TLS");
108128
customContext.init(null, null, null);
109129

110-
// A trust store and a client certificate normally conflict, but when a custom SSLContext is
111-
// supplied that material is ignored, so the conflict must not be reported and the client builds.
112130
try (Client client = new Client.Builder()
113131
.addEndpoint("https://localhost:8443")
114132
.setUsername("default")
115133
.setPassword("")
116-
.setSSLTrustStore("/path/to/truststore.jks")
117-
.setClientCertificate("client.crt")
134+
.setSSLMode(SSLMode.VERIFY_CA)
118135
.setSSLContext(customContext)
119136
.build()) {
120137
Assert.assertSame(extractConfiguration(client).get(ClientConfigProperties.SSL_CONTEXT.getKey()),
121-
customContext, "The application-supplied SSLContext should be used as is");
138+
customContext);
139+
Assert.assertEquals(client.getConfiguration().get(ClientConfigProperties.SSL_MODE.getKey()),
140+
SSLMode.VERIFY_CA.name());
122141
}
123142
}
124143

125144
@Test
126145
public void testTrustStoreAndClientCertificateConflictRejectedWithoutCustomContext() {
127-
// Contrast case: without a custom SSLContext the trust-store/certificate conflict must still be
128-
// rejected exactly as before - the fix only suppresses the check when a context is supplied.
146+
// Contrast: without a custom SSLContext the trust-store/certificate conflict is still rejected.
129147
Assert.expectThrows(ClientMisconfigurationException.class, () -> new Client.Builder()
130148
.addEndpoint("https://localhost:8443")
131149
.setUsername("default")
@@ -162,23 +180,10 @@ public void testSetSSLContextStoredInConfiguration() throws Exception {
162180
}
163181

164182
@Test
165-
public void testCreateSSLContextReturnsCustomContext() throws Exception {
183+
public void testCreateSSLContextIgnoresCustomContext() throws Exception {
166184
SSLContext customContext = SSLContext.getInstance("TLS");
167185
customContext.init(null, null, null);
168186

169-
try (Client client = new Client.Builder()
170-
.addEndpoint("https://localhost:8443")
171-
.setUsername("default")
172-
.setPassword("")
173-
.setSSLContext(customContext)
174-
.build()) {
175-
HttpAPIClientHelper helper = extractHttpClientHelper(client);
176-
SSLContext resolved = helper.createSSLContext(extractConfiguration(client));
177-
Assert.assertSame(resolved, customContext,
178-
"createSSLContext must return the application-supplied context as is");
179-
}
180-
181-
// When no custom context is configured, createSSLContext builds a context (not the custom one).
182187
try (Client client = new Client.Builder()
183188
.addEndpoint("https://localhost:8443")
184189
.setUsername("default")
@@ -187,10 +192,9 @@ public void testCreateSSLContextReturnsCustomContext() throws Exception {
187192
HttpAPIClientHelper helper = extractHttpClientHelper(client);
188193
Map<String, Object> configWithCustom = new HashMap<>(extractConfiguration(client));
189194
configWithCustom.put(ClientConfigProperties.SSL_CONTEXT.getKey(), customContext);
190-
Assert.assertSame(helper.createSSLContext(configWithCustom), customContext,
191-
"createSSLContext must honor a custom context supplied via the configuration map");
192-
Assert.assertNotSame(helper.createSSLContext(extractConfiguration(client)), customContext,
193-
"createSSLContext must build its own context when none is supplied");
195+
// createSSLContext only builds from trust/key material; selecting a custom context is
196+
// createHttpClient's responsibility, so this must not return the supplied context.
197+
Assert.assertNotSame(helper.createSSLContext(configWithCustom), customContext);
194198
}
195199
}
196200

0 commit comments

Comments
 (0)