This is an automated email from the ASF dual-hosted git repository. ifesdjeen pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/cassandra.git
The following commit(s) were added to refs/heads/trunk by this push: new cbf4da4 CASSANDRA-14991 Follow-up: clean up SSL Hot Reloading code cbf4da4 is described below commit cbf4da4397c2cec34d6a240b0e917a847c46b3d0 Author: Dinesh A. Joshi <dinesh.jo...@apple.com> AuthorDate: Mon Feb 11 17:29:44 2019 -0800 CASSANDRA-14991 Follow-up: clean up SSL Hot Reloading code patch by Dinesh Joshi, reviewed by Alex Petrov for CASSANDRA-15018 --- .../cassandra/config/DatabaseDescriptor.java | 3 +- .../apache/cassandra/config/EncryptionOptions.java | 30 ++++++++++--- .../apache/cassandra/net/async/NettyFactory.java | 4 +- .../cassandra/net/async/OptionalSslHandler.java | 2 +- .../org/apache/cassandra/security/SSLFactory.java | 30 +++++++++---- .../org/apache/cassandra/transport/Server.java | 2 +- .../apache/cassandra/transport/SimpleClient.java | 5 +-- .../apache/cassandra/security/SSLFactoryTest.java | 52 ++++++++++------------ 8 files changed, 76 insertions(+), 52 deletions(-) diff --git a/src/java/org/apache/cassandra/config/DatabaseDescriptor.java b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java index 9b7a6e5..5c70c9a 100644 --- a/src/java/org/apache/cassandra/config/DatabaseDescriptor.java +++ b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java @@ -905,7 +905,8 @@ public class DatabaseDescriptor try { SSLFactory.initHotReloading(conf.server_encryption_options, conf.client_encryption_options, false); - } catch(IOException e) + } + catch(IOException e) { throw new ConfigurationException("Failed to initialize SSL hot reloading", e); } diff --git a/src/java/org/apache/cassandra/config/EncryptionOptions.java b/src/java/org/apache/cassandra/config/EncryptionOptions.java index 45579fb..9524cec 100644 --- a/src/java/org/apache/cassandra/config/EncryptionOptions.java +++ b/src/java/org/apache/cassandra/config/EncryptionOptions.java @@ -57,6 +57,10 @@ public class EncryptionOptions optional = options.optional; } + /** + * The method is being mainly used to cache SslContexts therefore, we only consider + * fields that would make a difference when the TrustStore or KeyStore files are updated + */ @Override public boolean equals(Object o) { @@ -66,23 +70,37 @@ public class EncryptionOptions return false; EncryptionOptions opt = (EncryptionOptions)o; - return Objects.equals(keystore, opt.keystore) && + return enabled == opt.enabled && + optional == opt.optional && + require_client_auth == opt.require_client_auth && + require_endpoint_verification == opt.require_endpoint_verification && + Objects.equals(keystore, opt.keystore) && + Objects.equals(keystore_password, opt.keystore_password) && Objects.equals(truststore, opt.truststore) && - Objects.equals(algorithm, opt.algorithm) && + Objects.equals(truststore_password, opt.truststore_password) && Objects.equals(protocol, opt.protocol) && - Arrays.equals(cipher_suites, opt.cipher_suites) && - require_client_auth == opt.require_client_auth && - require_endpoint_verification == opt.require_endpoint_verification; + Objects.equals(algorithm, opt.algorithm) && + Objects.equals(store_type, opt.store_type) && + Arrays.equals(cipher_suites, opt.cipher_suites); } + /** + * The method is being mainly used to cache SslContexts therefore, we only consider + * fields that would make a difference when the TrustStore or KeyStore files are updated + */ @Override public int hashCode() { int result = 0; result += 31 * (keystore == null ? 0 : keystore.hashCode()); + result += 31 * (keystore_password == null ? 0 : keystore_password.hashCode()); result += 31 * (truststore == null ? 0 : truststore.hashCode()); - result += 31 * (algorithm == null ? 0 : algorithm.hashCode()); + result += 31 * (truststore_password == null ? 0 : truststore_password.hashCode()); result += 31 * (protocol == null ? 0 : protocol.hashCode()); + result += 31 * (algorithm == null ? 0 : algorithm.hashCode()); + result += 31 * (store_type == null ? 0 : store_type.hashCode()); + result += 31 * Boolean.hashCode(enabled); + result += 31 * Boolean.hashCode(optional); result += 31 * Arrays.hashCode(cipher_suites); result += 31 * Boolean.hashCode(require_client_auth); result += 31 * Boolean.hashCode(require_endpoint_verification); diff --git a/src/java/org/apache/cassandra/net/async/NettyFactory.java b/src/java/org/apache/cassandra/net/async/NettyFactory.java index 3752927..346a067 100644 --- a/src/java/org/apache/cassandra/net/async/NettyFactory.java +++ b/src/java/org/apache/cassandra/net/async/NettyFactory.java @@ -292,7 +292,7 @@ public final class NettyFactory } else { - SslContext sslContext = SSLFactory.getSslContext(encryptionOptions, true, SSLFactory.SocketType.SERVER); + SslContext sslContext = SSLFactory.getOrCreateSslContext(encryptionOptions, true, SSLFactory.SocketType.SERVER); InetSocketAddress peer = encryptionOptions.require_endpoint_verification ? channel.remoteAddress() : null; SslHandler sslHandler = newSslHandler(channel, sslContext, peer); logger.trace("creating inbound netty SslContext: context={}, engine={}", sslContext.getClass().getName(), sslHandler.engine().getClass().getName()); @@ -369,7 +369,7 @@ public final class NettyFactory // order of handlers: ssl -> logger -> handshakeHandler if (params.encryptionOptions != null) { - SslContext sslContext = SSLFactory.getSslContext(params.encryptionOptions, true, SSLFactory.SocketType.CLIENT); + SslContext sslContext = SSLFactory.getOrCreateSslContext(params.encryptionOptions, true, SSLFactory.SocketType.CLIENT); // for some reason channel.remoteAddress() will return null InetAddressAndPort address = params.connectionId.remote(); InetSocketAddress peer = params.encryptionOptions.require_endpoint_verification ? new InetSocketAddress(address.address, address.port) : null; diff --git a/src/java/org/apache/cassandra/net/async/OptionalSslHandler.java b/src/java/org/apache/cassandra/net/async/OptionalSslHandler.java index 3b4f794..3fb8562 100644 --- a/src/java/org/apache/cassandra/net/async/OptionalSslHandler.java +++ b/src/java/org/apache/cassandra/net/async/OptionalSslHandler.java @@ -51,7 +51,7 @@ public class OptionalSslHandler extends ByteToMessageDecoder if (SslHandler.isEncrypted(in)) { // Connection uses SSL/TLS, replace the detection handler with a SslHandler and so use encryption. - SslContext sslContext = SSLFactory.getSslContext(encryptionOptions, true, SSLFactory.SocketType.SERVER); + SslContext sslContext = SSLFactory.getOrCreateSslContext(encryptionOptions, true, SSLFactory.SocketType.SERVER); Channel channel = ctx.channel(); InetSocketAddress peer = encryptionOptions.require_endpoint_verification ? (InetSocketAddress) channel.remoteAddress() : null; SslHandler sslHandler = NettyFactory.newSslHandler(channel, sslContext, peer); diff --git a/src/java/org/apache/cassandra/security/SSLFactory.java b/src/java/org/apache/cassandra/security/SSLFactory.java index 700142d..7519875 100644 --- a/src/java/org/apache/cassandra/security/SSLFactory.java +++ b/src/java/org/apache/cassandra/security/SSLFactory.java @@ -129,6 +129,15 @@ public final class SSLFactory lastModTime = curModTime; return result; } + + @Override + public String toString() + { + return "HotReloadableFile{" + + "file=" + file + + ", lastModTime=" + lastModTime + + '}'; + } } /** @@ -221,18 +230,18 @@ public final class SSLFactory /** * get a netty {@link SslContext} instance */ - public static SslContext getSslContext(EncryptionOptions options, boolean buildTruststore, - SocketType socketType) throws IOException + public static SslContext getOrCreateSslContext(EncryptionOptions options, boolean buildTruststore, + SocketType socketType) throws IOException { - return getSslContext(options, buildTruststore, socketType, OpenSsl.isAvailable()); + return getOrCreateSslContext(options, buildTruststore, socketType, OpenSsl.isAvailable()); } /** * Get a netty {@link SslContext} instance. */ @VisibleForTesting - static SslContext getSslContext(EncryptionOptions options, boolean buildTruststore, - SocketType socketType, boolean useOpenSsl) throws IOException + static SslContext getOrCreateSslContext(EncryptionOptions options, boolean buildTruststore, + SocketType socketType, boolean useOpenSsl) throws IOException { CacheKey key = new CacheKey(options, socketType); SslContext sslContext; @@ -302,7 +311,7 @@ public final class SSLFactory if (!isHotReloadingInitialized) throw new IllegalStateException("Hot reloading functionality has not been initialized."); - logger.trace("Checking whether certificates have been updated"); + logger.debug("Checking whether certificates have been updated {}", hotReloadableFiles); if (hotReloadableFiles.stream().anyMatch(HotReloadableFile::shouldReload)) { @@ -311,7 +320,8 @@ public final class SSLFactory { validateSslCerts(serverOpts, clientOpts); cachedSslContexts.clear(); - } catch(Exception e) + } + catch(Exception e) { logger.error("Failed to hot reload the SSL Certificates! Please check the certificate files.", e); } @@ -378,7 +388,8 @@ public final class SSLFactory createNettySslContext(serverOpts, true, SocketType.SERVER, OpenSsl.isAvailable()); createNettySslContext(serverOpts, true, SocketType.CLIENT, OpenSsl.isAvailable()); } - } catch (Exception e) + } + catch (Exception e) { throw new IOException("Failed to create SSL context using server_encryption_options!", e); } @@ -391,7 +402,8 @@ public final class SSLFactory createNettySslContext(clientOpts, clientOpts.require_client_auth, SocketType.SERVER, OpenSsl.isAvailable()); createNettySslContext(clientOpts, clientOpts.require_client_auth, SocketType.CLIENT, OpenSsl.isAvailable()); } - } catch (Exception e) + } + catch (Exception e) { throw new IOException("Failed to create SSL context using client_encryption_options!", e); } diff --git a/src/java/org/apache/cassandra/transport/Server.java b/src/java/org/apache/cassandra/transport/Server.java index 056a4a0..33cd0fb 100644 --- a/src/java/org/apache/cassandra/transport/Server.java +++ b/src/java/org/apache/cassandra/transport/Server.java @@ -406,7 +406,7 @@ public class Server implements CassandraDaemon.Server protected final SslHandler createSslHandler(ByteBufAllocator allocator) throws IOException { - SslContext sslContext = SSLFactory.getSslContext(encryptionOptions, encryptionOptions.require_client_auth, SSLFactory.SocketType.SERVER); + SslContext sslContext = SSLFactory.getOrCreateSslContext(encryptionOptions, encryptionOptions.require_client_auth, SSLFactory.SocketType.SERVER); return sslContext.newHandler(allocator); } } diff --git a/src/java/org/apache/cassandra/transport/SimpleClient.java b/src/java/org/apache/cassandra/transport/SimpleClient.java index 0db9136..9ed4272 100644 --- a/src/java/org/apache/cassandra/transport/SimpleClient.java +++ b/src/java/org/apache/cassandra/transport/SimpleClient.java @@ -25,7 +25,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.concurrent.BlockingQueue; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.SynchronousQueue; @@ -292,8 +291,8 @@ public class SimpleClient implements Closeable protected void initChannel(Channel channel) throws Exception { super.initChannel(channel); - SslContext sslContext = SSLFactory.getSslContext(encryptionOptions, encryptionOptions.require_client_auth, - SSLFactory.SocketType.CLIENT); + SslContext sslContext = SSLFactory.getOrCreateSslContext(encryptionOptions, encryptionOptions.require_client_auth, + SSLFactory.SocketType.CLIENT); channel.pipeline().addFirst("ssl", sslContext.newHandler(channel.alloc())); } } diff --git a/test/unit/org/apache/cassandra/security/SSLFactoryTest.java b/test/unit/org/apache/cassandra/security/SSLFactoryTest.java index b253c59..5fdbe7b 100644 --- a/test/unit/org/apache/cassandra/security/SSLFactoryTest.java +++ b/test/unit/org/apache/cassandra/security/SSLFactoryTest.java @@ -96,7 +96,7 @@ public class SSLFactoryTest } EncryptionOptions options = addKeystoreOptions(encryptionOptions); - SslContext sslContext = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, true); + SslContext sslContext = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, true); Assert.assertNotNull(sslContext); Assert.assertTrue(sslContext instanceof OpenSslContext); } @@ -105,7 +105,7 @@ public class SSLFactoryTest public void getSslContext_JdkSsl() throws IOException { EncryptionOptions options = addKeystoreOptions(encryptionOptions); - SslContext sslContext = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, false); + SslContext sslContext = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, false); Assert.assertNotNull(sslContext); Assert.assertTrue(sslContext instanceof JdkSslContext); Assert.assertEquals(Arrays.asList(encryptionOptions.cipher_suites), sslContext.cipherSuites()); @@ -174,16 +174,16 @@ public class SSLFactoryTest SSLFactory.initHotReloading((ServerEncryptionOptions) options, options, true); - SslContext oldCtx = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl + SslContext oldCtx = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl .isAvailable()); File keystoreFile = new File(options.keystore); SSLFactory.checkCertFilesForHotReloading((ServerEncryptionOptions) options, options); - Thread.sleep(5000); - FileUtils.touch(keystoreFile); + + keystoreFile.setLastModified(System.currentTimeMillis() + 15000); SSLFactory.checkCertFilesForHotReloading((ServerEncryptionOptions) options, options);; - SslContext newCtx = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl + SslContext newCtx = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl .isAvailable()); Assert.assertNotSame(oldCtx, newCtx); @@ -209,36 +209,31 @@ public class SSLFactoryTest } @Test - public void testSslFactoryHotReload_BadPassword_DoesNotClearExistingSslContext() throws IOException, - InterruptedException + public void testSslFactoryHotReload_BadPassword_DoesNotClearExistingSslContext() throws IOException { try { addKeystoreOptions(encryptionOptions); - EncryptionOptions options = new ServerEncryptionOptions(encryptionOptions); + ServerEncryptionOptions options = new ServerEncryptionOptions(encryptionOptions); options.enabled = true; - SSLFactory.initHotReloading((ServerEncryptionOptions) options, options, true); - SslContext oldCtx = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl + SSLFactory.initHotReloading(options, options, true); + SslContext oldCtx = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl .isAvailable()); File keystoreFile = new File(options.keystore); - SSLFactory.checkCertFilesForHotReloading((ServerEncryptionOptions) options, options); - Thread.sleep(5000); - FileUtils.touch(keystoreFile); + SSLFactory.checkCertFilesForHotReloading(options, options); + keystoreFile.setLastModified(System.currentTimeMillis() + 5000); - options.keystore_password = "bad password"; - SSLFactory.checkCertFilesForHotReloading((ServerEncryptionOptions) options, options);; - SslContext newCtx = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl + ServerEncryptionOptions modOptions = new ServerEncryptionOptions(options); + modOptions.keystore_password = "bad password"; + SSLFactory.checkCertFilesForHotReloading(modOptions, modOptions); + SslContext newCtx = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl .isAvailable()); Assert.assertSame(oldCtx, newCtx); } - catch (Exception e) - { - throw e; - } finally { DatabaseDescriptor.loadConfig(); @@ -261,16 +256,15 @@ public class SSLFactoryTest options.enabled = true; SSLFactory.initHotReloading((ServerEncryptionOptions) options, options, true); - SslContext oldCtx = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl + SslContext oldCtx = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl .isAvailable()); SSLFactory.checkCertFilesForHotReloading((ServerEncryptionOptions) options, options); - Thread.sleep(5000); - FileUtils.touch(testKeystoreFile); + testKeystoreFile.setLastModified(System.currentTimeMillis() + 15000); FileUtils.forceDelete(testKeystoreFile); SSLFactory.checkCertFilesForHotReloading((ServerEncryptionOptions) options, options);; - SslContext newCtx = SSLFactory.getSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl + SslContext newCtx = SSLFactory.getOrCreateSslContext(options, true, SSLFactory.SocketType.CLIENT, OpenSsl .isAvailable()); Assert.assertSame(oldCtx, newCtx); @@ -293,16 +287,16 @@ public class SSLFactoryTest options.enabled = true; options.cipher_suites = new String[]{ "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256" }; - SslContext ctx1 = SSLFactory.getSslContext(options, true, - SSLFactory.SocketType.SERVER, OpenSsl.isAvailable()); + SslContext ctx1 = SSLFactory.getOrCreateSslContext(options, true, + SSLFactory.SocketType.SERVER, OpenSsl.isAvailable()); Assert.assertTrue(ctx1.isServer()); Assert.assertArrayEquals(ctx1.cipherSuites().toArray(), options.cipher_suites); options.cipher_suites = new String[]{ "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256" }; - SslContext ctx2 = SSLFactory.getSslContext(options, true, - SSLFactory.SocketType.CLIENT, OpenSsl.isAvailable()); + SslContext ctx2 = SSLFactory.getOrCreateSslContext(options, true, + SSLFactory.SocketType.CLIENT, OpenSsl.isAvailable()); Assert.assertTrue(ctx2.isClient()); Assert.assertArrayEquals(ctx2.cipherSuites().toArray(), options.cipher_suites); --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org