-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-9320: Enable TLSv1.3 by default (KIP-573) #8695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 24 commits
18a1bca
1076e51
7dec0d6
f6afbb9
142e487
ac448d1
e1287c6
b310e60
518eb77
5b5f37e
c7000d9
862f7ae
d1dd114
5578192
c1847e7
c901254
61cd6c5
fd1f48b
e1a2fe4
4e7eaec
c756720
a231e2f
7ab2f39
17612ac
ebb20e1
1b55587
9da1c21
3e6c445
14bf85a
67f0ef9
869e342
ca81fcd
6a82441
ce7505f
b293578
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,7 @@ | |
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
| import static org.junit.Assume.assumeTrue; | ||
|
|
||
| /** | ||
| * Tests for the SSL transport layer. These use a test harness that runs a simple socket server that echos back responses. | ||
|
|
@@ -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", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason for this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use correct cipher for the server(which uses
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous logic is weird, I agree. I think the idea here is to simply pick a different supported cipher in the server vs the client. I think we can drop Also, we should change the following to simply use the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, note that we also have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| ((SSLServerSocketFactory) SSLServerSocketFactory.getDefault()).getSupportedCipherSuites()[1] | ||
| }; | ||
| } else { | ||
| cipherSuites = ((SSLServerSocketFactory) SSLServerSocketFactory.getDefault()).getSupportedCipherSuites(); | ||
| } | ||
|
|
||
| if (cipherSuites != null && cipherSuites.length > 1) { | ||
| sslServerConfigs = serverCertStores.getTrustingConfig(clientCertStores); | ||
| sslServerConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuites[0])); | ||
|
|
@@ -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 testCiphersSuiteForTls12FailsForTls13() throws Exception { | ||
| assumeTrue(Java.IS_JAVA11_COMPATIBLE); | ||
|
|
||
| SSLContext context = SSLContext.getInstance(tlsProtocol); | ||
|
nizhikov marked this conversation as resolved.
Outdated
|
||
| context.init(null, null, null); | ||
|
nizhikov marked this conversation as resolved.
Outdated
|
||
|
|
||
| String cipherSuite = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"; | ||
|
|
||
| sslServerConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, "TLSv1.3"); | ||
| sslServerConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Collections.singletonList("TLSv1.3")); | ||
| sslServerConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuite)); | ||
| server = createEchoServer(SecurityProtocol.SSL); | ||
|
|
||
| sslClientConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Collections.singletonList("TLSv1.3")); | ||
| sslClientConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuite)); | ||
|
|
||
| checkAuthentiationFailed("0", "TLSv1.3"); | ||
| server.verifyAuthenticationMetrics(0, 1); | ||
| } | ||
|
|
||
| /** | ||
| * Tests that connections can't be made if server uses TLSv1.2 with custom cipher suite and client uses TLSv1.3. | ||
| */ | ||
| @Test | ||
| public void testCiphersSuiteFailForServerTls12ClientTls13() throws Exception { | ||
| assumeTrue(Java.IS_JAVA11_COMPATIBLE); | ||
|
|
||
| String tls12CipherSuite = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"; | ||
| String tls13CipherSuite = "TLS_AES_128_GCM_SHA256"; | ||
|
|
||
| sslServerConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, "TLSv1.2"); | ||
| sslServerConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Collections.singletonList("TLSv1.2")); | ||
| sslServerConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(tls12CipherSuite)); | ||
| server = createEchoServer(SecurityProtocol.SSL); | ||
|
|
||
| sslClientConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, "TLSv1.3"); | ||
| sslClientConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(tls13CipherSuite)); | ||
|
|
||
| checkAuthentiationFailed("0", "TLSv1.3"); | ||
|
nizhikov marked this conversation as resolved.
Outdated
|
||
| server.verifyAuthenticationMetrics(0, 1); | ||
| } | ||
|
|
||
| /** | ||
| * Tests that connections can be made with TLSv1.3 cipher suite. | ||
| */ | ||
| @Test | ||
| public void testCiphersSuiteForTls13() throws Exception { | ||
|
nizhikov marked this conversation as resolved.
Outdated
|
||
| assumeTrue(Java.IS_JAVA11_COMPATIBLE); | ||
|
|
||
| String node = "0"; | ||
| SSLContext context = SSLContext.getInstance(tlsProtocol); | ||
| context.init(null, null, null); | ||
|
|
||
| String cipherSuite = "TLS_AES_128_GCM_SHA256"; | ||
|
|
||
| sslServerConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, SslConfigs.DEFAULT_SSL_PROTOCOL); | ||
| sslServerConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Arrays.asList(SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS.split(","))); | ||
| sslServerConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuite)); | ||
| server = createEchoServer(SecurityProtocol.SSL); | ||
|
|
||
| sslClientConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, SslConfigs.DEFAULT_SSL_PROTOCOL); | ||
| sslClientConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Arrays.asList(SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS.split(","))); | ||
| sslClientConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuite)); | ||
| createSelector(sslClientConfigs); | ||
| InetSocketAddress addr = new InetSocketAddress("localhost", server.port()); | ||
| selector.connect(node, addr, BUFFER_SIZE, BUFFER_SIZE); | ||
|
|
||
| NetworkTestUtils.waitForChannelClose(selector, node, ChannelState.State.READY); | ||
| server.verifyAuthenticationMetrics(1, 0); | ||
| } | ||
|
|
||
| /** | ||
| * Tests that connections can be made with TLSv1.2 cipher suite. | ||
| */ | ||
| @Test | ||
| public void testCiphersSuiteForTls12() throws Exception { | ||
| String node = "0"; | ||
| SSLContext context = SSLContext.getInstance(tlsProtocol); | ||
| context.init(null, null, null); | ||
|
|
||
| String cipherSuite = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"; | ||
|
|
||
| sslServerConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, SslConfigs.DEFAULT_SSL_PROTOCOL); | ||
| sslServerConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Arrays.asList(SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS.split(","))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you want to leave this as the default and see if it works correctly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello. Sorry, I don't understand your concern :)
This property modified inside this test so I forcefully set it as default value.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see, you are forcefully setting it to the default. Makes sense. OK, so this test shows that we can negotiate successfully even though we have no cipher suites that work with TLS 1.3. Can we also test that if the client sets TLS 1.3, it will fail?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests added.
nizhikov marked this conversation as resolved.
Outdated
|
||
| sslServerConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuite)); | ||
| server = createEchoServer(SecurityProtocol.SSL); | ||
|
|
||
| sslClientConfigs.put(SslConfigs.SSL_PROTOCOL_CONFIG, SslConfigs.DEFAULT_SSL_PROTOCOL); | ||
| sslClientConfigs.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, Arrays.asList(SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS.split(","))); | ||
| sslClientConfigs.put(SslConfigs.SSL_CIPHER_SUITES_CONFIG, Collections.singletonList(cipherSuite)); | ||
| createSelector(sslClientConfigs); | ||
| InetSocketAddress addr = new InetSocketAddress("localhost", server.port()); | ||
| selector.connect(node, addr, BUFFER_SIZE, BUFFER_SIZE); | ||
|
|
||
| NetworkTestUtils.waitForChannelClose(selector, node, ChannelState.State.READY); | ||
| server.verifyAuthenticationMetrics(1, 0); | ||
| } | ||
|
|
||
| /** | ||
| * Tests that connections cannot be made with unsupported TLS cipher suites | ||
| */ | ||
|
|
@@ -1250,7 +1362,7 @@ private interface FailureAction { | |
| void run() throws IOException; | ||
| } | ||
|
|
||
| private static class TestSslChannelBuilder extends SslChannelBuilder { | ||
| static class TestSslChannelBuilder extends SslChannelBuilder { | ||
|
|
||
| private Integer netReadBufSizeOverride; | ||
| private Integer netWriteBufSizeOverride; | ||
|
|
@@ -1361,7 +1473,7 @@ private void resetDelayedFlush() { | |
| } | ||
| } | ||
|
|
||
| private static class ResizeableBufferSize { | ||
| static class ResizeableBufferSize { | ||
| private Integer bufSizeOverride; | ||
| ResizeableBufferSize(Integer bufSizeOverride) { | ||
| this.bufSizeOverride = bufSizeOverride; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| /* | ||
| * 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.Arrays; | ||
| 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 List<String> tlsServerProtocols; | ||
| private final List<String> tlsClientProtocols; | ||
|
|
||
| @Parameterized.Parameters(name = "tlsServerProtocol={0},tlsClientProtocol={1}") | ||
| public static Collection<Object[]> data() { | ||
| List<Object[]> values = new ArrayList<>(); | ||
| values.add(new Object[] {Collections.singletonList("TLSv1.2"), Collections.singletonList("TLSv1.2")}); | ||
| if (Java.IS_JAVA11_COMPATIBLE) { | ||
| values.add(new Object[] {Arrays.asList("TLSv1.2", "TLSv1.3"), Collections.singletonList("TLSv1.3")}); | ||
| values.add(new Object[] {Arrays.asList("TLSv1.2", "TLSv1.3"), Collections.singletonList("TLSv1.2")}); | ||
| values.add(new Object[] {Arrays.asList("TLSv1.2", "TLSv1.3"), Arrays.asList("TLSv1.2", "TLSv1.3")}); | ||
| values.add(new Object[] {Arrays.asList("TLSv1.2", "TLSv1.3"), Arrays.asList("TLSv1.3", "TLSv1.2")}); | ||
|
|
||
| values.add(new Object[] {Arrays.asList("TLSv1.3", "TLSv1.2"), Collections.singletonList("TLSv1.3")}); | ||
| values.add(new Object[] {Arrays.asList("TLSv1.3", "TLSv1.2"), Collections.singletonList("TLSv1.2")}); | ||
| values.add(new Object[] {Arrays.asList("TLSv1.3", "TLSv1.2"), Arrays.asList("TLSv1.2", "TLSv1.3")}); | ||
| values.add(new Object[] {Arrays.asList("TLSv1.3", "TLSv1.2"), Arrays.asList("TLSv1.3", "TLSv1.2")}); | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did we make these changes? I think what we had was good.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, test fails for a single case: This means:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting! Good that we added that test. :) |
||
| } | ||
| return values; | ||
| } | ||
|
|
||
| public SslVersionsTransportLayerTest(List<String> tlsServerProtocols, List<String> tlsClientProtocols) { | ||
| this.tlsServerProtocols = tlsServerProtocols; | ||
| this.tlsClientProtocols = tlsClientProtocols; | ||
| } | ||
|
|
||
| /** | ||
| * Tests that connection success with the default TLS version. | ||
| */ | ||
| @Test | ||
| public void testTlsDefaults() throws Exception { | ||
| // Create certificates for use by client and server. Add server cert to client truststore and vice versa. | ||
| CertStores serverCertStores = new CertStores(true, "server", "localhost"); | ||
| CertStores clientCertStores = new CertStores(false, "client", "localhost"); | ||
|
|
||
| Map<String, Object> sslClientConfigs = getTrustingConfig(clientCertStores, serverCertStores, tlsClientProtocols); | ||
| Map<String, Object> sslServerConfigs = getTrustingConfig(serverCertStores, clientCertStores, tlsServerProtocols); | ||
|
|
||
| NioEchoServer server = NetworkTestUtils.createEchoServer(ListenerName.forSecurityProtocol(SecurityProtocol.SSL), | ||
| SecurityProtocol.SSL, | ||
| new TestSecurityConfig(sslServerConfigs), | ||
| null, | ||
| TIME); | ||
| Selector selector = createSelector(sslClientConfigs); | ||
|
|
||
| String node = "0"; | ||
| selector.connect(node, new InetSocketAddress("localhost", server.port()), BUFFER_SIZE, BUFFER_SIZE); | ||
|
|
||
| if (!Collections.disjoint(tlsServerProtocols, tlsClientProtocols)) { | ||
| NetworkTestUtils.waitForChannelReady(selector, node); | ||
|
|
||
| int msgSz = 1024 * 1024; | ||
| String message = TestUtils.randomString(msgSz); | ||
| selector.send(new NetworkSend(node, ByteBuffer.wrap(message.getBytes()))); | ||
| while (selector.completedReceives().isEmpty()) { | ||
| selector.poll(100L); | ||
| } | ||
| int totalBytes = msgSz + 4; // including 4-byte size | ||
| server.waitForMetric("incoming-byte", totalBytes); | ||
| server.waitForMetric("outgoing-byte", totalBytes); | ||
| server.waitForMetric("request", 1); | ||
| server.waitForMetric("response", 1); | ||
| } else { | ||
| NetworkTestUtils.waitForChannelClose(selector, node, ChannelState.State.AUTHENTICATION_FAILED); | ||
| } | ||
| } | ||
|
|
||
| private static Map<String, Object> getTrustingConfig(CertStores certStores, CertStores peerCertStores, List<String> tlsProtocols) { | ||
| Map<String, Object> configs = certStores.getTrustingConfig(peerCertStores); | ||
| configs.putAll(sslConfig(tlsProtocols)); | ||
| return configs; | ||
| } | ||
|
|
||
| private static Map<String, Object> sslConfig(List<String> tlsServerProtocols) { | ||
| Map<String, Object> sslConfig = new HashMap<>(); | ||
| sslConfig.put(SslConfigs.SSL_PROTOCOL_CONFIG, tlsServerProtocols.get(0)); | ||
| sslConfig.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, tlsServerProtocols); | ||
| return sslConfig; | ||
| } | ||
|
|
||
| private Selector createSelector(Map<String, Object> sslClientConfigs) { | ||
|
nizhikov marked this conversation as resolved.
Outdated
|
||
| SslTransportLayerTest.TestSslChannelBuilder channelBuilder = new SslTransportLayerTest.TestSslChannelBuilder(Mode.CLIENT); | ||
| channelBuilder.configureBufferSizes(null, null, null); | ||
| channelBuilder.configure(sslClientConfigs); | ||
| return new Selector(100 * 5000, new Metrics(), TIME, "MetricGroup", channelBuilder, new LogContext()); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about: