Skip to content

Commit d17cf3d

Browse files
client-v2: cover SSL cipher-suite factory selection to satisfy coverage gate
Adds focused unit tests for HttpAPIClientHelper.createHttpClient's TLS socket-factory selection, which was the uncovered new code behind the SonarCloud "coverage on new code" quality-gate failure (73% < 80%). The tests drive the real createHttpClient SSL branch (via Mockito mockConstruction) and pin the factory wiring for each mode: STRICT + cipher suites forwards the suites and keeps hostname verification (null verifier); TRUST and VERIFY_CA install the permissive verifier; a custom SNI installs the permissive verifier and passes the SNI through; SNI + cipher suites combine; and the default STRICT path plus an empty cipher list keep the plain verifying SSLConnectionSocketFactory (no restriction). Test-only change; no existing tests modified. New-code coverage for the cipher-suite change is now 100%.
1 parent 3588e30 commit d17cf3d

1 file changed

Lines changed: 153 additions & 0 deletions

File tree

client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
11
package com.clickhouse.client.api.internal;
22

3+
import com.clickhouse.client.api.ClientConfigProperties;
4+
import com.clickhouse.client.api.enums.SSLMode;
35
import com.clickhouse.client.api.internal.HttpAPIClientHelper.CustomSSLConnectionFactory;
6+
import net.jpountz.lz4.LZ4Factory;
47
import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory;
58
import org.mockito.ArgumentCaptor;
9+
import org.mockito.MockedConstruction;
610
import org.testng.annotations.Test;
711

812
import javax.net.ssl.SNIHostName;
@@ -11,14 +15,21 @@
1115
import javax.net.ssl.SSLParameters;
1216
import javax.net.ssl.SSLSocket;
1317
import java.lang.reflect.Field;
18+
import java.util.ArrayList;
19+
import java.util.Arrays;
20+
import java.util.Collections;
21+
import java.util.HashMap;
1422
import java.util.List;
23+
import java.util.Map;
1524

1625
import static org.mockito.ArgumentMatchers.any;
1726
import static org.mockito.Mockito.mock;
27+
import static org.mockito.Mockito.mockConstruction;
1828
import static org.mockito.Mockito.never;
1929
import static org.mockito.Mockito.verify;
2030
import static org.mockito.Mockito.when;
2131
import static org.testng.Assert.assertEquals;
32+
import static org.testng.Assert.assertNotNull;
2233
import static org.testng.Assert.assertNull;
2334

