diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.java b/core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.java index d50ad0f2397..d65eaa864aa 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.java @@ -133,20 +133,6 @@ public SSLEngine newSslEngine(@NonNull EndPoint remoteEndpoint) { return engine; } - /** - * Whether {@link #newSslEngine} configures the engine to validate the server certificate against - * the node's host name, as passed to the constructor. - * - *

A diagnostic accessor, read by the driver-configuration report sent to the server at - * connection time. Deliberately not on {@link SslEngineFactory}: an arbitrary factory can neither - * be assumed to validate host names nor be assumed not to, and a default answer on the interface - * would misdescribe a security control for every implementation that never considered the - * question. The report names the factories it recognizes and says nothing about the rest. - */ - public boolean isHostnameValidationRequired() { - return requireHostnameValidation; - } - @Override public void close() { // nothing to do 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/config/cloud/CloudConfigFactory.java b/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactory.java index 817b3263d25..cd649649548 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactory.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactory.java @@ -138,7 +138,12 @@ public CloudConfig createCloudConfig(@NonNull InputStream cloudConfig) InetSocketAddress sniProxyAddress = getSniProxyAddress(proxyMetadataJson); List endPoints = getEndPoints(proxyMetadataJson, sniProxyAddress); String localDatacenter = getLocalDatacenter(proxyMetadataJson); - SniSslEngineFactory sslEngineFactory = new SniSslEngineFactory(sslContext); + // A subclass can override the protected SSL-context/trust-manager builders, so only the exact + // built-in path can promise diagnostics that the endpoint-identification algorithm is honored. + SniSslEngineFactory sslEngineFactory = + getClass() == CloudConfigFactory.class + ? SniSslEngineFactory.forCloudBundle(sslContext) + : new SniSslEngineFactory(sslContext); validateIfBundleContainsUsernamePassword(configJson); return new CloudConfig(sniProxyAddress, endPoints, localDatacenter, sslEngineFactory); } 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..c6e6ce46cc0 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,9 +37,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; @@ -50,7 +46,9 @@ 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 java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.Map; @@ -104,13 +102,12 @@ * 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 SSL handler or engine in force does + * 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 — which is also why neither is asked of the SPI itself: an accessor on either SPI would + * have needed a default, and a default answer is exactly the guess being avoided. * *

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: @@ -189,8 +186,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 +228,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 +245,7 @@ public void populateControlConnectionOptions(Map startupOptions) if (!isEnabled()) { return; } - String json = buildJson(); + String json = buildJson(channel); if (json == null) { return; } @@ -288,22 +286,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 state recorded for the control channel by the {@code JdkSslHandlerFactory} + * in force rather than resolving the factory 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 +317,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 +334,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 +342,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); } @@ -1059,9 +1058,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 @@ -1078,7 +1077,7 @@ private static Optional clientTimestamps(TimestampGenerator generator) * so presence of the group is what reports that it is on. */ @Nullable - private ObjectNode tls() { + private ObjectNode tls(Channel channel) { // 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 @@ -1090,61 +1089,30 @@ private ObjectNode tls() { 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: + // Host name validation, on the other hand, is read from the SSLParameters of the engine the + // JdkSslHandlerFactory actually wrapped for the connection, but only when the built-in factory + // also knows which trust-manager path interprets those parameters. An arbitrary extended trust + // manager can ignore a nonempty endpoint-identification algorithm or verify names without one. + // Anything else (a native-OpenSSL handler, a bespoke or programmatic JDK context) leaves it + // unknown, and the schema's field is optional precisely so that unknown can be said by + // omission. 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. + // Note this is the engine's own state, not the SSL_HOSTNAME_VALIDATION config option. A factory + // supplied through SessionBuilder.withSslContext(...) ignores that option, and its arbitrary + // SSLContext may enforce or ignore host names independently of the endpoint-identification + // algorithm; that case is therefore unknown rather than guessed from config or parameters. SslHandlerFactory factory = handlerFactory.get(); if (factory.getClass() == JdkSslHandlerFactory.class) { - SslEngineFactory engineFactory = ((JdkSslHandlerFactory) factory).getSslEngineFactory(); - hostnameValidation(engineFactory).ifPresent(v -> n.put("hostname-verification", v)); + Boolean hostnameValidationRequired = + ((JdkSslHandlerFactory) factory).getHostnameValidationRequired(channel); + if (hostnameValidationRequired != null) { + n.put("hostname-verification", hostnameValidationRequired); + } } 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 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/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java b/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java index 3b7edfa7265..24608ac6278 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java @@ -70,12 +70,16 @@ public class DefaultSslEngineFactory implements SslEngineFactory { private final SSLContext sslContext; private final String[] cipherSuites; private final boolean requireHostnameValidation; + private final boolean hostnameValidationKnown; private final boolean allowDnsReverseLookupSan; private ReloadingKeyManagerFactory kmf; /** Builds a new instance from the driver configuration. */ public DefaultSslEngineFactory(DriverContext driverContext) { DriverExecutionProfile config = driverContext.getConfig().getDefaultProfile(); + this.hostnameValidationKnown = + config.isDefined(DefaultDriverOption.SSL_KEYSTORE_PATH) + || config.isDefined(DefaultDriverOption.SSL_TRUSTSTORE_PATH); try { this.sslContext = buildContext(config); } catch (Exception e) { @@ -137,18 +141,9 @@ public SSLEngine newSslEngine(@NonNull EndPoint remoteEndpoint) { return engine; } - /** - * Whether {@link #newSslEngine} configures the engine to validate the server certificate against - * the node's host name, from {@code advanced.ssl-engine-factory.hostname-validation}. - * - *

A diagnostic accessor, read by the driver-configuration report sent to the server at - * connection time. Deliberately not on {@link SslEngineFactory}: an arbitrary factory can neither - * be assumed to validate host names nor be assumed not to, and a default answer on the interface - * would misdescribe a security control for every implementation that never considered the - * question. The report names the factories it recognizes and says nothing about the rest. - */ - public boolean isHostnameValidationRequired() { - return requireHostnameValidation; + /** Whether this factory built the trust-manager path that interprets the engine's parameters. */ + boolean isHostnameValidationKnown() { + return hostnameValidationKnown; } protected SSLContext buildContext(DriverExecutionProfile config) throws Exception { diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java b/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java index 5dd625a70f0..51919643b97 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java @@ -19,40 +19,88 @@ import com.datastax.oss.driver.api.core.metadata.EndPoint; import com.datastax.oss.driver.api.core.ssl.SslEngineFactory; +import edu.umd.cs.findbugs.annotations.Nullable; import io.netty.channel.Channel; import io.netty.handler.ssl.SslHandler; +import io.netty.util.AttributeKey; +import java.lang.ref.WeakReference; import javax.net.ssl.SSLEngine; import net.jcip.annotations.ThreadSafe; /** SSL handler factory used when JDK-based SSL was configured through the driver's public API. */ @ThreadSafe public class JdkSslHandlerFactory implements SslHandlerFactory { + private static final AttributeKey HANDLER_REFERENCE = + AttributeKey.valueOf(JdkSslHandlerFactory.class, "sslHandler"); + private final SslEngineFactory sslEngineFactory; public JdkSslHandlerFactory(SslEngineFactory sslEngineFactory) { this.sslEngineFactory = sslEngineFactory; } + @Override + public SslHandler newSslHandler(Channel channel, EndPoint remoteEndpoint) { + SSLEngine engine = sslEngineFactory.newSslEngine(remoteEndpoint); + SslHandler handler = new SslHandler(engine); + // ChannelFactory calls this before NettyOptions.afterChannelInitialized(), so the first handler + // recorded here is the one installed by the driver even if the hook adds more SSL handlers. + channel.attr(HANDLER_REFERENCE).setIfAbsent(new HandlerReference(this, handler)); + return handler; + } + /** - * The engine factory this handler factory actually builds its engines from. + * Whether the SSL engine built for {@code channel} verifies host names, or {@code null} when it + * cannot be determined. * - *

Not necessarily the one behind {@code DriverContext#getSslEngineFactory()}: a context that - * overrides {@code buildSslHandlerFactory()} may wrap an engine factory of its own choosing, in - * which case the configured one is never consulted on the connection path. Diagnostics that want - * to describe the engine in force must read it from here rather than from the context. + *

This deliberately records the exact handler returned by {@link #newSslHandler} through a + * weak reference, and reads its engine lazily only while that handler remains in the channel + * pipeline. Engine introspection is diagnostic work and belongs under the configuration + * reporter's fail-safe; doing it while the handler is created would let a user-supplied engine + * that throws from {@code getSSLParameters()} prevent every connection, even when reporting is + * disabled. */ - public SslEngineFactory getSslEngineFactory() { - return sslEngineFactory; + @Nullable + public Boolean getHostnameValidationRequired(Channel channel) { + HandlerReference reference = channel.attr(HANDLER_REFERENCE).get(); + if (reference == null || reference.factory != this) { + return null; + } + SslHandler handler = reference.handler.get(); + if (handler == null || channel.pipeline().context(handler) == null) { + return null; + } + if (!isHostnameValidationKnown()) { + return null; + } + String endpointIdentificationAlgorithm = + handler.engine().getSSLParameters().getEndpointIdentificationAlgorithm(); + return endpointIdentificationAlgorithm != null && !endpointIdentificationAlgorithm.isEmpty(); } - @Override - public SslHandler newSslHandler(Channel channel, EndPoint remoteEndpoint) { - SSLEngine engine = sslEngineFactory.newSslEngine(remoteEndpoint); - return new SslHandler(engine); + private boolean isHostnameValidationKnown() { + if (sslEngineFactory.getClass() == DefaultSslEngineFactory.class) { + return ((DefaultSslEngineFactory) sslEngineFactory).isHostnameValidationKnown(); + } else if (sslEngineFactory.getClass() == SniSslEngineFactory.class) { + return ((SniSslEngineFactory) sslEngineFactory).isHostnameValidationKnown(); + } + // ProgrammaticSslEngineFactory and arbitrary factories can wrap a custom trust manager that + // ignores a nonempty endpoint-identification algorithm or verifies names without one. + return false; } @Override public void close() throws Exception { sslEngineFactory.close(); } + + private static final class HandlerReference { + private final JdkSslHandlerFactory factory; + private final WeakReference handler; + + private HandlerReference(JdkSslHandlerFactory factory, SslHandler handler) { + this.factory = factory; + this.handler = new WeakReference<>(handler); + } + } } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java b/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java index 4d2cb69fbfc..c040f110259 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java @@ -39,14 +39,26 @@ public class SniSslEngineFactory implements SslEngineFactory { private final SSLContext sslContext; private final CopyOnWriteArrayList fakePorts = new CopyOnWriteArrayList<>(); private final boolean allowDnsReverseLookupSan; + private final boolean hostnameValidationKnown; public SniSslEngineFactory(SSLContext sslContext) { - this(sslContext, true); + this(sslContext, true, false); } public SniSslEngineFactory(SSLContext sslContext, boolean allowDnsReverseLookupSan) { + this(sslContext, allowDnsReverseLookupSan, false); + } + + private SniSslEngineFactory( + SSLContext sslContext, boolean allowDnsReverseLookupSan, boolean hostnameValidationKnown) { this.sslContext = sslContext; this.allowDnsReverseLookupSan = allowDnsReverseLookupSan; + this.hostnameValidationKnown = hostnameValidationKnown; + } + + /** Builds the factory for a cloud-bundle context whose trust managers the driver created. */ + public static SniSslEngineFactory forCloudBundle(SSLContext sslContext) { + return new SniSslEngineFactory(sslContext, true, true); } @NonNull @@ -87,6 +99,11 @@ public SSLEngine newSslEngine(@NonNull EndPoint remoteEndpoint) { return engine; } + /** Whether the trust-manager path is known to interpret the engine's parameters. */ + boolean isHostnameValidationKnown() { + return hostnameValidationKnown; + } + private int getFakePort(String sniServerName) { fakePorts.addIfAbsent(sniServerName); return FAKE_PORT_OFFSET + fakePorts.indexOf(sniServerName); 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..a4d6464bdba 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,9 +164,10 @@ public void should_initialize() { private void stubConfigReporter() { when(internalDriverContext.getDriverConfigReporter()) .thenReturn( - startupOptions -> - startupOptions.put( - DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}")); + (startupOptions, controlChannel) -> { + assertThat(controlChannel).isSameAs(channel); + startupOptions.put(DefaultDriverConfigReporter.DRIVER_CONFIG_KEY, "{\"version\":1}"); + }); } @Test @@ -216,7 +218,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..da513f21c32 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 @@ -18,7 +18,12 @@ package com.datastax.oss.driver.internal.core.context; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; import com.datastax.dse.driver.internal.core.loadbalancing.DseDcInferringLoadBalancingPolicy; @@ -33,6 +38,7 @@ import com.datastax.oss.driver.api.core.connection.ReconnectionPolicy; import com.datastax.oss.driver.api.core.context.DriverContext; import com.datastax.oss.driver.api.core.loadbalancing.LoadBalancingPolicy; +import com.datastax.oss.driver.api.core.metadata.EndPoint; import com.datastax.oss.driver.api.core.metadata.Node; import com.datastax.oss.driver.api.core.retry.RetryPolicy; import com.datastax.oss.driver.api.core.session.Request; @@ -45,6 +51,8 @@ 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; +import com.datastax.oss.driver.internal.core.metadata.DefaultEndPoint; +import com.datastax.oss.driver.internal.core.metadata.SniEndPoint; import com.datastax.oss.driver.internal.core.retry.ConsistencyDowngradingRetryPolicy; import com.datastax.oss.driver.internal.core.retry.DefaultRetryPolicy; import com.datastax.oss.driver.internal.core.specex.ConstantSpeculativeExecutionPolicy; @@ -65,8 +73,13 @@ 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.net.InetSocketAddress; import java.nio.charset.StandardCharsets; +import java.nio.file.Paths; import java.time.Duration; import java.util.HashMap; import java.util.Map; @@ -75,6 +88,9 @@ import java.util.function.Consumer; import java.util.function.Supplier; import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -107,6 +123,7 @@ private static JsonSchema loadSchema() { private InternalDriverContext mockContext; private DriverExecutionProfile mockProfile; private DefaultDriverConfigReporter reporter; + private EmbeddedChannel reportingChannel; @Before public void setup() { @@ -116,6 +133,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 +150,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 +165,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 +176,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 +187,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 +200,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 +211,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 +223,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 +232,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 +309,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 +318,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 +338,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(); } @@ -1431,8 +1455,10 @@ 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. + public void should_treat_a_programmatic_ssl_context_with_an_algorithm_as_unknown() + throws Exception { + // A custom trust manager can ignore the engine's endpoint-identification algorithm, so its + // presence does not prove that the handshake verifies the host name. SslEngineFactory factory = new ProgrammaticSslEngineFactory( SSLContext.getDefault(), null, /* requireHostnameValidation= */ true); @@ -1448,16 +1474,16 @@ public void should_report_tls_enabled_with_hostname_verification() throws Except JsonNode connection = report(r).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(); + assertThat(connection.get("tls").has("hostname-verification")).isFalse(); } @Test - public void should_report_hostname_verification_from_factory_not_config_option() + public void should_treat_a_programmatic_ssl_context_without_an_algorithm_as_unknown() 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. + // ProgrammaticSslEngineFactory (as built by SessionBuilder.withSslContext(...)) ignores the + // SSL_HOSTNAME_VALIDATION config option, and the arbitrary SSLContext it wraps may contain a + // custom trust manager that validates host names without using an endpoint-identification + // algorithm. Neither true nor false can be inferred safely, so the optional field is omitted. SslEngineFactory programmatic = new ProgrammaticSslEngineFactory(SSLContext.getDefault()); DefaultDriverConfigReporter r = reporterWith( @@ -1471,7 +1497,7 @@ public void should_report_hostname_verification_from_factory_not_config_option() JsonNode connection = report(r).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()).isFalse(); + assertThat(connection.get("tls").has("hostname-verification")).isFalse(); } @Test @@ -1511,6 +1537,7 @@ public void should_omit_hostname_verification_for_an_unrecognized_engine_factory // 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); + when(unrecognized.newSslEngine(any())).thenReturn(clientSslEngine()); DefaultDriverConfigReporter r = reporterWith( defaults(map -> {}), @@ -1528,24 +1555,35 @@ public void should_omit_hostname_verification_for_an_unrecognized_engine_factory } @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(); + public void should_treat_a_custom_engine_factory_with_an_algorithm_as_unknown() throws Exception { + // The factory configures an algorithm, but its arbitrary SSLContext may use an extended trust + // manager that ignores the engine parameters. + SslEngineFactory factory = + new ProgrammaticSslEngineFactory(SSLContext.getDefault()) { + @Override + public SSLEngine newSslEngine(EndPoint remoteEndpoint) { + SSLEngine engine = super.newSslEngine(remoteEndpoint); + SSLParameters parameters = engine.getSSLParameters(); + parameters.setEndpointIdentificationAlgorithm("HTTPS"); + engine.setSSLParameters(parameters); + return engine; + } + }; + + assertThat(hostnameVerificationNode(factory)).isNull(); } - /** The {@code connection.tls.hostname-verification} a report built over this factory carries. */ - private Boolean hostnameVerificationOf(SslEngineFactory factory) throws Exception { + @Test + public void should_treat_an_engine_factory_subclass_without_an_algorithm_as_unknown() + throws Exception { + SslEngineFactory factory = + new ProgrammaticSslEngineFactory(SSLContext.getDefault()) { + @Override + public SSLEngine newSslEngine(EndPoint remoteEndpoint) { + return super.newSslEngine(remoteEndpoint); + } + }; + DefaultDriverConfigReporter r = reporterWith( defaults(map -> {}), @@ -1555,11 +1593,150 @@ private Boolean hostnameVerificationOf(SslEngineFactory factory) throws Exceptio loadBalancing(DefaultLoadBalancingPolicy.class), clientSideGenerator(), Optional.of(factory)); - JsonNode verification = report(r).get("connection").get("tls").get("hostname-verification"); + assertThat(report(r).get("connection").get("tls").has("hostname-verification")).isFalse(); + } + + @Test + public void should_keep_hostname_verification_state_on_the_control_channel() throws Exception { + SslEngineFactory engineFactory = knownSniFactory(); + JdkSslHandlerFactory handlerFactory = + (JdkSslHandlerFactory) activeJdkSslHandler(engineFactory, reportingChannel); + EmbeddedChannel pooledChannel = new EmbeddedChannel(); + try { + SslHandler pooledHandler = + handlerFactory.newSslHandler( + pooledChannel, + new SniEndPoint( + new InetSocketAddress("127.0.0.2", 9042), "another-node.example.com")); + pooledChannel.pipeline().addLast(pooledHandler); + SSLParameters pooledParameters = pooledHandler.engine().getSSLParameters(); + pooledParameters.setEndpointIdentificationAlgorithm(null); + pooledHandler.engine().setSSLParameters(pooledParameters); + DefaultDriverConfigReporter r = + reporterWith( + defaults(map -> {}), + exponentialReconnection(), + mock(DefaultRetryPolicy.class), + mock(NoSpeculativeExecutionPolicy.class), + loadBalancing(DefaultLoadBalancingPolicy.class), + clientSideGenerator(), + Optional.of(engineFactory), + Optional.of(handlerFactory), + /* programmaticLocalDc= */ null); + + assertThat(report(r).get("connection").get("tls").get("hostname-verification").asBoolean()) + .isTrue(); + } finally { + pooledChannel.finishAndReleaseAll(); + } + } + + @Test + public void should_ignore_an_unrelated_ssl_handler_ahead_of_the_driver_handler() + throws Exception { + SslEngineFactory engineFactory = knownSniFactory(); + SslHandlerFactory handlerFactory = activeJdkSslHandler(engineFactory, reportingChannel); + reportingChannel.pipeline().addFirst("unrelatedSsl", clientSslHandler()); + DefaultDriverConfigReporter r = + reporterWith( + defaults(map -> {}), + exponentialReconnection(), + mock(DefaultRetryPolicy.class), + mock(NoSpeculativeExecutionPolicy.class), + loadBalancing(DefaultLoadBalancingPolicy.class), + clientSideGenerator(), + Optional.of(engineFactory), + Optional.of(handlerFactory), + /* programmaticLocalDc= */ null); + + assertThat(report(r).get("connection").get("tls").get("hostname-verification").asBoolean()) + .isTrue(); + } + + @Test + public void should_not_break_the_connection_when_engine_introspection_fails() throws Exception { + SSLEngine engine = spy(SSLContext.getDefault().createSSLEngine("node.example.com", 9042)); + SSLParameters parameters = engine.getSSLParameters(); + doReturn(parameters) + .doThrow(new IllegalStateException("engine introspection blew up")) + .when(engine) + .getSSLParameters(); + SSLContext sslContext = spy(SSLContext.getDefault()); + doReturn(engine).when(sslContext).createSSLEngine(anyString(), anyInt()); + SslEngineFactory engineFactory = SniSslEngineFactory.forCloudBundle(sslContext); + JdkSslHandlerFactory handlerFactory = new JdkSslHandlerFactory(engineFactory); + + // Handler creation is mandatory connection work and must not perform diagnostic inspection. + reportingChannel + .pipeline() + .addLast( + handlerFactory.newSslHandler( + reportingChannel, + new SniEndPoint(new InetSocketAddress("127.0.0.1", 9042), "node.example.com"))); + + DefaultDriverConfigReporter r = + reporterWith( + defaults(map -> {}), + exponentialReconnection(), + mock(DefaultRetryPolicy.class), + mock(NoSpeculativeExecutionPolicy.class), + loadBalancing(DefaultLoadBalancingPolicy.class), + clientSideGenerator(), + Optional.of(engineFactory), + Optional.of(handlerFactory), + /* programmaticLocalDc= */ null); + Map options = new HashMap<>(); + r.populateControlConnectionOptions(options, reportingChannel); // must not throw + assertThat(options).isEmpty(); + } + + @Test + public void should_report_only_built_in_factories_with_known_validators() throws Exception { + assertThat(hostnameVerificationOf(configuredDefaultSslEngineFactory(true))).isTrue(); + assertThat(hostnameVerificationOf(configuredDefaultSslEngineFactory(false))).isFalse(); + assertThat(hostnameVerificationOf(knownSniFactory())).isTrue(); + + // These factories all accept a process-wide or caller-supplied SSLContext, whose custom trust + // manager may enforce or ignore host names independently of the engine parameters. + assertThat(hostnameVerificationNode(new DefaultSslEngineFactory(policyConstructionContext()))) + .isNull(); + assertThat(hostnameVerificationNode(new SniSslEngineFactory(SSLContext.getDefault()))).isNull(); + assertThat( + hostnameVerificationNode( + new ProgrammaticSslEngineFactory( + SSLContext.getDefault(), null, /* requireHostnameValidation= */ true))) + .isNull(); + } + + /** The {@code connection.tls.hostname-verification} a report built over this factory carries. */ + private Boolean hostnameVerificationOf(SslEngineFactory factory) throws Exception { + JsonNode verification = hostnameVerificationNode(factory); assertThat(verification).isNotNull(); return verification.asBoolean(); } + @Nullable + private JsonNode hostnameVerificationNode(SslEngineFactory factory) throws Exception { + EmbeddedChannel channel = new EmbeddedChannel(); + try { + SslHandlerFactory handlerFactory = activeJdkSslHandler(factory, channel); + DefaultDriverConfigReporter r = + reporterWith( + defaults(map -> {}), + exponentialReconnection(), + mock(DefaultRetryPolicy.class), + mock(NoSpeculativeExecutionPolicy.class), + loadBalancing(DefaultLoadBalancingPolicy.class), + clientSideGenerator(), + Optional.of(factory), + Optional.of(handlerFactory), + /* programmaticLocalDc= */ null); + return report(r, channel).get("connection").get("tls").get("hostname-verification"); + } finally { + channel.finishAndReleaseAll(); + } + } + @Test public void should_report_hostname_verification_from_the_engine_the_handler_actually_wraps() throws Exception { @@ -1567,9 +1744,7 @@ public void should_report_hostname_verification_from_the_engine_the_handler_actu // 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 wrapped = knownSniFactory(); SslEngineFactory configuredButUnused = new ProgrammaticSslEngineFactory(SSLContext.getDefault()); DefaultDriverConfigReporter r = @@ -1581,7 +1756,7 @@ 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(activeJdkSslHandler(wrapped, reportingChannel)), /* programmaticLocalDc= */ null); JsonNode connection = report(r).get("connection"); assertThat(connection.get("tls").get("hostname-verification").asBoolean()).isTrue(); @@ -1597,10 +1772,8 @@ 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); + SslEngineFactory wrapped = knownSniFactory(); + SslHandlerFactory handlerFactory = activeJdkSslHandler(wrapped, reportingChannel); // 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); @@ -1620,7 +1793,8 @@ 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()); + 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()) @@ -2398,9 +2572,7 @@ public void should_conform_to_schema_for_rack_auto_node_location() throws Except @Test public void should_conform_to_schema_for_tls_enabled_with_hostname_verification() throws Exception { - SslEngineFactory factory = - new ProgrammaticSslEngineFactory( - SSLContext.getDefault(), null, /* requireHostnameValidation= */ true); + SslEngineFactory factory = knownSniFactory(); assertConformsToSchema( report( reporterWith( @@ -2595,7 +2767,11 @@ private static TimestampGenerator clientSideGenerator() { } private JsonNode report(DefaultDriverConfigReporter reporter) throws Exception { - return MAPPER.readTree(reporter.buildJson()); + return report(reporter, reportingChannel); + } + + private JsonNode report(DefaultDriverConfigReporter reporter, Channel channel) throws Exception { + return MAPPER.readTree(reporter.buildJson(channel)); } /** A real default execution profile with the given customizations applied. */ @@ -2649,10 +2825,22 @@ private DefaultDriverConfigReporter reporterWith( loadBalancing, timestamps, ssl, - ssl.map(JdkSslHandlerFactory::new), + ssl.map(engineFactory -> activeJdkSslHandler(engineFactory, reportingChannel)), programmaticLocalDc); } + /** Mirrors ChannelFactory installing the JDK handler before STARTUP builds the report. */ + private SslHandlerFactory activeJdkSslHandler(SslEngineFactory engineFactory, Channel channel) { + JdkSslHandlerFactory handlerFactory = new JdkSslHandlerFactory(engineFactory); + EndPoint endPoint = + engineFactory instanceof SniSslEngineFactory + ? new SniEndPoint(new InetSocketAddress("127.0.0.1", 9042), "node.example.com") + : new DefaultEndPoint(new InetSocketAddress("127.0.0.1", 9042)); + channel.pipeline().addLast(handlerFactory.newSslHandler(channel, endPoint)); + assertThat(channel.pipeline().get(SslHandler.class)).isNotNull(); + return handlerFactory; + } + /** * Same as the 8-arg overload, with the SSL handler factory set independently of the engine * factory — as an override of {@code DefaultDriverContext.buildSslHandlerFactory()} would. @@ -2711,11 +2899,46 @@ private InternalDriverContext contextWith( return ctx; } + private SslEngineFactory knownSniFactory() throws Exception { + return SniSslEngineFactory.forCloudBundle(SSLContext.getDefault()); + } + + private SSLEngine clientSslEngine() throws Exception { + SSLEngine engine = SSLContext.getDefault().createSSLEngine(); + engine.setUseClientMode(true); + return engine; + } + + private SslHandler clientSslHandler() throws Exception { + return new SslHandler(clientSslEngine()); + } + + private DefaultSslEngineFactory configuredDefaultSslEngineFactory( + boolean requireHostnameValidation) throws Exception { + String truststorePath = + Paths.get( + DefaultDriverConfigReporterTest.class + .getResource("/config/cloud/trustStore.jks") + .toURI()) + .toString(); + return new DefaultSslEngineFactory( + policyConstructionContext( + map -> { + map.put(TypedDriverOption.SSL_TRUSTSTORE_PATH, truststorePath); + map.put(TypedDriverOption.SSL_TRUSTSTORE_PASSWORD, "fakePasswordForTests2"); + map.put(TypedDriverOption.SSL_HOSTNAME_VALIDATION, requireHostnameValidation); + })); + } + /** A minimal {@link DriverContext} good enough to construct a real built-in policy instance. */ private DriverContext policyConstructionContext() { + return policyConstructionContext(map -> {}); + } + + private DriverContext policyConstructionContext(Consumer customize) { DriverContext ctx = mock(DriverContext.class); DriverConfig config = mock(DriverConfig.class); - DriverExecutionProfile profile = defaults(map -> {}); + DriverExecutionProfile profile = defaults(customize); when(ctx.getConfig()).thenReturn(config); when(config.getDefaultProfile()).thenReturn(profile); when(config.getProfile(DriverExecutionProfile.DEFAULT_NAME)).thenReturn(profile);