Enable TLS1.3 by default of JDK SSLEngine implementation does by defa…

…ult (netty#10451)

Motiviation:

When TLSv1.3 was introduced almost 2 years ago, it was decided to disable it by default, even when it's supported by the underlying TLS implementation.

TLSv13 is pretty stable now in Java (out of the box in Java 11, OpenJSSE for Java 8, BoringSSL and OpenSSL) and may be enabled by default.

Modifications:

Ensure TLSv13 is enabled by default when the underyling JDK SSLEngine implementation enables it as well

Result:

TLSv1.3 is now enabled by default, so users don't have to explicitly enable it.

Co-authored-by: Stephane Landelle <[email protected]>

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

@@ -18,6 +18,7 @@
 
 import io.netty.buffer.ByteBufAllocator;
 import io.netty.util.ReferenceCountUtil;
+import io.netty.util.internal.EmptyArrays;
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
 
@@ -106,11 +107,11 @@ private static String[] defaultProtocols(SSLContext context, SSLEngine engine) {
         List<String> protocols = new ArrayList<String>();
         addIfSupported(
                 supportedProtocolsSet, protocols,
-                // Do not include TLSv1.3 for now by default.
-                SslUtils.PROTOCOL_TLS_V1_2, SslUtils.PROTOCOL_TLS_V1_1, SslUtils.PROTOCOL_TLS_V1);
+                SslUtils.PROTOCOL_TLS_V1_3, SslUtils.PROTOCOL_TLS_V1_2,
+                SslUtils.PROTOCOL_TLS_V1_1, SslUtils.PROTOCOL_TLS_V1);
 
         if (!protocols.isEmpty()) {
-            return protocols.toArray(new String[0]);
+            return protocols.toArray(EmptyArrays.EMPTY_STRINGS);
         }
         return engine.getEnabledProtocols();
     }

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

@@ -260,9 +260,6 @@ public ApplicationProtocolConfig.SelectedListenerFailureBehavior selectedListene
             int options = SSLContext.getOptions(ctx) |
                           SSL.SSL_OP_NO_SSLv2 |
                           SSL.SSL_OP_NO_SSLv3 |
-                          // Disable TLSv1.3 by default for now. Even if TLSv1.3 is not supported this will
-                          // work fine as in this case SSL_OP_NO_TLSv1_3 will be 0.
-                          SSL.SSL_OP_NO_TLSv1_3 |
 
                           SSL.SSL_OP_CIPHER_SERVER_PREFERENCE |
 

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

@@ -68,4 +68,20 @@ public static boolean isTlsv13Supported(final SslProvider provider) {
                 throw new Error("Unknown SslProvider: " + provider);
         }
     }
+
+    /**
+     * Returns {@code true} if the specified {@link SslProvider} enables
+     * <a href="https://tools.ietf.org/html/rfc8446">TLS 1.3</a> by default, {@code false} otherwise.
+     */
+    static boolean isTlsv13EnabledByDefault(final SslProvider provider) {
+        switch (provider) {
+            case JDK:
+                return SslUtils.isTLSv13EnabledByJDK();
+            case OPENSSL:
+            case OPENSSL_REFCNT:
+                return OpenSsl.isTlsv13Supported();
+            default:
+                throw new Error("Unknown SslProvider: " + provider);
+        }
+    }
 }

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

