From ce39773e04d6adb063cef62c0c31675272566880 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 26 Oct 2018 15:29:49 -0700 Subject: [PATCH] Add support for boringssl and TLSv1.3 (#8412) Motivation: 0ddc62cec0b4715ae37cef0e6a9f8c79d42d74e9 added support for TLSv1.3 when using openssl 1.1.1. Now that BoringSSL chromium-stable branch supports it as well we can also support it with netty-tcnative-boringssl-static. During this some unit tests failed with BoringSSL which was caused by not correctly handling flush() while the handshake is still in progress. Modification: - Upgrade netty-tcnative version which also supports TLSv1.3 when using BoringSSL - Correctly handle flush() when done while the handshake is still in progress in all cases. Result: Easier for people to enable TLSv1.3 when using native SSL impl. Ensure flush() while handshake is in progress will always be honored. --- .../java/io/netty/handler/ssl/OpenSsl.java | 22 +++++++++++++++---- .../java/io/netty/handler/ssl/SslHandler.java | 17 ++++++++------ .../netty/handler/ssl/OpenSslTestUtils.java | 4 ---- .../io/netty/handler/ssl/SSLEngineTest.java | 2 +- .../handler/ssl/SniClientJava8TestUtil.java | 2 +- .../io/netty/handler/ssl/SslErrorTest.java | 4 ++-- pom.xml | 2 +- 7 files changed, 33 insertions(+), 20 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java b/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java index c5086cc14d..e22457152c 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java @@ -66,7 +66,7 @@ public final class OpenSsl { "TLS_AES_128_CCM_8_SHA256" + ':' + "TLS_AES_128_CCM_SHA256"; private static final boolean TLSV13_SUPPORTED; - + private static final boolean IS_BORINGSSL; static final Set SUPPORTED_PROTOCOLS_SET; static { @@ -141,6 +141,8 @@ public final class OpenSsl { boolean supportsHostNameValidation = false; boolean tlsv13Supported = false; + IS_BORINGSSL = "BoringSSL".equals(versionString()); + try { final long sslCtx = SSLContext.make(SSL.SSL_PROTOCOL_ALL, SSL.SSL_MODE_SERVER); long certBio = 0; @@ -160,12 +162,19 @@ public final class OpenSsl { // Filter out bad input. if (c == null || c.isEmpty() || availableOpenSslCipherSuites.contains(c) || // Filter out TLSv1.3 ciphers if not supported. - !tlsv13Supported && SslUtils.isTLSv13Cipher(c)) { + !tlsv13Supported && isTLSv13Cipher(c)) { continue; } availableOpenSslCipherSuites.add(c); } - + if (IS_BORINGSSL) { + // Currently BoringSSL does not include these when calling SSL.getCiphers() even when these + // are supported. + Collections.addAll(availableOpenSslCipherSuites, + "TLS_AES_128_GCM_SHA256", + "TLS_AES_256_GCM_SHA384" , + "TLS_CHACHA20_POLY1305_SHA256"); + } try { SSL.setHostNameValidation(ssl, 0, "netty.io"); supportsHostNameValidation = true; @@ -240,7 +249,7 @@ public final class OpenSsl { AVAILABLE_OPENSSL_CIPHER_SUITES.size() * 2); for (String cipher: AVAILABLE_OPENSSL_CIPHER_SUITES) { // Included converted but also openssl cipher name - if (!SslUtils.isTLSv13Cipher(cipher)) { + if (!isTLSv13Cipher(cipher)) { availableJavaCipherSuites.add(CipherSuiteConverter.toJava(cipher, "TLS")); availableJavaCipherSuites.add(CipherSuiteConverter.toJava(cipher, "SSL")); } else { @@ -312,6 +321,7 @@ public final class OpenSsl { SUPPORTED_PROTOCOLS_SET = Collections.emptySet(); SUPPORTS_OCSP = false; TLSV13_SUPPORTED = false; + IS_BORINGSSL = false; } } @@ -510,4 +520,8 @@ public final class OpenSsl { static boolean isTlsv13Supported() { return TLSV13_SUPPORTED; } + + static boolean isBoringSSL() { + return IS_BORINGSSL; + } } diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java index 7358d42307..f1de1360bf 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -1385,13 +1385,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH wrapLater = true; continue; } - if (flushedBeforeHandshake) { - // We need to call wrap(...) in case there was a flush done before the handshake completed. - // - // See https://github.com/netty/netty/pull/2437 - flushedBeforeHandshake = false; - wrapLater = true; - } + // If we are not handshaking and there is no more data to unwrap then the next call to unwrap // will not produce any data. We can avoid the potentially costly unwrap operation and break // out of the loop. @@ -1414,6 +1408,15 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } } + if (flushedBeforeHandshake && handshakePromise.isDone()) { + // We need to call wrap(...) in case there was a flush done before the handshake completed to ensure + // we do not stale. + // + // See https://github.com/netty/netty/pull/2437 + flushedBeforeHandshake = false; + wrapLater = true; + } + if (wrapLater) { wrap(ctx, true); } diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslTestUtils.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslTestUtils.java index e8c46ed7a0..7882a61b4f 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslTestUtils.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslTestUtils.java @@ -24,8 +24,4 @@ final class OpenSslTestUtils { static void checkShouldUseKeyManagerFactory() { assumeTrue(OpenSsl.supportsKeyManagerFactory() && OpenSsl.useKeyManagerFactory()); } - - static boolean isBoringSSL() { - return "BoringSSL".equals(OpenSsl.versionString()); - } } diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index fc6826c0ee..b15a1beb11 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1087,7 +1087,7 @@ public abstract class SSLEngineTest { MessageReceiver receiver) throws Exception { List dataCapture = null; try { - sendChannel.writeAndFlush(message); + assertTrue(sendChannel.writeAndFlush(message).await(5, TimeUnit.SECONDS)); receiverLatch.await(5, TimeUnit.SECONDS); message.resetReaderIndex(); ArgumentCaptor captor = ArgumentCaptor.forClass(ByteBuf.class); diff --git a/handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java b/handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java index d033ca9365..26904c0117 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java +++ b/handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java @@ -182,7 +182,7 @@ final class SniClientJava8TestUtil { if (clientSide) { Assert.assertEquals(0, extendedSSLSession.getPeerSupportedSignatureAlgorithms().length); } else { - if (session instanceof OpenSslSession && OpenSslTestUtils.isBoringSSL()) { + if (session instanceof OpenSslSession && OpenSsl.isBoringSSL()) { // BoringSSL does not support SSL_get_sigalgs(...) // https://boringssl.googlesource.com/boringssl/+/ba16a1e405c617f4179bd780ad15522fb25b0a65%5E%21/ Assert.assertEquals(0, extendedSSLSession.getPeerSupportedSignatureAlgorithms().length); diff --git a/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java b/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java index 97727ac1fd..9f205825e5 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java @@ -205,7 +205,7 @@ public class SslErrorTest { verifyException(unwrappedCause, "expired", promise); } else if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) { // BoringSSL uses "expired" in this case while others use "bad" - if (OpenSslTestUtils.isBoringSSL()) { + if (OpenSsl.isBoringSSL()) { verifyException(unwrappedCause, "expired", promise); } else { verifyException(unwrappedCause, "bad", promise); @@ -217,7 +217,7 @@ public class SslErrorTest { verifyException(unwrappedCause, "expired", promise); } else if (exception instanceof CertificateNotYetValidException) { // BoringSSL uses "expired" in this case while others use "bad" - if (OpenSslTestUtils.isBoringSSL()) { + if (OpenSsl.isBoringSSL()) { verifyException(unwrappedCause, "expired", promise); } else { verifyException(unwrappedCause, "bad", promise); diff --git a/pom.xml b/pom.xml index 162696d261..78187ab3f8 100644 --- a/pom.xml +++ b/pom.xml @@ -241,7 +241,7 @@ fedora netty-tcnative - 2.0.18.Final + 2.0.19.Final ${os.detected.classifier} org.conscrypt conscrypt-openjdk-uber