From 7f9b9a60dab990efb79df89a6603e78d9ce5a34f Mon Sep 17 00:00:00 2001 From: Rajini Sivaram Date: Wed, 17 Apr 2019 18:17:40 +0100 Subject: [PATCH] KAFKA-8241; Handle configs without truststore for broker keystore update (#6585) Reviewers: Manikumar Reddy --- .../kafka/common/security/ssl/SslFactory.java | 41 +++++++----- .../common/security/ssl/SslFactoryTest.java | 67 +++++++++++++++++++ 2 files changed, 93 insertions(+), 15 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java b/clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java index 0c5094df4a5..a03d1bcae8b 100644 --- a/clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java +++ b/clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java @@ -161,7 +161,8 @@ public class SslFactory implements Reconfigurable { createSSLContext(keystore, truststore); } } catch (Exception e) { - throw new ConfigException("Validation of dynamic config update failed", e); + log.debug("Validation of dynamic config update of SSL keystore/truststore failed", e); + throw new ConfigException("Validation of dynamic config update of SSL keystore/truststore failed: " + e); } } @@ -178,21 +179,24 @@ public class SslFactory implements Reconfigurable { this.keystore = keystore; this.truststore = truststore; } catch (Exception e) { - throw new ConfigException("Reconfiguration of SSL keystore/truststore failed", e); + log.debug("Reconfiguration of SSL keystore/truststore failed", e); + throw new ConfigException("Reconfiguration of SSL keystore/truststore failed: " + e); } } } private SecurityStore maybeCreateNewKeystore(Map configs) { - boolean keystoreChanged = !Objects.equals(configs.get(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG), keystore.type) || - !Objects.equals(configs.get(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG), keystore.path) || - !Objects.equals(configs.get(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG), keystore.password) || - !Objects.equals(configs.get(SslConfigs.SSL_KEY_PASSWORD_CONFIG), keystore.keyPassword); - - if (!keystoreChanged) { - keystoreChanged = keystore.modified(); + boolean keystoreChanged = false; + if (keystore != null) { + keystoreChanged = !Objects.equals(configs.get(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG), keystore.type) || + !Objects.equals(configs.get(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG), keystore.path) || + !Objects.equals(configs.get(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG), keystore.password) || + !Objects.equals(configs.get(SslConfigs.SSL_KEY_PASSWORD_CONFIG), keystore.keyPassword); + if (!keystoreChanged) { + keystoreChanged = keystore.modified(); + } } - if (keystoreChanged) { + if (keystoreChanged || keystore == null) { return createKeystore((String) configs.get(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG), (String) configs.get(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG), (Password) configs.get(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG), @@ -202,14 +206,18 @@ public class SslFactory implements Reconfigurable { } private SecurityStore maybeCreateNewTruststore(Map configs) { - boolean truststoreChanged = !Objects.equals(configs.get(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG), truststore.type) || + boolean truststoreChanged = false; + if (truststore != null) { + truststoreChanged = !Objects.equals(configs.get(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG), truststore.type) || !Objects.equals(configs.get(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG), truststore.path) || !Objects.equals(configs.get(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG), truststore.password); - if (!truststoreChanged) { - truststoreChanged = truststore.modified(); + if (!truststoreChanged) { + truststoreChanged = truststore.modified(); + } } - if (truststoreChanged) { + + if (truststoreChanged || truststore == null) { return createTruststore((String) configs.get(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG), (String) configs.get(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG), (Password) configs.get(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG)); @@ -244,8 +252,11 @@ public class SslFactory implements Reconfigurable { boolean verifyKeystore = keystore != null && keystore != this.keystore; boolean verifyTruststore = truststore != null && truststore != this.truststore; if (verifyKeystore || verifyTruststore) { - if (this.keystore == null) + if (this.keystore == null && keystore != null) throw new ConfigException("Cannot add SSL keystore to an existing listener for which no keystore was configured."); + if (this.truststore == null && truststore != null) + throw new ConfigException("Cannot add SSL truststore to an existing listener for which no truststore was configured."); + if (keystoreVerifiableUsingTruststore) { SSLConfigValidatorEngine.validate(this, sslContext, this.sslContext); SSLConfigValidatorEngine.validate(this, this.sslContext, sslContext); diff --git a/clients/src/test/java/org/apache/kafka/common/security/ssl/SslFactoryTest.java b/clients/src/test/java/org/apache/kafka/common/security/ssl/SslFactoryTest.java index 6c4a2391c8b..11035d05514 100644 --- a/clients/src/test/java/org/apache/kafka/common/security/ssl/SslFactoryTest.java +++ b/clients/src/test/java/org/apache/kafka/common/security/ssl/SslFactoryTest.java @@ -25,6 +25,7 @@ import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLHandshakeException; +import org.apache.kafka.common.config.ConfigException; import org.apache.kafka.common.config.SslConfigs; import org.apache.kafka.common.config.types.Password; import org.apache.kafka.test.TestSslUtils; @@ -134,6 +135,72 @@ public class SslFactoryTest { assertSame("SSL context recreated unnecessarily", sslContext, sslFactory.sslContext()); } + @Test + public void testReconfigurationWithoutTruststore() throws Exception { + File trustStoreFile = File.createTempFile("truststore", ".jks"); + Map sslConfig = TestSslUtils + .createSslConfig(false, true, Mode.SERVER, trustStoreFile, "server"); + sslConfig.remove(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG); + sslConfig.remove(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG); + sslConfig.remove(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG); + SslFactory sslFactory = new SslFactory(Mode.SERVER); + sslFactory.configure(sslConfig); + SSLContext sslContext = sslFactory.sslContext(); + assertNotNull("SSL context not created", sslContext); + assertSame("SSL context recreated unnecessarily", sslContext, sslFactory.sslContext()); + assertFalse(sslFactory.createSslEngine("localhost", 0).getUseClientMode()); + + trustStoreFile = File.createTempFile("truststore", ".jks"); + sslConfig = TestSslUtils.createSslConfig(false, true, Mode.SERVER, trustStoreFile, "server"); + sslConfig.remove(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG); + sslConfig.remove(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG); + sslConfig.remove(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG); + sslFactory.reconfigure(sslConfig); + assertNotSame("SSL context not recreated", sslContext, sslFactory.sslContext()); + + trustStoreFile = File.createTempFile("truststore", ".jks"); + sslConfig = TestSslUtils.createSslConfig(false, true, Mode.SERVER, trustStoreFile, "server"); + try { + sslFactory.reconfigure(sslConfig); + fail("Truststore configured dynamically for listener without previous truststore"); + } catch (ConfigException e) { + // Expected exception + } + } + + @Test + public void testReconfigurationWithoutKeystore() throws Exception { + File trustStoreFile = File.createTempFile("truststore", ".jks"); + Map sslConfig = TestSslUtils + .createSslConfig(false, true, Mode.SERVER, trustStoreFile, "server"); + sslConfig.remove(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG); + sslConfig.remove(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG); + sslConfig.remove(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG); + SslFactory sslFactory = new SslFactory(Mode.SERVER); + sslFactory.configure(sslConfig); + SSLContext sslContext = sslFactory.sslContext(); + assertNotNull("SSL context not created", sslContext); + assertSame("SSL context recreated unnecessarily", sslContext, sslFactory.sslContext()); + assertFalse(sslFactory.createSslEngine("localhost", 0).getUseClientMode()); + + trustStoreFile = File.createTempFile("truststore", ".jks"); + sslConfig = TestSslUtils.createSslConfig(false, true, Mode.SERVER, trustStoreFile, "server"); + sslConfig.remove(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG); + sslConfig.remove(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG); + sslConfig.remove(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG); + sslFactory.reconfigure(sslConfig); + assertNotSame("SSL context not recreated", sslContext, sslFactory.sslContext()); + + trustStoreFile = File.createTempFile("truststore", ".jks"); + sslConfig = TestSslUtils.createSslConfig(false, true, Mode.SERVER, trustStoreFile, "server"); + try { + sslFactory.reconfigure(sslConfig); + fail("Keystore configured dynamically for listener without previous keystore"); + } catch (ConfigException e) { + // Expected exception + } + } + @Test public void testKeyStoreTrustStoreValidation() throws Exception { File trustStoreFile = File.createTempFile("truststore", ".jks");