jonmeredith commented on a change in pull request #1027: URL: https://github.com/apache/cassandra/pull/1027#discussion_r688530766
########## File path: src/java/org/apache/cassandra/security/DefaultSslContextFactory.java ########## @@ -0,0 +1,203 @@ +/* + * 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.security; + +import java.io.File; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Date; +import java.util.Enumeration; +import java.util.List; +import java.util.Map; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManagerFactory; + +import com.google.common.annotations.VisibleForTesting; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Cassandra's default implementation class for the configuration key {@code ssl_context_factory}. It uses + * file based keystores. + */ +public final class DefaultSslContextFactory extends AbstractSslContextFactory +{ + private static final Logger logger = LoggerFactory.getLogger(DefaultSslContextFactory.class); + + @VisibleForTesting + volatile boolean checkedExpiry = false; + + /** + * List of files that trigger hot reloading of SSL certificates + */ + private volatile List<HotReloadableFile> hotReloadableFiles = new ArrayList<>(); + + private final String keystore; + private final String keystore_password; + private final String truststore; + private final String truststore_password; + + public DefaultSslContextFactory() + { + keystore = "conf/.keystore"; + keystore_password = "cassandra"; + truststore = "conf/.truststore"; + truststore_password = "cassandra"; + } + + public DefaultSslContextFactory(Map<String, Object> parameters) + { + super(parameters); + keystore = getString("keystore"); + keystore_password = getString("keystore_password"); + truststore = getString("truststore"); + truststore_password = getString("truststore_password"); + } + + @Override + public boolean shouldReload() + { + return hotReloadableFiles.stream().anyMatch(HotReloadableFile::shouldReload); + } + + @Override + public boolean hasKeystore() { + return keystore != null && new File(keystore).exists(); + } + + private boolean hasTruststore() { + return truststore != null && new File(truststore).exists(); + } + + @Override + public synchronized void initHotReloading() { + boolean hasKeystore = hasKeystore(); + boolean hasTruststore = hasTruststore(); + + if (hasKeystore || hasTruststore) { + List<HotReloadableFile> fileList = new ArrayList<>(); + if ( hasKeystore ) + { + fileList.add(new HotReloadableFile(keystore)); + } + if ( hasTruststore ) { + fileList.add(new HotReloadableFile(truststore)); + } + hotReloadableFiles = fileList; + } + } + + /** + * Builds required KeyManagerFactory from the file based keystore. It also checks for the PrivateKey's certificate's + * expiry and logs {@code warning} for each expired PrivateKey's certitificate. + * + * @return KeyManagerFactory built from the file based keystore. + * @throws SSLException if any issues encountered during the build process + */ + protected KeyManagerFactory buildKeyManagerFactory() throws SSLException Review comment: I don't think there's any point having protected members in a `final` class. ########## File path: src/java/org/apache/cassandra/security/ISslContextFactory.java ########## @@ -0,0 +1,118 @@ +/* + * 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.security; + +import java.util.List; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLException; + +import io.netty.handler.ssl.CipherSuiteFilter; +import io.netty.handler.ssl.SslContext; + +/** + * The purpose of this interface is to provide pluggable mechanism for creating custom JSSE and Netty SSLContext + * objects. Please use the Cassandra configuration key {@code ssl_context_factory} as part of {@code + * client_encryption_options}/{@code server_encryption_options} and provide a custom class-name implementing this + * interface with parameters to be used to plugin a your own way to load the SSLContext. + * + * Implementation of this interface must have a constructor with argument of type {@code Map<String,Object>} to allow + * custom parameters, needed by the implementation, to be passed from the yaml configuration. Common SSL + * configurations like {@code protocol, algorithm, cipher_suites, accepted_protocols, require_client_auth, + * require_endpoint_verification, enabled, optional} will also be passed to that map by Cassanddra. + * + * Since on top of Netty, Cassandra is internally using JSSE SSLContext also for certain use-cases- this interface + * has methods for both. + * + * Below is an example of how to configure a custom implementation with parameters + * <pre> + * ssl_context_factory: + * class_name: org.apache.cassandra.security.YourSslContextFactoryImpl + * parameters: + * key1: "value1" + * key2: "value2" + * key3: "value3" + * </pre> + */ +public interface ISslContextFactory +{ + /** + * Creates JSSE SSLContext. + * + * @param buildTruststore {@code true} if the caller requires Truststore; {@code false} otherwise + * @return JSSE's {@link SSLContext} + * @throws SSLException in case the Ssl Context creation fails for some reason + */ + SSLContext createJSSESslContext(boolean buildTruststore) throws SSLException; + + /** + * Creates Netty's SslContext object. + * + * @param buildTruststore {@code true} if the caller requires Truststore; {@code false} otherwise + * @param socketType {@link SocketType} for Netty's Inbound or Outbound channels + * @param useOpenSsl {@code true} if openSsl is enabled;{@code false} otherwise Review comment: I think that would probably work. There's a very small time-of-check, time-of-use issue that I'm not sure if it would be a problem in that you say 'ready to reload' and 'reload'. CEP-9 was considering the use case where the key/trust store materials are retrieved from some remote service. It's probably ok to poll for changes and load, and probably just as bad as the local filesystem as you can't atomically replace both the keystone and truststore at the same time if they were separate files. If the implementation does have to track some kind of state about whether to reload, when should that be cleared? Is there a contract with the interface that if `shouldReload()` is called the SSLContext will be revalidated and the cache cleared? So the other thing that could be an issue is configurations where different SSL factories are used for the client and internode connections. At the moment if either of the configurations flags a reload, then both are reloaded which may be unexpected by an implementation. I think it would be cleaner to move all of the hot-loading stuff moved out to something like `HotReloadingSslFactory` and just expose an `SSLFactory.validateAndRefreshCache` method that can be called to indicate the cache is now stale and can be cleared. The 'cache' itself is a bit weird, only likely containing at most eight items, and more likely just two. ########## File path: src/java/org/apache/cassandra/security/DefaultSslContextFactory.java ########## @@ -0,0 +1,203 @@ +/* + * 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.security; + +import java.io.File; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Date; +import java.util.Enumeration; +import java.util.List; +import java.util.Map; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManagerFactory; + +import com.google.common.annotations.VisibleForTesting; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Cassandra's default implementation class for the configuration key {@code ssl_context_factory}. It uses + * file based keystores. + */ +public final class DefaultSslContextFactory extends AbstractSslContextFactory Review comment: Thanks - this is along the lines of what I was thinking. I think having the `DefaultSslContextFactory` as final is a good thing but it would prevent the use case of somebody who just wants to just do minor tweaks the the netty SSL context and keep all of the netty machinery in place. What do you think about renaming this to a non-final `HotReloadingSslContextFactory` class and making a shell `final class DefaultSslContextFactory extends HotReloadingSslContextFactory`? Either that or we drop the `final` from this class so it can be extended, but not sure if that opens up any security issues - probably not as an attacker would have had sufficient credentials to alter the config to pick up a hostile parameterized class anyway. -- 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]
