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.