[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695: URL: https://github.com/apache/kafka/pull/8695#discussion_r431678155 ## File path: tests/kafkatest/services/kafka/kafka.py ## @@ -352,17 +357,18 @@ def start_cmd(self, node): KafkaService.STDOUT_STDERR_CAPTURE) return cmd -def start_node(self, node, timeout_sec=60): +def start_node(self, node, timeout_sec=180): Review comment: No, sorry. Reverted. 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: us...@infra.apache.org
[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695: URL: https://github.com/apache/kafka/pull/8695#discussion_r431677154 ## File path: tests/kafkatest/services/kafka/kafka.py ## @@ -52,6 +51,7 @@ def advertised_listener(self, node): def listener_security_protocol(self): return "%s:%s" % (self.name, self.security_protocol) + Review comment: This change not necessary. But PyCharm and IDEA print warning without it: "PEP 8: E302 expected 2 blank lines, found 1" I will revert it. 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: us...@infra.apache.org
[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695: URL: https://github.com/apache/kafka/pull/8695#discussion_r431675317 ## File path: tests/kafkatest/benchmarks/core/benchmark_test.py ## @@ -236,9 +238,9 @@ def test_producer_and_consumer(self, compression_type="none", security_protocol= return data @cluster(num_nodes=6) -@parametrize(security_protocol='SSL', interbroker_security_protocol='PLAINTEXT') -@matrix(security_protocol=['PLAINTEXT', 'SSL'], compression_type=["none", "snappy"]) -def test_consumer_throughput(self, compression_type="none", security_protocol="PLAINTEXT", +@matrix(security_protocol=['SSL'], interbroker_security_protocol=['PLAINTEXT'], tls_version=['TLSv1.2', 'TLSv1.3'], compression_type=["none", "snappy"]) +@matrix(security_protocol=['PLAINTEXT'], compression_type=["none", "snappy"]) Review comment: OK for me. Will revert other changes. 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: us...@infra.apache.org
[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695:
URL: https://github.com/apache/kafka/pull/8695#discussion_r431674950
##
File path: tests/docker/run_tests.sh
##
@@ -30,6 +30,6 @@ if [ "$REBUILD" == "t" ]; then
fi
if ${SCRIPT_DIR}/ducker-ak ssh | grep -q '(none)'; then
-${SCRIPT_DIR}/ducker-ak up -n "${KAFKA_NUM_CONTAINERS}" || die "ducker-ak
up failed"
+${SCRIPT_DIR}/ducker-ak up -j 'openjdk:11' -n "${KAFKA_NUM_CONTAINERS}" ||
die "ducker-ak up failed"
Review comment:
I think this should be reverted prior merge.
Changed only to test PR on external server(my local machine too slow to run
all changed tests)
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:
us...@infra.apache.org
[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695:
URL: https://github.com/apache/kafka/pull/8695#discussion_r431668846
##
File path:
clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java
##
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.network;
+
+import java.net.InetSocketAddress;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.metrics.Metrics;
+import org.apache.kafka.common.security.TestSecurityConfig;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import org.apache.kafka.common.utils.Java;
+import org.apache.kafka.common.utils.LogContext;
+import org.apache.kafka.common.utils.Time;
+import org.apache.kafka.test.TestUtils;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Tests for the SSL transport layer.
+ * Checks different versions of the protocol usage on the server and client.
+ */
+@RunWith(value = Parameterized.class)
+public class SslVersionsTransportLayerTest {
+private static final int BUFFER_SIZE = 4 * 1024;
+private static final Time TIME = Time.SYSTEM;
+
+private final String tlsServerProtocol;
+private final String tlsClientProtocol;
+
+@Parameterized.Parameters(name =
"tlsServerProtocol={0},tlsClientProtocol={1}")
+public static Collection data() {
+List values = new ArrayList<>();
+values.add(new Object[] {"TLSv1.2", "TLSv1.2"});
+if (Java.IS_JAVA11_COMPATIBLE) {
+values.add(new Object[] {"TLSv1.2", "TLSv1.3"});
+values.add(new Object[] {"TLSv1.3", "TLSv1.2"});
+values.add(new Object[] {"TLSv1.3", "TLSv1.3"});
+}
+return values;
+}
+
+public SslVersionsTransportLayerTest(String tlsServerProtocol, String
tlsClientProtocol) {
+this.tlsServerProtocol = tlsServerProtocol;
+this.tlsClientProtocol = tlsClientProtocol;
+}
+
+/**
+ * Tests that connection success with the default TLS version.
+ */
+@Test
+public void testTLSDefaults() throws Exception {
Review comment:
Fixed.
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:
us...@infra.apache.org
[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695:
URL: https://github.com/apache/kafka/pull/8695#discussion_r428173640
##
File path:
clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##
@@ -622,6 +622,34 @@ public void testUnsupportedTLSVersion() throws Exception {
server.verifyAuthenticationMetrics(0, 1);
}
+/**
+ * Tests that connections can be made with TLSv1.2 and custom cipher suite.
+ */
+@Test
+public void testCiphersSuiteForTLSv1_2() throws Exception {
+String node = "0";
+SSLContext context = SSLContext.getInstance(tlsProtocol);
+context.init(null, null, null);
+
+//Note, that only some ciphers works out of the box. Others requires
additional configuration.
+String cipherSuite = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384";
+
+sslServerConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, "TLSv1.2");
+sslServerConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG,
Arrays.asList(SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS.split(",")));
Review comment:
Tests added.
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:
us...@infra.apache.org
[GitHub] [kafka] nizhikov commented on a change in pull request #8695: KAFKA-9320: KIP-573 - Enable TLSv1.3 by default
nizhikov commented on a change in pull request #8695:
URL: https://github.com/apache/kafka/pull/8695#discussion_r428114569
##
File path:
clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##
@@ -622,6 +622,34 @@ public void testUnsupportedTLSVersion() throws Exception {
server.verifyAuthenticationMetrics(0, 1);
}
+/**
+ * Tests that connections can be made with TLSv1.2 and custom cipher suite.
+ */
+@Test
+public void testCiphersSuiteForTLSv1_2() throws Exception {
+String node = "0";
+SSLContext context = SSLContext.getInstance(tlsProtocol);
+context.init(null, null, null);
+
+//Note, that only some ciphers works out of the box. Others requires
additional configuration.
+String cipherSuite = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384";
+
+sslServerConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, "TLSv1.2");
+sslServerConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG,
Arrays.asList(SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS.split(",")));
Review comment:
Hello. Sorry, I don't understand your concern :)
1. DEFAULT_SSL_ENABLED_PROTOCOLS = TLSv1.2,TLSv1.3 for java11+
2. DEFAULT_SSL_ENABLED_PROTOCOLS = TLSv1.2 for others jdk.
This property modified inside this test so I forcefully set it as default
value.
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:
us...@infra.apache.org