@@ -106,10 +106,13 @@ final class SslUtils {
     static final String[] DEFAULT_TLSV13_CIPHER_SUITES;
     static final String[] TLSV13_CIPHER_SUITES = { "TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384" };
 
-    private static final boolean TLSV1_3_SUPPORTED;
+    private static final boolean TLSV1_3_JDK_SUPPORTED;
+    private static final boolean TLSV1_3_JDK_DEFAULT_ENABLED;
 
     static {
         boolean tlsv13Supported = false;
+        boolean tlsv13Enabled = false;
+
         Throwable cause = null;
         try {
             SSLContext context = SSLContext.getInstance("TLS");
@@ -120,6 +123,12 @@ final class SslUtils {
                     break;
                 }
             }
+            for (String enabled: context.getDefaultSSLParameters().getProtocols()) {
+                if (PROTOCOL_TLS_V1_3.equals(enabled)) {
+                    tlsv13Enabled = true;
+                    break;
+                }
+            }
         } catch (Throwable error) {
             cause = error;
         }
@@ -128,9 +137,9 @@ final class SslUtils {
         } else {
             logger.debug("Unable to detect if JDK SSLEngine supports TLSv1.3, assuming no", cause);
         }
-        TLSV1_3_SUPPORTED = tlsv13Supported;
-
-        if (TLSV1_3_SUPPORTED) {
+        TLSV1_3_JDK_SUPPORTED = tlsv13Supported;
+        TLSV1_3_JDK_DEFAULT_ENABLED = tlsv13Enabled;
+        if (TLSV1_3_JDK_SUPPORTED) {
             DEFAULT_TLSV13_CIPHER_SUITES = TLSV13_CIPHER_SUITES;
         } else {
             DEFAULT_TLSV13_CIPHER_SUITES = EmptyArrays.EMPTY_STRINGS;
@@ -153,14 +162,21 @@ final class SslUtils {
 
         Collections.addAll(defaultCiphers, DEFAULT_TLSV13_CIPHER_SUITES);
 
-        DEFAULT_CIPHER_SUITES = defaultCiphers.toArray(new String[0]);
+        DEFAULT_CIPHER_SUITES = defaultCiphers.toArray(EmptyArrays.EMPTY_STRINGS);
     }
 
     /**
      * Returns {@code true} if the JDK itself supports TLSv1.3, {@code false} otherwise.
      */
     static boolean isTLSv13SupportedByJDK() {
-        return TLSV1_3_SUPPORTED;
+        return TLSV1_3_JDK_SUPPORTED;
+    }
+
+    /**
+     * Returns {@code true} if the JDK itself supports TLSv1.3 and enabled it by default, {@code false} otherwise.
+     */
+    static boolean isTLSv13EnabledByJDK() {
+        return TLSV1_3_JDK_DEFAULT_ENABLED;
     }
 
     /**

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

@@ -1148,29 +1148,37 @@ private static void runTasksIfNeeded(SSLEngine engine) {
 
     @Test
     public void testExtractMasterkeyWorksCorrectly() throws Exception {
+        if (protocolCipherCombo != ProtocolCipherCombo.tlsv12()) {
+            return;
+        }
         SelfSignedCertificate cert = new SelfSignedCertificate();
         serverSslCtx = wrapContext(SslContextBuilder.forServer(cert.key(), cert.cert())
+                .protocols(protocols())
+                .ciphers(ciphers())
                 .sslProvider(SslProvider.OPENSSL).build());
         final SSLEngine serverEngine =
                 wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
         clientSslCtx = wrapContext(SslContextBuilder.forClient()
                 .trustManager(cert.certificate())
+                .protocols(protocols())
+                .ciphers(ciphers())
                 .sslProvider(SslProvider.OPENSSL).build());
         final SSLEngine clientEngine =
                 wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
 
+        final String enabledCipher = "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256";
         try {
             //lets set the cipher suite to a specific one with DHE
             assumeTrue("The diffie hellman cipher is not supported on your runtime.",
                     Arrays.asList(clientEngine.getSupportedCipherSuites())
-                            .contains("TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"));
+                            .contains(enabledCipher));
 
             //https://www.ietf.org/rfc/rfc5289.txt
             //For cipher suites ending with _SHA256, the PRF is the TLS PRF
             //[RFC5246] with SHA-256 as the hash function.  The MAC is HMAC
             //[RFC2104] with SHA-256 as the hash function.
-            clientEngine.setEnabledCipherSuites(new String[] { "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256" });
-            serverEngine.setEnabledCipherSuites(new String[] { "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256" });
+            clientEngine.setEnabledCipherSuites(new String[] { enabledCipher });
+            serverEngine.setEnabledCipherSuites(new String[] { enabledCipher });
 
             int appBufferMax = clientEngine.getSession().getApplicationBufferSize();
             int netBufferMax = clientEngine.getSession().getPacketBufferSize();
@@ -1186,8 +1194,8 @@ public void testExtractMasterkeyWorksCorrectly() throws Exception {
             ByteBuffer cTOs = ByteBuffer.allocate(netBufferMax);
             ByteBuffer sTOc = ByteBuffer.allocate(netBufferMax);
 
-            ByteBuffer clientOut = ByteBuffer.wrap("Hi Server, I'm Client".getBytes());
-            ByteBuffer serverOut = ByteBuffer.wrap("Hello Client, I'm Server".getBytes());
+            ByteBuffer clientOut = ByteBuffer.wrap("Hi Server, I'm Client".getBytes(CharsetUtil.US_ASCII));
+            ByteBuffer serverOut = ByteBuffer.wrap("Hello Client, I'm Server".getBytes(CharsetUtil.US_ASCII));
 
             // This implementation is largely imitated from
             // https://docs.oracle.com/javase/8/docs/technotes/

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

@@ -275,7 +275,7 @@ public String toString() {
     }
 
     private final BufferType type;
-    private final ProtocolCipherCombo protocolCipherCombo;
+    protected final ProtocolCipherCombo protocolCipherCombo;
     private final boolean delegate;
     private ExecutorService delegatingExecutor;
 
@@ -3780,7 +3780,9 @@ private static void assertArrayContains(String expected, String[] array) {
 
     @Test
     public void testMasterKeyLogging() throws Exception {
-
+        if (protocolCipherCombo != ProtocolCipherCombo.tlsv12()) {
+            return;
+        }
         /*
          * At the moment master key logging is not supported for conscrypt
          */
@@ -3799,6 +3801,8 @@ public void testMasterKeyLogging() throws Exception {
         serverSslCtx = wrapContext(SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
                 .sslProvider(sslServerProvider())
                 .sslContextProvider(serverSslContextProvider())
+                .protocols(protocols())
+                .ciphers(ciphers())
                 .build());
         Socket socket = null;
 
@@ -3935,6 +3939,51 @@ public java.security.cert.X509Certificate[] getAcceptedIssuers() {
         }
     }
 
+    @Test
+    public void testDefaultProtocolsIncludeTLSv13() throws Exception {
+        // Don't specify the protocols as we want to test the default selection
+        clientSslCtx = wrapContext(SslContextBuilder.forClient()
+                .trustManager(InsecureTrustManagerFactory.INSTANCE)
+                .sslProvider(sslClientProvider())
+                .sslContextProvider(clientSslContextProvider())
+                .ciphers(ciphers())
+                .build());
+        SelfSignedCertificate ssc = new SelfSignedCertificate();
+        serverSslCtx = wrapContext(SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
+                .sslProvider(sslServerProvider())
+                .sslContextProvider(serverSslContextProvider())
+                .ciphers(ciphers())
+                .build());
+        SSLEngine clientEngine = null;
+        SSLEngine serverEngine = null;
+        String[] clientProtocols;
+        String[] serverProtocols;
+        try {
+            clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
+            serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
+            clientProtocols = clientEngine.getEnabledProtocols();
+            serverProtocols = serverEngine.getEnabledProtocols();
+        } finally {
+            cleanupClientSslEngine(clientEngine);
+            cleanupServerSslEngine(serverEngine);
+            ssc.delete();
+        }
+
+        assertEquals(SslProvider.isTlsv13EnabledByDefault(sslClientProvider()),
+                arrayContains(clientProtocols, PROTOCOL_TLS_V1_3));
+        assertEquals(SslProvider.isTlsv13EnabledByDefault(sslServerProvider()),
+                arrayContains(serverProtocols, PROTOCOL_TLS_V1_3));
+    }
+
+    private static boolean arrayContains(String[] array, String value) {
+        for (String v: array) {
+            if (value.equals(v)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     protected SSLEngine wrapEngine(SSLEngine engine) {
         return engine;
     }