You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kafka.apache.org by jg...@apache.org on 2018/05/24 23:25:07 UTC
[kafka] branch trunk updated: KAFKA-6911;
Fix dynamic keystore/truststore update check (#5029)
This is an automated email from the ASF dual-hosted git repository.
jgus pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/kafka.git
The following commit(s) were added to refs/heads/trunk by this push:
new ff9f928 KAFKA-6911; Fix dynamic keystore/truststore update check (#5029)
ff9f928 is described below
commit ff9f928c16ddf95311f1c1badc64212b4975e623
Author: Rajini Sivaram <ra...@googlemail.com>
AuthorDate: Fri May 25 00:24:37 2018 +0100
KAFKA-6911; Fix dynamic keystore/truststore update check (#5029)
Fix the check, add unit test to verify the change, update `DynamicBrokerReconfigurationTest` to avoid dynamic keystore update in tests which are not expected to update keystores.
---
.../kafka/common/security/ssl/SslFactory.java | 14 +++----
.../common/network/SslTransportLayerTest.java | 47 +++++++++++++---------
.../server/DynamicBrokerReconfigurationTest.scala | 10 +++--
3 files changed, 40 insertions(+), 31 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 6989349..055404c 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
@@ -176,10 +176,10 @@ public class SslFactory implements Reconfigurable {
}
private SecurityStore maybeCreateNewKeystore(Map<String, ?> 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);
+ 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) {
return createKeystore((String) configs.get(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG),
@@ -191,9 +191,9 @@ public class SslFactory implements Reconfigurable {
}
private SecurityStore maybeCreateNewTruststore(Map<String, ?> configs) {
- boolean 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);
+ boolean 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) {
return createTruststore((String) configs.get(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG),
diff --git a/clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java b/clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
index f375dd6..2df4c4f 100644
--- a/clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
+++ b/clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
@@ -847,18 +847,14 @@ public class SslTransportLayerTest {
CertStores invalidCertStores = new CertStores(true, "server", "127.0.0.1");
Map<String, Object> invalidConfigs = invalidCertStores.getTrustingConfig(clientCertStores);
- try {
- reconfigurableBuilder.validateReconfiguration(invalidConfigs);
- fail("Should have failed validation with an exception with different SubjectAltName");
- } catch (KafkaException e) {
- // expected exception
- }
- try {
- reconfigurableBuilder.reconfigure(invalidConfigs);
- fail("Should have failed to reconfigure with different SubjectAltName");
- } catch (KafkaException e) {
- // expected exception
- }
+ verifyInvalidReconfigure(reconfigurableBuilder, invalidConfigs, "keystore with different SubjectAltName");
+
+ Map<String, Object> missingStoreConfigs = new HashMap<>();
+ missingStoreConfigs.put(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG, "PKCS12");
+ missingStoreConfigs.put(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG, "some.keystore.path");
+ missingStoreConfigs.put(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG, new Password("some.keystore.password"));
+ missingStoreConfigs.put(SslConfigs.SSL_KEY_PASSWORD_CONFIG, new Password("some.key.password"));
+ verifyInvalidReconfigure(reconfigurableBuilder, missingStoreConfigs, "keystore not found");
// Verify that new connections continue to work with the server with previously configured keystore after failed reconfiguration
newClientSelector.connect("3", addr, BUFFER_SIZE, BUFFER_SIZE);
@@ -911,22 +907,33 @@ public class SslTransportLayerTest {
Map<String, Object> invalidConfigs = new HashMap<>(newTruststoreConfigs);
invalidConfigs.put(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG, "INVALID_TYPE");
+ verifyInvalidReconfigure(reconfigurableBuilder, invalidConfigs, "invalid truststore type");
+
+ Map<String, Object> missingStoreConfigs = new HashMap<>();
+ missingStoreConfigs.put(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG, "PKCS12");
+ missingStoreConfigs.put(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG, "some.truststore.path");
+ missingStoreConfigs.put(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, new Password("some.truststore.password"));
+ verifyInvalidReconfigure(reconfigurableBuilder, missingStoreConfigs, "truststore not found");
+
+ // Verify that new connections continue to work with the server with previously configured keystore after failed reconfiguration
+ newClientSelector.connect("3", addr, BUFFER_SIZE, BUFFER_SIZE);
+ NetworkTestUtils.checkClientConnection(newClientSelector, "3", 100, 10);
+ }
+
+ private void verifyInvalidReconfigure(ListenerReconfigurable reconfigurable,
+ Map<String, Object> invalidConfigs, String errorMessage) {
try {
- reconfigurableBuilder.validateReconfiguration(invalidConfigs);
- fail("Should have failed validation with an exception with invalid truststore type");
+ reconfigurable.validateReconfiguration(invalidConfigs);
+ fail("Should have failed validation with an exception: " + errorMessage);
} catch (KafkaException e) {
// expected exception
}
try {
- reconfigurableBuilder.reconfigure(invalidConfigs);
- fail("Should have failed to reconfigure with with invalid truststore type");
+ reconfigurable.reconfigure(invalidConfigs);
+ fail("Should have failed to reconfigure: " + errorMessage);
} catch (KafkaException e) {
// expected exception
}
-
- // Verify that new connections continue to work with the server with previously configured keystore after failed reconfiguration
- newClientSelector.connect("3", addr, BUFFER_SIZE, BUFFER_SIZE);
- NetworkTestUtils.checkClientConnection(newClientSelector, "3", 100, 10);
}
private Selector createSelector(Map<String, Object> sslClientConfigs) {
diff --git a/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala b/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala
index fb96f9d..a4854ae 100644
--- a/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala
+++ b/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala
@@ -119,12 +119,13 @@ class DynamicBrokerReconfigurationTest extends ZooKeeperTestHarness with SaslSet
props ++= sslProperties1
props ++= securityProps(sslProperties1, KEYSTORE_PROPS, listenerPrefix(SecureInternal))
- // Set invalid static properties to ensure that dynamic config is used
+ // Set invalid top-level properties to ensure that listener config is used
+ // Don't set any dynamic configs here since they get overridden in tests
props ++= invalidSslProperties
- props ++= securityProps(invalidSslProperties, KEYSTORE_PROPS, listenerPrefix(SecureExternal))
+ props ++= securityProps(invalidSslProperties, KEYSTORE_PROPS, "")
+ props ++= securityProps(sslProperties1, KEYSTORE_PROPS, listenerPrefix(SecureExternal))
val kafkaConfig = KafkaConfig.fromProps(props)
- configureDynamicKeystoreInZooKeeper(kafkaConfig, Seq(brokerId), sslProperties1)
servers += TestUtils.createServer(kafkaConfig)
}
@@ -183,7 +184,7 @@ class DynamicBrokerReconfigurationTest extends ZooKeeperTestHarness with SaslSet
if (overrideCount > 0) {
val listenerPrefix = "listener.name.external.ssl."
verifySynonym(configName, synonyms.get(0), isSensitive, listenerPrefix, ConfigSource.DYNAMIC_BROKER_CONFIG, sslProperties1)
- verifySynonym(configName, synonyms.get(1), isSensitive, listenerPrefix, ConfigSource.STATIC_BROKER_CONFIG, invalidSslProperties)
+ verifySynonym(configName, synonyms.get(1), isSensitive, listenerPrefix, ConfigSource.STATIC_BROKER_CONFIG, sslProperties1)
}
verifySynonym(configName, synonyms.get(overrideCount), isSensitive, "ssl.", ConfigSource.STATIC_BROKER_CONFIG, invalidSslProperties)
defaultValue.foreach { value =>
@@ -204,6 +205,7 @@ class DynamicBrokerReconfigurationTest extends ZooKeeperTestHarness with SaslSet
}
val adminClient = adminClients.head
+ alterSslKeystoreUsingConfigCommand(sslProperties1, SecureExternal)
val configDesc = describeConfig(adminClient)
verifySslConfig("listener.name.external.", sslProperties1, configDesc)
--
To stop receiving notification emails like this one, please contact
jgus@apache.org.