From 240148d3393bbf96f8b0b34e99d590c11e2612d4 Mon Sep 17 00:00:00 2001 From: Dmitry Kropachev Date: Wed, 19 Aug 2026 14:20:18 -0400 Subject: [PATCH 1/2] fix: report TLS from active channel handlers Derive TLS presence from the control channel pipeline after NettyOptions customization, so handlers added or removed by custom hooks are reported accurately. Compatibility note: this intentionally changes DriverConfigReporter, which is part of the explicitly unstable internal API. Keeping the channel-less contract would preserve an entry point that cannot report effective per-connection TLS state; custom internal reporters must be recompiled. --- .../core/channel/ProtocolInitHandler.java | 4 +- .../context/DefaultDriverConfigReporter.java | 58 +++++++----- .../core/context/DriverConfigReporter.java | 13 ++- .../context/NoopDriverConfigReporter.java | 5 +- .../core/channel/ChannelFactoryTestBase.java | 2 +- .../core/channel/ProtocolInitHandlerTest.java | 7 +- .../DefaultDriverConfigReporterTest.java | 92 ++++++++++++++++--- 7 files changed, 133 insertions(+), 48 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java index dd7630a6530..3800434873a 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java @@ -197,7 +197,9 @@ Message getRequest() { // SESSION_ID that every connection already carries from context.getStartupOptions(). // No-op when driver config reporting is disabled. if (options.reportConfig) { - context.getDriverConfigReporter().populateControlConnectionOptions(startupOptions); + context + .getDriverConfigReporter() + .populateControlConnectionOptions(startupOptions, ctx.channel()); } return request = new Startup(startupOptions); case GET_CLUSTER_NAME: diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java index f48d686573c..a6a35fd80f9 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java @@ -50,7 +50,10 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; +import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; +import io.netty.channel.Channel; +import io.netty.handler.ssl.SslHandler; import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.Map; @@ -189,8 +192,8 @@ * cross-driver schema doesn't define; this is a known gap, not an oversight. * *

Thread safety: this class is safe to use as shipped, and holds no mutable state. Note - * that {@code buildJson()} runs on every control-connection (re)initialization, and may be called - * concurrently with a reconnect racing a fresh session start. + * that {@code buildJson(Channel)} runs on every control-connection (re)initialization, and may be + * called concurrently with a reconnect racing a fresh session start. */ @ThreadSafe public class DefaultDriverConfigReporter implements DriverConfigReporter { @@ -231,7 +234,8 @@ public DefaultDriverConfigReporter(InternalDriverContext context) { } @Override - public void populateControlConnectionOptions(Map startupOptions) { + public void populateControlConnectionOptions( + @NonNull Map startupOptions, @NonNull Channel channel) { // Configuration reporting is a best-effort diagnostic aid: it runs on the connection // initialization path, so any failure here (a bad config read, a misbehaving policy while // introspecting, a serialization error) must be swallowed rather than allowed to break the @@ -247,7 +251,7 @@ public void populateControlConnectionOptions(Map startupOptions) if (!isEnabled()) { return; } - String json = buildJson(); + String json = buildJson(channel); if (json == null) { return; } @@ -288,22 +292,23 @@ private boolean isEnabled() { * class's to enforce: a future change to session bootstrap that dropped one of those from the * eager list would quietly reintroduce that. * - *

The configured SSL engine factory is deliberately not among them: {@link #tls()} - * reads the engine factory held by the {@code JdkSslHandlerFactory} in force rather than the one - * behind {@code getSslEngineFactory()}. Those can differ — a context that overrides {@code - * buildSslHandlerFactory()} may wrap an engine factory of its own — and going through the context - * would both describe an engine nothing on the connection path uses and risk being the first - * caller to resolve it, which for the built-in factory means reading keystore/truststore files on - * a Netty event-loop thread (and failing the whole report if that throws). + *

The configured SSL engine factory is deliberately not among them: {@link + * #tls(Channel)} reads the engine factory held by the {@code JdkSslHandlerFactory} in force + * rather than the one behind {@code getSslEngineFactory()}. Those can differ — a context that + * overrides {@code buildSslHandlerFactory()} may wrap an engine factory of its own — and going + * through the context would both describe an engine nothing on the connection path uses and risk + * being the first caller to resolve it, which for the built-in factory means reading + * keystore/truststore files on a Netty event-loop thread (and failing the whole report if that + * throws). * * @return the report, or {@code null} if it could not be serialized — in which case {@code * DRIVER_CONFIG} is skipped rather than the connection failed. */ @Nullable - String buildJson() { + String buildJson(Channel channel) { ObjectNode root = OBJECT_MAPPER.createObjectNode(); root.put("version", SCHEMA_VERSION); - populateConfig(root, context.getConfig().getDefaultProfile()); + populateConfig(root, context.getConfig().getDefaultProfile(), channel); try { return OBJECT_MAPPER.writeValueAsString(root); } catch (JsonProcessingException e) { @@ -318,14 +323,14 @@ String buildJson() { * plus the context's policies. Each group follows the cross-driver schema; a key the Java driver * has no equivalent for is omitted rather than reported as {@code null}. */ - private void populateConfig(ObjectNode root, DriverExecutionProfile config) { + private void populateConfig(ObjectNode root, DriverExecutionProfile config, Channel channel) { // Resolved once and shared: the load balancing policy decides both its own group and the // node-location preferences reported under two different parents, and resolving it twice would // mean a second SPI lookup on the Netty event-loop thread that is building STARTUP. LoadBalancingPolicy loadBalancingPolicy = context.getLoadBalancingPolicy(DriverExecutionProfile.DEFAULT_NAME); NodeLocation nodeLocation = nodeLocation(config, loadBalancingPolicy); - root.set("connection", connection(config, nodeLocation)); + root.set("connection", connection(config, nodeLocation, channel)); root.set("control-plane", controlPlane(config)); root.set("query", query(config, loadBalancingPolicy, nodeLocation)); } @@ -335,7 +340,7 @@ private void populateConfig(ObjectNode root, DriverExecutionProfile config) { * top of it, how it is re-established, and which part of the cluster gets one at all. */ private ObjectNode connection( - DriverExecutionProfile config, @Nullable NodeLocation nodeLocation) { + DriverExecutionProfile config, @Nullable NodeLocation nodeLocation, Channel channel) { ObjectNode n = connectionTimeouts(config); n.set("socket", socket(config)); ObjectNode reconnection = OBJECT_MAPPER.createObjectNode(); @@ -343,7 +348,7 @@ private ObjectNode connection( n.set("reconnection", reconnection); // Optional, and absent rather than false when off: presence of the group is what says TLS is // enabled, since the schema dropped the boolean that used to carry it. - ObjectNode tls = tls(); + ObjectNode tls = tls(channel); if (tls != null) { n.set("tls", tls); } @@ -1078,15 +1083,16 @@ private static Optional clientTimestamps(TimestampGenerator generator) * so presence of the group is what reports that it is on. */ @Nullable - private ObjectNode tls() { - // TLS is on exactly when the channel pipeline gets an SSL handler, which ChannelFactory decides - // from the low-level SslHandlerFactory. Deliberately not getSslEngineFactory(): that is only - // the public JDK-based path that DefaultDriverContext.buildSslHandlerFactory() wraps, and an + private ObjectNode tls(Channel channel) { + // TLS is on exactly when the channel pipeline has an SSL handler. ChannelFactory installs one + // from the low-level SslHandlerFactory before NettyOptions.afterChannelInitialized(), but that + // hook may add, replace, or remove handlers; report the pipeline left in force rather than the + // factory's configuration intent. Deliberately not getSslEngineFactory(): that is only the + // public JDK-based path that DefaultDriverContext.buildSslHandlerFactory() wraps, and an // override of that method (the documented expert extension point, e.g. Netty's native OpenSSL) // supplies a handler factory with no engine factory at all — a session that is encrypted all // the same. - Optional handlerFactory = context.getSslHandlerFactory(); - if (!handlerFactory.isPresent()) { + if (channel.pipeline().get(SslHandler.class) == null) { return null; } ObjectNode n = OBJECT_MAPPER.createObjectNode(); @@ -1105,8 +1111,10 @@ private ObjectNode tls() { // SessionBuilder.withSslContext(...) (ProgrammaticSslEngineFactory) validates only if // explicitly asked to (default off) regardless of that option, so reading the option here would // falsely report validation as on when it isn't. - SslHandlerFactory factory = handlerFactory.get(); - if (factory.getClass() == JdkSslHandlerFactory.class) { + Optional handlerFactory = context.getSslHandlerFactory(); + if (handlerFactory.isPresent() + && handlerFactory.get().getClass() == JdkSslHandlerFactory.class) { + SslHandlerFactory factory = handlerFactory.get(); SslEngineFactory engineFactory = ((JdkSslHandlerFactory) factory).getSslEngineFactory(); hostnameValidation(engineFactory).ifPresent(v -> n.put("hostname-verification", v)); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java index bbabe2c8b3f..4731e9a7e84 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java @@ -17,6 +17,8 @@ */ package com.datastax.oss.driver.internal.core.context; +import edu.umd.cs.findbugs.annotations.NonNull; +import io.netty.channel.Channel; import java.util.Map; /** @@ -43,8 +45,13 @@ public interface DriverConfigReporter { * failure to build the report must be swallowed (and logged) rather than propagated, otherwise it * would prevent the session from establishing or reconnecting. * - *

The report describes the driver's own configuration only, so nothing here depends on which - * backend answered: it can be built before the connection learns anything about its peer. + *

The report describes the driver's own configuration and the effective SSL state of the + * control connection. It does not depend on which backend answered, but the SSL handler must + * already be installed on {@code channel}. + * + * @param startupOptions startup options to add the report to + * @param channel control connection whose effective SSL state is reported */ - void populateControlConnectionOptions(Map startupOptions); + void populateControlConnectionOptions( + @NonNull Map startupOptions, @NonNull Channel channel); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java index 213c3657585..36500ad50fa 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java @@ -17,6 +17,8 @@ */ package com.datastax.oss.driver.internal.core.context; +import edu.umd.cs.findbugs.annotations.NonNull; +import io.netty.channel.Channel; import java.util.Map; import net.jcip.annotations.ThreadSafe; @@ -41,7 +43,8 @@ public class NoopDriverConfigReporter implements DriverConfigReporter { @Override - public void populateControlConnectionOptions(Map startupOptions) { + public void populateControlConnectionOptions( + @NonNull Map startupOptions, @NonNull Channel channel) { // nothing to do } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java index ed6668a6c83..8ec6b85e342 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java @@ -142,7 +142,7 @@ public void setup() throws InterruptedException { when(context.getWriteCoalescer()).thenReturn(new PassThroughWriteCoalescer(null)); when(context.getCompressor()).thenReturn(compressor); // The init handler consults the config reporter for the control connection; default to a no-op. - when(context.getDriverConfigReporter()).thenReturn(startupOptions -> {}); + when(context.getDriverConfigReporter()).thenReturn((startupOptions, controlChannel) -> {}); // Start local server ServerBootstrap serverBootstrap = diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java index 682caac198d..b76ed857055 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java @@ -107,7 +107,8 @@ public void setup() { .thenReturn(Duration.ofSeconds(30)); when(internalDriverContext.getProtocolVersionRegistry()).thenReturn(protocolVersionRegistry); // The init handler consults the config reporter for the control connection; default to a no-op. - when(internalDriverContext.getDriverConfigReporter()).thenReturn(startupOptions -> {}); + when(internalDriverContext.getDriverConfigReporter()) + .thenReturn((startupOptions, controlChannel) -> {}); channel .pipeline() @@ -163,7 +164,7 @@ public void should_initialize() { private void stubConfigReporter() { when(internalDriverContext.getDriverConfigReporter()) .thenReturn( - startupOptions -> + (startupOptions, controlChannel) -> startupOptions.put( DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}")); } @@ -216,7 +217,7 @@ public void should_not_consult_the_config_reporter_on_pool_connection() { assertThat(requestFrame.message).isInstanceOf(Startup.class); Startup startup = (Startup) requestFrame.message; assertThat(startup.options).doesNotContainKey(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY); - verify(reporter, never()).populateControlConnectionOptions(any()); + verify(reporter, never()).populateControlConnectionOptions(any(), any()); } @Test diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java index 4122f82a399..05b78ae4b5c 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java @@ -65,6 +65,9 @@ import com.networknt.schema.ValidationMessage; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; +import io.netty.channel.Channel; +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.handler.ssl.SslHandler; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.time.Duration; @@ -75,6 +78,8 @@ import java.util.function.Consumer; import java.util.function.Supplier; import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -107,6 +112,7 @@ private static JsonSchema loadSchema() { private InternalDriverContext mockContext; private DriverExecutionProfile mockProfile; private DefaultDriverConfigReporter reporter; + private EmbeddedChannel reportingChannel; @Before public void setup() { @@ -116,6 +122,12 @@ public void setup() { when(mockContext.getConfig()).thenReturn(config); when(config.getDefaultProfile()).thenReturn(mockProfile); reporter = new DefaultDriverConfigReporter(mockContext); + reportingChannel = new EmbeddedChannel(); + } + + @After + public void cleanup() { + reportingChannel.finishAndReleaseAll(); } private void enableReporting(boolean enabled) { @@ -127,7 +139,7 @@ private void enableReporting(boolean enabled) { private DefaultDriverConfigReporter reporterReporting(Supplier json) { return new DefaultDriverConfigReporter(mockContext) { @Override - String buildJson() { + String buildJson(Channel channel) { return json.get(); } }; @@ -142,7 +154,8 @@ String buildJson() { public void should_add_driver_config_when_enabled() { enableReporting(true); Map options = new HashMap<>(); - reporterReporting(() -> "{\"version\":1}").populateControlConnectionOptions(options); + reporterReporting(() -> "{\"version\":1}") + .populateControlConnectionOptions(options, reportingChannel); assertThat(options) .hasSize(1) .containsEntry(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}"); @@ -152,7 +165,7 @@ public void should_add_driver_config_when_enabled() { public void should_add_nothing_when_disabled() { enableReporting(false); Map options = new HashMap<>(); - reporter.populateControlConnectionOptions(options); + reporter.populateControlConnectionOptions(options, reportingChannel); assertThat(options).isEmpty(); } @@ -163,7 +176,7 @@ public void should_add_driver_config_when_the_option_is_not_defined() { // getBoolean(), ignoring the fallback that is under test here. Map options = new HashMap<>(); defaultsReporter(map -> map.remove(TypedDriverOption.DRIVER_CONFIG_REPORTING_ENABLED)) - .populateControlConnectionOptions(options); + .populateControlConnectionOptions(options, reportingChannel); assertThat(options).containsKey(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY); } @@ -176,7 +189,7 @@ public void should_report_nothing_at_all_without_jackson() { // in-process; what is checked here is that the substitute contributes nothing and, in // particular, does not need a context to say so. Map options = new HashMap<>(); - new NoopDriverConfigReporter().populateControlConnectionOptions(options); + new NoopDriverConfigReporter().populateControlConnectionOptions(options, reportingChannel); assertThat(options).isEmpty(); } @@ -187,7 +200,7 @@ public void should_not_throw_when_reading_the_flag_fails() { when(mockProfile.getBoolean(DefaultDriverOption.DRIVER_CONFIG_REPORTING_ENABLED, true)) .thenThrow(new IllegalStateException("config blew up")); Map options = new HashMap<>(); - reporter.populateControlConnectionOptions(options); // must not throw + reporter.populateControlConnectionOptions(options, reportingChannel); // must not throw assertThat(options).isEmpty(); } @@ -199,7 +212,7 @@ public void should_skip_driver_config_when_building_fails() { () -> { throw new IllegalStateException("introspection blew up"); }) - .populateControlConnectionOptions(options); // must not throw + .populateControlConnectionOptions(options, reportingChannel); // must not throw assertThat(options).isEmpty(); } @@ -208,7 +221,7 @@ public void should_skip_driver_config_when_serialization_fails() { // buildJson() returns null when Jackson fails to serialize the node tree. enableReporting(true); Map options = new HashMap<>(); - reporterReporting(() -> null).populateControlConnectionOptions(options); + reporterReporting(() -> null).populateControlConnectionOptions(options, reportingChannel); assertThat(options).isEmpty(); } @@ -285,7 +298,7 @@ public void should_skip_driver_config_when_it_exceeds_the_size_limit() { enableReporting(true); Map options = new HashMap<>(); reporterReporting(() -> oversizedReport()) - .populateControlConnectionOptions(options); // must not throw + .populateControlConnectionOptions(options, reportingChannel); // must not throw assertThat(options).isEmpty(); } @@ -294,7 +307,7 @@ public void should_add_driver_config_that_is_just_within_the_size_limit() { enableReporting(true); Map options = new HashMap<>(); String atLimit = padTo(DefaultDriverConfigReporter.MAX_DRIVER_CONFIG_LENGTH); - reporterReporting(() -> atLimit).populateControlConnectionOptions(options); + reporterReporting(() -> atLimit).populateControlConnectionOptions(options, reportingChannel); assertThat(options).containsEntry(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, atLimit); } @@ -314,13 +327,13 @@ public void should_skip_a_report_a_configuration_pushes_over_the_size_limit() th // Built, well-formed and over the limit: it is dropped for its size, not because building it // failed. Reporting is left at the shipped default here, since defaultsReporter() reads a real // profile rather than the bare mock the tests above use. - String json = reporter.buildJson(); + String json = reporter.buildJson(reportingChannel); assertConformsToSchema(MAPPER.readTree(json)); assertThat(json.getBytes(StandardCharsets.UTF_8).length) .isGreaterThan(DefaultDriverConfigReporter.MAX_DRIVER_CONFIG_LENGTH); Map options = new HashMap<>(); - reporter.populateControlConnectionOptions(options); + reporter.populateControlConnectionOptions(options, reportingChannel); assertThat(options).isEmpty(); } @@ -1504,6 +1517,42 @@ public void should_report_tls_enabled_for_a_custom_ssl_handler_factory() throws assertConformsToSchema(report); } + @Test + public void should_not_report_tls_when_the_configured_ssl_handler_was_removed() throws Exception { + SslEngineFactory factory = new ProgrammaticSslEngineFactory(SSLContext.getDefault()); + DefaultDriverConfigReporter r = + reporterWith( + defaults(map -> {}), + exponentialReconnection(), + mock(DefaultRetryPolicy.class), + mock(NoSpeculativeExecutionPolicy.class), + loadBalancing(DefaultLoadBalancingPolicy.class), + clientSideGenerator(), + Optional.of(factory)); + + reportingChannel.pipeline().remove(SslHandler.class); + + assertThat(report(r).get("connection").has("tls")).isFalse(); + } + + @Test + public void should_report_tls_when_a_pipeline_hook_added_an_ssl_handler() throws Exception { + reportingChannel.pipeline().addLast(clientSslHandler()); + DefaultDriverConfigReporter r = + reporterWith( + defaults(map -> {}), + exponentialReconnection(), + mock(DefaultRetryPolicy.class), + mock(NoSpeculativeExecutionPolicy.class), + loadBalancing(DefaultLoadBalancingPolicy.class), + clientSideGenerator(), + Optional.empty()); + + JsonNode tls = report(r).get("connection").get("tls"); + assertThat(tls).isNotNull(); + assertThat(tls.has("hostname-verification")).isFalse(); + } + @Test public void should_omit_hostname_verification_for_an_unrecognized_engine_factory() throws Exception { @@ -1620,7 +1669,9 @@ public void should_not_resolve_the_configured_engine_factory_at_all() throws Exc when(ctx.getSslEngineFactory()) .thenThrow(new AssertionError("the configured engine factory must not be resolved")); - JsonNode report = MAPPER.readTree(new DefaultDriverConfigReporter(ctx).buildJson()); + reportingChannel.pipeline().addLast(clientSslHandler()); + JsonNode report = + MAPPER.readTree(new DefaultDriverConfigReporter(ctx).buildJson(reportingChannel)); // The group is built from the wrapped engine factory alone; getSslEngineFactory() throwing // proves it was never consulted. assertThat(report.get("connection").get("tls").get("hostname-verification").asBoolean()) @@ -2595,7 +2646,7 @@ private static TimestampGenerator clientSideGenerator() { } private JsonNode report(DefaultDriverConfigReporter reporter) throws Exception { - return MAPPER.readTree(reporter.buildJson()); + return MAPPER.readTree(reporter.buildJson(reportingChannel)); } /** A real default execution profile with the given customizations applied. */ @@ -2667,6 +2718,9 @@ private DefaultDriverConfigReporter reporterWith( Optional ssl, Optional sslHandler, String programmaticLocalDc) { + if (sslHandler.isPresent() && reportingChannel.pipeline().get(SslHandler.class) == null) { + reportingChannel.pipeline().addLast(clientSslHandler()); + } return new DefaultDriverConfigReporter( contextWith( profile, @@ -2680,6 +2734,16 @@ private DefaultDriverConfigReporter reporterWith( programmaticLocalDc)); } + private static SslHandler clientSslHandler() { + try { + SSLEngine engine = SSLContext.getDefault().createSSLEngine(); + engine.setUseClientMode(true); + return new SslHandler(engine); + } catch (Exception e) { + throw new AssertionError("Could not create test SSL handler", e); + } + } + /** * The context the reporters above read from. Separate from {@link #reporterWith} only so that a * test can build a reporter subclass over it, the way the {@code simpleName} seam needs. From 88b4976e4c130de032cdc7a35d828279b74d8fb6 Mon Sep 17 00:00:00 2001 From: Dmitry Kropachev Date: Fri, 21 Aug 2026 10:33:27 -0400 Subject: [PATCH 2/2] fix: report TLS from active SSL engine --- .../api/core/config/DefaultDriverOption.java | 6 +- .../core/channel/ProtocolInitHandler.java | 43 +++- .../context/DefaultDriverConfigReporter.java | 130 +++--------- .../core/context/DriverConfigReporter.java | 59 +++++- .../context/NoopDriverConfigReporter.java | 3 +- .../channel/ChannelFactoryPipelineTest.java | 120 +++++++++++ .../core/channel/ChannelFactoryTestBase.java | 2 +- .../core/channel/ProtocolInitHandlerTest.java | 66 +++++- .../DefaultDriverConfigReporterTest.java | 200 +++++------------- upgrade_guide/README.md | 5 +- 10 files changed, 350 insertions(+), 284 deletions(-) create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPipelineTest.java diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java index dd60a2487fb..babeffffdcf 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java @@ -1184,9 +1184,9 @@ public enum DefaultDriverOption implements DriverOption { * system_views.clients} on Cassandra 4.1+) so operators can inspect driver settings while * investigating incidents. It describes the effective configuration of the driver's default * execution profile (connection/socket settings, timeouts, retry/reconnection/ - * speculative-execution/load-balancing policies, connection pooling, query defaults, and TLS). - * Only the control connection sends it, since it describes the whole session. When {@code false}, - * {@code DRIVER_CONFIG} is not sent. + * speculative-execution/load-balancing policies, connection pooling, and query defaults), plus + * the effective TLS state of the control connection carrying it. Only the control connection + * sends it. When {@code false}, {@code DRIVER_CONFIG} is not sent. * *

This option governs {@code DRIVER_CONFIG} only. The {@code SESSION_ID} startup option, which * lets the server group all of a session's connections, is an innate driver behavior: it is sent diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java index 3800434873a..d7e9bd06c1d 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java @@ -35,6 +35,7 @@ import com.datastax.oss.driver.api.core.metadata.EndPoint; import com.datastax.oss.driver.api.core.type.codec.TypeCodecs; import com.datastax.oss.driver.internal.core.DefaultProtocolFeature; +import com.datastax.oss.driver.internal.core.context.DriverConfigReporter.TlsInfo; import com.datastax.oss.driver.internal.core.context.InternalDriverContext; import com.datastax.oss.driver.internal.core.protocol.BytesToSegmentDecoder; import com.datastax.oss.driver.internal.core.protocol.FrameDecoder; @@ -62,8 +63,10 @@ import com.datastax.oss.protocol.internal.response.Supported; import com.datastax.oss.protocol.internal.response.result.Rows; import com.datastax.oss.protocol.internal.response.result.SetKeyspace; +import io.netty.channel.Channel; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelPipeline; +import io.netty.handler.ssl.SslHandler; import java.nio.ByteBuffer; import java.util.HashMap; import java.util.List; @@ -192,14 +195,14 @@ Message getRequest() { case STARTUP: Map startupOptions = new HashMap<>(context.getStartupOptions()); featureStore.populateStartupOptions(startupOptions); - // The DRIVER_CONFIG blob describes the whole session, so only the control connection - // carries it (options.reportConfig); the other connections are correlated to it by the - // SESSION_ID that every connection already carries from context.getStartupOptions(). - // No-op when driver config reporting is disabled. + // Most of DRIVER_CONFIG describes the whole session; its TLS group describes the control + // connection carrying it. Other connections are correlated to it by the SESSION_ID that + // every connection already carries from context.getStartupOptions(). No-op when driver + // config reporting is disabled. if (options.reportConfig) { context .getDriverConfigReporter() - .populateControlConnectionOptions(startupOptions, ctx.channel()); + .populateControlConnectionOptions(startupOptions, currentTlsInfo()); } return request = new Startup(startupOptions); case GET_CLUSTER_NAME: @@ -215,6 +218,21 @@ Message getRequest() { } } + private TlsInfo currentTlsInfo() { + try { + return tlsInfo(channel); + } catch (RuntimeException e) { + // Configuration reporting is best-effort and must never prevent a connection. TLS presence + // is still assumed because the failure came while inspecting its handler, but the schema + // permits hostname-verification to be omitted when unknown. + LOG.warn( + "[{}] Could not inspect hostname verification on the active SSL engine; omitting it", + logPrefix, + e); + return TlsInfo.enabledWithUnknownHostnameVerification(); + } + } + @Override void send() { stepNumber++; @@ -418,6 +436,21 @@ public String toString() { } } + static TlsInfo tlsInfo(Channel channel) { + // SslHandlerFactory always returns SslHandler, and this reads the pipeline after + // NettyOptions.afterChannelInitialized() has had a chance to add, replace, remove, or + // reconfigure it. A hook that implements encryption with a handler unrelated to SslHandler + // cannot be identified generically and is therefore reported as TLS-disabled. + SslHandler sslHandler = channel.pipeline().get(SslHandler.class); + if (sslHandler == null) { + return TlsInfo.disabled(); + } + String endpointIdentificationAlgorithm = + sslHandler.engine().getSSLParameters().getEndpointIdentificationAlgorithm(); + return TlsInfo.enabled( + endpointIdentificationAlgorithm != null && !endpointIdentificationAlgorithm.isEmpty()); + } + /** * Conditionally rebuilds pipeline. * diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java index a6a35fd80f9..d90a2e7fea0 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.java @@ -26,8 +26,6 @@ import com.datastax.oss.driver.api.core.loadbalancing.LoadBalancingPolicy; import com.datastax.oss.driver.api.core.retry.RetryPolicy; import com.datastax.oss.driver.api.core.specex.SpeculativeExecutionPolicy; -import com.datastax.oss.driver.api.core.ssl.ProgrammaticSslEngineFactory; -import com.datastax.oss.driver.api.core.ssl.SslEngineFactory; import com.datastax.oss.driver.api.core.time.TimestampGenerator; import com.datastax.oss.driver.internal.core.channel.ChannelFactory; import com.datastax.oss.driver.internal.core.connection.ConstantReconnectionPolicy; @@ -39,10 +37,6 @@ import com.datastax.oss.driver.internal.core.retry.DefaultRetryPolicy; import com.datastax.oss.driver.internal.core.specex.ConstantSpeculativeExecutionPolicy; import com.datastax.oss.driver.internal.core.specex.NoSpeculativeExecutionPolicy; -import com.datastax.oss.driver.internal.core.ssl.DefaultSslEngineFactory; -import com.datastax.oss.driver.internal.core.ssl.JdkSslHandlerFactory; -import com.datastax.oss.driver.internal.core.ssl.SniSslEngineFactory; -import com.datastax.oss.driver.internal.core.ssl.SslHandlerFactory; import com.datastax.oss.driver.internal.core.time.AtomicTimestampGenerator; import com.datastax.oss.driver.internal.core.time.ServerSideTimestampGenerator; import com.datastax.oss.driver.internal.core.time.ThreadLocalTimestampGenerator; @@ -52,8 +46,6 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; -import io.netty.channel.Channel; -import io.netty.handler.ssl.SslHandler; import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.Map; @@ -78,8 +70,8 @@ * {@code connection.reconnection.policy}, {@code query.speculative-execution.policy}, {@code * query.load-balancing.policy.adaptive-ordering}, the first term of {@code * fallback-to-non-preferred-nodes}, both {@code node-preference} groups, and {@code - * connection.tls.hostname-verification} off the active handler factory rather than the configured - * engine factory. + * connection.tls.hostname-verification} from an immutable snapshot of the active control-channel + * engine rather than the configured engine factory. * *

Anything added here owes the same question: find what consumes the option, and if it stores * the value rather than re-reading it, expose an accessor and read that. Every field that got this @@ -107,13 +99,10 @@ * basic.request.serial-consistency} outside the schema's two serial levels and the like are omitted * rather than emitted as a value the schema rejects. Two optional booleans are omitted for a third * reason — the answer is genuinely unknown, which is the only thing the schema lets their absence - * mean: {@code connection.tls.hostname-verification} when the SSL handler or engine factory in - * force is not one this class recognizes, and {@code query.defaults.client-timestamps} when the - * timestamp generator is not (see {@link #hostnameValidation} and {@link #clientTimestamps}). - * Guessing a boolean there would describe a security control, or a write-timestamp source, that may - * well be the opposite — which is also why neither is asked of the SPI itself: an accessor on - * {@link SslEngineFactory} or {@link TimestampGenerator} would have needed a default, and a default - * answer is exactly the guess being avoided. + * mean: {@code connection.tls.hostname-verification} when the active SSL engine could not expose + * it, and {@code query.defaults.client-timestamps} when the timestamp generator is not one this + * class recognizes (see {@link #tls} and {@link #clientTimestamps}). Guessing a boolean there would + * describe a security control, or a write-timestamp source, that may well be the opposite. * *

A new field owes three checks, each of which this class has already got wrong once and * each of which is cheap to run before review does it for you: @@ -192,8 +181,8 @@ * cross-driver schema doesn't define; this is a known gap, not an oversight. * *

Thread safety: this class is safe to use as shipped, and holds no mutable state. Note - * that {@code buildJson(Channel)} runs on every control-connection (re)initialization, and may be - * called concurrently with a reconnect racing a fresh session start. + * that {@code buildJson(DriverConfigReporter.TlsInfo)} runs on every control-connection + * (re)initialization, and may be called concurrently with a reconnect racing a fresh session start. */ @ThreadSafe public class DefaultDriverConfigReporter implements DriverConfigReporter { @@ -235,7 +224,7 @@ public DefaultDriverConfigReporter(InternalDriverContext context) { @Override public void populateControlConnectionOptions( - @NonNull Map startupOptions, @NonNull Channel channel) { + @NonNull Map startupOptions, @NonNull TlsInfo tlsInfo) { // Configuration reporting is a best-effort diagnostic aid: it runs on the connection // initialization path, so any failure here (a bad config read, a misbehaving policy while // introspecting, a serialization error) must be swallowed rather than allowed to break the @@ -251,7 +240,7 @@ public void populateControlConnectionOptions( if (!isEnabled()) { return; } - String json = buildJson(channel); + String json = buildJson(tlsInfo); if (json == null) { return; } @@ -293,22 +282,19 @@ private boolean isEnabled() { * eager list would quietly reintroduce that. * *

The configured SSL engine factory is deliberately not among them: {@link - * #tls(Channel)} reads the engine factory held by the {@code JdkSslHandlerFactory} in force - * rather than the one behind {@code getSslEngineFactory()}. Those can differ — a context that - * overrides {@code buildSslHandlerFactory()} may wrap an engine factory of its own — and going - * through the context would both describe an engine nothing on the connection path uses and risk - * being the first caller to resolve it, which for the built-in factory means reading - * keystore/truststore files on a Netty event-loop thread (and failing the whole report if that - * throws). + * #tls(DriverConfigReporter.TlsInfo)} reads the immutable snapshot captured from the active + * control-channel handler. Resolving the configured factory here could describe an engine the + * connection does not use and, for the built-in factory, read keystore/truststore files on a + * Netty event-loop thread. * * @return the report, or {@code null} if it could not be serialized — in which case {@code * DRIVER_CONFIG} is skipped rather than the connection failed. */ @Nullable - String buildJson(Channel channel) { + String buildJson(TlsInfo tlsInfo) { ObjectNode root = OBJECT_MAPPER.createObjectNode(); root.put("version", SCHEMA_VERSION); - populateConfig(root, context.getConfig().getDefaultProfile(), channel); + populateConfig(root, context.getConfig().getDefaultProfile(), tlsInfo); try { return OBJECT_MAPPER.writeValueAsString(root); } catch (JsonProcessingException e) { @@ -323,14 +309,14 @@ String buildJson(Channel channel) { * plus the context's policies. Each group follows the cross-driver schema; a key the Java driver * has no equivalent for is omitted rather than reported as {@code null}. */ - private void populateConfig(ObjectNode root, DriverExecutionProfile config, Channel channel) { + private void populateConfig(ObjectNode root, DriverExecutionProfile config, TlsInfo tlsInfo) { // Resolved once and shared: the load balancing policy decides both its own group and the // node-location preferences reported under two different parents, and resolving it twice would // mean a second SPI lookup on the Netty event-loop thread that is building STARTUP. LoadBalancingPolicy loadBalancingPolicy = context.getLoadBalancingPolicy(DriverExecutionProfile.DEFAULT_NAME); NodeLocation nodeLocation = nodeLocation(config, loadBalancingPolicy); - root.set("connection", connection(config, nodeLocation, channel)); + root.set("connection", connection(config, nodeLocation, tlsInfo)); root.set("control-plane", controlPlane(config)); root.set("query", query(config, loadBalancingPolicy, nodeLocation)); } @@ -340,7 +326,7 @@ private void populateConfig(ObjectNode root, DriverExecutionProfile config, Chan * top of it, how it is re-established, and which part of the cluster gets one at all. */ private ObjectNode connection( - DriverExecutionProfile config, @Nullable NodeLocation nodeLocation, Channel channel) { + DriverExecutionProfile config, @Nullable NodeLocation nodeLocation, TlsInfo tlsInfo) { ObjectNode n = connectionTimeouts(config); n.set("socket", socket(config)); ObjectNode reconnection = OBJECT_MAPPER.createObjectNode(); @@ -348,7 +334,7 @@ private ObjectNode connection( n.set("reconnection", reconnection); // Optional, and absent rather than false when off: presence of the group is what says TLS is // enabled, since the schema dropped the boolean that used to carry it. - ObjectNode tls = tls(channel); + ObjectNode tls = tls(tlsInfo); if (tls != null) { n.set("tls", tls); } @@ -1064,9 +1050,9 @@ private ObjectNode queryDefaults(DriverExecutionProfile config) { * which both of them extend, is package-private, so nothing outside its own package can inherit * its behavior without going through one of these two. * - *

{@code instanceof}, not the exact-class checks the policy branches use, for the same reason - * as in {@link #hostnameValidation}: this reads a property the generator has rather than deciding - * which built-in is in force, and a subclass inherits the {@code next()} that supplies it. + *

{@code instanceof}, not the exact-class checks the policy branches use: this reads a + * property the generator has rather than deciding which built-in is in force, and a subclass + * inherits the {@code next()} that supplies it. */ private static Optional clientTimestamps(TimestampGenerator generator) { if (generator instanceof AtomicTimestampGenerator @@ -1083,76 +1069,18 @@ private static Optional clientTimestamps(TimestampGenerator generator) * so presence of the group is what reports that it is on. */ @Nullable - private ObjectNode tls(Channel channel) { - // TLS is on exactly when the channel pipeline has an SSL handler. ChannelFactory installs one - // from the low-level SslHandlerFactory before NettyOptions.afterChannelInitialized(), but that - // hook may add, replace, or remove handlers; report the pipeline left in force rather than the - // factory's configuration intent. Deliberately not getSslEngineFactory(): that is only the - // public JDK-based path that DefaultDriverContext.buildSslHandlerFactory() wraps, and an - // override of that method (the documented expert extension point, e.g. Netty's native OpenSSL) - // supplies a handler factory with no engine factory at all — a session that is encrypted all - // the same. - if (channel.pipeline().get(SslHandler.class) == null) { + private ObjectNode tls(TlsInfo tlsInfo) { + // The snapshot is captured from the active pipeline immediately before STARTUP, after the + // per-channel customization hook has run. Deliberately not getSslEngineFactory(): that is + // configuration intent and may describe an engine this connection does not use. + if (!tlsInfo.isEnabled()) { return null; } ObjectNode n = OBJECT_MAPPER.createObjectNode(); - // Host name validation, on the other hand, is a property of the JDK SSLEngine that the engine - // factory configures, so it can only be read on the JDK path — when the handler factory in - // force is the JdkSslHandlerFactory that buildSslHandlerFactory() wraps an engine factory in — - // and read off that handler rather than through the context (see #buildJson for why the two can - // disagree, and why resolving the context's is worse). Anything else (a native-OpenSSL handler, - // a bespoke one) leaves it unknown, and the schema's field is optional precisely so that - // unknown can be said by omission: reporting false would claim a session is not checking host - // names when it may well be. Exact-class check, like the policy branches above: - // JdkSslHandlerFactory is not final, and a subclass need not use the engine it was given. - // - // Note this is the factory's own state, not the SSL_HOSTNAME_VALIDATION config option: that - // option only governs the built-in DefaultSslEngineFactory. A factory supplied via - // SessionBuilder.withSslContext(...) (ProgrammaticSslEngineFactory) validates only if - // explicitly asked to (default off) regardless of that option, so reading the option here would - // falsely report validation as on when it isn't. - Optional handlerFactory = context.getSslHandlerFactory(); - if (handlerFactory.isPresent() - && handlerFactory.get().getClass() == JdkSslHandlerFactory.class) { - SslHandlerFactory factory = handlerFactory.get(); - SslEngineFactory engineFactory = ((JdkSslHandlerFactory) factory).getSslEngineFactory(); - hostnameValidation(engineFactory).ifPresent(v -> n.put("hostname-verification", v)); - } + tlsInfo.getHostnameVerification().ifPresent(v -> n.put("hostname-verification", v)); return n; } - /** - * Whether the engine factory in force validates host names, or {@link Optional#empty()} when it - * is not one this class recognizes. - * - *

Read by naming the driver's own factories rather than through an accessor on {@link - * SslEngineFactory}, deliberately: the interface obliges nobody to answer, so a default answer - * there would have described a security control on behalf of every implementation that never - * considered the question — including the ones that misdescribe it. Unknown is instead said by - * omission, which is what the schema's optional field is for. - * - *

{@code instanceof}, not the exact-class checks the policy branches use, and for the same - * reason as {@link #nodeLocation}: those decide which built-in is in force and must not - * be fooled by a subclass, whereas this one reads a value the factory already holds, and a - * subclass inherits it along with the {@code newSslEngine} that acts on it. A subclass that - * overrides {@code newSslEngine} to configure the engine differently — the only way to break that - * — reports its parent's answer; extending one of these factories is documented as a way to reuse - * it, not to invert it. - */ - private static Optional hostnameValidation(@Nullable SslEngineFactory engineFactory) { - if (engineFactory instanceof DefaultSslEngineFactory) { - return Optional.of(((DefaultSslEngineFactory) engineFactory).isHostnameValidationRequired()); - } else if (engineFactory instanceof ProgrammaticSslEngineFactory) { - return Optional.of( - ((ProgrammaticSslEngineFactory) engineFactory).isHostnameValidationRequired()); - } else if (engineFactory instanceof SniSslEngineFactory) { - // No accessor to read: SniSslEngineFactory sets the "HTTPS" endpoint identification algorithm - // on every engine it builds, unconditionally. - return Optional.of(true); - } - return Optional.empty(); - } - /** * A duration in milliseconds, floored at 1 for any strictly positive duration, and 0 for a zero * or negative one. diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java index 4731e9a7e84..c4783e7c916 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/DriverConfigReporter.java @@ -18,23 +18,62 @@ package com.datastax.oss.driver.internal.core.context; import edu.umd.cs.findbugs.annotations.NonNull; -import io.netty.channel.Channel; import java.util.Map; +import java.util.Optional; /** * Adds the {@code DRIVER_CONFIG} entry to the control connection's CQL {@code STARTUP} options, so * ScyllaDB can store it in {@code system.clients.client_options} and operators can inspect the * driver's effective settings while investigating incidents. * - *

The blob describes the whole session, so only the control connection carries it — pooled - * connections are correlated back to it through the {@link StartupOptionsBuilder#SESSION_ID_KEY - * SESSION_ID} startup option, which the driver sends on every connection unconditionally and - * independently of this reporter. + *

Most of the blob describes the whole session; its TLS group describes the control connection + * carrying it. Pooled connections are correlated back to that control connection through the {@link + * StartupOptionsBuilder#SESSION_ID_KEY SESSION_ID} startup option, which the driver sends on every + * connection unconditionally and independently of this reporter. * *

Governed by {@code advanced.driver-config-reporting.enabled} (enabled by default). */ public interface DriverConfigReporter { + /** Immutable snapshot of the effective TLS state of the reporting control connection. */ + final class TlsInfo { + private static final TlsInfo DISABLED = new TlsInfo(false, Optional.empty()); + private static final TlsInfo ENABLED_UNKNOWN = new TlsInfo(true, Optional.empty()); + + private final boolean enabled; + private final Optional hostnameVerification; + + private TlsInfo(boolean enabled, Optional hostnameVerification) { + this.enabled = enabled; + this.hostnameVerification = hostnameVerification; + } + + /** Returns a snapshot for a connection without a Netty {@code SslHandler}. */ + public static TlsInfo disabled() { + return DISABLED; + } + + /** Returns a snapshot for a TLS connection whose hostname-verification state is known. */ + public static TlsInfo enabled(boolean hostnameVerification) { + return new TlsInfo(true, Optional.of(hostnameVerification)); + } + + /** Returns a snapshot for a TLS connection whose hostname-verification state is unknown. */ + public static TlsInfo enabledWithUnknownHostnameVerification() { + return ENABLED_UNKNOWN; + } + + public boolean isEnabled() { + return enabled; + } + + /** Empty when TLS is disabled or its active engine could not expose this state. */ + @NonNull + public Optional getHostnameVerification() { + return hostnameVerification; + } + } + /** * Adds the {@code DRIVER_CONFIG} blob to the given startup options, unless configuration * reporting is disabled. @@ -45,13 +84,13 @@ public interface DriverConfigReporter { * failure to build the report must be swallowed (and logged) rather than propagated, otherwise it * would prevent the session from establishing or reconnecting. * - *

The report describes the driver's own configuration and the effective SSL state of the - * control connection. It does not depend on which backend answered, but the SSL handler must - * already be installed on {@code channel}. + *

The report describes the driver's own configuration and the supplied effective TLS state of + * the control connection. The caller must capture that state after channel customization and + * immediately before this method is invoked. * * @param startupOptions startup options to add the report to - * @param channel control connection whose effective SSL state is reported + * @param tlsInfo immutable snapshot of the control connection's effective TLS state */ void populateControlConnectionOptions( - @NonNull Map startupOptions, @NonNull Channel channel); + @NonNull Map startupOptions, @NonNull TlsInfo tlsInfo); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java index 36500ad50fa..eed8b28b00a 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/NoopDriverConfigReporter.java @@ -18,7 +18,6 @@ package com.datastax.oss.driver.internal.core.context; import edu.umd.cs.findbugs.annotations.NonNull; -import io.netty.channel.Channel; import java.util.Map; import net.jcip.annotations.ThreadSafe; @@ -44,7 +43,7 @@ public class NoopDriverConfigReporter implements DriverConfigReporter { @Override public void populateControlConnectionOptions( - @NonNull Map startupOptions, @NonNull Channel channel) { + @NonNull Map startupOptions, @NonNull TlsInfo tlsInfo) { // nothing to do } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPipelineTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPipelineTest.java new file mode 100644 index 00000000000..ba2ef2bb52b --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPipelineTest.java @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.channel; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.datastax.oss.driver.api.core.DefaultProtocolVersion; +import com.datastax.oss.driver.api.core.config.DefaultDriverOption; +import com.datastax.oss.driver.api.core.config.DriverConfig; +import com.datastax.oss.driver.api.core.config.DriverExecutionProfile; +import com.datastax.oss.driver.internal.core.context.InternalDriverContext; +import com.datastax.oss.driver.internal.core.context.NettyOptions; +import com.datastax.oss.driver.internal.core.metrics.MetricsFactory; +import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; +import com.datastax.oss.driver.internal.core.metrics.NoopSessionMetricUpdater; +import com.datastax.oss.driver.internal.core.ssl.SslHandlerFactory; +import com.datastax.oss.protocol.internal.FrameCodec; +import io.netty.buffer.ByteBuf; +import io.netty.channel.Channel; +import io.netty.channel.ChannelInitializer; +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.handler.ssl.SslHandler; +import java.time.Duration; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.Test; + +public class ChannelFactoryPipelineTest { + + @Test + @SuppressWarnings("unchecked") + public void should_install_ssl_before_protocol_init_and_channel_customization() { + InternalDriverContext context = mock(InternalDriverContext.class); + DriverConfig config = mock(DriverConfig.class); + DriverExecutionProfile profile = mock(DriverExecutionProfile.class); + NettyOptions nettyOptions = mock(NettyOptions.class); + MetricsFactory metricsFactory = mock(MetricsFactory.class); + SslHandlerFactory sslHandlerFactory = mock(SslHandlerFactory.class); + SslHandler sslHandler = mock(SslHandler.class); + FrameCodec frameCodec = mock(FrameCodec.class); + + when(context.getSessionName()).thenReturn("test"); + when(context.getConfig()).thenReturn(config); + when(config.getDefaultProfile()).thenReturn(profile); + when(profile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(profile.getDuration(DefaultDriverOption.CONNECTION_SET_KEYSPACE_TIMEOUT)) + .thenReturn(Duration.ofSeconds(1)); + when(profile.getDuration(DefaultDriverOption.CONNECTION_INIT_QUERY_TIMEOUT)) + .thenReturn(Duration.ofSeconds(1)); + when(profile.getDuration(DefaultDriverOption.HEARTBEAT_INTERVAL)) + .thenReturn(Duration.ofSeconds(30)); + when(profile.getBytes(DefaultDriverOption.PROTOCOL_MAX_FRAME_LENGTH)).thenReturn(1024L); + when(profile.getInt(DefaultDriverOption.CONNECTION_MAX_REQUESTS)).thenReturn(128); + when(profile.getInt(DefaultDriverOption.CONNECTION_MAX_ORPHAN_REQUESTS)).thenReturn(32); + when(context.getSslHandlerFactory()).thenReturn(Optional.of(sslHandlerFactory)); + when(sslHandlerFactory.newSslHandler(any(), eq(ChannelFactoryTestBase.SERVER_ADDRESS))) + .thenReturn(sslHandler); + when(context.getMetricsFactory()).thenReturn(metricsFactory); + when(metricsFactory.getSessionUpdater()).thenReturn(NoopSessionMetricUpdater.INSTANCE); + when(context.getFrameCodec()).thenReturn(frameCodec); + when(context.getNettyOptions()).thenReturn(nettyOptions); + + AtomicBoolean observedCompletePipeline = new AtomicBoolean(); + doAnswer( + invocation -> { + Channel channel = invocation.getArgument(0); + List names = channel.pipeline().names(); + assertThat(channel.pipeline().get(ChannelFactory.SSL_HANDLER_NAME)) + .isSameAs(sslHandler); + assertThat(names.indexOf(ChannelFactory.SSL_HANDLER_NAME)) + .isLessThan(names.indexOf(ChannelFactory.INIT_HANDLER_NAME)); + observedCompletePipeline.set(true); + return null; + }) + .when(nettyOptions) + .afterChannelInitialized(any()); + + ChannelFactory factory = new ChannelFactory(context); + CompletableFuture resultFuture = new CompletableFuture<>(); + ChannelInitializer initializer = + factory.initializer( + ChannelFactoryTestBase.SERVER_ADDRESS, + DefaultProtocolVersion.V4, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE, + resultFuture); + + EmbeddedChannel channel = new EmbeddedChannel(initializer); + try { + assertThat(observedCompletePipeline).isTrue(); + assertThat(resultFuture).isNotCompletedExceptionally(); + verify(nettyOptions).afterChannelInitialized(channel); + } finally { + channel.finishAndReleaseAll(); + } + } +} diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java index 8ec6b85e342..0bf4b762aa9 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java @@ -142,7 +142,7 @@ public void setup() throws InterruptedException { when(context.getWriteCoalescer()).thenReturn(new PassThroughWriteCoalescer(null)); when(context.getCompressor()).thenReturn(compressor); // The init handler consults the config reporter for the control connection; default to a no-op. - when(context.getDriverConfigReporter()).thenReturn((startupOptions, controlChannel) -> {}); + when(context.getDriverConfigReporter()).thenReturn((startupOptions, tlsInfo) -> {}); // Start local server ServerBootstrap serverBootstrap = diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java index b76ed857055..30ac4d2cbc5 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java @@ -19,6 +19,7 @@ import static com.datastax.oss.driver.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -43,6 +44,7 @@ import com.datastax.oss.driver.internal.core.TestResponses; import com.datastax.oss.driver.internal.core.context.DefaultDriverConfigReporter; import com.datastax.oss.driver.internal.core.context.DriverConfigReporter; +import com.datastax.oss.driver.internal.core.context.DriverConfigReporter.TlsInfo; import com.datastax.oss.driver.internal.core.context.InternalDriverContext; import com.datastax.oss.driver.internal.core.context.StartupOptionsBuilder; import com.datastax.oss.driver.internal.core.metadata.TestNodeFactory; @@ -64,6 +66,8 @@ import com.datastax.oss.protocol.internal.response.result.SetKeyspace; import com.datastax.oss.protocol.internal.util.Bytes; import io.netty.channel.ChannelFuture; +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.handler.ssl.SslHandler; import java.io.IOException; import java.net.InetSocketAddress; import java.time.Duration; @@ -72,8 +76,12 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.TimeUnit; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.slf4j.LoggerFactory; @@ -108,7 +116,7 @@ public void setup() { when(internalDriverContext.getProtocolVersionRegistry()).thenReturn(protocolVersionRegistry); // The init handler consults the config reporter for the control connection; default to a no-op. when(internalDriverContext.getDriverConfigReporter()) - .thenReturn((startupOptions, controlChannel) -> {}); + .thenReturn((startupOptions, tlsInfo) -> {}); channel .pipeline() @@ -161,17 +169,24 @@ public void should_initialize() { } // Mirrors the real reporter, which only ever sees the control connection. - private void stubConfigReporter() { - when(internalDriverContext.getDriverConfigReporter()) - .thenReturn( - (startupOptions, controlChannel) -> - startupOptions.put( - DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}")); + private DriverConfigReporter stubConfigReporter() { + DriverConfigReporter reporter = mock(DriverConfigReporter.class); + doAnswer( + invocation -> { + invocation + .>getArgument(0) + .put(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}"); + return null; + }) + .when(reporter) + .populateControlConnectionOptions(any(), any()); + when(internalDriverContext.getDriverConfigReporter()).thenReturn(reporter); + return reporter; } @Test public void should_report_driver_config_on_control_connection() { - stubConfigReporter(); + DriverConfigReporter reporter = stubConfigReporter(); channel .pipeline() .addLast( @@ -191,6 +206,41 @@ public void should_report_driver_config_on_control_connection() { assertThat(requestFrame.message).isInstanceOf(Startup.class); Startup startup = (Startup) requestFrame.message; assertThat(startup.options).containsKey(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY); + + ArgumentCaptor tlsInfo = ArgumentCaptor.forClass(TlsInfo.class); + verify(reporter).populateControlConnectionOptions(any(), tlsInfo.capture()); + assertThat(tlsInfo.getValue().isEnabled()).isFalse(); + } + + @Test + public void should_read_tls_from_the_replacement_ssl_engine() throws Exception { + EmbeddedChannel sslChannel = new EmbeddedChannel(); + try { + sslChannel + .pipeline() + .addLast(ChannelFactory.SSL_HANDLER_NAME, sslHandler(/* hostnameVerification= */ true)); + sslChannel + .pipeline() + .replace( + ChannelFactory.SSL_HANDLER_NAME, + ChannelFactory.SSL_HANDLER_NAME, + sslHandler(/* hostnameVerification= */ false)); + + TlsInfo tlsInfo = ProtocolInitHandler.tlsInfo(sslChannel); + assertThat(tlsInfo.isEnabled()).isTrue(); + assertThat(tlsInfo.getHostnameVerification()).contains(false); + } finally { + sslChannel.finishAndReleaseAll(); + } + } + + private static SslHandler sslHandler(boolean hostnameVerification) throws Exception { + SSLEngine engine = SSLContext.getDefault().createSSLEngine(); + engine.setUseClientMode(true); + SSLParameters parameters = engine.getSSLParameters(); + parameters.setEndpointIdentificationAlgorithm(hostnameVerification ? "HTTPS" : null); + engine.setSSLParameters(parameters); + return new SslHandler(engine, true); } @Test diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java index 05b78ae4b5c..f707cebaecf 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java @@ -42,6 +42,7 @@ import com.datastax.oss.driver.api.core.time.TimestampGenerator; import com.datastax.oss.driver.internal.core.connection.ConstantReconnectionPolicy; import com.datastax.oss.driver.internal.core.connection.ExponentialReconnectionPolicy; +import com.datastax.oss.driver.internal.core.context.DriverConfigReporter.TlsInfo; import com.datastax.oss.driver.internal.core.loadbalancing.BasicLoadBalancingPolicy; import com.datastax.oss.driver.internal.core.loadbalancing.DcInferringLoadBalancingPolicy; import com.datastax.oss.driver.internal.core.loadbalancing.DefaultLoadBalancingPolicy; @@ -49,9 +50,7 @@ import com.datastax.oss.driver.internal.core.retry.DefaultRetryPolicy; import com.datastax.oss.driver.internal.core.specex.ConstantSpeculativeExecutionPolicy; import com.datastax.oss.driver.internal.core.specex.NoSpeculativeExecutionPolicy; -import com.datastax.oss.driver.internal.core.ssl.DefaultSslEngineFactory; import com.datastax.oss.driver.internal.core.ssl.JdkSslHandlerFactory; -import com.datastax.oss.driver.internal.core.ssl.SniSslEngineFactory; import com.datastax.oss.driver.internal.core.ssl.SslHandlerFactory; import com.datastax.oss.driver.internal.core.time.AtomicTimestampGenerator; import com.datastax.oss.driver.internal.core.time.ServerSideTimestampGenerator; @@ -65,9 +64,6 @@ import com.networknt.schema.ValidationMessage; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; -import io.netty.channel.Channel; -import io.netty.channel.embedded.EmbeddedChannel; -import io.netty.handler.ssl.SslHandler; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.time.Duration; @@ -78,8 +74,6 @@ import java.util.function.Consumer; import java.util.function.Supplier; import javax.net.ssl.SSLContext; -import javax.net.ssl.SSLEngine; -import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -112,7 +106,6 @@ private static JsonSchema loadSchema() { private InternalDriverContext mockContext; private DriverExecutionProfile mockProfile; private DefaultDriverConfigReporter reporter; - private EmbeddedChannel reportingChannel; @Before public void setup() { @@ -122,12 +115,6 @@ public void setup() { when(mockContext.getConfig()).thenReturn(config); when(config.getDefaultProfile()).thenReturn(mockProfile); reporter = new DefaultDriverConfigReporter(mockContext); - reportingChannel = new EmbeddedChannel(); - } - - @After - public void cleanup() { - reportingChannel.finishAndReleaseAll(); } private void enableReporting(boolean enabled) { @@ -139,7 +126,7 @@ private void enableReporting(boolean enabled) { private DefaultDriverConfigReporter reporterReporting(Supplier json) { return new DefaultDriverConfigReporter(mockContext) { @Override - String buildJson(Channel channel) { + String buildJson(TlsInfo tlsInfo) { return json.get(); } }; @@ -155,7 +142,7 @@ public void should_add_driver_config_when_enabled() { enableReporting(true); Map options = new HashMap<>(); reporterReporting(() -> "{\"version\":1}") - .populateControlConnectionOptions(options, reportingChannel); + .populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options) .hasSize(1) .containsEntry(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}"); @@ -165,7 +152,7 @@ public void should_add_driver_config_when_enabled() { public void should_add_nothing_when_disabled() { enableReporting(false); Map options = new HashMap<>(); - reporter.populateControlConnectionOptions(options, reportingChannel); + reporter.populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options).isEmpty(); } @@ -176,7 +163,7 @@ public void should_add_driver_config_when_the_option_is_not_defined() { // getBoolean(), ignoring the fallback that is under test here. Map options = new HashMap<>(); defaultsReporter(map -> map.remove(TypedDriverOption.DRIVER_CONFIG_REPORTING_ENABLED)) - .populateControlConnectionOptions(options, reportingChannel); + .populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options).containsKey(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY); } @@ -189,7 +176,7 @@ public void should_report_nothing_at_all_without_jackson() { // in-process; what is checked here is that the substitute contributes nothing and, in // particular, does not need a context to say so. Map options = new HashMap<>(); - new NoopDriverConfigReporter().populateControlConnectionOptions(options, reportingChannel); + new NoopDriverConfigReporter().populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options).isEmpty(); } @@ -200,7 +187,7 @@ public void should_not_throw_when_reading_the_flag_fails() { when(mockProfile.getBoolean(DefaultDriverOption.DRIVER_CONFIG_REPORTING_ENABLED, true)) .thenThrow(new IllegalStateException("config blew up")); Map options = new HashMap<>(); - reporter.populateControlConnectionOptions(options, reportingChannel); // must not throw + reporter.populateControlConnectionOptions(options, TlsInfo.disabled()); // must not throw assertThat(options).isEmpty(); } @@ -212,7 +199,7 @@ public void should_skip_driver_config_when_building_fails() { () -> { throw new IllegalStateException("introspection blew up"); }) - .populateControlConnectionOptions(options, reportingChannel); // must not throw + .populateControlConnectionOptions(options, TlsInfo.disabled()); // must not throw assertThat(options).isEmpty(); } @@ -221,7 +208,7 @@ public void should_skip_driver_config_when_serialization_fails() { // buildJson() returns null when Jackson fails to serialize the node tree. enableReporting(true); Map options = new HashMap<>(); - reporterReporting(() -> null).populateControlConnectionOptions(options, reportingChannel); + reporterReporting(() -> null).populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options).isEmpty(); } @@ -298,7 +285,7 @@ public void should_skip_driver_config_when_it_exceeds_the_size_limit() { enableReporting(true); Map options = new HashMap<>(); reporterReporting(() -> oversizedReport()) - .populateControlConnectionOptions(options, reportingChannel); // must not throw + .populateControlConnectionOptions(options, TlsInfo.disabled()); // must not throw assertThat(options).isEmpty(); } @@ -307,7 +294,7 @@ public void should_add_driver_config_that_is_just_within_the_size_limit() { enableReporting(true); Map options = new HashMap<>(); String atLimit = padTo(DefaultDriverConfigReporter.MAX_DRIVER_CONFIG_LENGTH); - reporterReporting(() -> atLimit).populateControlConnectionOptions(options, reportingChannel); + reporterReporting(() -> atLimit).populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options).containsEntry(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, atLimit); } @@ -327,13 +314,13 @@ public void should_skip_a_report_a_configuration_pushes_over_the_size_limit() th // Built, well-formed and over the limit: it is dropped for its size, not because building it // failed. Reporting is left at the shipped default here, since defaultsReporter() reads a real // profile rather than the bare mock the tests above use. - String json = reporter.buildJson(reportingChannel); + String json = reporter.buildJson(TlsInfo.disabled()); assertConformsToSchema(MAPPER.readTree(json)); assertThat(json.getBytes(StandardCharsets.UTF_8).length) .isGreaterThan(DefaultDriverConfigReporter.MAX_DRIVER_CONFIG_LENGTH); Map options = new HashMap<>(); - reporter.populateControlConnectionOptions(options, reportingChannel); + reporter.populateControlConnectionOptions(options, TlsInfo.disabled()); assertThat(options).isEmpty(); } @@ -1445,10 +1432,6 @@ private Boolean clientTimestampsOf(TimestampGenerator generator) throws Exceptio @Test public void should_report_tls_enabled_with_hostname_verification() throws Exception { - // hostname-verification comes from the factory's own state, not the config option. - SslEngineFactory factory = - new ProgrammaticSslEngineFactory( - SSLContext.getDefault(), null, /* requireHostnameValidation= */ true); DefaultDriverConfigReporter r = reporterWith( defaults(map -> {}), @@ -1457,21 +1440,16 @@ public void should_report_tls_enabled_with_hostname_verification() throws Except mock(NoSpeculativeExecutionPolicy.class), loadBalancing(DefaultLoadBalancingPolicy.class), clientSideGenerator(), - Optional.of(factory)); - JsonNode connection = report(r).get("connection"); + Optional.empty()); + JsonNode connection = report(r, TlsInfo.enabled(true)).get("connection"); // Presence of the group is what reports TLS as on: the schema dropped the "enabled" boolean. assertThat(connection.has("tls")).isTrue(); assertThat(connection.get("tls").get("hostname-verification").asBoolean()).isTrue(); } @Test - public void should_report_hostname_verification_from_factory_not_config_option() + public void should_report_hostname_verification_from_active_tls_snapshot_not_config_option() throws Exception { - // Regression for the false-report bug: a ProgrammaticSslEngineFactory (as built by - // SessionBuilder.withSslContext(...)) does NO hostname validation by default and ignores the - // SSL_HOSTNAME_VALIDATION config option. The report must reflect the factory's real state - // (false), not the config option (true here) — otherwise it falsely claims validation is on. - SslEngineFactory programmatic = new ProgrammaticSslEngineFactory(SSLContext.getDefault()); DefaultDriverConfigReporter r = reporterWith( defaults(map -> map.put(TypedDriverOption.SSL_HOSTNAME_VALIDATION, true)), @@ -1480,40 +1458,26 @@ public void should_report_hostname_verification_from_factory_not_config_option() mock(NoSpeculativeExecutionPolicy.class), loadBalancing(DefaultLoadBalancingPolicy.class), clientSideGenerator(), - Optional.of(programmatic)); - JsonNode connection = report(r).get("connection"); - // Presence of the group is what reports TLS as on: the schema dropped the "enabled" boolean. + Optional.empty()); + JsonNode connection = report(r, TlsInfo.enabled(false)).get("connection"); assertThat(connection.has("tls")).isTrue(); assertThat(connection.get("tls").get("hostname-verification").asBoolean()).isFalse(); } @Test - public void should_report_tls_enabled_for_a_custom_ssl_handler_factory() throws Exception { - // Overriding DefaultDriverContext.buildSslHandlerFactory() is the driver's documented low-level - // SSL extension point (e.g. Netty's native OpenSSL), and such an override supplies no - // SslEngineFactory at all. That session is still encrypted, so TLS must be read from the - // handler - // factory — the same reference ChannelFactory installs the SSL handler from — and not from - // getSslEngineFactory(). Host name validation is a property of the JDK SSLEngine and cannot be - // read on this path at all, so it is omitted rather than guessed in either direction. + public void should_omit_hostname_verification_when_active_engine_state_is_unknown() + throws Exception { DefaultDriverConfigReporter r = reporterWith( - defaults(map -> map.put(TypedDriverOption.SSL_HOSTNAME_VALIDATION, true)), + defaults(map -> {}), exponentialReconnection(), mock(DefaultRetryPolicy.class), mock(NoSpeculativeExecutionPolicy.class), loadBalancing(DefaultLoadBalancingPolicy.class), clientSideGenerator(), - /* ssl= */ Optional.empty(), - Optional.of(mock(SslHandlerFactory.class)), - /* programmaticLocalDc= */ null); - JsonNode report = report(r); - JsonNode connection = report.get("connection"); - // Presence of the group is what reports TLS as on: the schema dropped the "enabled" boolean. - assertThat(connection.has("tls")).isTrue(); - assertThat(connection.get("tls").has("hostname-verification")).isFalse(); - // An empty tls group is a valid document: the schema made hostname-verification optional so - // that "unknown" has a representation. + Optional.empty()); + JsonNode report = report(r, TlsInfo.enabledWithUnknownHostnameVerification()); + assertThat(report.get("connection").get("tls").has("hostname-verification")).isFalse(); assertConformsToSchema(report); } @@ -1530,14 +1494,11 @@ public void should_not_report_tls_when_the_configured_ssl_handler_was_removed() clientSideGenerator(), Optional.of(factory)); - reportingChannel.pipeline().remove(SslHandler.class); - assertThat(report(r).get("connection").has("tls")).isFalse(); } @Test public void should_report_tls_when_a_pipeline_hook_added_an_ssl_handler() throws Exception { - reportingChannel.pipeline().addLast(clientSslHandler()); DefaultDriverConfigReporter r = reporterWith( defaults(map -> {}), @@ -1548,17 +1509,16 @@ public void should_report_tls_when_a_pipeline_hook_added_an_ssl_handler() throws clientSideGenerator(), Optional.empty()); - JsonNode tls = report(r).get("connection").get("tls"); + JsonNode report = report(r, TlsInfo.enabled(false)); + JsonNode tls = report.get("connection").get("tls"); assertThat(tls).isNotNull(); - assertThat(tls.has("hostname-verification")).isFalse(); + assertThat(tls.get("hostname-verification").asBoolean()).isFalse(); + assertConformsToSchema(report); } @Test - public void should_omit_hostname_verification_for_an_unrecognized_engine_factory() + public void should_report_active_hostname_verification_for_an_unrecognized_engine_factory() throws Exception { - // The JDK path, but with a custom engine factory that is none of the driver's own, so its host - // name handling is unknown. Guessing false here would report a session as not checking host - // names when its factory may well be doing exactly that. SslEngineFactory unrecognized = mock(SslEngineFactory.class); DefaultDriverConfigReporter r = reporterWith( @@ -1569,58 +1529,17 @@ public void should_omit_hostname_verification_for_an_unrecognized_engine_factory loadBalancing(DefaultLoadBalancingPolicy.class), clientSideGenerator(), Optional.of(unrecognized)); - JsonNode report = report(r); + JsonNode report = report(r, TlsInfo.enabled(true)); JsonNode tls = report.get("connection").get("tls"); assertThat(tls).isNotNull(); - assertThat(tls.has("hostname-verification")).isFalse(); + assertThat(tls.get("hostname-verification").asBoolean()).isTrue(); assertConformsToSchema(report); } @Test - public void should_report_every_built_in_engine_factory() throws Exception { - // None of the built-ins is ever reported as unknown. Real instances rather than mocks, so that - // the branches are pinned to the classes the driver actually instantiates — and, for the - // configured one, to the whole option-to-field-to-report chain. - assertThat(hostnameVerificationOf(new DefaultSslEngineFactory(policyConstructionContext()))) - .isTrue(); - assertThat(hostnameVerificationOf(new SniSslEngineFactory(SSLContext.getDefault()))).isTrue(); - assertThat(hostnameVerificationOf(new ProgrammaticSslEngineFactory(SSLContext.getDefault()))) - .isFalse(); - assertThat( - hostnameVerificationOf( - new ProgrammaticSslEngineFactory( - SSLContext.getDefault(), null, /* requireHostnameValidation= */ true))) - .isTrue(); - } - - /** The {@code connection.tls.hostname-verification} a report built over this factory carries. */ - private Boolean hostnameVerificationOf(SslEngineFactory factory) throws Exception { - DefaultDriverConfigReporter r = - reporterWith( - defaults(map -> {}), - exponentialReconnection(), - mock(DefaultRetryPolicy.class), - mock(NoSpeculativeExecutionPolicy.class), - loadBalancing(DefaultLoadBalancingPolicy.class), - clientSideGenerator(), - Optional.of(factory)); - JsonNode verification = report(r).get("connection").get("tls").get("hostname-verification"); - assertThat(verification).isNotNull(); - return verification.asBoolean(); - } - - @Test - public void should_report_hostname_verification_from_the_engine_the_handler_actually_wraps() + public void should_report_active_hostname_verification_not_the_configured_factory() throws Exception { - // The configured engine factory and the one the active handler wraps can be different objects: - // a context that overrides buildSslHandlerFactory() may pass an engine factory of its own while - // advanced.ssl-engine-factory.class still names another. The report has to describe the engine - // that actually builds the connection's SSLEngine, so the wrapped one wins. - SslEngineFactory wrapped = - new ProgrammaticSslEngineFactory( - SSLContext.getDefault(), null, /* requireHostnameValidation= */ true); - SslEngineFactory configuredButUnused = - new ProgrammaticSslEngineFactory(SSLContext.getDefault()); + SslEngineFactory configuredButUnused = mock(SslEngineFactory.class); DefaultDriverConfigReporter r = reporterWith( defaults(map -> {}), @@ -1630,10 +1549,10 @@ public void should_report_hostname_verification_from_the_engine_the_handler_actu loadBalancing(DefaultLoadBalancingPolicy.class), clientSideGenerator(), Optional.of(configuredButUnused), - Optional.of(new JdkSslHandlerFactory(wrapped)), + Optional.of(mock(SslHandlerFactory.class)), /* programmaticLocalDc= */ null); - JsonNode connection = report(r).get("connection"); - assertThat(connection.get("tls").get("hostname-verification").asBoolean()).isTrue(); + JsonNode connection = report(r, TlsInfo.enabled(false)).get("connection"); + assertThat(connection.get("tls").get("hostname-verification").asBoolean()).isFalse(); } @Test @@ -1646,10 +1565,6 @@ public void should_not_resolve_the_configured_engine_factory_at_all() throws Exc // Mockito cannot have a when(...) open while another begins. ReconnectionPolicy reconnection = exponentialReconnection(); TimestampGenerator timestamps = clientSideGenerator(); - SslEngineFactory wrapped = - new ProgrammaticSslEngineFactory( - SSLContext.getDefault(), null, /* requireHostnameValidation= */ true); - SslHandlerFactory handlerFactory = new JdkSslHandlerFactory(wrapped); // Built before the stubbing chain below: the helper stubs the policy itself, and Mockito // rejects a nested when() inside an unfinished one. LoadBalancingPolicy policy = loadBalancing(DefaultLoadBalancingPolicy.class); @@ -1665,28 +1580,19 @@ public void should_not_resolve_the_configured_engine_factory_at_all() throws Exc .thenReturn(mock(NoSpeculativeExecutionPolicy.class)); when(ctx.getLoadBalancingPolicy(DriverExecutionProfile.DEFAULT_NAME)).thenReturn(policy); when(ctx.getTimestampGenerator()).thenReturn(timestamps); - when(ctx.getSslHandlerFactory()).thenReturn(Optional.of(handlerFactory)); + when(ctx.getSslHandlerFactory()).thenThrow(new AssertionError("must not be resolved")); when(ctx.getSslEngineFactory()) .thenThrow(new AssertionError("the configured engine factory must not be resolved")); - reportingChannel.pipeline().addLast(clientSslHandler()); JsonNode report = - MAPPER.readTree(new DefaultDriverConfigReporter(ctx).buildJson(reportingChannel)); - // The group is built from the wrapped engine factory alone; getSslEngineFactory() throwing - // proves it was never consulted. + MAPPER.readTree(new DefaultDriverConfigReporter(ctx).buildJson(TlsInfo.enabled(true))); assertThat(report.get("connection").get("tls").get("hostname-verification").asBoolean()) .isTrue(); } @Test - public void should_not_report_hostname_verification_from_an_unused_engine_factory() + public void should_not_report_hostname_verification_from_an_unused_engine_factory_as_enabled() throws Exception { - // The handler factory and the engine factory are independent: a context can override - // buildSslHandlerFactory() (so the pipeline gets a handler the driver knows nothing about) and - // still have advanced.ssl-engine-factory.class configured, leaving a fully built engine factory - // that nothing on the connection path ever consults. Reading it would claim host name - // validation the custom handler does not perform, so hostname-verification is omitted unless - // the handler in force is the driver's own JdkSslHandlerFactory. SslEngineFactory validating = new ProgrammaticSslEngineFactory( SSLContext.getDefault(), null, /* requireHostnameValidation= */ true); @@ -1701,10 +1607,9 @@ public void should_not_report_hostname_verification_from_an_unused_engine_factor Optional.of(validating), Optional.of(mock(SslHandlerFactory.class)), /* programmaticLocalDc= */ null); - JsonNode connection = report(r).get("connection"); - // Presence of the group is what reports TLS as on: the schema dropped the "enabled" boolean. + JsonNode connection = report(r, TlsInfo.enabled(false)).get("connection"); assertThat(connection.has("tls")).isTrue(); - assertThat(connection.get("tls").has("hostname-verification")).isFalse(); + assertThat(connection.get("tls").get("hostname-verification").asBoolean()).isFalse(); } @Test @@ -2646,7 +2551,11 @@ private static TimestampGenerator clientSideGenerator() { } private JsonNode report(DefaultDriverConfigReporter reporter) throws Exception { - return MAPPER.readTree(reporter.buildJson(reportingChannel)); + return report(reporter, TlsInfo.disabled()); + } + + private JsonNode report(DefaultDriverConfigReporter reporter, TlsInfo tlsInfo) throws Exception { + return MAPPER.readTree(reporter.buildJson(tlsInfo)); } /** A real default execution profile with the given customizations applied. */ @@ -2690,8 +2599,8 @@ private DefaultDriverConfigReporter reporterWith( TimestampGenerator timestamps, Optional ssl, String programmaticLocalDc) { - // tls.enabled reads the low-level handler factory, which DefaultDriverContext derives from the - // engine factory when SSL was configured through the public API; mirror that wrapping here. + // DefaultDriverContext derives the low-level handler factory from the public engine factory; + // mirror the context shape even though the reporter now reads neither one. return reporterWith( profile, reconnection, @@ -2718,9 +2627,6 @@ private DefaultDriverConfigReporter reporterWith( Optional ssl, Optional sslHandler, String programmaticLocalDc) { - if (sslHandler.isPresent() && reportingChannel.pipeline().get(SslHandler.class) == null) { - reportingChannel.pipeline().addLast(clientSslHandler()); - } return new DefaultDriverConfigReporter( contextWith( profile, @@ -2734,16 +2640,6 @@ private DefaultDriverConfigReporter reporterWith( programmaticLocalDc)); } - private static SslHandler clientSslHandler() { - try { - SSLEngine engine = SSLContext.getDefault().createSSLEngine(); - engine.setUseClientMode(true); - return new SslHandler(engine); - } catch (Exception e) { - throw new AssertionError("Could not create test SSL handler", e); - } - } - /** * The context the reporters above read from. Separate from {@link #reporterWith} only so that a * test can build a reporter subclass over it, the way the {@code simpleName} seam needs. diff --git a/upgrade_guide/README.md b/upgrade_guide/README.md index 83963f94775..211f3580d92 100644 --- a/upgrade_guide/README.md +++ b/upgrade_guide/README.md @@ -32,8 +32,9 @@ a client's connections and inspect its driver settings while investigating an in turn it off. It is not derived from `CLIENT_ID`, which remains user-settable and unchanged. * `DRIVER_CONFIG` — a compact JSON description of the effective configuration of the session's default execution profile (connection/socket settings, timeouts, - retry/reconnection/speculative-execution/load-balancing policies, connection pooling, query - defaults, and TLS). Only the control connection sends it, since it describes the whole session. + retry/reconnection/speculative-execution/load-balancing policies, connection pooling, and query + defaults), plus the effective TLS state of the control connection carrying it. Only the control + connection sends it. It reports settings only — never credentials, statements or data — and identifies non-built-in policies by class name: the simple name, or the fully-qualified name when the policy is an anonymous class (which has no simple name). Reporting it is best-effort: if the report cannot be