[GitHub] [kafka] ijuma commented on a change in pull request #8695: KAFKA-9320: Enable TLSv1.3 by default (KIP-573)

[GitBox <gi...@apache.org>]

ijuma commented on a change in pull request #8695:
URL: https://github.com/apache/kafka/pull/8695#discussion_r432987394



##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -354,15 +358,16 @@ def start_cmd(self, node):
 
     def start_node(self, node, timeout_sec=60):
         node.account.mkdirs(KafkaService.PERSISTENT_ROOT)
+
+        self.security_config.setup_node(node)
+        self.security_config.setup_credentials(node, self.path, self.zk_connect_setting(), broker=True)

Review comment:
       Why do we need this change?

##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -259,6 +267,9 @@ def setup_node(self, node):
         if self.has_sasl:
             self.setup_sasl(node)
 
+        if java_version(node) <= 9 and self.properties['tls.version'] == 'TLSv1.3':

Review comment:
       For consistency, shall we use `11` here?

##########
File path: clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##########
@@ -580,7 +581,16 @@ public void testTLSDefaults() throws Exception {
 
     @Test
     public void testUnsupportedCipher() throws Exception {
-        String[] cipherSuites = ((SSLServerSocketFactory) SSLServerSocketFactory.getDefault()).getSupportedCipherSuites();
+        String[] cipherSuites;
+        if (Java.IS_JAVA11_COMPATIBLE) {
+            cipherSuites = new String[] {
+                "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384",

Review comment:
       What is the reason for this?

##########
File path: tests/kafkatest/tests/core/replication_test.py
##########
@@ -126,9 +126,11 @@ def min_cluster_size(self):
             security_protocol="SASL_SSL", client_sasl_mechanism="SCRAM-SHA-256", interbroker_sasl_mechanism="SCRAM-SHA-512")
     @matrix(failure_mode=["clean_shutdown", "hard_shutdown", "clean_bounce", "hard_bounce"],
             security_protocol=["PLAINTEXT"], broker_type=["leader"], compression_type=["gzip"])
+    @matrix(failure_mode=["clean_shutdown", "hard_shutdown", "clean_bounce", "hard_bounce"],
+            security_protocol=["SSL"], broker_type=["leader"], compression_type=["gzip"], tls_version=["TLSv1.2", "TLSv1.3"])

Review comment:
       I think we can change one of the `SASL_SSL` entries to include multiple TLS versions. This way, we also verify that it works for SASL_SSL.

##########
File path: clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##########
@@ -622,6 +632,108 @@ public void testUnsupportedTLSVersion() throws Exception {
         server.verifyAuthenticationMetrics(0, 1);
     }
 
+    /**
+     * Tests that connections fails if TLSv1.3 enabled but cipher suite suitable only for TLSv1.2 used.
+     */
+    @Test
+    public void testCiphersSuiteForTls12_FailsForTls13() throws Exception {

Review comment:
       We normally don't have `_` in method names. Can we remove it from here and other test methods?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org