Workaround possible JDK bug in SSLEngineImpl when using TLSv1.3 that lead to multiple notifications (#10860)

Motivation:

When using the JDKs SSLEngineImpl with TLSv1.3 it sometimes returns HandshakeResult.FINISHED multiple times. This can lead to have SslHandshakeCompletionEvents to be fired multiple times.

Modifications:

- Keep track of if we notified before and if so not do so again if we use TLSv1.3
- Add unit test

Result:

Consistent usage of events

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

@@ -1800,17 +1800,24 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
      * Notify all the handshake futures about the successfully handshake
      */
     private void setHandshakeSuccess() {
-        handshakePromise.trySuccess(ctx.channel());
+        boolean notified = handshakePromise.trySuccess(ctx.channel());
+        SSLSession session = engine.getSession();
 
-        if (logger.isDebugEnabled()) {
-            SSLSession session = engine.getSession();
-            logger.debug(
-              "{} HANDSHAKEN: protocol:{} cipher suite:{}",
-              ctx.channel(),
-              session.getProtocol(),
-              session.getCipherSuite());
+        // There seems to be a bug in the SSLEngineImpl that is part of the OpenJDK that results in returning
+        // HandshakeStatus.FINISHED multiple times which is not expected. This only happens in TLSv1.3 so lets
+        // ensure we only notify once in this case.
+        //
+        // This is safe as TLSv1.3 does not support renegotiation and so we should never see two handshake events.
+        if (notified || !SslUtils.PROTOCOL_TLS_V1_3.equals(session.getProtocol())) {
+            if (logger.isDebugEnabled()) {
+                logger.debug(
+                        "{} HANDSHAKEN: protocol:{} cipher suite:{}",
+                        ctx.channel(),
+                        session.getProtocol(),
+                        session.getCipherSuite());
+            }
+            ctx.fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS);
         }
-        ctx.fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS);
 
         if (readDuringHandshake && !ctx.channel().config().isAutoRead()) {
             readDuringHandshake = false;

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

@@ -46,7 +46,10 @@ public abstract class RenegotiateTest {
         EventLoopGroup group = new LocalEventLoopGroup();
         try {
             final SslContext context = SslContextBuilder.forServer(cert.key(), cert.cert())
-                    .sslProvider(serverSslProvider()).build();
+                    .sslProvider(serverSslProvider())
+                    .protocols(SslUtils.PROTOCOL_TLS_V1_2)
+                    .build();
+
             ServerBootstrap sb = new ServerBootstrap();
             sb.group(group).channel(LocalServerChannel.class)
                     .childHandler(new ChannelInitializer<Channel>() {
@@ -95,7 +98,10 @@ public abstract class RenegotiateTest {
             Channel channel = sb.bind(new LocalAddress("test")).syncUninterruptibly().channel();
 
             final SslContext clientContext = SslContextBuilder.forClient()
-                    .trustManager(InsecureTrustManagerFactory.INSTANCE).sslProvider(SslProvider.JDK).build();
+                    .trustManager(InsecureTrustManagerFactory.INSTANCE)
+                    .sslProvider(SslProvider.JDK)
+                    .protocols(SslUtils.PROTOCOL_TLS_V1_2)
+                    .build();
 
             Bootstrap bootstrap = new Bootstrap();
             bootstrap.group(group).channel(LocalChannel.class)

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

@@ -1482,4 +1482,117 @@ public class SslHandlerTest {
         }
     }
 
+    @Test
+    public void testHandshakeEventsTls12JDK() throws Exception {
+        testHandshakeEvents(SslProvider.JDK, SslUtils.PROTOCOL_TLS_V1_2);
+    }
+
+    @Test
+    public void testHandshakeEventsTls12Openssl() throws Exception {
+        assumeTrue(OpenSsl.isAvailable());
+        testHandshakeEvents(SslProvider.OPENSSL, SslUtils.PROTOCOL_TLS_V1_2);
+    }
+
+    @Test
+    public void testHandshakeEventsTls13JDK() throws Exception {
+        assumeTrue(SslProvider.isTlsv13Supported(SslProvider.JDK));
+        testHandshakeEvents(SslProvider.JDK, SslUtils.PROTOCOL_TLS_V1_3);
+    }
+
+    @Test
+    public void testHandshakeEventsTls13Openssl() throws Exception {
+        assumeTrue(OpenSsl.isAvailable());
+        assumeTrue(SslProvider.isTlsv13Supported(SslProvider.OPENSSL));
+        testHandshakeEvents(SslProvider.OPENSSL, SslUtils.PROTOCOL_TLS_V1_3);
+    }
+
+    private void testHandshakeEvents(SslProvider provider, String protocol) throws Exception {
+        final SslContext sslClientCtx = SslContextBuilder.forClient()
+                .trustManager(InsecureTrustManagerFactory.INSTANCE)
+                .protocols(protocol)
+                .sslProvider(provider).build();
+
+        final SelfSignedCertificate cert = new SelfSignedCertificate();
+        final SslContext sslServerCtx = SslContextBuilder.forServer(cert.key(), cert.cert())
+                .protocols(protocol)
+                .sslProvider(provider).build();
+
+        EventLoopGroup group = new NioEventLoopGroup();
+
+        final LinkedBlockingQueue<SslHandshakeCompletionEvent> serverCompletionEvents =
+                new LinkedBlockingQueue<SslHandshakeCompletionEvent>();
+
+        final LinkedBlockingQueue<SslHandshakeCompletionEvent> clientCompletionEvents =
+                new LinkedBlockingQueue<SslHandshakeCompletionEvent>();
+        try {
+            Channel sc = new ServerBootstrap()
+                    .group(group)
+                    .channel(NioServerSocketChannel.class)
+                    .childHandler(new ChannelInitializer<Channel>() {
+                        @Override
+                        protected void initChannel(Channel ch) throws Exception {
+                            ch.pipeline().addLast(sslServerCtx.newHandler(UnpooledByteBufAllocator.DEFAULT));
+                            ch.pipeline().addLast(new SslHandshakeCompletionEventHandler(serverCompletionEvents));
+                        }
+                    })
+                    .bind(new InetSocketAddress(0)).syncUninterruptibly().channel();
+
+            Bootstrap bs = new Bootstrap()
+                    .group(group)
+                    .channel(NioSocketChannel.class)
+                    .handler(new ChannelInitializer<Channel>() {
+                        @Override
+                        protected void initChannel(Channel ch) {
+                            ch.pipeline().addLast(sslClientCtx.newHandler(
+                                    UnpooledByteBufAllocator.DEFAULT, "netty.io", 9999));
+                            ch.pipeline().addLast(new SslHandshakeCompletionEventHandler(clientCompletionEvents));
+                        }
+                    })
+                    .remoteAddress(sc.localAddress());
+
+            Channel cc1 = bs.connect().sync().channel();
+            Channel cc2 = bs.connect().sync().channel();
+
+            // We expect 4 events as we have 2 connections and for each connection there should be one event
+            // on the server-side and one on the client-side.
+            for (int i = 0; i < 2; i++) {
+                SslHandshakeCompletionEvent event = clientCompletionEvents.take();
+                assertTrue(event.isSuccess());
+            }
+            for (int i = 0; i < 2; i++) {
+                SslHandshakeCompletionEvent event = serverCompletionEvents.take();
+                assertTrue(event.isSuccess());
+            }
+
+            cc1.close().sync();
+            cc2.close().sync();
+            sc.close().sync();
+            assertEquals(0, clientCompletionEvents.size());
+            assertEquals(0, serverCompletionEvents.size());
+        } finally {
+            group.shutdownGracefully();
+            ReferenceCountUtil.release(sslClientCtx);
+            ReferenceCountUtil.release(sslServerCtx);
+        }
+    }
+
+    private static class SslHandshakeCompletionEventHandler extends ChannelInboundHandlerAdapter {
+        private final Queue<SslHandshakeCompletionEvent> completionEvents;
+
+        SslHandshakeCompletionEventHandler(Queue<SslHandshakeCompletionEvent> completionEvents) {
+            this.completionEvents = completionEvents;
+        }
+
+        @Override
+        public void userEventTriggered(ChannelHandlerContext ctx, Object evt) {
+            if (evt instanceof SslHandshakeCompletionEvent) {
+                completionEvents.add((SslHandshakeCompletionEvent) evt);
+            }
+        }
+
+        @Override
+        public boolean isSharable() {
+            return true;
+        }
+    };
 }