Unit tests for TLS protocol validation#1898
Open
shreyabiradar07 wants to merge 12 commits into
Open
Conversation
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Reviewer's GuideAdds a comprehensive TLS protocol validation test suite for GenericRestApiClient using real SSLSocket/SSLContext configurations and adjusts HttpResponseWrapper to be a static inner class to support testability and independent usage. Sequence diagram for TLS handshake validation with system defaultssequenceDiagram
actor Test as GenericRestApiClientTLSTest
participant SSLContext
participant SSLSocket
participant JVMSystemDefaults
Test->>SSLContext: getDefault()
SSLContext-->>Test: SSLContext
Test->>SSLContext: createSSLSocket()
SSLContext-->>Test: SSLSocket
Test->>SSLSocket: setEnabledProtocols(clientProtocols)
Test->>SSLSocket: startHandshake()
alt [no common protocol]
SSLSocket-->>Test: SSLHandshakeException
else [TLSv1_2 or TLSv1_3]
SSLSocket-->>Test: handshake success
end
Note over JVMSystemDefaults,SSLSocket: Null protocols delegate to JVMSystemDefaults enabled TLS versions
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several tests (e.g.,
shouldAcceptTLS13,shouldAcceptTLS12,shouldHandleProtocolVersionError) only assert on mocked socket state or hard-coded strings and never exerciseGenericRestApiClient; consider refactoring them to go through the client API so they validate actual production behavior rather than mock setup. - The
shouldCreateSSLConnectionSocketFactoryWithNullProtocolstest currently just assertstrue, which doesn't validate any behavior; either remove it or replace it with an assertion that inspects the factory/protocols actually created byGenericRestApiClientwhen passing null. - In the test setup you create
mockDataSourceInfo,mockSSLContext, andmockSSLSocketFactory, but they are not used beyond basic stubbing; if they are not needed to verify behavior, simplify the setup to only keep necessary mocks to reduce noise and improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests (e.g., `shouldAcceptTLS13`, `shouldAcceptTLS12`, `shouldHandleProtocolVersionError`) only assert on mocked socket state or hard-coded strings and never exercise `GenericRestApiClient`; consider refactoring them to go through the client API so they validate actual production behavior rather than mock setup.
- The `shouldCreateSSLConnectionSocketFactoryWithNullProtocols` test currently just asserts `true`, which doesn't validate any behavior; either remove it or replace it with an assertion that inspects the factory/protocols actually created by `GenericRestApiClient` when passing null.
- In the test setup you create `mockDataSourceInfo`, `mockSSLContext`, and `mockSSLSocketFactory`, but they are not used beyond basic stubbing; if they are not needed to verify behavior, simplify the setup to only keep necessary mocks to reduce noise and improve maintainability.
## Individual Comments
### Comment 1
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="85-94" />
<code_context>
+ assertEquals("TLSv1.2", protocols[1], "TLS 1.2 should be second");
+ }
+
+ @Test
+ @DisplayName("Should not include SSLv3 in supported protocols")
+ void shouldNotSupportSSLv3() {
+ // Given: System default protocols
+ String[] systemProtocols = {"TLSv1.3", "TLSv1.2"};
+
+ when(mockSSLSocket.getEnabledProtocols()).thenReturn(systemProtocols);
+
+ // When: Check enabled protocols
+ String[] protocols = mockSSLSocket.getEnabledProtocols();
+
+ // Then: SSLv3 should not be present
+ assertNotNull(protocols);
+ assertFalse(Arrays.asList(protocols).contains("SSLv3"),
+ "SSLv3 should never be enabled");
+ assertFalse(Arrays.asList(protocols).contains("SSLv2"),
+ "SSLv2 should never be enabled");
+ }
+}
</code_context>
<issue_to_address>
**issue (testing):** TLS 1.3/1.2 tests never exercise GenericRestApiClient, they only assert on a standalone mock socket
Because these tests only stub and assert on `mockSSLSocket.getEnabledProtocols()`, they never verify how `GenericRestApiClient` actually configures TLS. To make them meaningful, have the mocked `SSLContext`/`SSLSocketFactory` used by a `GenericRestApiClient` instance (or a spy), trigger a real client call or the internal TLS factory builder, and assert the enabled protocols on the socket created by the client. This ensures regressions in client TLS configuration are caught, not just behavior of a standalone mock.
</issue_to_address>
### Comment 2
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="63-72" />
<code_context>
+ private SSLSocketFactory mockSSLSocketFactory;
+ private SSLSocket mockSSLSocket;
+
+ @BeforeEach
+ void setup() throws Exception {
+ // Create mock objects
+ mockDataSourceInfo = mock(DataSourceInfo.class);
+ mockSSLContext = mock(SSLContext.class);
+ mockSSLSocketFactory = mock(SSLSocketFactory.class);
+ mockSSLSocket = mock(SSLSocket.class);
+
+ // Setup basic mock behavior
+ when(mockSSLContext.getSocketFactory()).thenReturn(mockSSLSocketFactory);
+ when(mockSSLSocketFactory.createSocket()).thenReturn(mockSSLSocket);
+
+ // Initialize client
+ client = new GenericRestApiClient();
+ client.setBaseURL("https://test-prometheus.example.com:9090");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Several mocks set up in @BeforeEach are unused and can be removed or actually wired into the client
In `setup()`, these mocks are created and partially configured but never used by `GenericRestApiClient`, which adds noise and can mislead readers about what’s actually under test. Either wire these mocks into the client and assert its TLS behavior (e.g., enabled protocols), or remove them and keep the setup focused on protocol list testing. Wiring them in would also improve coverage of the client’s TLS handling.
Suggested implementation:
```java
class GenericRestApiClientTLSTest {
private static final Logger LOGGER = LoggerFactory.getLogger(GenericRestApiClientTLSTest.class);
private GenericRestApiClient client;
private DataSourceInfo mockDataSourceInfo;
```
```java
```
1. In the same file, simplify the `setup()` method so it only creates the `mockDataSourceInfo` and initializes the `GenericRestApiClient`, removing the unused TLS-related mocks and their stubbing. For example:
@BeforeEach
void setup() throws Exception {
mockDataSourceInfo = mock(DataSourceInfo.class);
client = new GenericRestApiClient();
client.setBaseURL("https://test-prometheus.example.com:9090");
}
2. Remove now-unused imports such as `javax.net.ssl.SSLContext`, `javax.net.ssl.SSLSocketFactory`, and `javax.net.ssl.SSLSocket` if they are no longer referenced anywhere in the test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Most tests repeat the same reflection-based setup (obtaining setupHttpClient, building SSLContext, creating factory/socket); consider extracting this into shared helper methods or a setup utility to reduce duplication and make the tests easier to maintain.
- The tests assert specific system default TLS protocols (e.g., absence of TLSv1.0/1.1, ordering of TLSv1.3) which may vary by JDK/security policy; you may want to relax or parameterize these assertions to avoid brittleness across different runtime environments.
- Several tests create an SSLConnectionSocketFactory instance (createTestFactory) but never use the factory; either use it to exercise the behavior under test or remove it to avoid dead code and clarify what is actually being validated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Most tests repeat the same reflection-based setup (obtaining setupHttpClient, building SSLContext, creating factory/socket); consider extracting this into shared helper methods or a setup utility to reduce duplication and make the tests easier to maintain.
- The tests assert specific system default TLS protocols (e.g., absence of TLSv1.0/1.1, ordering of TLSv1.3) which may vary by JDK/security policy; you may want to relax or parameterize these assertions to avoid brittleness across different runtime environments.
- Several tests create an SSLConnectionSocketFactory instance (createTestFactory) but never use the factory; either use it to exercise the behavior under test or remove it to avoid dead code and clarify what is actually being validated.
## Individual Comments
### Comment 1
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="83-92" />
<code_context>
+ httpClient.close();
+ }
+
+ @Test
+ @DisplayName("Should handle protocol_version error for legacy TLS")
+ void shouldHandleProtocolVersionError() {
+ LOGGER.info("=== Testing Protocol Version Error Handling ===");
+
+ String errorMessage = "Received fatal alert: protocol_version";
+ boolean isProtocolVersionError = errorMessage.contains("protocol_version");
+
+ assertTrue(isProtocolVersionError,
+ "Error message should indicate protocol version mismatch");
+
+ LOGGER.info("Protocol version error correctly identified");
+ }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** TLS 1.0/1.1 tests don't validate the advertised SSLHandshakeException behavior
The tests `shouldRejectTLS11ThroughSystemDefaults` and `shouldRejectTLS10ThroughSystemDefaults` only assert that TLS 1.0/1.1 are absent from `getEnabledProtocols()`, which verifies JVM defaults but not `GenericRestApiClient`’s behavior during a real handshake.
Please add at least one test that performs an actual HTTPS call via `GenericRestApiClient` against a TLS 1.0/1.1-only endpoint (or equivalent setup using an `SSLSocket` configured with `TLSv1`/`TLSv1.1`) and asserts that the handshake fails with `SSLHandshakeException`. This will align the tests with the described behavior and catch regressions in handshake handling.
Suggested implementation:
```java
@Test
@DisplayName("Should handle protocol_version error for legacy TLS")
void shouldHandleProtocolVersionError() {
LOGGER.info("=== Testing Protocol Version Error Handling ===");
String errorMessage = "Received fatal alert: protocol_version";
boolean isProtocolVersionError = errorMessage.contains("protocol_version");
assertTrue(isProtocolVersionError,
"Error message should indicate protocol version mismatch");
LOGGER.info("Protocol version error correctly identified");
}
/**
* Validates that a real TLS handshake against a TLS 1.0–only endpoint fails with
* SSLHandshakeException when using system-default client protocols (which exclude TLS 1.0/1.1).
*
* This goes beyond inspecting enabled protocols and exercises the actual handshake path.
*/
@Test
@DisplayName("Should fail handshake against TLS 1.0-only endpoint with SSLHandshakeException")
void shouldFailHandshakeAgainstTls10OnlyEndpoint() throws Exception {
LOGGER.info("=== Testing handshake failure against TLS 1.0-only endpoint ===");
// Start a local TLS 1.0-only SSL server socket on an ephemeral port
SSLContext legacyTlsContext = createLegacyTls10ServerContext();
SSLServerSocketFactory serverSocketFactory = legacyTlsContext.getServerSocketFactory();
try (SSLServerSocket serverSocket = (SSLServerSocket) serverSocketFactory.createServerSocket(0)) {
serverSocket.setEnabledProtocols(new String[]{"TLSv1"});
final int port = serverSocket.getLocalPort();
LOGGER.info("Started TLS 1.0-only test server on port {}", port);
// Accept connections in a background thread; the handshake is expected to fail
ExecutorService executor = Executors.newSingleThreadExecutor();
Future<?> serverFuture = executor.submit(() -> {
try (SSLSocket socket = (SSLSocket) serverSocket.accept()) {
socket.startHandshake();
} catch (Exception e) {
// Handshake is expected to fail with protocol_version, so swallow exceptions here
LOGGER.debug("Expected exception during legacy TLS server handshake", e);
}
});
try {
// Use system-default SSLContext for the client side, which should not include TLS 1.0/1.1
SSLContext clientContext = SSLContext.getDefault();
SSLSocketFactory clientSocketFactory = clientContext.getSocketFactory();
SSLHandshakeException handshakeException = assertThrows(
SSLHandshakeException.class,
() -> {
try (SSLSocket clientSocket = (SSLSocket) clientSocketFactory.createSocket("localhost", port)) {
clientSocket.startHandshake();
}
},
"Handshake against TLS 1.0-only endpoint should fail with SSLHandshakeException"
);
LOGGER.info("Handshake failed as expected with message: {}", handshakeException.getMessage());
assertTrue(
handshakeException.getMessage() == null
|| handshakeException.getMessage().contains("protocol_version"),
"Handshake failure should indicate a protocol version mismatch"
);
} finally {
serverFuture.cancel(true);
executor.shutdownNow();
}
}
}
/**
* Creates an SSLContext configured with a server key/certificate for a TLS 1.0-only endpoint.
*
* The keystore is loaded from a classpath resource so that we can run a real SSLServerSocket
* bound to TLSv1. System-default client settings should then fail to negotiate.
*/
private SSLContext createLegacyTls10ServerContext() throws Exception {
// The project should provide a test keystore containing a server certificate for localhost.
// This method intentionally uses a classpath resource so that the keystore can be managed
// alongside other test resources.
char[] password = "changeit".toCharArray();
try (InputStream keystoreStream =
GenericRestApiClientTLSTest.class.getResourceAsStream("/tls/legacy-tls-keystore.jks")) {
// If the keystore is not present, skip the test rather than failing the build
Assumptions.assumeTrue(
keystoreStream != null,
"Test keystore /tls/legacy-tls-keystore.jks is required for legacy TLS handshake tests"
);
KeyStore keyStore = KeyStore.getInstance("JKS");
keyStore.load(keystoreStream, password);
KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
kmf.init(keyStore, password);
SSLContext sslContext = SSLContext.getInstance("TLSv1");
sslContext.init(kmf.getKeyManagers(), null, null);
return sslContext;
}
}
}
```
1. Ensure the following imports are present at the top of `GenericRestApiClientTLSTest.java`:
- `import javax.net.ssl.SSLContext;`
- `import javax.net.ssl.SSLServerSocket;`
- `import javax.net.ssl.SSLServerSocketFactory;`
- `import javax.net.ssl.SSLSocket;`
- `import javax.net.ssl.SSLSocketFactory;`
- `import javax.net.ssl.SSLHandshakeException;`
- `import java.io.InputStream;`
- `import java.security.KeyStore;`
- `import javax.net.ssl.KeyManagerFactory;`
- `import java.util.concurrent.ExecutorService;`
- `import java.util.concurrent.Executors;`
- `import java.util.concurrent.Future;`
- `import org.junit.jupiter.api.Assumptions;`
2. Add the test keystore file `legacy-tls-keystore.jks` under `src/test/resources/tls/` (or adjust the resource path and filename in `createLegacyTls10ServerContext()` accordingly). The keystore should contain a server certificate for `localhost` with password `changeit`, or you can update the password in the code to match your keystore.
3. If your logging framework does not support parameterized messages with `{}` (e.g., if you are using a plain JDK logger instead of SLF4J), adjust the `LOGGER.info` and `LOGGER.debug` calls accordingly.
4. If you prefer to exercise `GenericRestApiClient` directly instead of using `SSLSocket`, you can reuse the `createLegacyTls10ServerContext()` and server setup but replace the client-side SSLSocket logic with a `GenericRestApiClient` call to `https://localhost:<port>/` and assert that it fails with `SSLHandshakeException`.
</issue_to_address>
### Comment 2
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="100" />
<code_context>
+ .loadTrustMaterial((chain, authType) -> true)
+ .build();
+
+ SSLConnectionSocketFactory factory = createTestFactory(sslContext);
+
+ // Create a socket using the same SSLContext to verify system defaults
</code_context>
<issue_to_address>
**suggestion (testing):** Tests don't verify the actual SSLConnectionSocketFactory used by GenericRestApiClient
The tests build an `SSLConnectionSocketFactory` via `createTestFactory(sslContext)` but never assert anything about the factory actually used by `GenericRestApiClient.setupHttpClient()`. Assertions only exercise an `SSLSocket` built directly from the `SSLContext`, so they primarily validate JVM TLS defaults rather than the client's configuration.
Either assert against the `SSLConnectionSocketFactory` actually installed in the HTTP client's connection manager (e.g. via reflection) to confirm it uses `null` protocols, or remove the unused `factory` variable and clarify in comments that the tests target system defaults instead of the client's factory configuration.
Suggested implementation:
```java
assertNotNull(httpClient, "HTTP client should be created by GenericRestApiClient");
// Then: Verify the HTTP client is configured with an SSLConnectionSocketFactory that relies on system default protocols (null)
SSLConnectionSocketFactory httpsFactory = extractSslConnectionSocketFactory(httpClient);
assertNotNull(httpsFactory, "HTTPS socket factory should be configured on the HTTP client's connection manager");
String[] supportedProtocols = getSupportedProtocols(httpsFactory);
assertNull(supportedProtocols, "Configured supported protocols should be null to indicate use of system defaults");
// When: Test the same configuration pattern (null protocols) that GenericRestApiClient uses
SSLContext sslContext = SSLContexts.custom()
.loadTrustMaterial((chain, authType) -> true)
.build();
// Create a socket using the same SSLContext to verify system defaults
SSLSocket socket = (SSLSocket) sslContext.getSocketFactory().createSocket();
// Then: Verify system defaults provide secure TLS configuration
String[] enabledProtocols = socket.getEnabledProtocols();
LOGGER.info("System defaults with null protocols - Enabled: {}", Arrays.toString(enabledProtocols));
assertNotNull(enabledProtocols);
List<String> protocolList = Arrays.asList(enabledProtocols);
assertTrue(protocolList.contains("TLSv1.3") || protocolList.contains("TLSv1.2"),
```
To fully implement the reflection-based assertions, you also need to:
1. **Add helper methods inside `GenericRestApiClientTLSTest` (or the relevant test class):**
```java
private SSLConnectionSocketFactory extractSslConnectionSocketFactory(CloseableHttpClient httpClient) {
try {
// Most Apache HttpClient implementations in tests will be InternalHttpClient
if (httpClient instanceof org.apache.http.impl.client.InternalHttpClient) {
org.apache.http.impl.client.InternalHttpClient internal =
(org.apache.http.impl.client.InternalHttpClient) httpClient;
org.apache.http.conn.HttpClientConnectionManager connectionManager = internal.getConnectionManager();
if (connectionManager instanceof org.apache.http.impl.conn.PoolingHttpClientConnectionManager) {
org.apache.http.impl.conn.PoolingHttpClientConnectionManager poolingMgr =
(org.apache.http.impl.conn.PoolingHttpClientConnectionManager) connectionManager;
org.apache.http.config.Lookup<org.apache.http.conn.socket.ConnectionSocketFactory> registry =
poolingMgr.getSocketFactoryRegistry();
org.apache.http.conn.socket.ConnectionSocketFactory csf = registry.lookup("https");
if (csf instanceof SSLConnectionSocketFactory) {
return (SSLConnectionSocketFactory) csf;
}
}
}
// Fallback via reflection for other CloseableHttpClient implementations
java.lang.reflect.Field mgrField = httpClient.getClass().getDeclaredField("connManager");
mgrField.setAccessible(true);
Object manager = mgrField.get(httpClient);
if (manager instanceof org.apache.http.impl.conn.PoolingHttpClientConnectionManager) {
org.apache.http.impl.conn.PoolingHttpClientConnectionManager poolingMgr =
(org.apache.http.impl.conn.PoolingHttpClientConnectionManager) manager;
org.apache.http.config.Lookup<org.apache.http.conn.socket.ConnectionSocketFactory> registry =
poolingMgr.getSocketFactoryRegistry();
org.apache.http.conn.socket.ConnectionSocketFactory csf = registry.lookup("https");
if (csf instanceof SSLConnectionSocketFactory) {
return (SSLConnectionSocketFactory) csf;
}
}
return null;
} catch (NoSuchFieldException | IllegalAccessException e) {
fail("Failed to introspect HTTP client SSL configuration: " + e.getMessage());
return null; // unreachable, fail() throws AssertionError
}
}
private String[] getSupportedProtocols(SSLConnectionSocketFactory factory) {
try {
java.lang.reflect.Field field = SSLConnectionSocketFactory.class.getDeclaredField("supportedProtocols");
field.setAccessible(true);
return (String[]) field.get(factory);
} catch (NoSuchFieldException | IllegalAccessException e) {
fail("Failed to introspect SSLConnectionSocketFactory supportedProtocols: " + e.getMessage());
return null; // unreachable, fail() throws AssertionError
}
}
```
2. **Ensure the following imports (or their equivalents) exist at the top of the test file:**
```java
import org.apache.http.conn.HttpClientConnectionManager;
import org.apache.http.conn.socket.ConnectionSocketFactory;
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.InternalHttpClient;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
import org.apache.http.config.Lookup;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.fail;
```
These changes make the tests assert against the actual `SSLConnectionSocketFactory` configured by `GenericRestApiClient.setupHttpClient()`, verifying that its protocols are `null` (system defaults), while retaining the existing checks that ensure the JVM's default TLS configuration remains secure.
</issue_to_address>
### Comment 3
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="88-97" />
<code_context>
+ // Given: Verify GenericRestApiClient.setupHttpClient() works
</code_context>
<issue_to_address>
**suggestion:** Repeated reflective setup and resource management could be centralized for clarity
Most tests duplicate the same reflective `setupHttpClient` call and manual closing of `CloseableHttpClient`/`SSLSocket`, which hurts maintainability and risks inconsistent cleanup.
Consider centralizing this by adding a helper (or `@BeforeEach`/`@AfterEach`) that handles client creation via reflection and guarantees proper teardown (e.g., via try-with-resources), so individual tests focus solely on their TLS assertions while setup/teardown stays consistent and easy to update if `GenericRestApiClient` initialization changes.
Suggested implementation:
```java
@Test
@DisplayName("Should accept TLS 1.3 connections using GenericRestApiClient's configuration pattern")
void shouldAcceptTLS13ThroughClientConfiguration() throws Exception {
LOGGER.info("=== Testing TLS 1.3 Acceptance via Client Configuration Pattern ===");
// Given: Verify GenericRestApiClient.setupHttpClient() works via centralized helper
CloseableHttpClient httpClient = getHttpClientViaSetup();
// When: Test the same configuration pattern (null protocols) that GenericRestApiClient uses
SSLContext sslContext = SSLContexts.custom()
.loadTrustMaterial((chain, authType) -> true)
.build();
```
To fully centralize the reflective setup and teardown as suggested, you should also:
1. Add centralized reflective setup for `CloseableHttpClient`:
- Introduce a field in `GenericRestApiClientTLSTest`:
- `private Method setupHttpClientMethod;`
- `private CloseableHttpClient httpClient;`
- Add a `@BeforeEach` method:
```java
@BeforeEach
void setUpHttpClient() throws Exception {
setupHttpClientMethod = GenericRestApiClient.class.getDeclaredMethod("setupHttpClient");
setupHttpClientMethod.setAccessible(true);
httpClient = (CloseableHttpClient) setupHttpClientMethod.invoke(client);
assertNotNull(httpClient, "HTTP client should be created by GenericRestApiClient");
}
```
- Implement the helper used in the edited test:
```java
private CloseableHttpClient getHttpClientViaSetup() {
assertNotNull(httpClient, "HTTP client should be initialized in setUpHttpClient()");
return httpClient;
}
```
2. Add centralized teardown guaranteeing proper cleanup:
- Add a `@AfterEach` method:
```java
@AfterEach
void tearDownHttpClient() throws IOException {
if (httpClient != null) {
httpClient.close();
httpClient = null;
}
}
```
- If tests open `SSLSocket` instances directly, either:
- Use try-with-resources in each test, or
- Track them in a collection and close them in `@AfterEach`.
3. Update other tests in this class that currently:
- Reflectively obtain `setupHttpClient` and/or manually close the returned `CloseableHttpClient`,
- To instead use the shared `getHttpClientViaSetup()` helper and rely on `@AfterEach` for cleanup.
4. Ensure imports for lifecycle annotations if not already present:
```java
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.AfterEach;
```
These changes will remove duplicated reflective setup logic and ensure consistent resource management across all TLS tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
createTestFactoryhelper method inGenericRestApiClientTLSTestis never used; consider removing it or wiring it into the tests to avoid dead/unused code. - There is a lot of duplicated logic across the TLS 1.0/1.1 acceptance/rejection tests (socket setup, logging, assertions on enabled protocols); consider extracting common helpers or parameterizing the TLS version to make the tests shorter and easier to maintain.
- Protocol names like
"TLSv1","TLSv1.1", and"TLSv1.3"are hard-coded in many places; consider centralizing them as constants to reduce typo risk and ease future changes if supported protocol sets evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `createTestFactory` helper method in `GenericRestApiClientTLSTest` is never used; consider removing it or wiring it into the tests to avoid dead/unused code.
- There is a lot of duplicated logic across the TLS 1.0/1.1 acceptance/rejection tests (socket setup, logging, assertions on enabled protocols); consider extracting common helpers or parameterizing the TLS version to make the tests shorter and easier to maintain.
- Protocol names like `"TLSv1"`, `"TLSv1.1"`, and `"TLSv1.3"` are hard-coded in many places; consider centralizing them as constants to reduce typo risk and ease future changes if supported protocol sets evolve.
## Individual Comments
### Comment 1
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="397-271" />
<code_context>
+ SSLSocket socket = createSocketWithSystemDefaults(sslContext);
+ String[] protocols = socket.getEnabledProtocols();
+
+ LOGGER.info("System default protocols: {}", Arrays.toString(protocols));
+
+ // Then: Only modern TLS versions should be enabled
+ assertNotNull(protocols);
+ assertTrue(protocols.length >= 1, "Should have at least 1 protocol enabled");
+
+ List<String> protocolList = Arrays.asList(protocols);
+ assertTrue(protocolList.contains("TLSv1.3") || protocolList.contains("TLSv1.2"),
+ "Modern TLS (1.3 or 1.2) should be enabled by system defaults");
+ assertFalse(protocolList.contains("TLSv1.1"),
+ "TLS 1.1 should not be enabled by system defaults");
+ assertFalse(protocolList.contains("TLSv1"),
+ "TLS 1.0 should not be enabled by system defaults");
+
+ socket.close();
+ httpClient.close();
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Tests rely on specific system default protocol sets, which may cause brittleness across JDKs or environments.
These tests currently assert a specific TLS protocol set (no TLSv1/TLSv1.1, presence of TLSv1.2/1.3), which depends on the JDK version and local security configuration. On older or differently configured JVMs, they may fail even when `GenericRestApiClient` correctly defers to system defaults.
To reduce brittleness, consider:
- Relaxing expectations to focus on excluding legacy protocols only when the JDK supports doing so, or
- Guarding them with assumptions (e.g., Java version, security properties), or
- Asserting only that the client uses system defaults, without fixing the exact protocol list.
This keeps the tests portable while still validating the intended configuration behaviour.
Suggested implementation:
```java
SSLSocket socket = createSocketWithSystemDefaults(sslContext);
String[] enabledProtocols = socket.getEnabledProtocols();
String[] supportedProtocols = socket.getSupportedProtocols();
LOGGER.info("System default enabled protocols: {}", Arrays.toString(enabledProtocols));
LOGGER.info("System supported protocols: {}", Arrays.toString(supportedProtocols));
// Then: verify we have some protocols enabled and, where supported by the JDK, legacy
// protocols are not enabled. This focuses on excluding legacy protocols without
// asserting an exact modern TLS set, keeping the test portable across JDKs.
assertNotNull(enabledProtocols);
assertTrue(enabledProtocols.length >= 1, "Should have at least 1 protocol enabled");
List<String> enabledList = Arrays.asList(enabledProtocols);
List<String> supportedList = Arrays.asList(supportedProtocols);
// Only assert on legacy protocols if the JVM actually supports them; this avoids
// failures on JDKs where TLSv1/TLSv1.1 are fully disabled/removed.
if (supportedList.contains("TLSv1") || supportedList.contains("TLSv1.1")) {
assertFalse(enabledList.contains("TLSv1"),
"TLS 1.0 should not be enabled by system defaults when supported by the JDK");
assertFalse(enabledList.contains("TLSv1.1"),
"TLS 1.1 should not be enabled by system defaults when supported by the JDK");
}
socket.close();
httpClient.close();
}
```
1. Ensure `java.util.Arrays` and `java.util.List` are imported at the top of the file (if not already present).
2. If there are other tests that assert specific TLS protocol sets, consider aligning them with this pattern (log enabled/supported protocols, assert basic non-emptiness, and conditionally exclude legacy protocols based on `getSupportedProtocols()`).
</issue_to_address>
### Comment 2
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="74-80" />
<code_context>
+ * This allows us to verify that the configuration approach used by the client results in
+ * secure TLS settings.
+ */
+ private SSLConnectionSocketFactory createTestFactory(SSLContext sslContext) {
+ return new SSLConnectionSocketFactory(
+ sslContext,
+ null, // protocols - null delegates to system defaults (as GenericRestApiClient does)
+ null, // cipher suites
+ NoopHostnameVerifier.INSTANCE
+ );
+ }
+
</code_context>
<issue_to_address>
**nitpick:** Unused helper method `createTestFactory` adds noise and can be removed or used.
Since this helper isn’t referenced, either refactor the tests to use it (to explicitly cover the `null` protocols case) or remove it to avoid dead code and keep the test intent clear.
</issue_to_address>
### Comment 3
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="90-95" />
<code_context>
+ * @return The CloseableHttpClient created by GenericRestApiClient
+ * @throws Exception if reflection or client setup fails
+ */
+ private CloseableHttpClient setupAndVerifyHttpClient() throws Exception {
+ Method setupMethod = GenericRestApiClient.class.getDeclaredMethod("setupHttpClient");
+ setupMethod.setAccessible(true);
+ CloseableHttpClient httpClient = (CloseableHttpClient) setupMethod.invoke(client);
+ assertNotNull(httpClient, "HTTP client should be created by GenericRestApiClient");
+ return httpClient;
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Reflective access to `setupHttpClient` makes tests tightly coupled to private implementation details.
Using reflection here gives good coverage but also couples these tests to private internals, so minor refactors (renames, visibility or structural changes) may fail tests without changing behavior.
Consider either:
- Introducing a test seam (e.g., package-visible method, factory, or configuration hook), or
- Confining reflective usage to a small wiring-focused test, with other TLS tests exercising only public behavior.
This isn’t strictly blocking, but a change like this would improve long-term test maintainability as `GenericRestApiClient` evolves.
Suggested implementation:
```java
```
```java
import org.apache.http.conn.ssl.NoopHostnameVerifier;
```
1. Remove the `setupAndVerifyHttpClient()` helper method entirely from `GenericRestApiClientTLSTest` (its declaration and body), as it uses reflection on `setupHttpClient`.
2. Update any tests in this class that currently call `setupAndVerifyHttpClient()` to instead exercise only public behavior of `GenericRestApiClient` (e.g., by performing actual requests or inspecting TLS-related configuration exposed through public APIs).
3. If you still want a wiring-level check for `setupHttpClient`, introduce a separate, focused test class (e.g., `GenericRestApiClientWiringTest`) that contains the reflective access, keeping this TLS-focused test decoupled from private implementation details.
4. Optionally, if you decide to introduce a test seam later (such as a package-visible factory or hook in `GenericRestApiClient`), you can refactor the TLS tests to use that seam instead of reflection, but that would require changes in the production code as well.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Some tests (e.g.,
shouldPreferTLS13OverTLS12InClient) make assumptions about protocol ordering (protocols[0] == TLSv1.3) that may not hold across JDKs or configurations; consider asserting presence and relative preference in a less brittle way (e.g., checking index order only if both are present, or just that TLS 1.3 is enabled). - Several tests still use hard-coded protocol strings like "TLSv1.1" and "TLSv1" instead of the defined constants (
TLS_1_1,TLS_1_0); aligning all usages to the constants will avoid typos and make future updates easier. - The unit tests log a large amount of informational output for normal conditions; consider trimming logs or lowering log levels in non-failure cases to keep test output concise and focused on failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Some tests (e.g., `shouldPreferTLS13OverTLS12InClient`) make assumptions about protocol ordering (`protocols[0] == TLSv1.3`) that may not hold across JDKs or configurations; consider asserting presence and relative preference in a less brittle way (e.g., checking index order only if both are present, or just that TLS 1.3 is enabled).
- Several tests still use hard-coded protocol strings like "TLSv1.1" and "TLSv1" instead of the defined constants (`TLS_1_1`, `TLS_1_0`); aligning all usages to the constants will avoid typos and make future updates easier.
- The unit tests log a large amount of informational output for normal conditions; consider trimming logs or lowering log levels in non-failure cases to keep test output concise and focused on failures.
## Individual Comments
### Comment 1
<location path="src/test/java/com/autotune/utils/GenericRestApiClientTLSTest.java" line_range="165-168" />
<code_context>
+ LOGGER.info("JDK supported protocols: {}", Arrays.toString(supportedProtocols));
+
+ // Only run this test if the protocol is supported by the JDK
+ if (!supportedList.contains(protocol)) {
+ LOGGER.info("{} not supported by JDK - skipping test as it's already disabled at JDK level",
+ protocolDisplayName);
+ return;
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Use JUnit assumptions to explicitly mark unsupported protocols as skipped rather than silently passing.
Right now, if the JDK doesn’t support a legacy protocol, the helper logs and returns, so the test appears to pass even though the scenario isn’t actually run. Prefer using `Assumptions.assumeTrue(supportedList.contains(protocol), ...)` here so these cases are marked as skipped rather than passing when TLS 1.0/1.1 isn’t available on the runtime.
Suggested implementation:
```java
LOGGER.info("JDK supported protocols: {}", Arrays.toString(supportedProtocols));
// Only run this test if the protocol is supported by the JDK
Assumptions.assumeTrue(
supportedList.contains(protocol),
() -> protocolDisplayName + " not supported by JDK - skipping test as it's already disabled at JDK level"
);
```
```java
import org.apache.http.ssl.SSLContexts;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@chandrams [Edit] All the unit tests are run with Docker build using command: |
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
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
2 participants
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
Add unit tests
GenericRestApiClientTLSTest.javafor TLS protocol validation inGenericRestApiClientto ensure secure communication standards.Updates
HttpResponseWrapperto a static inner class.Tests use real SSLSocket instances (not mocks) configured with system default protocols to validate that: modern TLS is enabled, legacy TLS is excluded, and handshake simulations confirm protocol mismatch scenarios. This mirrors the production code's approach of passing null for protocols to delegate to JVM system defaults.
Test Cases:
Unit tests output:
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Tests:
Summary by Sourcery
Add TLS protocol validation coverage for GenericRestApiClient and adjust response wrapper accessibility to support the new tests.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Add TLS protocol validation tests for GenericRestApiClient to ensure it relies on secure system default TLS settings and rejects legacy protocols.
Enhancements:
Tests: