KAFKA-9320: Enable TLSv1.3 by default (KIP-573) #8695

nizhikov · 2020-05-19T19:12:57Z

Enables TLSv1.3 by default with Java 11 or newer.
Add unit tests that cover the various TLSv1.2 and TLSv1.3 combinations.
Extend benchmark_test.py and replication_test.py to run with 'TLSv1.2'
or 'TLSv1.3'.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

nizhikov · 2020-05-20T08:36:41Z

@ijuma Looks like tests are OK.
Please, take a look at this preliminary PR.

ijuma · 2020-05-20T13:41:38Z

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java

+        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(",")));


I think you want to leave this as the default and see if it works correctly.

Hello. Sorry, I don't understand your concern :)

DEFAULT_SSL_ENABLED_PROTOCOLS = TLSv1.2,TLSv1.3 for java11+

DEFAULT_SSL_ENABLED_PROTOCOLS = TLSv1.2 for others jdk.

This property modified inside this test so I forcefully set it as default value.

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?

Tests added.

ijuma · 2020-05-20T17:00:22Z

One question: any downside to setting ssl.protocol=TLSv1.3 instead of ssl.protocol=TLSv1.2?

nizhikov · 2020-05-20T17:23:20Z

@ijuma I can't see downside in forcing usage of the latest TLS version.
Added this change to PR.

nizhikov · 2020-05-21T07:01:21Z

retest this, please

ijuma · 2020-05-21T13:02:24Z

@nizhikov Thanks. Can you update the KIP and start the voting on it?

nizhikov · 2020-05-21T13:30:14Z

@ijuma KIP-573 updated.
Actually, I'm started one (link), but didn't get any votes :)
Should I start another?

ijuma · 2020-05-21T19:48:38Z

Since the vote passed, can we flesh out the PR to include more tests that exercise TLS 1.3? A few things to think about:

Unit tests like the ones included in the PR currently. Can we go through the various possible combinations of client and server configuration and check that they all work or fail in the way we expect.
Make sure the integration tests use the same TLS configuration we use by default (if they don't already). Since Java 8 sticks to TLS 1.2 for now, we will get coverage of the old and new approach this way.
Adjust system tests to use TLS 1.3 by default, but also include variants where client uses TLS 1.2 and broker uses 1.3, the reverse and finally where TLS 1.2 is used for both.

ijuma · 2020-05-25T19:06:09Z

A few failures seem related to the changes in this PR:

kafka.network.SocketServerTest.testConnectionIdReuse
kafka.network.SocketServerTest.remoteCloseWithBufferedReceivesFailedSend
kafka.network.SocketServerTest.remoteCloseSendFailure
kafka.network.SocketServerTest.remoteCloseWithoutBufferedReceives
kafka.network.SocketServerTest.remoteCloseWithCompleteAndIncompleteBufferedReceives
kafka.network.SocketServerTest.remoteCloseWithIncompleteBufferedReceive
kafka.network.SocketServerTest.closingChannelWithBufferedReceives
kafka.network.SocketServerTest.closingChannelSendFailure
kafka.network.SocketServerTest.idleExpiryWithBufferedReceives
kafka.network.SocketServerTest.closingChannelWithBufferedReceivesFailedSend
kafka.network.SocketServerTest.remoteCloseWithBufferedReceives
kafka.network.SocketServerTest.closingChannelWithCompleteAndIncompleteBufferedReceives

nizhikov · 2020-05-26T14:20:19Z

retest this, please

ijuma

Thanks for the updates. We're closer. :) A few comments below.

tests/kafkatest/services/kafka/kafka.py

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java

clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java

tests/docker/run_tests.sh

tests/kafkatest/benchmarks/core/benchmark_test.py

clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java

nizhikov · 2020-06-02T12:39:31Z

@ijuma I found explanation of the test behavior.

Full information can be found in the guide Please, navigate to the "Send ClientHello Message". You may want to take a look at the "client version" and "supported_versions (43)" fields.

The root of the "strange" behavior is the structure of the SSL ClientHello message(quote from tutorial):

Client version: For TLS 1.3, this has a fixed value, TLSv1.2; TLS 1.3 uses the extension supported_versions and not this field to negotiate protocol version
...
supported_versions: Lists which versions of TLS the client supports. In particular, if the client
requests TLS 1.3, then the client version field has the value TLSv1.2 and this extension
contains the value TLSv1.3; if the client requests TLS 1.2, then the client version field has the
value TLSv1.2 and this extension either doesn’t exist or contains the value TLSv1.2 but not the value TLSv1.3.

This means we can't connect with the following configuration:
client:

ssl.protocol=TLSv1.2 #this will be used for ClientHello
ssl.enabled.protocols=TLSv1.2,TLSv1.3 #TLS v1.3 will be ignored in ClientHello message.

Server:

ssl.protocol=TLSv1.3
ssl.enabled.protocols=TLSv1.3 # Accept only TLSv1.3

You can see all details of the SSL connection process in the javax.net log.
It can be enabled like the following:

    public SslVersionsTransportLayerTest(List<String> serverProtocols, List<String> clientProtocols) {
        System.setProperty("javax.net.debug", "ssl:handshake"); //This will turn on the log from jdk SSL system classes.
        this.serverProtocols = serverProtocols;
        this.clientProtocols = clientProtocols;
    }

So correct check should be:

    private boolean isCompatible(List<String> serverProtocols, List<String> clientProtocols) {
        return serverProtocols.contains(clientProtocols.get(0)) ||
            (clientProtocols.get(0).equals("TLSv1.3") && clientProtocols.contains("TLSv1.2"));
    }

clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java

ijuma

Thanks for figuring this out. Very helpful. Can we update SSL_PROTOCOL_DOC to include what we've learned here? It's not particularly intuitive. :) Aside from that and a couple of nits below, I think we're good to merge.

clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java

ijuma · 2020-06-02T14:11:29Z

Oh, one more thing, let's please add an entry to upgrade.html.

ijuma · 2020-06-02T14:19:17Z

retest this please

nizhikov · 2020-06-02T14:20:48Z

I think, currently, the trunk is broken with the c6633a1
I prepared oneliner fix for it - #8779

ijuma · 2020-06-02T15:49:27Z

clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java

            + "Allowed values in recent JVMs are TLSv1.2 and TLSv1.3. TLS, TLSv1.1, SSL, SSLv2 and SSLv3 "
-            + "may be supported in older JVMs, but their usage is discouraged due to known security vulnerabilities.";
+            + "may be supported in older JVMs, but their usage is discouraged due to known security vulnerabilities."
+            + "Please, note, TLSv1.2 clients can't connect to the servers with TLSv1.3 only even if ssl.enabled.protocols contains TLSv1.3";


How about:

"The SSL protocol used to generate the SSLContext. " + "The default is TLSv1.3 when running with Java 11 or newer, TLSv1.2 otherwise. " + "This value should be fine for most use cases. " + "Allowed values in recent JVMs are TLSv1.2 and TLSv1.3. TLS, TLSv1.1, SSL, SSLv2 and SSLv3 " + "may be supported in older JVMs, but their usage is discouraged due to known security vulnerabilities. "; + "With the default value for this config and ssl.enabled.protocols, clients will downgrade to TLSv1.2 if " + "the server does not support TLSv1.3. If this config is set to TLSv1.2, clients will not use TLSv1.3 even " + "if it is one of the values in ssl.enabled.protocols and the server only supports TLSv1.3."

ijuma · 2020-06-02T15:54:49Z

clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java

@@ -64,7 +66,17 @@

    public static final String SSL_ENABLED_PROTOCOLS_CONFIG = "ssl.enabled.protocols";
    public static final String SSL_ENABLED_PROTOCOLS_DOC = "The list of protocols enabled for SSL connections.";


How about:

The list of protocols enabled for SSL connections. The default is 'TLSv1.2,TLSv1.3' when running with Java 11 or newer, 'TLSv1.2' otherwise. With the default value for Java 11, clients and servers will prefer TLSv1.3 if both support it and fallback to TLSv1.2 otherwise (assuming both support at least TLSv1.2). This default should be fine for most cases. Also see the `ssl.protocol` config documentation.

ijuma · 2020-06-02T15:58:09Z

docs/upgrade.html

@@ -18,6 +18,10 @@
 <script><!--#include virtual="js/templateData.js" --></script>

 <script id="upgrade-template" type="text/x-handlebars-template">
+<h5><a id="upgrade_270_notable" href="#upgrade_270_notable">Notable changes in 2.7.0</a></h5>
+<ul>
+    <li>TLSv1.3 have been enabled by default for JDK11+. See <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-573%3A+Enable+TLSv1.3+by+default">KIP-573</a> for full details.</li>


One more nit: "TLSv1.3 has been enabled by default for Java 11 or newer. The client and server will negotiate TLSv1.3 if both support it and fallback to TLSv1.2 otherwise. See...

ijuma · 2020-06-02T16:02:09Z

@nizhikov I think we're good to merge this after the non code suggestions above are addressed (assuming we can get a Jenkins build, I merged your other PR fixing the build issue).

ijuma

LGTM, thanks!

