KAFKA-13418: Support key updates with TLS 1.3 #11966

ijuma · 2022-03-29T14:32:58Z

Key updates with TLS 1.3 trigger code paths similar to renegotiation with TLS 1.2.
Update the read/write paths not to throw an exception in this case (kept the exception
in the handshake method).

With the default configuration, key updates happen after 2^37 bytes are encrypted.
There is a security property to adjust this configuration, but the change has to be
done before it is used for the first time and it cannot be changed after that. As such,
it is best done via a system test (filed KAFKA-13779).

To validate the change, I wrote a unit test that forces key updates and manually ran
a producer workload that produced more than 2^37 bytes. Both cases failed without
these changes and pass with them.

Note that Shylaja Kokoori attached a patch with the SslTransportLayer fix and hence
included them as a co-author of this change.

Co-authored-by: Shylaja Kokoori

Committer Checklist (excluded from commit message)

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

ijuma · 2022-03-29T14:33:35Z

I intend to cherry-pick this change to the 3.2, 3.1 and 3.0 branches. cc @cadonna

ijuma · 2022-03-29T14:34:28Z

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

@@ -1015,7 +1011,6 @@ public void testChannelCloseWhileProcessingReceives() throws Exception {

    private String blockingRequest(String node, String s) throws IOException {
        selector.send(createSend(node, s));
-        selector.poll(1000L);


Removed since it's redundant.

ijuma · 2022-03-29T14:35:01Z

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

@@ -110,10 +110,6 @@ public void tearDown() throws Exception {
        }
    }

-    public SecurityProtocol securityProtocol() {


This was not used.

rajinisivaram · 2022-03-29T14:53:31Z

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

+        }
+        selector.send(createSend(node, node + "-" + 0));
+        selector.poll(0L);
+        server.renegotiate();


Won't this test fail with Java 8 where we use TLSv1.2 as default?

Good point. I'll verify that it fails (Jenkins should show it) and update the test to be a TLS 1.3 test that only runs if Java is 11 or newer.

We did indeed see the Java 8 failure before my last change:

Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.network.DefaultTlsSelectorTest.testKeyUpdate()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.network.DefaultTlsSelectorTest.testKeyUpdate()
Build / ARM / org.apache.kafka.common.network.DefaultTlsSelectorTest.testKeyUpdate()
Build / ARM / org.apache.kafka.common.network.DefaultTlsSelectorTest.testKeyUpdate()

…th Java 11 or newer

rajinisivaram

@ijuma Thanks for the PR, LGTM

ijuma · 2022-03-29T21:58:24Z

Various builds passed, there was one flake:

Build / JDK 17 and Scala 2.13 / kafka.admin.LeaderElectionCommandTest.[1] Type=Raft, Name=testElectionResultOutput, Security=PLAINTEXT

Key updates with TLS 1.3 trigger code paths similar to renegotiation with TLS 1.2. Update the read/write paths not to throw an exception in this case (kept the exception in the `handshake` method). With the default configuration, key updates happen after 2^37 bytes are encrypted. There is a security property to adjust this configuration, but the change has to be done before it is used for the first time and it cannot be changed after that. As such, it is best done via a system test (filed KAFKA-13779). To validate the change, I wrote a unit test that forces key updates and manually ran a producer workload that produced more than 2^37 bytes. Both cases failed without these changes and pass with them. Note that Shylaja Kokoori attached a patch with the SslTransportLayer fix and hence included them as a co-author of this change. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Shylaja Kokoori

cadonna · 2022-03-30T09:04:34Z

I intend to cherry-pick this change to the 3.2, 3.1 and 3.0 branches. cc @cadonna

@tombentley FYI

Key updates with TLS 1.3 trigger code paths similar to renegotiation with TLS 1.2. Update the read/write paths not to throw an exception in this case (kept the exception in the `handshake` method). With the default configuration, key updates happen after 2^37 bytes are encrypted. There is a security property to adjust this configuration, but the change has to be done before it is used for the first time and it cannot be changed after that. As such, it is best done via a system test (filed KAFKA-13779). To validate the change, I wrote a unit test that forces key updates and manually ran a producer workload that produced more than 2^37 bytes. Both cases failed without these changes and pass with them. Note that Shylaja Kokoori attached a patch with the SslTransportLayer fix and hence included them as a co-author of this change. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Shylaja Kokoori

…cs-12-may-2022 * apache-kafka/3.0: (14 commits) fix: make sliding window works without grace period (#kafka-13739) (apache#11980) KAFKA-13794: Follow up to fix producer batch comparator (apache#12006) KAFKA-13794; Fix comparator of `inflightBatchesBySequence` in `TransactionManager` (apache#11991) KAFKA-13748: Do not include file stream connectors in Connect's CLASSPATH and plugin.path by default (apache#11908) KAFKA-13418: Support key updates with TLS 1.3 (apache#11966) KAFKA-13770: Restore compatibility with KafkaBasedLog using older Kafka brokers (apache#11946) KAFKA-13761: KafkaLog4jAppender deadlocks when idempotence is enabled (apache#11939) KAFKA-13759: Disable idempotence by default in producers instantiated by Connect (apache#11933) MINOR: Fix `ConsumerConfig.ISOLATION_LEVEL_DOC` (apache#11915) KAFKA-13750; Client Compatability KafkaTest uses invalid idempotency configs (apache#11909) ...

…e#11966) LI_DESCRIPTION=This may improve p99 inter-broker latency under TLS1.3 -- Original message -- Key updates with TLS 1.3 trigger code paths similar to renegotiation with TLS 1.2. Update the read/write paths not to throw an exception in this case (kept the exception in the `handshake` method). With the default configuration, key updates happen after 2^37 bytes are encrypted. There is a security property to adjust this configuration, but the change has to be done before it is used for the first time and it cannot be changed after that. As such, it is best done via a system test (filed KAFKA-13779). To validate the change, I wrote a unit test that forces key updates and manually ran a producer workload that produced more than 2^37 bytes. Both cases failed without these changes and pass with them. Note that Shylaja Kokoori attached a patch with the SslTransportLayer fix and hence included them as a co-author of this change. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Shylaja Kokoori

…e#11966) (#485) LI_DESCRIPTION=This may improve p99 inter-broker latency under TLS1.3 -- Original message -- Key updates with TLS 1.3 trigger code paths similar to renegotiation with TLS 1.2. Update the read/write paths not to throw an exception in this case (kept the exception in the `handshake` method). With the default configuration, key updates happen after 2^37 bytes are encrypted. There is a security property to adjust this configuration, but the change has to be done before it is used for the first time and it cannot be changed after that. As such, it is best done via a system test (filed KAFKA-13779). To validate the change, I wrote a unit test that forces key updates and manually ran a producer workload that produced more than 2^37 bytes. Both cases failed without these changes and pass with them. Note that Shylaja Kokoori attached a patch with the SslTransportLayer fix and hence included them as a co-author of this change. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Shylaja Kokoori Co-authored-by: Ismael Juma <ismael@juma.me.uk>

ijuma added 4 commits March 29, 2022 07:09

Extract Tls12SelectorTest

16355ff

Remove unused securityProtocol and redundant poll

ba68610

Introduce DefaultTlsSelectorTest

193b09b

Support key updates with TLS 1.3

cd794bb

ijuma requested review from omkreddy and rajinisivaram March 29, 2022 14:32

ijuma commented Mar 29, 2022

View reviewed changes

Fix checkstyle

bee2ee8

rajinisivaram reviewed Mar 29, 2022

View reviewed changes

Rename DefaultTlsSelectorTest to Tls13SelectorTest and only run it wi…

80850c1

…th Java 11 or newer

rajinisivaram approved these changes Mar 29, 2022

View reviewed changes

ijuma merged commit 5aed178 into apache:trunk Mar 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-13418: Support key updates with TLS 1.3 #11966

KAFKA-13418: Support key updates with TLS 1.3 #11966

ijuma commented Mar 29, 2022 •

edited

Loading

ijuma commented Mar 29, 2022

ijuma Mar 29, 2022

ijuma Mar 29, 2022

rajinisivaram Mar 29, 2022

ijuma Mar 29, 2022

ijuma Mar 29, 2022

ijuma Mar 29, 2022

rajinisivaram left a comment

ijuma commented Mar 29, 2022

cadonna commented Mar 30, 2022

KAFKA-13418: Support key updates with TLS 1.3 #11966

KAFKA-13418: Support key updates with TLS 1.3 #11966

Conversation

ijuma commented Mar 29, 2022 • edited Loading

Committer Checklist (excluded from commit message)

ijuma commented Mar 29, 2022

ijuma Mar 29, 2022

Choose a reason for hiding this comment

ijuma Mar 29, 2022

Choose a reason for hiding this comment

rajinisivaram Mar 29, 2022

Choose a reason for hiding this comment

ijuma Mar 29, 2022

Choose a reason for hiding this comment

ijuma Mar 29, 2022

Choose a reason for hiding this comment

ijuma Mar 29, 2022

Choose a reason for hiding this comment

rajinisivaram left a comment

Choose a reason for hiding this comment

ijuma commented Mar 29, 2022

cadonna commented Mar 30, 2022

ijuma commented Mar 29, 2022 •

edited

Loading