SSL connection not closed properly after handshake failure

Motivation:

When SSL handshake fails, the connection should be closed. This is not true anymore after 978a46c.

Modifications:

- Ensure we always flush and close the channel on handshake failure.
- Add testcase.

Result:

Fixes [netty#7724].

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

@@ -81,7 +81,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
                                         "not an SSL/TLS record: " + ByteBufUtil.hexDump(in));
                                 in.skipBytes(in.readableBytes());
                                 ctx.fireUserEventTriggered(new SniCompletionEvent(e));
-                                SslUtils.notifyHandshakeFailure(ctx, e, true);
+                                SslUtils.handleHandshakeFailure(ctx, e, true);
                                 throw e;
                             }
                             if (len == SslUtils.NOT_ENOUGH_DATA ||

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

@@ -865,7 +865,7 @@ private void finishWrap(ChannelHandlerContext ctx, ByteBuf out, ChannelPromise p
 
     /**
      * This method will not call
-     * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable, boolean, boolean)} or
+     * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable, boolean, boolean, boolean)} or
      * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable)}.
      * @return {@code true} if this method ends on {@link SSLEngineResult.HandshakeStatus#NOT_HANDSHAKING}.
      */
@@ -1002,7 +1002,7 @@ private SSLEngineResult wrap(ByteBufAllocator alloc, SSLEngine engine, ByteBuf i
     public void channelInactive(ChannelHandlerContext ctx) throws Exception {
         // Make sure to release SSLEngine,
         // and notify the handshake future if the connection has been closed during handshake.
-        setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed, handshakeStarted);
+        setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed, handshakeStarted, false);
 
         // Ensure we always notify the sslClosePromise as well
         notifyClosePromise(CHANNEL_CLOSED);
@@ -1191,7 +1191,8 @@ private void handleUnwrapThrowable(ChannelHandlerContext ctx, Throwable cause) {
             logger.debug("SSLException during trying to call SSLEngine.wrap(...)" +
                     " because of an previous SSLException, ignoring...", ex);
         } finally {
-            setHandshakeFailure(ctx, cause, true, false);
+            // ensure we always flush and close the channel.
+            setHandshakeFailure(ctx, cause, true, false, true);
         }
         PlatformDependent.throwException(cause);
     }
@@ -1498,13 +1499,14 @@ private void setHandshakeSuccess() {
      * Notify all the handshake futures about the failure during the handshake.
      */
     private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause) {
-        setHandshakeFailure(ctx, cause, true, true);
+        setHandshakeFailure(ctx, cause, true, true, false);
     }
 
     /**
      * Notify all the handshake futures about the failure during the handshake.
      */
-    private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean closeInbound, boolean notify) {
+    private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean closeInbound,
+                                     boolean notify, boolean alwaysFlushAndClose) {
         try {
             // Release all resources such as internal buffers that SSLEngine
             // is managing.
@@ -1526,7 +1528,9 @@ private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boo
                     }
                 }
             }
-            notifyHandshakeFailure(cause, notify);
+            if (handshakePromise.tryFailure(cause) || alwaysFlushAndClose) {
+                SslUtils.handleHandshakeFailure(ctx, cause, notify);
+            }
         } finally {
             // Ensure we remove and fail all pending writes in all cases and so release memory quickly.
             releaseAndFailAll(cause);
@@ -1539,12 +1543,6 @@ private void releaseAndFailAll(Throwable cause) {
         }
     }
 
-    private void notifyHandshakeFailure(Throwable cause, boolean notify) {
-        if (handshakePromise.tryFailure(cause)) {
-            SslUtils.notifyHandshakeFailure(ctx, cause, notify);
-        }
-    }
-
     private void notifyClosePromise(Throwable cause) {
         if (cause == null) {
             if (sslClosePromise.trySuccess(ctx.channel())) {
@@ -1725,7 +1723,9 @@ public void run() {
                     return;
                 }
                 try {
-                    notifyHandshakeFailure(HANDSHAKE_TIMED_OUT, true);
+                    if (handshakePromise.tryFailure(HANDSHAKE_TIMED_OUT)) {
+                        SslUtils.handleHandshakeFailure(ctx, HANDSHAKE_TIMED_OUT, true);
+                    }
                 } finally {
                     releaseAndFailAll(HANDSHAKE_TIMED_OUT);
                 }

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

@@ -310,7 +310,7 @@ private static int getEncryptedPacketLength(ByteBuffer buffer) {
         return packetLength;
     }
 
-    static void notifyHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean notify) {
+    static void handleHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean notify) {
         // We have may haven written some parts of data before an exception was thrown so ensure we always flush.
         // See https://github.com/netty/netty/issues/3900#issuecomment-172481830
         ctx.flush();

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

@@ -599,4 +599,58 @@ protected void initChannel(Channel ch) throws Exception {
             ReferenceCountUtil.release(sslClientCtx);
         }
     }
+
+    @Test(timeout = 10000)
+    public void testCloseOnHandshakeFailure() throws Exception {
+        final SelfSignedCertificate ssc = new SelfSignedCertificate();
+
+        final SslContext sslServerCtx = SslContextBuilder.forServer(ssc.key(), ssc.cert()).build();
+        final SslContext sslClientCtx = SslContextBuilder.forClient()
+                .trustManager(new SelfSignedCertificate().cert())
+                .build();
+
+        EventLoopGroup group = new NioEventLoopGroup(1);
+        Channel sc = null;
+        Channel cc = null;
+        try {
+            LocalAddress address = new LocalAddress(getClass().getSimpleName() + ".testCloseOnHandshakeFailure");
+            ServerBootstrap sb = new ServerBootstrap()
+                    .group(group)
+                    .channel(LocalServerChannel.class)
+                    .childHandler(new ChannelInitializer<Channel>() {
+                        @Override
+                        protected void initChannel(Channel ch) {
+                            ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc()));
+                        }
+                    });
+            sc = sb.bind(address).syncUninterruptibly().channel();
+
+            Bootstrap b = new Bootstrap()
+                    .group(group)
+                    .channel(LocalChannel.class)
+                    .handler(new ChannelInitializer<Channel>() {
+                        @Override
+                        protected void initChannel(Channel ch) {
+                            ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc()));
+                        }
+                    });
+            cc = b.connect(sc.localAddress()).syncUninterruptibly().channel();
+            SslHandler handler = cc.pipeline().get(SslHandler.class);
+            handler.handshakeFuture().awaitUninterruptibly();
+            assertFalse(handler.handshakeFuture().isSuccess());
+
+            cc.closeFuture().syncUninterruptibly();
+        } finally {
+            if (cc != null) {
+                cc.close().syncUninterruptibly();
+            }
+            if (sc != null) {
+                sc.close().syncUninterruptibly();
+            }
+            group.shutdownGracefully();
+
+            ReferenceCountUtil.release(sslServerCtx);
+            ReferenceCountUtil.release(sslClientCtx);
+        }
+    }
 }