JeetKunDoug commented on code in PR #3638: URL: https://github.com/apache/cassandra/pull/3638#discussion_r1848628584
########## test/distributed/org/apache/cassandra/distributed/impl/JmxTestClientSslContextFactory.java: ########## @@ -0,0 +1,145 @@ +/* + * 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.impl; + +import java.io.InputStream; +import java.nio.file.Files; +import java.security.KeyStore; +import java.util.Map; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManager; +import javax.net.ssl.TrustManagerFactory; + +import org.apache.commons.lang3.StringUtils; + +import org.apache.cassandra.config.EncryptionOptions; +import org.apache.cassandra.io.util.File; + +/** + * Simplied and independent version of {@link org.apache.cassandra.security.FileBasedSslContextFactory} for Review Comment: NIT: Spelling - assuming this was meant to be `Simplified`? ```suggestion * Simplified and independent version of {@link org.apache.cassandra.security.FileBasedSslContextFactory} for ``` ########## test/distributed/org/apache/cassandra/distributed/test/AbstractEncryptionOptionsImpl.java: ########## @@ -70,11 +70,16 @@ public class AbstractEncryptionOptionsImpl extends TestBaseImpl final static String validTrustStorePassword = TlsTestUtils.SERVER_TRUSTSTORE_PASSWORD; // Base configuration map for a valid keystore that can be opened - final static Map<String,Object> validKeystore = ImmutableMap.of("keystore", validKeyStorePath, + protected final static Map<String,Object> validKeystore = ImmutableMap.of("keystore", validKeyStorePath, "keystore_password", validKeyStorePassword, "truststore", validTrustStorePath, "truststore_password", validTrustStorePassword); + // Configuration for a valid PEM based keystore + final static Map<String, Object> validPEMKeystore = ImmutableMap.of("ssl_context_factory", ImmutableMap.of("class_name", "org.apache.cassandra.security.PEMBasedSslContextFactory"), Review Comment: This appears to be unused - we should either add a test that uses it, or remove it. I would guess we want to add a test somewhere? ########## test/unit/org/apache/cassandra/utils/jmx/JMXSslDefaultEncryptionOptionsTest.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.management.remote.rmi.RMIConnectorServer; +import javax.net.ssl.SSLException; +import javax.rmi.ssl.SslRMIServerSocketFactory; + +import org.apache.commons.lang3.StringUtils; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.config.EncryptionOptions; +import org.apache.cassandra.distributed.shared.WithProperties; +import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.utils.JMXServerUtils; + +import static org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_CONFIG; +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; + +/** + * Tests for Default JMX SSL configuration specified in the cassandra.yaml using jmx_encryption_options. + * The default configurtion means keystore/truststore configured with file paths and using {@code DefaultSslContextFactory} + * to initialize the SSLContext. + */ +public class JMXSslDefaultEncryptionOptionsTest +{ + static WithProperties properties; + + @BeforeClass + public static void setupDatabaseDescriptor() + { + properties = new WithProperties().set(CASSANDRA_CONFIG, "cassandra-jmx-sslconfig.yaml"); + DatabaseDescriptor.daemonInitialization(); + } + + @AfterClass + public static void tearDownDatabaseDescriptor() + { + properties.close(); + } + + @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(); Review Comment: Same comment about pulling this into the JMXTestsUtil class. ########## test/distributed/org/apache/cassandra/distributed/impl/JmxTestClientSslContextFactory.java: ########## @@ -0,0 +1,145 @@ +/* + * 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.impl; + +import java.io.InputStream; +import java.nio.file.Files; +import java.security.KeyStore; +import java.util.Map; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManager; +import javax.net.ssl.TrustManagerFactory; + +import org.apache.commons.lang3.StringUtils; + +import org.apache.cassandra.config.EncryptionOptions; +import org.apache.cassandra.io.util.File; + +/** + * Simplied and independent version of {@link org.apache.cassandra.security.FileBasedSslContextFactory} for + * testing SSL based JMX clients that require configuring keystore and/or truststore. + */ +public class JmxTestClientSslContextFactory +{ + protected final Map<String, Object> parameters; + // keystore is not needed when the JMX server does not require client-auth + protected String keystore; + // keystore could be null in case JMX server does not require client-auth + protected String keystore_password; Review Comment: NIT: both of these can be final as well, and all of the member variables can be private. ########## test/distributed/org/apache/cassandra/distributed/impl/IsolatedJmxSocketFactory.java: ########## @@ -0,0 +1,96 @@ +/* + * 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.impl; + +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.SSLContext; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.cassandra.config.CassandraRelevantProperties; +import org.apache.cassandra.utils.RMIClientSocketFactoryImpl; +import org.apache.cassandra.utils.jmx.AbstractJmxSocketFactory; + +/** + * JMX Socket factory used for the isolated JMX testing. + */ +public class IsolatedJmxSocketFactory extends AbstractJmxSocketFactory +{ + private static final Logger logger = LoggerFactory.getLogger(IsolatedJmxSocketFactory.class); + + @Override + public void configureLocalSocketFactory(Map<String, Object> env, InetAddress serverAddress) Review Comment: NIT: This should be named `configureSslServerSocketFactories` since it configured both. ########## test/distributed/org/apache/cassandra/distributed/impl/RMISslClientSocketFactoryImpl.java: ########## @@ -0,0 +1,148 @@ +/* + * 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.impl; + +import java.io.IOException; +import java.io.Serializable; +import java.net.InetAddress; +import java.net.Socket; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +import java.util.StringTokenizer; +import javax.net.SocketFactory; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; + +import org.apache.cassandra.utils.RMICloseableClientSocketFactory; + +/** + * {@code RMIClientSocketFactory} for testing SSL based JMX clients. + * This class is used to override the local address the JMX client calculates when trying to connect, + * which can otherwise be influenced by the system property "java.rmi.server.hostname" in strange and + * unpredictable ways. + */ +public class RMISslClientSocketFactoryImpl implements Serializable, RMICloseableClientSocketFactory +{ + private static final long serialVersionUID = 9054380061905145241L; + private static final List<Socket> sockets = new ArrayList<>(); + private static SocketFactory defaultSocketFactory = null; + private final InetAddress localAddress; + private final String enabledCipherSuites; + private final String enabledProtocols; + + public RMISslClientSocketFactoryImpl(InetAddress localAddress, String enabledCipherSuites, String enabledProtocls) + { + this.localAddress = localAddress; + this.enabledCipherSuites = enabledCipherSuites; + this.enabledProtocols = enabledProtocls; + } + + private static synchronized SocketFactory getDefaultClientSocketFactory() + { + if (defaultSocketFactory == null) Review Comment: Is there a reason we do this lazy initialization here? it's always initialized to the same value, it isn't changed externally with a setter or anything like that. Further, `SSLSocketFactory.getDefault()` is _already synchronized and lazy_ so we could just call it directly in the one place this method is used. ########## test/distributed/org/apache/cassandra/distributed/impl/RMISslClientSocketFactoryImpl.java: ########## @@ -0,0 +1,148 @@ +/* + * 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.impl; + +import java.io.IOException; +import java.io.Serializable; +import java.net.InetAddress; +import java.net.Socket; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +import java.util.StringTokenizer; +import javax.net.SocketFactory; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; + +import org.apache.cassandra.utils.RMICloseableClientSocketFactory; + +/** + * {@code RMIClientSocketFactory} for testing SSL based JMX clients. + * This class is used to override the local address the JMX client calculates when trying to connect, + * which can otherwise be influenced by the system property "java.rmi.server.hostname" in strange and + * unpredictable ways. + */ +public class RMISslClientSocketFactoryImpl implements Serializable, RMICloseableClientSocketFactory +{ + private static final long serialVersionUID = 9054380061905145241L; + private static final List<Socket> sockets = new ArrayList<>(); + private static SocketFactory defaultSocketFactory = null; + private final InetAddress localAddress; + private final String enabledCipherSuites; + private final String enabledProtocols; + + public RMISslClientSocketFactoryImpl(InetAddress localAddress, String enabledCipherSuites, String enabledProtocls) + { + this.localAddress = localAddress; + this.enabledCipherSuites = enabledCipherSuites; + this.enabledProtocols = enabledProtocls; + } + + private static synchronized SocketFactory getDefaultClientSocketFactory() + { + if (defaultSocketFactory == null) + defaultSocketFactory = SSLSocketFactory.getDefault(); + return defaultSocketFactory; + } + + @Override + public Socket createSocket(String host, int port) throws IOException + { + Socket socket = createSslSocket(port); + sockets.add(socket); + return socket; + } + + private Socket createSslSocket(int port) throws IOException + { + final SocketFactory sslSocketFactory = getDefaultClientSocketFactory(); + final SSLSocket sslSocket = (SSLSocket) + sslSocketFactory.createSocket(localAddress, port); + if (enabledCipherSuites != null) + { + StringTokenizer st = new StringTokenizer(enabledCipherSuites, ","); + int tokens = st.countTokens(); + String[] enabledCipherSuitesList = new String[tokens]; + for (int i = 0; i < tokens; i++) + { + enabledCipherSuitesList[i] = st.nextToken(); + } + try + { + sslSocket.setEnabledCipherSuites(enabledCipherSuitesList); + } + catch (IllegalArgumentException e) + { + throw (IOException) + new IOException(e.getMessage(), e); + } + } + if (enabledProtocols != null) + { + StringTokenizer st = new StringTokenizer(enabledProtocols, ","); + int tokens = st.countTokens(); + String[] enabledProtocolsList = new String[tokens]; + for (int i = 0; i < tokens; i++) + { + enabledProtocolsList[i] = st.nextToken(); + } + try + { + sslSocket.setEnabledProtocols(enabledProtocolsList); + } + catch (IllegalArgumentException e) + { + throw (IOException) + new IOException(e.getMessage(), e); + } + } + return sslSocket; + } Review Comment: A few suggestions for simplifying this: 1) Extract the string splitting into a method, and maybe just use a regex: ``` private static final Pattern COMMA_SPLITTER = Pattern.compile(","); private String[] splitCommaSeparatedString(String stringToSplit) { if (stringToSplit == null) return null; return COMMA_SPLITTER.split(stringToSplit); } ``` 2) Change the two string members (`enabledCipherSuites` and `enabledProtocols`) to `String[]`s rather than just strings. 3) Split them in the constructor rather than every time through this method 4) Then you can just check for null and, if they are null, call the approbate `set` method directly. ########## test/distributed/org/apache/cassandra/distributed/impl/JmxTestClientSslContextFactory.java: ########## @@ -0,0 +1,145 @@ +/* + * 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.impl; + +import java.io.InputStream; +import java.nio.file.Files; +import java.security.KeyStore; +import java.util.Map; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManager; +import javax.net.ssl.TrustManagerFactory; + +import org.apache.commons.lang3.StringUtils; + +import org.apache.cassandra.config.EncryptionOptions; +import org.apache.cassandra.io.util.File; + +/** + * Simplied and independent version of {@link org.apache.cassandra.security.FileBasedSslContextFactory} for + * testing SSL based JMX clients that require configuring keystore and/or truststore. + */ +public class JmxTestClientSslContextFactory +{ + protected final Map<String, Object> parameters; + // keystore is not needed when the JMX server does not require client-auth + protected String keystore; + // keystore could be null in case JMX server does not require client-auth + protected String keystore_password; + protected final String truststore; + protected final String truststore_password; + protected final String protocol; + protected final String algorithm; + protected final String store_type; + + public JmxTestClientSslContextFactory(Map<String, Object> parameters) + { + this.parameters = parameters; + keystore = getString(EncryptionOptions.ConfigKey.KEYSTORE.toString()); + keystore_password = getString(EncryptionOptions.ConfigKey.KEYSTORE_PASSWORD.toString()); + truststore = getString(EncryptionOptions.ConfigKey.TRUSTSTORE.toString()); + truststore_password = getString(EncryptionOptions.ConfigKey.TRUSTSTORE_PASSWORD.toString()); + protocol = getString(EncryptionOptions.ConfigKey.PROTOCOL.toString(), "TLS"); + algorithm = getString(EncryptionOptions.ConfigKey.ALGORITHM.toString()); + store_type = getString(EncryptionOptions.ConfigKey.STORE_TYPE.toString(), "JKS"); + } + + protected String getString(String key, String defaultValue) + { + return parameters.get(key) == null ? defaultValue : (String) parameters.get(key); + } + + protected String getString(String key) + { + return (String) parameters.get(key); + } + + protected Boolean getBoolean(String key, boolean defaultValue) + { + return parameters.get(key) == null ? defaultValue : (Boolean) parameters.get(key); + } + + protected Boolean getBoolean(String key) + { + return (Boolean) this.parameters.get(key); + } Review Comment: both `getBoolean` methods are unused. ########## test/distributed/org/apache/cassandra/distributed/test/jmx/JMXSslConfigDistributedTest.java: ########## @@ -0,0 +1,202 @@ +/* + * 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.jmx; + +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.shared.WithProperties; +import org.apache.cassandra.distributed.test.AbstractEncryptionOptionsImpl; + +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() + { + // The below properties are set as side effect of other properties when tests run. Hence, these are reset here + // vs using try-with-resouces block + 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(); + } + + @Test + public void testDefaultEncryptionOptions() throws Throwable + { + // 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 + try (WithProperties ignored = new WithProperties().with("javax.net.ssl.trustStore", (String) validKeystore.get("truststore"), + "javax.net.ssl.trustStorePassword", (String) validKeystore.get("truststore_password"), + "javax.net.ssl.keyStore", (String) validKeystore.get("keystore"), + "javax.net.ssl.keyStorePassword", (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).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 Review Comment: NIT: I think you can remove these comments now that this code is refactored into the test utility. If not, at least please update the class name at the end of the comments to reference the place where the code lives. ########## test/distributed/org/apache/cassandra/distributed/test/jmx/JMXSslConfigDistributedTest.java: ########## @@ -0,0 +1,202 @@ +/* + * 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.jmx; + +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.shared.WithProperties; +import org.apache.cassandra.distributed.test.AbstractEncryptionOptionsImpl; + +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() + { + // The below properties are set as side effect of other properties when tests run. Hence, these are reset here + // vs using try-with-resouces block + 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(); Review Comment: This code is repeated several times in different tests. Please refactor it to JmxTestsUtil.java and call it from the different tests that use it. ########## test/unit/org/apache/cassandra/utils/jmx/JMXSslConfigTest.java: ########## @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.utils.jmx; + +import java.net.InetAddress; +import java.util.Map; +import javax.management.remote.rmi.RMIConnectorServer; +import javax.net.ssl.SSLException; +import javax.rmi.ssl.SslRMIServerSocketFactory; + +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.distributed.shared.WithProperties; +import org.apache.cassandra.utils.JMXServerUtils; + +import static org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_CONFIG; +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; + +/** + * Tests for Local JMX server, the remote JMX SSL configuration via System properties in absence of jmx_encryption_options + * in the cassandra.yaml. This is the behavior before CASSANDRA-18508. + */ +public class JMXSslConfigTest +{ + static WithProperties properties; + + @BeforeClass + public static void setupDatabaseDescriptor() + { + properties = new WithProperties().set(CASSANDRA_CONFIG, "cassandra.yaml"); + DatabaseDescriptor.daemonInitialization(); + } + + @AfterClass + public static void tearDownDatabaseDescriptor() + { + properties.close(); + } + + @After + public void resetJmxSslSystemProperties() + { + // The below properties are set as side effect of other properties when tests run. Hence, these are reset here + // vs using try-with-resouces block + JAVAX_RMI_SSL_CLIENT_ENABLED_PROTOCOLS.reset(); + JAVAX_RMI_SSL_CLIENT_ENABLED_CIPHER_SUITES.reset(); Review Comment: The fact that some of this is done here, and some with the `WithProperties` below let me to think there's a missing feature in `WithProperties` - if you add the following method there: ```java public WithProperties preserve(CassandraRelevantProperties prop) { String previous = prop.getString(); // because all properties are strings rollback.add(previous == null ? prop::clearValue : () -> prop.setString(previous)); return this; } ``` You can specify all of the properties you need to roll back in the single `WithProperties` block (and maybe could use that in other places where we're rolling back other properties?) I can take a look at this separately if you're not comfortable making this change, but it would be a nice addition to `WithProperties` ########## test/distributed/org/apache/cassandra/distributed/test/AbstractEncryptionOptionsImpl.java: ########## @@ -70,11 +70,16 @@ public class AbstractEncryptionOptionsImpl extends TestBaseImpl final static String validTrustStorePassword = TlsTestUtils.SERVER_TRUSTSTORE_PASSWORD; // Base configuration map for a valid keystore that can be opened - final static Map<String,Object> validKeystore = ImmutableMap.of("keystore", validKeyStorePath, + protected final static Map<String,Object> validKeystore = ImmutableMap.of("keystore", validKeyStorePath, Review Comment: NIT: This is a bit more than a valid _keystore_ - maybe call it `validFileBasedStores`? That way if we keep `validPEMKeystore` we can rename it `validPEMBasedStores`? ########## test/unit/org/apache/cassandra/utils/jmx/JMXSslDefaultEncryptionOptionsTest.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.management.remote.rmi.RMIConnectorServer; +import javax.net.ssl.SSLException; +import javax.rmi.ssl.SslRMIServerSocketFactory; + +import org.apache.commons.lang3.StringUtils; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.config.EncryptionOptions; +import org.apache.cassandra.distributed.shared.WithProperties; +import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.utils.JMXServerUtils; + +import static org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_CONFIG; +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; + +/** + * Tests for Default JMX SSL configuration specified in the cassandra.yaml using jmx_encryption_options. + * The default configurtion means keystore/truststore configured with file paths and using {@code DefaultSslContextFactory} + * to initialize the SSLContext. + */ +public class JMXSslDefaultEncryptionOptionsTest Review Comment: The `Default` name here really confused me for a while... can we call this test something like `JMXSslConfiguredWithYamlFileOptionsTest` or something more descriptive, as the _default_ is really no JMX encryption. It seems like `JMXSslConfigTest#testLocalJmxServer` represents the "default" configuration right? ########## test/unit/org/apache/cassandra/utils/jmx/JMXSslDisabledEncryptionOptionsTest.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.management.remote.rmi.RMIConnectorServer; +import javax.net.ssl.SSLException; + +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.distributed.shared.WithProperties; +import org.apache.cassandra.utils.JMXServerUtils; + +import static org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_CONFIG; +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; + +/** + * Tests for disabling jmx_encryption_options in the cassandra.yaml. + */ +public class JMXSslDisabledEncryptionOptionsTest +{ + static WithProperties properties; + + @BeforeClass + public static void setupDatabaseDescriptor() + { + properties = new WithProperties().set(CASSANDRA_CONFIG, "cassandra-jmx-disabled-sslconfig.yaml"); + DatabaseDescriptor.daemonInitialization(); + } + + @AfterClass + public static void tearDownDatabaseDescriptor() + { + properties.close(); + } + + @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(); + } + + /** + * Tests absence of all JMX SSL configurations, + * 1. local only JMX server + * 2. System properties set for remote JMX SSL + * 3. jmx_encryption_options in the cassandra.yaml + */ + @Test + public void testAbsenceOfAllJmxSslConfigs() throws SSLException Review Comment: The comments and the setup don't really match in this case - the _absense_ of all JMX SSL configuration should be no JMX config in cassandra.yaml, right? But this class loads a config that has those settings, but disabled. ########## test/unit/org/apache/cassandra/utils/jmx/JMXSslDisabledEncryptionOptionsTest.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.management.remote.rmi.RMIConnectorServer; +import javax.net.ssl.SSLException; + +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.distributed.shared.WithProperties; +import org.apache.cassandra.utils.JMXServerUtils; + +import static org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_CONFIG; +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; + +/** + * Tests for disabling jmx_encryption_options in the cassandra.yaml. + */ +public class JMXSslDisabledEncryptionOptionsTest +{ + static WithProperties properties; + + @BeforeClass + public static void setupDatabaseDescriptor() + { + properties = new WithProperties().set(CASSANDRA_CONFIG, "cassandra-jmx-disabled-sslconfig.yaml"); + DatabaseDescriptor.daemonInitialization(); + } + + @AfterClass + public static void tearDownDatabaseDescriptor() + { + properties.close(); + } + + @After + public void resetJmxSslSystemProperties() + { + COM_SUN_MANAGEMENT_JMXREMOTE_SSL.reset(); Review Comment: Same comment about moving these into the utility class. -- 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: pr-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org For additional commands, e-mail: pr-h...@cassandra.apache.org
