smiklosovic commented on code in PR #3638:
URL: https://github.com/apache/cassandra/pull/3638#discussion_r1820802420
##########
conf/cassandra-env.sh:
##########
@@ -236,6 +236,8 @@ else
JVM_OPTS="$JVM_OPTS -Dcom.sun.management.jmxremote.authenticate=true"
# jmx ssl options
+ # Consider using the jmx_encryption_options section of cassandra.yaml
instead to prevent sensitive information being
Review Comment:
could we make it like "exposed" is not on a new line alone? Either make it
one long line or break it in the middle and create two if you feel like it.
##########
examples/ssl-factory/test/unit/org/apache/cassandra/security/KubernetesSecretsPEMSslContextFactoryTest.java:
##########
@@ -114,7 +114,7 @@ public void setup()
* In order to test with real 'env' variables comment out this line
and set appropriate env variable. This is
* done to avoid having a dependency on env in the unit test.
*/
- commonConfig.put("require_client_auth", "false";
Review Comment:
ouch
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -3651,12 +3661,23 @@ public static EncryptionOptions
getNativeProtocolEncryptionOptions()
return conf.client_encryption_options;
}
+ public static EncryptionOptions getJmxEncryptionOptions()
+ {
+ return conf.jmx_encryption_options;
+ }
+
@VisibleForTesting
public static void
updateNativeProtocolEncryptionOptions(Function<EncryptionOptions,
EncryptionOptions> update)
{
conf.client_encryption_options =
update.apply(conf.client_encryption_options);
}
+ @VisibleForTesting
+ public static void updateJmxEncryptionOptions(Function<EncryptionOptions,
EncryptionOptions> update)
Review Comment:
@maulin-vasavada my IDE tells me that this method is not used anywhere.
Where is it used? If not used then it should not be there.
##########
src/java/org/apache/cassandra/utils/jmx/AbstractJmxSocketFactory.java:
##########
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.utils.jmx;
+
+import java.net.InetAddress;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.config.EncryptionOptions;
+
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS;
+
+/**
+ * Abstracts out the most common workflow in setting up the SSL client and
server socket factorires for JMX.
+ * First, it checks the system properties (see <a
href="https://docs.oracle.com/en/java/javase/17/management/monitoring-and-management-using-jmx-technology.html#GUID-F08985BB-629A-4FBF-A0CB-8762DF7590E0";>Java
Documentation</a> to read the SSL configuration.
+ * Next, it checks the provided {@code jmxEncryptionOptions} to read the SSL
configuration.
+ * If none of them is enabled, it checks the provided {@code localOnly} flag
to configure the JMX server socket
+ * factory for the local JMX connection.
+ */
+abstract public class AbstractJmxSocketFactory implements IJmxSocketFactory
+{
+ private static final Logger logger =
LoggerFactory.getLogger(AbstractJmxSocketFactory.class);
+
+ @Override
+ public Map<String, Object> configure(InetAddress serverAddress, boolean
localOnly,
+ EncryptionOptions
jmxEncryptionOptions) throws SSLException
+ {
+ Map<String, Object> env = new HashMap<>();
+ if (COM_SUN_MANAGEMENT_JMXREMOTE_SSL.getBoolean())
+ {
+ logger.info("Enabling JMX SSL using environment file properties");
+ logger.warn("Consider using the jmx_encryption_options section of
cassandra.yaml instead to prevent " +
+ "sensitive information being exposed");
+ boolean requireClientAuth =
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH.getBoolean();
+ String[] protocols = null;
+ String protocolList =
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS.getString();
+ if (protocolList != null)
+ {
+ JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS.setString(protocolList);
+ protocols = StringUtils.split(protocolList, ',');
+ }
+
+ String[] ciphers = null;
+ String cipherList =
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES.getString();
+ if (cipherList != null)
+ {
+
JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES.setString(cipherList);
+ ciphers = StringUtils.split(cipherList, ',');
+ }
+
+ configureSslClientSocketFactory(env, serverAddress);
+ configureSslServerSocketFactory(env, serverAddress, ciphers,
protocols, requireClientAuth);
+ }
+ else if (jmxEncryptionOptions != null &&
jmxEncryptionOptions.getEnabled() != null
+ && jmxEncryptionOptions.getEnabled())
+ {
+ logger.info("Enabling JMX SSL using jmx_encryption_options from
cassandra.yaml");
+ setJmxSystemProperties(jmxEncryptionOptions);
+ configureSslClientSocketFactory(env, serverAddress);
+ configureSslServerSocketFactory(env, serverAddress,
jmxEncryptionOptions);
+ }
+ else if (localOnly)
+ {
+ configureLocalSocketFactory(env,serverAddress);
+ }
+
+ return env;
+ }
+
+ /**
+ * Configures the non-SSL socket factories for the local JMX.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ */
+ abstract public void configureLocalSocketFactory(Map<String, Object> env,
InetAddress serverAddress);
+
+ /**
+ * Configures SSL based client socket factory.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ */
+ abstract public void configureSslClientSocketFactory(Map<String, Object>
env, InetAddress serverAddress);
+
+ /**
+ * Configures SSL based server socket factory.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ * @param enabledCipherSuites for the SSL communication
+ * @param enabledProtocols for the SSL communication
+ * @param needClientAuth {@code true} if it requires the client-auth;
{@code false} otherwise
+ */
+ abstract public void configureSslServerSocketFactory(Map<String, Object>
env, InetAddress serverAddress,
+ String[]
enabledCipherSuites, String[] enabledProtocols,
+ boolean
needClientAuth);
+
+ /**
+ * Configures SSL based server socket factory.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ * @param jmxEncryptionOptions for the SSL communication
+ * @throws SSLException if fails to configure the SSL based server socket
factory
+ */
+ abstract public void configureSslServerSocketFactory(Map<String, Object>
env, InetAddress serverAddress,
+ EncryptionOptions
jmxEncryptionOptions) throws SSLException;
+
+ /**
+ * Sets the following JMX system properties.
+ * <pre>
+ * com.sun.management.jmxremote.ssl=true
+ * javax.rmi.ssl.client.enabledCipherSuites=<applicable cipher
suites provided in the configuration>
+ * javax.rmi.ssl.client.enabledProtocols=<applicable protocols
provided in the configuration>
+ * </pre>
+ * @param jmxEncryptionOptions
+ */
+ void setJmxSystemProperties(EncryptionOptions jmxEncryptionOptions)
+ {
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL.setBoolean(true);
+ if (jmxEncryptionOptions.getAcceptedProtocols() != null)
Review Comment:
remove braces for `if`s, they are not necessary.
##########
test/distributed/org/apache/cassandra/distributed/test/JMXSslConfigDistributedTest.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.distributed.test;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.management.remote.rmi.RMIConnectorServer;
+import javax.net.ssl.SSLException;
+import javax.rmi.ssl.SslRMIClientSocketFactory;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.After;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslContextFactory;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslSocketFactory;
+import org.apache.cassandra.distributed.test.jmx.JMXGetterCheckTest;
+
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS;
+
+/**
+ * Distributed tests for JMX SSL configuration via the system properties OR
the encryption options in the cassandra.yaml.
+ */
+public class JMXSslConfigDistributedTest extends AbstractEncryptionOptionsImpl
Review Comment:
It is given this test is distributed when it is in distributed package.
"Distributed" is redundant.
##########
test/distributed/org/apache/cassandra/distributed/test/JMXSslConfigDistributedTest.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.distributed.test;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.management.remote.rmi.RMIConnectorServer;
+import javax.net.ssl.SSLException;
+import javax.rmi.ssl.SslRMIClientSocketFactory;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.After;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslContextFactory;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslSocketFactory;
+import org.apache.cassandra.distributed.test.jmx.JMXGetterCheckTest;
+
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS;
+
+/**
+ * Distributed tests for JMX SSL configuration via the system properties OR
the encryption options in the cassandra.yaml.
+ */
+public class JMXSslConfigDistributedTest extends AbstractEncryptionOptionsImpl
+{
+ @After
+ public void resetJmxSslSystemProperties()
+ {
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES.reset();
+ JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS.reset();
+ JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES.reset();
+ }
+
+ @SuppressWarnings("unchecked")
+ private void configureClientSocketFactory(Map<String, Object> jmxEnv,
Map<String, Object> encryptionOptionsMap) throws SSLException
+ {
+ JmxTestClientSslContextFactory clientSslContextFactory = new
JmxTestClientSslContextFactory(encryptionOptionsMap);
+ List<String> cipherSuitesList = (List<String>)
encryptionOptionsMap.get("cipher_suites");
+ String[] cipherSuites = cipherSuitesList == null ? null :
cipherSuitesList.toArray(new String[0]);
+ List<String> acceptedProtocolList = (List<String>)
encryptionOptionsMap.get("accepted_protocols");
+ String[] acceptedProtocols = acceptedProtocolList == null ? null :
acceptedProtocolList.toArray(new String[0]);
+ JmxTestClientSslSocketFactory clientFactory = new
JmxTestClientSslSocketFactory(clientSslContextFactory.createSSLContext(),
+
cipherSuites, acceptedProtocols);
+ jmxEnv.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE,
clientFactory);
+ jmxEnv.put("com.sun.jndi.rmi.factory.socket", clientFactory);
+ }
+
+ @Test
+ public void testDefaultEncryptionOptions() throws Throwable
+ {
+ setSystemTrustStore((String)validKeystore.get("truststore"),
(String)validKeystore.get("truststore_password"));
+ // We must set the keystore in the system variable to make sure that
the call to SSLContext.getDefault()
+ // uses it when Client SSL Socketfactory is initialized even if we
don't need it here.
+ // The same default SSLContext.getDefault() will be used by other
methods like testSystemSettings() in this test
+ // for the Server SSL Socketfactory and at that time we will need the
keystore to be available
+ // All of the above is the issue because we run everything (JMX
Server, Client) in the same JVM, multiple times
+ // and the SSLContext.getDefault() relies on static initialization
that is reused
+ setSystemKeyStore((String)validKeystore.get("keystore"),
(String)validKeystore.get("keystore_password"));
+ ImmutableMap<String, Object> encryptionOptionsMap =
ImmutableMap.<String, Object>builder().putAll(validKeystore)
+
.put("enabled", true)
+
.put("accepted_protocols", Arrays.asList("TLSv1.2", "TLSv1.3", "TLSv1.1"))
+
.build();
+
+ try (Cluster cluster = builder().withNodes(1).withConfig(c -> {
+ c.with(Feature.JMX);
+ c.set("jmx_encryption_options", encryptionOptionsMap);
Review Comment:
just inline options directly here, if you use WithProperties too, you'll
have it pretty nice.
##########
test/distributed/org/apache/cassandra/distributed/test/JMXSslConfigDistributedTest.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.distributed.test;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.management.remote.rmi.RMIConnectorServer;
+import javax.net.ssl.SSLException;
+import javax.rmi.ssl.SslRMIClientSocketFactory;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.After;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslContextFactory;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslSocketFactory;
+import org.apache.cassandra.distributed.test.jmx.JMXGetterCheckTest;
+
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS;
+
+/**
+ * Distributed tests for JMX SSL configuration via the system properties OR
the encryption options in the cassandra.yaml.
+ */
+public class JMXSslConfigDistributedTest extends AbstractEncryptionOptionsImpl
+{
+ @After
+ public void resetJmxSslSystemProperties()
+ {
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES.reset();
+ JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS.reset();
+ JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES.reset();
+ }
+
+ @SuppressWarnings("unchecked")
+ private void configureClientSocketFactory(Map<String, Object> jmxEnv,
Map<String, Object> encryptionOptionsMap) throws SSLException
+ {
+ JmxTestClientSslContextFactory clientSslContextFactory = new
JmxTestClientSslContextFactory(encryptionOptionsMap);
+ List<String> cipherSuitesList = (List<String>)
encryptionOptionsMap.get("cipher_suites");
+ String[] cipherSuites = cipherSuitesList == null ? null :
cipherSuitesList.toArray(new String[0]);
+ List<String> acceptedProtocolList = (List<String>)
encryptionOptionsMap.get("accepted_protocols");
+ String[] acceptedProtocols = acceptedProtocolList == null ? null :
acceptedProtocolList.toArray(new String[0]);
+ JmxTestClientSslSocketFactory clientFactory = new
JmxTestClientSslSocketFactory(clientSslContextFactory.createSSLContext(),
+
cipherSuites, acceptedProtocols);
+ jmxEnv.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE,
clientFactory);
+ jmxEnv.put("com.sun.jndi.rmi.factory.socket", clientFactory);
+ }
+
+ @Test
+ public void testDefaultEncryptionOptions() throws Throwable
+ {
+ setSystemTrustStore((String)validKeystore.get("truststore"),
(String)validKeystore.get("truststore_password"));
+ // We must set the keystore in the system variable to make sure that
the call to SSLContext.getDefault()
+ // uses it when Client SSL Socketfactory is initialized even if we
don't need it here.
+ // The same default SSLContext.getDefault() will be used by other
methods like testSystemSettings() in this test
+ // for the Server SSL Socketfactory and at that time we will need the
keystore to be available
+ // All of the above is the issue because we run everything (JMX
Server, Client) in the same JVM, multiple times
+ // and the SSLContext.getDefault() relies on static initialization
that is reused
+ setSystemKeyStore((String)validKeystore.get("keystore"),
(String)validKeystore.get("keystore_password"));
+ ImmutableMap<String, Object> encryptionOptionsMap =
ImmutableMap.<String, Object>builder().putAll(validKeystore)
+
.put("enabled", true)
+
.put("accepted_protocols", Arrays.asList("TLSv1.2", "TLSv1.3", "TLSv1.1"))
+
.build();
+
+ try (Cluster cluster = builder().withNodes(1).withConfig(c -> {
+ c.with(Feature.JMX);
+ c.set("jmx_encryption_options", encryptionOptionsMap);
+ }).start())
+ {
+ Map<String, Object> jmxEnv = new HashMap<>();
+ configureClientSocketFactory(jmxEnv, encryptionOptionsMap);
+ // Invoke the same code vs duplicating any code from the
JMXGetterCheckTest
+ JMXGetterCheckTest.testAllValidGetters(cluster, jmxEnv);
+ }
+ }
+
+ @Test
+ public void testClientAuth() throws Throwable
+ {
+ setSystemTrustStore((String)validKeystore.get("truststore"),
(String)validKeystore.get("truststore_password"));
+ setSystemKeyStore((String)validKeystore.get("keystore"),
(String)validKeystore.get("keystore_password"));
+ ImmutableMap<String, Object> encryptionOptionsMap =
ImmutableMap.<String, Object>builder().putAll(validKeystore)
+
.put("enabled", true)
+
.put("require_client_auth", true)
+
.put("accepted_protocols", Arrays.asList("TLSv1.2", "TLSv1.3", "TLSv1.1"))
+
.build();
+
+ try (Cluster cluster = builder().withNodes(1).withConfig(c -> {
+ c.with(Feature.JMX);
+ c.set("jmx_encryption_options", encryptionOptionsMap);
+ }).start())
+ {
+ Map<String, Object> jmxEnv = new HashMap<>();
+ configureClientSocketFactory(jmxEnv, encryptionOptionsMap);
+ // Invoke the same code vs duplicating any code from the
JMXGetterCheckTest
+ JMXGetterCheckTest.testAllValidGetters(cluster, jmxEnv);
+ }
+ }
+
+ @Test
+ public void testSystemSettings() throws Throwable
+ {
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL.setBoolean(true);
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH.setBoolean(false);
+
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS.setString("TLSv1.2,TLSv1.3,TLSv1.1");
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES.reset();
+ setSystemTrustStore((String)validKeystore.get("truststore"),
(String)validKeystore.get("truststore_password"));
+ setSystemKeyStore((String)validKeystore.get("keystore"),
(String)validKeystore.get("keystore_password"));
+
+ try (Cluster cluster = builder().withNodes(1).withConfig(c -> {
+ c.with(Feature.JMX);
+ }).start())
+ {
+ Map<String, Object> jmxEnv = new HashMap<>();
+ SslRMIClientSocketFactory clientFactory = new
SslRMIClientSocketFactory();
+ jmxEnv.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE,
clientFactory);
+ jmxEnv.put("com.sun.jndi.rmi.factory.socket", clientFactory);
+ // Invoke the same code vs duplicating any code from the
JMXGetterCheckTest
+ JMXGetterCheckTest.testAllValidGetters(cluster, jmxEnv);
+ }
+ }
+
+ @Test
+ public void testInvalidKeystorePath() throws Throwable
+ {
+ ImmutableMap<String, Object> encryptionOptionsMap =
ImmutableMap.<String, Object>builder()
+
.put("enabled", true)
+
.put("keystore", "/path/to/bad/keystore/that/should/not/exist")
+
.put("keystore_password", "cassandra")
+
.put("accepted_protocols", Arrays.asList("TLSv1.2", "TLSv1.3", "TLSv1.1"))
+
.build();
+
+ try (Cluster cluster = builder().withNodes(1).withConfig(c -> {
+ c.with(Feature.JMX);
+ c.set("jmx_encryption_options", encryptionOptionsMap);
+ }).createWithoutStarting())
+ {
+ assertCannotStartDueToConfigurationException(cluster);
+ }
+ }
+
+ /**
+ * Tests {@code disabled} jmx_encryption_options. Here even if the
configured {@code keystore} is invalid, it will
+ * not matter and the JMX server/client should start.
+ */
+ @Test
+ public void testDisabledEncryptionOptions() throws Throwable
+ {
+ try (Cluster cluster = builder().withNodes(1).withConfig(c -> {
+ c.with(Feature.JMX);
+ c.set("jmx_encryption_options",
+ ImmutableMap.builder()
+ .put("enabled", false)
+ .put("keystore",
"/path/to/bad/keystore/that/should/not/exist")
+ .put("keystore_password", "cassandra")
+ .build());
+ }).start())
+ {
+ // Invoke the same code vs duplicating any code from the
JMXGetterCheckTest
+ JMXGetterCheckTest.testAllValidGetters(cluster);
+ }
+ }
+
+ // checkstyle: suppress below 'blockSystemPropertyUsage'
+ void setSystemTrustStore(String trustStore, String trustStorePassword)
+ {
+ System.setProperty("javax.net.ssl.trustStore", trustStore);
Review Comment:
This is not a good pattern. Look for the usages of
`org.apache.cassandra.distributed.shared.WithProperties` class. Ideally, they
should be set in `try-catch` as that class implements `AutoCloseable` so on
exit from `try-catch` these properties would be automatically reset.
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -173,7 +173,7 @@ public enum ConfigKey
this.keyName=keyName;
}
- String getKeyName()
+ public String getKeyName()
Review Comment:
@maulin-vasavada Is this really needed? What about leveraging
`EncryptionOptions.toString()` where `toString` would return `keyName`?
`getKeyName` would not be necessary at all then.
While you are at it, could you fix spaces in body of the constructor around
`=` and remove braces for `for` in `asSet` ? (or at least move the opening one
to a new line).
##########
src/java/org/apache/cassandra/utils/jmx/DefaultJmxSocketFactory.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.utils.jmx;
+
+import java.net.InetAddress;
+import java.util.Arrays;
+import java.util.Map;
+import java.util.stream.Collectors;
+import javax.management.remote.rmi.RMIConnectorServer;
+import javax.net.ssl.SSLException;
+import javax.rmi.ssl.SslRMIClientSocketFactory;
+import javax.rmi.ssl.SslRMIServerSocketFactory;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.config.EncryptionOptions;
+import org.apache.cassandra.utils.JmxSslRMIServerSocketFactory;
+import org.apache.cassandra.utils.RMIServerSocketFactoryImpl;
+
+/**
+ * Default implementation of the {@link IJmxSocketFactory}.
+ */
+public final class DefaultJmxSocketFactory extends AbstractJmxSocketFactory
+{
+ private static final Logger logger =
LoggerFactory.getLogger(DefaultJmxSocketFactory.class);
+
+ @Override
+ public void configureLocalSocketFactory(Map<String, Object> env,
InetAddress serverAddress)
+ {
+ env.put(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE, new
RMIServerSocketFactoryImpl(serverAddress));
+ }
+
+ @Override
Review Comment:
While I was reading this, I was little bit confused what each
`configureSslServerSocketFactory` does. Three methods here are named same, it
is just they return something else and what they return is derived from what
parameters it takes. I would expect that the methods would be named more
descriptively so I know what it is going to return without looking into how it
is implemented.
##########
test/distributed/org/apache/cassandra/distributed/test/JMXSslConfigDistributedTest.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.distributed.test;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.management.remote.rmi.RMIConnectorServer;
+import javax.net.ssl.SSLException;
+import javax.rmi.ssl.SslRMIClientSocketFactory;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.After;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslContextFactory;
+import org.apache.cassandra.distributed.impl.JmxTestClientSslSocketFactory;
+import org.apache.cassandra.distributed.test.jmx.JMXGetterCheckTest;
+
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS;
+
+/**
+ * Distributed tests for JMX SSL configuration via the system properties OR
the encryption options in the cassandra.yaml.
+ */
+public class JMXSslConfigDistributedTest extends AbstractEncryptionOptionsImpl
+{
+ @After
+ public void resetJmxSslSystemProperties()
+ {
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS.reset();
+ COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES.reset();
+ JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS.reset();
+ JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES.reset();
+ }
+
+ @SuppressWarnings("unchecked")
+ private void configureClientSocketFactory(Map<String, Object> jmxEnv,
Map<String, Object> encryptionOptionsMap) throws SSLException
Review Comment:
Move this method to the end of this file. A reader just wants to see test
methods first.
##########
src/java/org/apache/cassandra/utils/jmx/AbstractJmxSocketFactory.java:
##########
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.utils.jmx;
+
+import java.net.InetAddress;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.config.EncryptionOptions;
+
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS;
+
+/**
+ * Abstracts out the most common workflow in setting up the SSL client and
server socket factorires for JMX.
+ * First, it checks the system properties (see <a
href="https://docs.oracle.com/en/java/javase/17/management/monitoring-and-management-using-jmx-technology.html#GUID-F08985BB-629A-4FBF-A0CB-8762DF7590E0";>Java
Documentation</a> to read the SSL configuration.
+ * Next, it checks the provided {@code jmxEncryptionOptions} to read the SSL
configuration.
+ * If none of them is enabled, it checks the provided {@code localOnly} flag
to configure the JMX server socket
+ * factory for the local JMX connection.
+ */
+abstract public class AbstractJmxSocketFactory implements IJmxSocketFactory
+{
+ private static final Logger logger =
LoggerFactory.getLogger(AbstractJmxSocketFactory.class);
+
+ @Override
+ public Map<String, Object> configure(InetAddress serverAddress, boolean
localOnly,
+ EncryptionOptions
jmxEncryptionOptions) throws SSLException
+ {
+ Map<String, Object> env = new HashMap<>();
+ if (COM_SUN_MANAGEMENT_JMXREMOTE_SSL.getBoolean())
+ {
+ logger.info("Enabling JMX SSL using environment file properties");
+ logger.warn("Consider using the jmx_encryption_options section of
cassandra.yaml instead to prevent " +
+ "sensitive information being exposed");
+ boolean requireClientAuth =
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH.getBoolean();
+ String[] protocols = null;
+ String protocolList =
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_PROTOCOLS.getString();
+ if (protocolList != null)
+ {
+ JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS.setString(protocolList);
+ protocols = StringUtils.split(protocolList, ',');
+ }
+
+ String[] ciphers = null;
+ String cipherList =
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_ENABLED_CIPHER_SUITES.getString();
+ if (cipherList != null)
+ {
+
JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES.setString(cipherList);
+ ciphers = StringUtils.split(cipherList, ',');
+ }
+
+ configureSslClientSocketFactory(env, serverAddress);
+ configureSslServerSocketFactory(env, serverAddress, ciphers,
protocols, requireClientAuth);
+ }
+ else if (jmxEncryptionOptions != null &&
jmxEncryptionOptions.getEnabled() != null
+ && jmxEncryptionOptions.getEnabled())
+ {
+ logger.info("Enabling JMX SSL using jmx_encryption_options from
cassandra.yaml");
+ setJmxSystemProperties(jmxEncryptionOptions);
+ configureSslClientSocketFactory(env, serverAddress);
+ configureSslServerSocketFactory(env, serverAddress,
jmxEncryptionOptions);
+ }
+ else if (localOnly)
+ {
+ configureLocalSocketFactory(env,serverAddress);
+ }
+
+ return env;
+ }
+
+ /**
+ * Configures the non-SSL socket factories for the local JMX.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ */
+ abstract public void configureLocalSocketFactory(Map<String, Object> env,
InetAddress serverAddress);
+
+ /**
+ * Configures SSL based client socket factory.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ */
+ abstract public void configureSslClientSocketFactory(Map<String, Object>
env, InetAddress serverAddress);
+
+ /**
+ * Configures SSL based server socket factory.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ * @param enabledCipherSuites for the SSL communication
+ * @param enabledProtocols for the SSL communication
+ * @param needClientAuth {@code true} if it requires the client-auth;
{@code false} otherwise
+ */
+ abstract public void configureSslServerSocketFactory(Map<String, Object>
env, InetAddress serverAddress,
+ String[]
enabledCipherSuites, String[] enabledProtocols,
+ boolean
needClientAuth);
+
+ /**
+ * Configures SSL based server socket factory.
+ * @param env output param containing the configured socket factories
+ * @param serverAddress the JMX server is bound to
+ * @param jmxEncryptionOptions for the SSL communication
+ * @throws SSLException if fails to configure the SSL based server socket
factory
+ */
+ abstract public void configureSslServerSocketFactory(Map<String, Object>
env, InetAddress serverAddress,
+ EncryptionOptions
jmxEncryptionOptions) throws SSLException;
+
+ /**
+ * Sets the following JMX system properties.
+ * <pre>
+ * com.sun.management.jmxremote.ssl=true
+ * javax.rmi.ssl.client.enabledCipherSuites=<applicable cipher
suites provided in the configuration>
+ * javax.rmi.ssl.client.enabledProtocols=<applicable protocols
provided in the configuration>
+ * </pre>
+ * @param jmxEncryptionOptions
+ */
+ void setJmxSystemProperties(EncryptionOptions jmxEncryptionOptions)
Review Comment:
can be `private`.
##########
src/java/org/apache/cassandra/utils/jmx/IJmxSocketFactory.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.utils.jmx;
+
+import java.net.InetAddress;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+
+import org.apache.cassandra.config.EncryptionOptions;
+
+/**
+ * The purpose of this interface is to allow customized configuration for the
JMX Client and Server Socket factories.
+ */
+public interface IJmxSocketFactory
+{
+ /**
+ * Configures the client and server socket factories for the JMX
connection.
+ * Specifically it configures below properties as applicable,
+ * <pre>
+ * jmx.remote.rmi.client.socket.factory
+ * jmx.remote.rmi.server.socket.factory
+ * com.sun.jndi.rmi.factory.socket
+ * </pre>
+ *
+ * In case of remote connection enabled, this also sets the following
system properties,
+ * <pre>
+ * com.sun.management.jmxremote.ssl=true
+ * javax.rmi.ssl.client.enabledCipherSuites=<applicable cipher
suites provided in the configuration>
+ * javax.rmi.ssl.client.enabledProtocols=<applicable protocols
provided in the configuration>
+ * </pre>
+ *
+ * @param serverAddress the JMX server is bound to
+ * @param localOnly {@code true} if the JMX server only allows local
connections; {@code false} if the JMX server
+ * allows the remote connections.
+ * @param jmxEncryptionOptions {@link EncryptionOptions} used for the SSL
configuration in case of the remote
+ * connections. Could
be {@code null} if system properties are
+ * used instead as
per <a
href="https://docs.oracle.com/en/java/javase/17/management/monitoring-and-management-using-jmx-technology.html#GUID-F08985BB-629A-4FBF-A0CB-8762DF7590E0";>Java
Documentation</a>
+ * @return Map<String, Object@gt; containing {@code
jmx.remote.rmi.client.socket.factory}, {@code
jmx.remote.rmi.server.socket.factory}
+ * and {@code com.sun.jndi.rmi.factory.socket} properties for the client
and server socket factories.
+ * @throws SSLException if it fails to configure the socket factories with
the given input
+ */
+ Map<String, Object> configure(InetAddress serverAddress, boolean
localOnly, EncryptionOptions jmxEncryptionOptions)
Review Comment:
I would move this all comment to place where it is implemented. Here you are
making assumption what the implementation does. Does not make sense to me to
have javadoc here.
##########
test/distributed/org/apache/cassandra/distributed/impl/IsolatedJmx.java:
##########
@@ -57,8 +60,8 @@ public class IsolatedJmx
private JMXServerUtils.JmxRegistry registry;
private RMIJRMPServerImpl jmxRmiServer;
private MBeanWrapper.InstanceMBeanWrapper wrapper;
- private RMIClientSocketFactoryImpl clientSocketFactory;
- private CollectingRMIServerSocketFactoryImpl serverSocketFactory;
+ private RMIClientSocketFactory clientSocketFactory;
Review Comment:
@maulin-vasavada why cant we have this on `RMICloseableSocketFactory` type
already? Then you do not need to cast it on line 99 and 100.
##########
conf/cassandra-env.sh:
##########
@@ -236,6 +236,8 @@ else
JVM_OPTS="$JVM_OPTS -Dcom.sun.management.jmxremote.authenticate=true"
# jmx ssl options
+ # Consider using the jmx_encryption_options section of cassandra.yaml
instead to prevent sensitive information being
Review Comment:
What if I have both cassandra.yaml as well as cassandra-env.sh configured?
What is the precedence? How does this work? It should be possible to configure
it at one place only, either here OR in cassandra.yaml and we should error out
on startup when this kind of configuration is found at both places. Having it
possibly at both places just brings confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