nizhikov · 2020-06-02T20:00:38Z

@ijuma Thanks for your time! Appreciate it.
Try my best to waste fewer rounds of review next time :)

ijuma · 2020-06-02T20:07:28Z

@nizhikov These changes are pretty tricky to validate. Thanks for being thorough on the tests. :)

ijuma · 2020-06-02T22:33:17Z

The failures are unrelated:

org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges

Also failing in master:

org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges

1. Enables `TLSv1.3` by default with Java 11 or newer. 2. Add unit tests that cover the various TLSv1.2 and TLSv1.3 combinations. 3. Extend `benchmark_test.py` and `replication_test.py` to run with 'TLSv1.2' or 'TLSv1.3'. Reviewers: Ismael Juma <ismael@juma.me.uk>

* apache-github/2.6: (32 commits) KAFKA-10083: fix failed testReassignmentWithRandomSubscriptionsAndChanges tests (apache#8786) KAFKA-9945: TopicCommand should support --if-exists and --if-not-exists when --bootstrap-server is used (apache#8737) KAFKA-9320: Enable TLSv1.3 by default (KIP-573) (apache#8695) KAFKA-10082: Fix the failed testMultiConsumerStickyAssignment (apache#8777) MINOR: Remove unused variable to fix spotBugs failure (apache#8779) MINOR: ChangelogReader should poll for duration 0 for standby restore (apache#8773) KAFKA-10030: Allow fetching a key from a single partition (apache#8706) Kafka-10064 Add documentation for KIP-571 (apache#8760) MINOR: Code cleanup and assertion message fixes in Connect integration tests (apache#8750) KAFKA-9987: optimize sticky assignment algorithm for same-subscription case (apache#8668) KAFKA-9392; Clarify deleteAcls javadoc and add test for create/delete timing (apache#7956) KAFKA-10074: Improve performance of `matchingAcls` (apache#8769) KAFKA-9494; Include additional metadata information in DescribeConfig response (KIP-569) (apache#8723) KAFKA-10056; Ensure consumer metadata contains new topics on subscription change (apache#8739) KAFKA-10029; Don't update completedReceives when channels are closed to avoid ConcurrentModificationException (apache#8705) KAFKA-10061; Fix flaky `ReassignPartitionsIntegrationTest.testCancellation` (apache#8749) KAFKA-9130; KIP-518 Allow listing consumer groups per state (apache#8238) KAFKA-9501: convert between active and standby without closing stores (apache#8248) MINOR: Relax Percentiles test (apache#8748) MINOR: regression test for task assignor config (apache#8743) ...

nizhikov added 2 commits May 19, 2020 12:36

KAFKA-9320: Initial commit.

18a1bca

KAFKA-9320: Initial commit.

1076e51

ijuma reviewed May 20, 2020

View reviewed changes

KAFKA-9320: Test added

7dec0d6

nizhikov added 3 commits May 20, 2020 20:08

KAFKA-9320: Test added

f6afbb9

KAFKA-9320: Test added

142e487

KAFKA-9320: Test added

ac448d1

KAFKA-9320: SslVersionsTransportLayerTest added.

e1287c6

KAFKA-9320: Tests fix.

b310e60

nizhikov added 3 commits May 26, 2020 19:49

KAFKA-9320: system tests updated.

518eb77

KAFKA-9320: system tests updated.

5b5f37e

KAFKA-9320: system tests updated.

c7000d9

ijuma reviewed May 28, 2020

View reviewed changes

nizhikov added 2 commits May 28, 2020 11:48

KAFKA-9320: code review fixes

862f7ae

Merge branch 'trunk' into KAFKA-9320

d1dd114

nizhikov changed the title ~~KAFKA-9320: KIP-573 - Enable TLSv1.3 by default~~ [WIP] KAFKA-9320: KIP-573 - Enable TLSv1.3 by default May 28, 2020

nizhikov added 4 commits May 28, 2020 12:09

KAFKA-9320: code review fixes

5578192

KAFKA-9320: code review fixes

c1847e7

KAFKA-9320: code review fixes

c901254

KAFKA-9320: code review fixes

61cd6c5

ijuma reviewed May 28, 2020

View reviewed changes

clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java Outdated Show resolved Hide resolved

ijuma reviewed Jun 1, 2020

View reviewed changes

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java Outdated Show resolved Hide resolved

ijuma reviewed Jun 1, 2020

View reviewed changes

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java Outdated Show resolved Hide resolved

nizhikov added 2 commits June 1, 2020 18:12

KAFKA-9320: unused code removed.

9da1c21

KAFKA-9320: code review fixes.

3e6c445

ijuma reviewed Jun 1, 2020

View reviewed changes

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java Show resolved Hide resolved

KAFKA-9320: TLSv1.3 vs TLSv1.2 explanation comments.

14bf85a

Merge branch 'trunk' into KAFKA-9320

67f0ef9

ijuma reviewed Jun 2, 2020

View reviewed changes

clients/src/test/java/org/apache/kafka/common/network/SslVersionsTransportLayerTest.java Outdated Show resolved Hide resolved

KAFKA-9320: code review fixes.

869e342

ijuma reviewed Jun 2, 2020

View reviewed changes

nizhikov added 2 commits June 2, 2020 17:37

KAFKA-9320: code review fixes.

ca81fcd

KAFKA-9320: SSL_PROTOCOL_DOC updated.

6a82441

ijuma reviewed Jun 2, 2020

View reviewed changes

Documentation tweaks

ce7505f

ijuma force-pushed the KAFKA-9320 branch from ebb5369 to ce7505f Compare June 2, 2020 19:10

ijuma approved these changes Jun 2, 2020

View reviewed changes

Merge branch 'trunk' into KAFKA-9320

b293578

ijuma merged commit 8b22b81 into apache:trunk Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-9320: Enable TLSv1.3 by default (KIP-573) #8695

KAFKA-9320: Enable TLSv1.3 by default (KIP-573) #8695

nizhikov commented May 19, 2020 •

edited by ijuma

Loading

nizhikov commented May 20, 2020 •

edited

Loading

ijuma May 20, 2020 •

edited

Loading

nizhikov May 20, 2020

ijuma May 20, 2020

nizhikov May 20, 2020

ijuma commented May 20, 2020

nizhikov commented May 20, 2020

nizhikov commented May 21, 2020

ijuma commented May 21, 2020

nizhikov commented May 21, 2020

ijuma commented May 21, 2020

ijuma commented May 25, 2020

nizhikov commented May 26, 2020

ijuma left a comment

nizhikov commented Jun 2, 2020

ijuma left a comment

ijuma commented Jun 2, 2020

ijuma commented Jun 2, 2020

nizhikov commented Jun 2, 2020

ijuma Jun 2, 2020

ijuma Jun 2, 2020 •

edited

Loading

ijuma Jun 2, 2020

ijuma commented Jun 2, 2020

ijuma left a comment

nizhikov commented Jun 2, 2020

ijuma commented Jun 2, 2020

ijuma commented Jun 2, 2020 •

edited

Loading

		@@ -64,7 +66,17 @@

		public static final String SSL_ENABLED_PROTOCOLS_CONFIG = "ssl.enabled.protocols";
		public static final String SSL_ENABLED_PROTOCOLS_DOC = "The list of protocols enabled for SSL connections.";

KAFKA-9320: Enable TLSv1.3 by default (KIP-573) #8695

KAFKA-9320: Enable TLSv1.3 by default (KIP-573) #8695

Conversation

nizhikov commented May 19, 2020 • edited by ijuma Loading

Committer Checklist (excluded from commit message)

nizhikov commented May 20, 2020 • edited Loading

ijuma May 20, 2020 • edited Loading

Choose a reason for hiding this comment

nizhikov May 20, 2020

Choose a reason for hiding this comment

ijuma May 20, 2020

Choose a reason for hiding this comment

nizhikov May 20, 2020

Choose a reason for hiding this comment

ijuma commented May 20, 2020

nizhikov commented May 20, 2020

nizhikov commented May 21, 2020

ijuma commented May 21, 2020

nizhikov commented May 21, 2020

ijuma commented May 21, 2020

ijuma commented May 25, 2020

nizhikov commented May 26, 2020

ijuma left a comment

Choose a reason for hiding this comment

nizhikov commented Jun 2, 2020

ijuma left a comment

Choose a reason for hiding this comment

ijuma commented Jun 2, 2020

ijuma commented Jun 2, 2020

nizhikov commented Jun 2, 2020

ijuma Jun 2, 2020

Choose a reason for hiding this comment

ijuma Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

ijuma Jun 2, 2020

Choose a reason for hiding this comment

ijuma commented Jun 2, 2020

ijuma left a comment

Choose a reason for hiding this comment

nizhikov commented Jun 2, 2020

ijuma commented Jun 2, 2020

ijuma commented Jun 2, 2020 • edited Loading

nizhikov commented May 19, 2020 •

edited by ijuma

Loading

nizhikov commented May 20, 2020 •

edited

Loading

ijuma May 20, 2020 •

edited

Loading

ijuma Jun 2, 2020 •

edited

Loading

ijuma commented Jun 2, 2020 •

edited

Loading