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 fd433a5112..933436f034 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/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; diff --git a/handler/src/test/java/io/netty/handler/ssl/RenegotiateTest.java b/handler/src/test/java/io/netty/handler/ssl/RenegotiateTest.java index 3c122c31d5..0cee84be9e 100644 --- a/handler/src/test/java/io/netty/handler/ssl/RenegotiateTest.java +++ b/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() { @@ -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) diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java index cac0cb8853..cc646d0570 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/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 serverCompletionEvents = + new LinkedBlockingQueue(); + + final LinkedBlockingQueue clientCompletionEvents = + new LinkedBlockingQueue(); + try { + Channel sc = new ServerBootstrap() + .group(group) + .channel(NioServerSocketChannel.class) + .childHandler(new ChannelInitializer() { + @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() { + @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 completionEvents; + + SslHandshakeCompletionEventHandler(Queue 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; + } + }; }