2435
public class HttpAPIClientHelperTest {
@@ -85,6 +96,148 @@ public void testBlankSniLeavesSocketParametersUntouched() throws Exception {
8596
verify(socket, never()).setSSLParameters(any());
8697
}
8798

99+
/**
100+
* When cipher suites are configured in STRICT mode (no SNI), {@code createHttpClient} must build the
101+
* cipher-aware {@link CustomSSLConnectionFactory}, forward the configured suites to it, and pass a
102+
* {@code null} hostname verifier so the base factory keeps its default (verifying) behaviour - i.e.
103+
* restricting cipher suites must not silently disable hostname verification.
104+
*/
105+
@Test
106+
public void testCreateHttpClientStrictWithCipherSuitesForwardsSuitesAndKeepsHostnameVerification() {
107+
Map<String, Object> config = new HashMap<>();
108+
config.put(ClientConfigProperties.SSL_CIPHER_SUITES.getKey(),
109+
Arrays.asList("TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256"));
110+
111+
List<List<?>> calls = captureCustomFactoryConstruction(config);
112+
113+
assertEquals(calls.size(), 1, "STRICT + cipher suites must build the cipher-aware custom factory");
114+
List<?> args = calls.get(0); // (socketSNI, sslContext, hostnameVerifier, enabledCipherSuites)
115+
assertNull(args.get(2), "STRICT must keep default hostname verification: the verifier passed to the "
116+
+ "factory must be null so the base factory verifies hostnames");
117+
assertEquals((String[]) args.get(3), new String[]{"TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256"},
118+
"the configured cipher suites must be forwarded to the connection socket factory");
119+
}
120+
121+
/**
122+
* TRUST mode opts out of hostname verification, so the factory must receive a permissive (non-null)
123+
* verifier; with no cipher suites configured the factory must keep the JVM defaults (no restriction).
124+
*/
125+
@Test
126+
public void testCreateHttpClientTrustModeInstallsPermissiveVerifierWithoutCipherRestriction() {
127+
Map<String, Object> config = new HashMap<>();
128+
config.put(ClientConfigProperties.SSL_MODE.getKey(), SSLMode.TRUST);
129+
130+
List<List<?>> calls = captureCustomFactoryConstruction(config);
131+
132+
assertEquals(calls.size(), 1, "TRUST mode must build the custom factory to skip hostname verification");
133+
List<?> args = calls.get(0);
134+
assertNotNull(args.get(2), "TRUST mode must install a permissive hostname verifier");
135+
assertNull(args.get(3), "TRUST mode without configured cipher suites must not restrict cipher suites");
136+
}
137+
138+
/**
139+
* A custom SNI host will not match the server certificate, so hostname verification is skipped (a
140+
* permissive verifier is installed) and the configured SNI is passed through to the factory.
141+
*/
142+
@Test
143+
public void testCreateHttpClientCustomSniInstallsPermissiveVerifier() {
144+
Map<String, Object> config = new HashMap<>();
145+
config.put(ClientConfigProperties.SSL_SOCKET_SNI.getKey(), "sni.example.com");
146+
147+
List<List<?>> calls = captureCustomFactoryConstruction(config);
148+
149+
assertEquals(calls.size(), 1, "a custom SNI must build the custom factory");
150+
List<?> args = calls.get(0);
151+
assertEquals(args.get(0), "sni.example.com", "the configured SNI must be passed to the factory");
152+
assertNotNull(args.get(2), "a custom SNI host won't match the certificate, so a permissive verifier "
153+
+ "is installed");
154+
}
155+
156+
/**
157+
* Contrast case: the default STRICT path with no SNI and no cipher restriction must be unchanged - it
158+
* must NOT build the custom factory, so the plain verifying {@link SSLConnectionSocketFactory} is used.
159+
*/
160+
@Test
161+
public void testCreateHttpClientStrictWithoutSniOrCiphersUsesPlainFactory() {
162+
List<List<?>> calls = captureCustomFactoryConstruction(new HashMap<>());
163+
164+
assertEquals(calls.size(), 0, "default STRICT with no SNI and no cipher suites must keep using the "
165+
+ "plain SSLConnectionSocketFactory (unchanged behaviour)");
166+
}
167+
168+
/**
169+
* VERIFY_CA validates the certificate chain but skips hostname verification (the connection hostname may
170+
* legitimately differ), so - like TRUST - it must install a permissive verifier. This pins the second
171+
* mode that opts out of hostname verification, distinct from the TRUST path.
172+
*/
173+
@Test
174+
public void testCreateHttpClientVerifyCaModeInstallsPermissiveVerifier() {
175+
Map<String, Object> config = new HashMap<>();
176+
config.put(ClientConfigProperties.SSL_MODE.getKey(), SSLMode.VERIFY_CA);
177+
178+
List<List<?>> calls = captureCustomFactoryConstruction(config);
179+
180+
assertEquals(calls.size(), 1, "VERIFY_CA must build the custom factory to skip hostname verification");
181+
List<?> args = calls.get(0);
182+
assertNotNull(args.get(2), "VERIFY_CA must install a permissive hostname verifier");
183+
assertNull(args.get(3), "VERIFY_CA without configured cipher suites must not restrict cipher suites");
184+
}
185+
186+
/**
187+
* SNI and cipher suites are independent concerns and must combine: a custom SNI still installs the
188+
* permissive verifier while the configured cipher suites are forwarded to the same factory - i.e. cipher
189+
* forwarding is not tied to the STRICT/cipher-triggered path.
190+
*/
191+
@Test
192+
public void testCreateHttpClientSniAndCipherSuitesCombine() {
193+
Map<String, Object> config = new HashMap<>();
194+
config.put(ClientConfigProperties.SSL_SOCKET_SNI.getKey(), "sni.example.com");
195+
config.put(ClientConfigProperties.SSL_CIPHER_SUITES.getKey(),
196+
Arrays.asList("TLS_AES_256_GCM_SHA384"));
197+
198+
List<List<?>> calls = captureCustomFactoryConstruction(config);
199+
200+
assertEquals(calls.size(), 1, "SNI + cipher suites must build the custom factory");
201+
List<?> args = calls.get(0);
202+
assertEquals(args.get(0), "sni.example.com", "the configured SNI must be passed to the factory");
203+
assertNotNull(args.get(2), "a custom SNI installs a permissive verifier even when cipher suites are set");
204+
assertEquals((String[]) args.get(3), new String[]{"TLS_AES_256_GCM_SHA384"},
205+
"the configured cipher suites must still be forwarded when SNI is also set");
206+
}
207+
208+
/**
209+
* Boundary case: an empty cipher-suite list must be treated as "no restriction" (JVM defaults), exactly
210+
* like an unset value - it must NOT be turned into an empty cipher array, which would enable zero suites
211+
* and make every handshake fail. STRICT with no SNI therefore keeps using the plain factory.
212+
*/
213+
@Test
214+
public void testCreateHttpClientEmptyCipherSuitesTreatedAsNoRestriction() {
215+
Map<String, Object> config = new HashMap<>();
216+
config.put(ClientConfigProperties.SSL_CIPHER_SUITES.getKey(), Collections.emptyList());
217+
218+
List<List<?>> calls = captureCustomFactoryConstruction(config);
219+
220+
assertEquals(calls.size(), 0, "an empty cipher-suite list must be treated as no restriction (JVM "
221+
+ "defaults), so the plain SSLConnectionSocketFactory is used");
222+
}
223+
224+
/**
225+
* Builds an {@link HttpAPIClientHelper} and invokes {@link HttpAPIClientHelper#createHttpClient} with SSL
226+
* enabled while intercepting every {@link CustomSSLConnectionFactory} construction, returning the
227+
* constructor arguments of each construction (empty when the plain factory branch is taken instead).
228+
*/
229+
private static List<List<?>> captureCustomFactoryConstruction(Map<String, Object> sslConfig) {
230+
HttpAPIClientHelper helper = new HttpAPIClientHelper(new HashMap<>(), null, false,
231+
LZ4Factory.fastestJavaInstance());
232+
List<List<?>> constructorArgs = new ArrayList<>();
233+
try (MockedConstruction<CustomSSLConnectionFactory> mocked = mockConstruction(
234+
CustomSSLConnectionFactory.class,
235+
(mock, context) -> constructorArgs.add(context.arguments()))) {
236+
helper.createHttpClient(true, sslConfig);
237+
}
238+
return constructorArgs;
239+
}
240+
88241
private static String[] baseSupportedCipherSuites(CustomSSLConnectionFactory factory) throws Exception {
89242
Field field = SSLConnectionSocketFactory.class.getDeclaredField("supportedCipherSuites");
90243
field.setAccessible(true);

0 commit comments

Comments
 (0)