[
https://issues.apache.org/jira/browse/KAFKA-6911?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16489975#comment-16489975
]
ASF GitHub Bot commented on KAFKA-6911:
---------------------------------------
hachikuji closed pull request #5029: KAFKA-6911: Fix dynamic
keystore/truststore update check
URL: https://github.com/apache/kafka/pull/5029
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 6989349fdbc..055404caab4 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 void reconfigure(Map<String, ?> configs) throws
KafkaException {
}
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 @@ private SecurityStore maybeCreateNewKeystore(Map<String, ?>
configs) {
}
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 f375dd68a75..2df4c4fe90f 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 void testServerKeystoreDynamicUpdate() throws
Exception {
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 void testServerTruststoreDynamicUpdate() throws
Exception {
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 fb96f9d1e91..a4854ae97a2 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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Incorrect check for keystore/truststore dynamic update
> ------------------------------------------------------
>
> Key: KAFKA-6911
> URL: https://issues.apache.org/jira/browse/KAFKA-6911
> Project: Kafka
> Issue Type: Task
> Components: core
> Affects Versions: 1.1.0
> Reporter: Rajini Sivaram
> Assignee: Rajini Sivaram
> Priority: Major
> Fix For: 2.0.0, 1.1.1
>
>
> The check to see if keystore or truststore needs updating is incorrect - it
> checks if one of the configs has not changed, rather than changed.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
