Add support for TLSv1.3 #8293

normanmaurer · 2018-09-17T13:53:04Z

Motivation:

TLSv1.3 support is included in java11 and is also supported by OpenSSL 1.1.1, so we should support when possible.

Modifications:

Add support for TLSv1.3 using either the JDK implementation or the native implementation provided by netty-tcnative when compiled against openssl 1.1.1
Adjust unit tests for semantics provided by TLSv1.3
Correctly handle custom Provider implementations that not support TLSv1.3

Result:

Be able to use TLSv1.3 with netty.

normanmaurer · 2018-09-17T13:53:34Z

This depends on netty/netty-tcnative#389.

normanmaurer · 2018-09-17T13:55:32Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

        synchronized (this) {
            if (!isDestroyed()) {
+                // TODO: Should we also adjust the protocols based on if there are any ciphers left that can be used
+                //       for TLSv1.3 or for previor SSL/TLS versions ?


@carl-mastrangelo @ejona86 @ryanoneill @bryce-anderson @Scottmitch @trustin not sure about this one...

Sounds kinda niche. Maybe we can wait until someone asks for it?

normanmaurer · 2018-09-17T13:56:04Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

@@ -2199,6 +2209,10 @@ public int getPeerPort() {

        @Override
        public int getPacketBufferSize() {
+            // TODO: Do a better job for TLSv1.3 here.


@carl-mastrangelo @ejona86 @ryanoneill @bryce-anderson @Scottmitch @trustin this needs to be fixed before merging as it may waste memory.

Currently checking what to do best...

normanmaurer · 2018-09-17T13:57:01Z

Once this is merged I have a followup PR that will allow to use TLSv1.3 also on Java8,9 and 10 :) But I want to have this one merged first to keep the scope small

normanmaurer · 2018-09-17T20:45:05Z

Also another question is if we should add TLSv1.3 by default to the protocols used or if the user should enable it explicit... thoughts ?

carl-mastrangelo · 2018-09-17T21:06:41Z

IIRC TLS 1.3 was subject (sometimes) to replay attacks, which considered ok for idempotent requests like GETs. I have no idea about the details, but I remember that and being surprised. Probably worth checking before turning it on by default.

carl-mastrangelo · 2018-09-17T21:24:23Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

        synchronized (this) {
            if (!isDestroyed()) {
+                // TODO: Should we also adjust the protocols based on if there are any ciphers left that can be used
+                //       for TLSv1.3 or for previor SSL/TLS versions ?


Sounds kinda niche. Maybe we can wait until someone asks for it?

carl-mastrangelo · 2018-09-17T21:25:17Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

@@ -1470,13 +1473,20 @@ public final void setEnabledProtocols(String[] protocols) {
                if (maxProtocolIndex < OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2) {
                    maxProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2;
                }
+            } else if (p.equals(PROTOCOL_TLS_V1_3)) {
+                if (minProtocolIndex > OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_3) {


minProtocolIndex = Math.min(minProtocolIndex, OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_3);

@carl-mastrangelo I could do this I just wanted to keep it consistent with the rest.

carl-mastrangelo · 2018-09-17T21:27:48Z

handler/src/test/java/io/netty/handler/ssl/SslContextBuilderTest.java

+            // This may fail or not depending on the OpenSSL version used
+            // See https://github.com/openssl/openssl/issues/7196
+            testInvalidCipher(SslProvider.OPENSSL);
+            if (!OpenSsl.versionString().contains("1.1.1")) {


Why not also asume the version string above?

@carl-mastrangelo because I want to have the test run with all versions and just test for different things.

normanmaurer · 2018-09-24T10:52:56Z

@carl-mastrangelo @ejona86 @Scottmitch @trustin any more comments ? I would love to pull this in...

trustin

Looks good in general. Didn't review very very closely though because I'm traveling.

trustin · 2018-09-24T12:39:58Z

handler/src/main/java/io/netty/handler/ssl/OpenSsl.java

@@ -450,4 +489,8 @@ static void releaseIfNeeded(ReferenceCounted counted) {
            ReferenceCountUtil.safeRelease(counted);
        }
    }
+
+    static boolean isTlsv13Supported() {


Tls13 or TlsV13?

I actually like isTlsv13Supported best... are you strong here ?

Lowercase v is what Google's style guide says, if it matters.

trustin · 2018-09-24T12:43:57Z

handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java

-import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1_2;
-import static io.netty.handler.ssl.SslUtils.SSL_RECORD_HEADER_LENGTH;
-
+import static io.netty.handler.ssl.SslUtils.*;


Hmm, did we change our coding style?

not sure :D I can revert this if you want.

Maybe it's time to adopt https://github.com/google/google-java-format for love and peace? 😄

normanmaurer · 2018-09-28T11:11:05Z

@carl-mastrangelo @trustin anything else ?

carl-mastrangelo

No other comments from me. LGTM

normanmaurer · 2018-10-15T09:40:27Z

@netty-bot test this please

normanmaurer · 2018-10-15T17:36:35Z

@netty-bot test this please

Motivation: TLSv1.3 support is included in java11 and is also supported by OpenSSL 1.1.1, so we should support when possible. Modifications: - Add support for TLSv1.3 using either the JDK implementation or the native implementation provided by netty-tcnative when compiled against openssl 1.1.1 - Adjust unit tests for semantics provided by TLSv1.3 - Correctly handle custom Provider implementations that not support TLSv1.3 Result: Be able to use TLSv1.3 with netty.

thekalinga · 2018-11-06T13:58:34Z

Once this is merged I have a followup PR that will allow to use TLSv1.3 also on Java8,9 and 10 :) But I want to have this one merged first to keep the scope small

Is it fair to say netty does not yet support TLS 1.3 if the jdk used is Java 8?

normanmaurer · 2018-11-06T13:59:48Z

@thekalinga nope... as long as you use netty-tcnative-boringssl-static it will also support TLSv1.3 with Java8

thekalinga · 2018-11-06T14:01:38Z

@normanmaurer Thanks for the clarification

thekalinga · 2018-11-06T14:04:54Z

@normanmaurer Request for one small clarification. If netty-tcnative-boringssl-static enables TLS 1.3 with java 8, can you please elaborate on what you mentioned in the comment (please note that I am complete noob to netty), as I'm a bit puzzled

normanmaurer · 2018-11-06T14:07:10Z

@thekalinga sorry I dont get what you are asking for... Can you clarify ?

thekalinga · 2018-11-06T14:13:12Z

I just came across the other pull request of yours where you added support for Java < 11 support

This clarifies my confusion

Please ignore my previous comment. Looks like my question caused more confusion

normanmaurer commented Sep 17, 2018

View reviewed changes

normanmaurer requested review from Scottmitch, carl-mastrangelo, ejona86, bryce-anderson, ryanoneill, trustin and vkostyukov and removed request for Scottmitch, carl-mastrangelo and ejona86 September 17, 2018 13:57

normanmaurer self-assigned this Sep 17, 2018

normanmaurer requested a review from mosesn September 17, 2018 13:58

normanmaurer force-pushed the openssl_1_1_1 branch from fe61871 to 2b20117 Compare September 17, 2018 14:00

carl-mastrangelo reviewed Sep 17, 2018

View reviewed changes

normanmaurer force-pushed the openssl_1_1_1 branch 2 times, most recently from 79ee03c to 344505f Compare September 24, 2018 10:52

trustin reviewed Sep 24, 2018

View reviewed changes

normanmaurer force-pushed the openssl_1_1_1 branch 2 times, most recently from 19bc197 to 20f3043 Compare September 28, 2018 09:36

carl-mastrangelo approved these changes Sep 28, 2018

View reviewed changes

normanmaurer force-pushed the openssl_1_1_1 branch from 8bb1e8c to fdacb32 Compare October 15, 2018 08:49

normanmaurer added this to the 4.1.31.Final milestone Oct 15, 2018

normanmaurer added improvement feature labels Oct 15, 2018

trustin approved these changes Oct 16, 2018

View reviewed changes

normanmaurer force-pushed the openssl_1_1_1 branch from fdacb32 to 59c9ad6 Compare October 16, 2018 18:11

normanmaurer merged commit 0ddc62c into 4.1 Oct 17, 2018

normanmaurer deleted the openssl_1_1_1 branch October 17, 2018 06:35

njhill mentioned this pull request Oct 30, 2018

Custom cipher list with BoringSSL breaks in 4.1.31.Final #8442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TLSv1.3 #8293

Add support for TLSv1.3 #8293

normanmaurer commented Sep 17, 2018

normanmaurer commented Sep 17, 2018

normanmaurer Sep 17, 2018

carl-mastrangelo Sep 17, 2018

normanmaurer Sep 17, 2018

normanmaurer Sep 17, 2018

normanmaurer commented Sep 17, 2018

normanmaurer commented Sep 17, 2018

carl-mastrangelo commented Sep 17, 2018

carl-mastrangelo Sep 17, 2018

carl-mastrangelo Sep 17, 2018

normanmaurer Sep 18, 2018

carl-mastrangelo Sep 17, 2018

normanmaurer Sep 18, 2018

normanmaurer commented Sep 24, 2018

trustin left a comment

trustin Sep 24, 2018

normanmaurer Sep 28, 2018

carl-mastrangelo Sep 28, 2018

trustin Sep 24, 2018

normanmaurer Sep 24, 2018

trustin Sep 24, 2018

normanmaurer commented Sep 28, 2018

carl-mastrangelo left a comment

normanmaurer commented Oct 15, 2018

normanmaurer commented Oct 15, 2018

thekalinga commented Nov 6, 2018

normanmaurer commented Nov 6, 2018

thekalinga commented Nov 6, 2018

thekalinga commented Nov 6, 2018 •

edited

normanmaurer commented Nov 6, 2018

thekalinga commented Nov 6, 2018

Add support for TLSv1.3 #8293

Add support for TLSv1.3 #8293

Conversation

normanmaurer commented Sep 17, 2018

normanmaurer commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Sep 17, 2018

normanmaurer commented Sep 17, 2018

carl-mastrangelo commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Sep 24, 2018

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Sep 28, 2018

carl-mastrangelo left a comment

Choose a reason for hiding this comment

normanmaurer commented Oct 15, 2018

normanmaurer commented Oct 15, 2018

thekalinga commented Nov 6, 2018

normanmaurer commented Nov 6, 2018

thekalinga commented Nov 6, 2018

thekalinga commented Nov 6, 2018 • edited

normanmaurer commented Nov 6, 2018

thekalinga commented Nov 6, 2018

thekalinga commented Nov 6, 2018 •

edited