Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11148

Merged
merged 1 commit into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package org.infinispan.client.hotrod;

import jakarta.transaction.TransactionManager;

import org.infinispan.client.hotrod.configuration.Configuration;
import org.infinispan.client.hotrod.configuration.TransactionMode;
import org.infinispan.commons.api.BasicCacheContainer;
import org.infinispan.commons.marshall.Marshaller;

import jakarta.transaction.TransactionManager;

public interface RemoteCacheContainer extends BasicCacheContainer {

/**
Expand Down Expand Up @@ -152,6 +152,13 @@ <K, V> RemoteCache<K, V> getCache(String cacheName, boolean forceReturnValue, Tr
*/
boolean switchToDefaultCluster();

/**
* Returns the name of the currently active cluster.
*
* @return the name of the active cluster
*/
String getCurrentClusterName();

Marshaller getMarshaller();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,11 @@ public boolean switchToDefaultCluster() {
return channelFactory.manualSwitchToCluster(ChannelFactory.DEFAULT_CLUSTER_NAME);
}

@Override
public String getCurrentClusterName() {
return channelFactory.getCurrentClusterName();
}

private Properties loadFromStream(InputStream stream) {
Properties properties = new Properties();
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ public class ClusterConfiguration {
private final List<ServerConfiguration> serverCluster;
private final String clusterName;
private final ClientIntelligence intelligence;
private final String sniHostName;

public ClusterConfiguration(List<ServerConfiguration> serverCluster, String clusterName, ClientIntelligence intelligence) {
public ClusterConfiguration(List<ServerConfiguration> serverCluster, String clusterName, ClientIntelligence intelligence, String sniHostName) {
this.serverCluster = serverCluster;
this.clusterName = clusterName;
this.intelligence = intelligence;
this.sniHostName = sniHostName;
}

public List<ServerConfiguration> getCluster() {
Expand All @@ -30,12 +32,17 @@ public ClientIntelligence getClientIntelligence() {
return intelligence;
}

public String sniHostName() {
return sniHostName;
}

@Override
public String toString() {
return "ClusterConfiguration{" +
"serverCluster=" + Util.toStr(serverCluster) +
", clusterName='" + clusterName + '\'' +
", intelligence=" + intelligence +
", sniHostName=" + sniHostName +
'}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public class ClusterConfigurationBuilder extends AbstractConfigurationChildBuild
private final List<ServerConfigurationBuilder> servers = new ArrayList<>();
private final String clusterName;
private ClientIntelligence intelligence;
private String sniHostName;

protected ClusterConfigurationBuilder(ConfigurationBuilder builder, String clusterName) {
super(builder);
Expand Down Expand Up @@ -53,6 +54,11 @@ public ClusterConfigurationBuilder clusterClientIntelligence(ClientIntelligence
return this;
}

public ClusterConfigurationBuilder clusterSniHostName(String clusterSniHostName) {
this.sniHostName = clusterSniHostName;
return this;
}

@Override
public void validate() {
if (clusterName == null || clusterName.isEmpty()) {
Expand All @@ -70,7 +76,7 @@ public void validate() {
public ClusterConfiguration create() {
List<ServerConfiguration> serverCluster = servers.stream()
.map(ServerConfigurationBuilder::create).collect(Collectors.toList());
return new ClusterConfiguration(serverCluster, clusterName, intelligence);
return new ClusterConfiguration(serverCluster, clusterName, intelligence, sniHostName);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ public class SslConfiguration {
private final String protocol;
private final Collection<String> ciphers;
private final String provider;
private final boolean hostnameValidation;

SslConfiguration(boolean enabled, String keyStoreFileName, String keyStoreType, char[] keyStorePassword, char[] keyStoreCertificatePassword, String keyAlias,
SSLContext sslContext,
String trustStoreFileName, String trustStorePath, String trustStoreType, char[] trustStorePassword, String sniHostName, String provider, String protocol, Collection<String> ciphers) {
String trustStoreFileName, String trustStorePath, String trustStoreType, char[] trustStorePassword, String sniHostName, String provider, String protocol, Collection<String> ciphers,
boolean hostnameValidation) {
this.enabled = enabled;
this.keyStoreFileName = keyStoreFileName;
this.keyStoreType = keyStoreType;
Expand All @@ -45,6 +47,7 @@ public class SslConfiguration {
this.provider = provider;
this.protocol = protocol;
this.ciphers = ciphers;
this.hostnameValidation = hostnameValidation;
}

public boolean enabled() {
Expand Down Expand Up @@ -107,6 +110,10 @@ public String provider() {
return provider;
}

public boolean hostnameValidation() {
return hostnameValidation;
}

@Override
public String toString() {
return "SslConfiguration{" +
Expand All @@ -121,6 +128,7 @@ public String toString() {
", provider='" + provider +'\'' +
", protocol='" + protocol + '\'' +
", ciphers='" + ciphers + '\'' +
", hostnameValidation=" + hostnameValidation +
'}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public class SslConfigurationBuilder extends AbstractSecurityConfigurationChildB
private String protocol;
private Collection<String> ciphers;
private String provider;
private boolean hostnameValidation = true;

protected SslConfigurationBuilder(SecurityConfigurationBuilder builder) {
super(builder);
Expand Down Expand Up @@ -223,6 +224,17 @@ public SslConfigurationBuilder ciphers(String... ciphers) {
return enable();
}

/**
* Configures whether to enable hostname validation according to <a href="https://datatracker.ietf.org/doc/html/rfc2818">RFC 2818</a>.
* This is enabled by default and requires that the server certificate includes a subjectAltName extension of type dNSName or iPAddress.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we're going to disable this by default on the 14.0.x branch? This could break a lot of existing deployments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to run this by PST

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PST confirms that, because this is a CVE, it needs to be enabled by default. No way around it.

*
* @param hostnameValidation whether to enable hostname validation
*/
public SslConfigurationBuilder hostnameValidation(boolean hostnameValidation) {
this.hostnameValidation = hostnameValidation;
return enable();
}

@Override
public void validate() {
if (enabled) {
Expand All @@ -244,6 +256,9 @@ public void validate() {
throw HOTROD.xorSSLContext();
}
}
if (hostnameValidation && sniHostName == null) {
throw HOTROD.missingSniHostName();
}
}
}

Expand All @@ -253,7 +268,7 @@ public SslConfiguration create() {
keyStoreFileName, keyStoreType, keyStorePassword, keyStoreCertificatePassword, keyAlias,
sslContext,
trustStoreFileName, trustStorePath, trustStoreType, trustStorePassword,
sniHostName, provider, protocol, ciphers);
sniHostName, provider, protocol, ciphers, hostnameValidation);
}

@Override
Expand All @@ -273,6 +288,7 @@ public SslConfigurationBuilder read(SslConfiguration template, Combine combine)
this.provider = template.provider();
this.protocol = template.protocol();
this.ciphers = template.ciphers() != null ? new ArrayList<>(template.ciphers()) : null;
this.hostnameValidation = template.hostnameValidation();
return this;
}

Expand Down Expand Up @@ -326,6 +342,9 @@ public ConfigurationBuilder withProperties(Properties properties) {
if (typed.containsKey(ConfigurationProperties.USE_SSL))
this.enabled(typed.getBooleanProperty(ConfigurationProperties.USE_SSL, enabled, true));

if (typed.containsKey(ConfigurationProperties.SSL_HOSTNAME_VALIDATION))
this.hostnameValidation(typed.getBooleanProperty(ConfigurationProperties.SSL_HOSTNAME_VALIDATION, hostnameValidation, true));

return builder.getBuilder();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,12 @@
* For details about cipher lists and possible values, refer to OpenSSL documentation at <a href="https://www.openssl.org/docs/man1.1.1/man1/ciphers">https://www.openssl.org/docs/man1.1.1/man1/ciphers</a></td>
* </tr>
* <tr>
* <td><b>infinispan.client.hotrod.ssl_hostname_validation</b></td>
* <td>boolean</td>
* <td>true</td>
* <td>Whether to enable TLS hostname validation using the rules defined in RFC 2818.</td>
* </tr>
* <tr>
* <td><b>infinispan.client.hotrod.ssl_protocol</b></td>
* <td>String</td>
* <td>N/A</td>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public class ConfigurationProperties {
public static final String SSL_PROTOCOL = ICH + "ssl_protocol";
public static final String SSL_CIPHERS = ICH + "ssl_ciphers";
public static final String SSL_CONTEXT = ICH + "ssl_context";
public static final String SSL_HOSTNAME_VALIDATION = ICH + "ssl_hostname_validation";
public static final String MAX_RETRIES = ICH + "max_retries";
// Authentication properties
public static final String USE_AUTH = ICH + "use_auth";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,22 @@ public class ClusterInfo {
// updates won't be allowed to apply since they refer to older views.
private final int topologyAge;
private final ClientIntelligence intelligence;
private final String sniHostName;

public ClusterInfo(String clusterName, List<InetSocketAddress> servers, ClientIntelligence intelligence) {
this(clusterName, servers, -1, intelligence);
public ClusterInfo(String clusterName, List<InetSocketAddress> servers, ClientIntelligence intelligence, String sniHostName) {
this(clusterName, servers, -1, intelligence, sniHostName);
}

private ClusterInfo(String clusterName, List<InetSocketAddress> servers, int topologyAge, ClientIntelligence intelligence) {
private ClusterInfo(String clusterName, List<InetSocketAddress> servers, int topologyAge, ClientIntelligence intelligence, String sniHostName) {
this.clusterName = clusterName;
this.servers = Immutables.immutableListCopy(servers);
this.topologyAge = topologyAge;
this.intelligence = Objects.requireNonNull(intelligence);
this.sniHostName = sniHostName;
}

public ClusterInfo withTopologyAge(int topologyAge) {
return new ClusterInfo(clusterName, servers, topologyAge, intelligence);
return new ClusterInfo(clusterName, servers, topologyAge, intelligence, sniHostName);
}

public String getName() {
Expand All @@ -53,13 +55,18 @@ public ClientIntelligence getIntelligence() {
return intelligence;
}

public String getSniHostName() {
return sniHostName;
}

@Override
public String toString() {
return "ClusterInfo{" +
"name='" + clusterName + '\'' +
", servers=" + servers +
", age=" + topologyAge +
", intelligence=" + intelligence +
", sniHostname=" + sniHostName +
'}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public class ChannelFactory {
@GuardedBy("lock")
private final Set<SocketAddress> failedServers = new HashSet<>();
private final CodecHolder codecHolder;
private AddressResolverGroup<?> dnsResolver;

public ChannelFactory(CodecHolder codecHolder) {
this.codecHolder = codecHolder;
Expand All @@ -123,12 +124,17 @@ public void start(Configuration configuration, Marshaller marshaller, ExecutorSe
// Note that each event loop opens a selector which counts
int maxExecutors = Math.min(asyncThreads, eventLoopThreads);
this.eventLoopGroup = configuration.transportFactory().createEventLoopGroup(maxExecutors, executorService);
DnsNameResolverBuilder builder = new DnsNameResolverBuilder()
.channelType(configuration.transportFactory().datagramChannelClass())
.ttl(configuration.dnsResolverMinTTL(), configuration.dnsResolverMaxTTL())
.negativeTtl(configuration.dnsResolverNegativeTTL());
this.dnsResolver = new RoundRobinDnsAddressResolverGroup(builder);

List<InetSocketAddress> initialServers = new ArrayList<>();
for (ServerConfiguration server : configuration.servers()) {
initialServers.add(InetSocketAddress.createUnresolved(server.host(), server.port()));
}
ClusterInfo mainCluster = new ClusterInfo(DEFAULT_CLUSTER_NAME, initialServers, configuration.clientIntelligence());
ClusterInfo mainCluster = new ClusterInfo(DEFAULT_CLUSTER_NAME, initialServers, configuration.clientIntelligence(), configuration.security().ssl().sniHostName());
List<ClusterInfo> clustersDefinitions = new ArrayList<>();
if (log.isDebugEnabled()) {
log.debugf("Statically configured servers: %s", initialServers);
Expand All @@ -145,8 +151,10 @@ public void start(Configuration configuration, Marshaller marshaller, ExecutorSe
ClientIntelligence intelligence = clusterConfiguration.getClientIntelligence() != null ?
clusterConfiguration.getClientIntelligence() :
configuration.clientIntelligence();

String sniHostName = clusterConfiguration.sniHostName() != null ? clusterConfiguration.sniHostName() : configuration.security().ssl().sniHostName();
ClusterInfo alternateCluster =
new ClusterInfo(clusterConfiguration.getClusterName(), alternateServers, intelligence);
new ClusterInfo(clusterConfiguration.getClusterName(), alternateServers, intelligence, sniHostName);
log.debugf("Add secondary cluster: %s", alternateCluster);
clustersDefinitions.add(alternateCluster);
}
Expand Down Expand Up @@ -187,12 +195,7 @@ public MarshallerRegistry getMarshallerRegistry() {
}

private ChannelPool newPool(SocketAddress address) {
log.debugf("Creating new channel pool for %s", address);
DnsNameResolverBuilder builder = new DnsNameResolverBuilder()
.channelType(configuration.transportFactory().datagramChannelClass())
.ttl(configuration.dnsResolverMinTTL(), configuration.dnsResolverMaxTTL())
.negativeTtl(configuration.dnsResolverNegativeTTL());
AddressResolverGroup<?> dnsResolver = new RoundRobinDnsAddressResolverGroup(builder);
log.debugf("Creating new channel pool for %s", address);
Bootstrap bootstrap = new Bootstrap()
.group(eventLoopGroup)
.channel(configuration.transportFactory().socketChannelClass())
Expand All @@ -210,7 +213,7 @@ private ChannelPool newPool(SocketAddress address) {
}

public ChannelInitializer createChannelInitializer(SocketAddress address, Bootstrap bootstrap) {
return new ChannelInitializer(bootstrap, address, operationsFactory, configuration, this);
return new ChannelInitializer(bootstrap, address, operationsFactory, configuration, this, topologyInfo.getCluster());
}

protected ChannelPool createChannelPool(Bootstrap bootstrap, ChannelInitializer channelInitializer, SocketAddress address) {
Expand Down