Repository: netty/netty
Issue: OpenSslEngine doesn't respond to close_notify
Labels: defect

### Expected behavior

When a Netty server receives a TLS close_notify, it should respond with a close_notify.

### Actual behavior

Netty's [OpenSslEngine](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java) (actually [ReferenceCountedOpenSslEngine](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java)) never sends the close_notify response.  When a close_notify comes in, it's processed inside of [unwrap()](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java#L699), eventually reaching this [closeAll()](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java#L845) call on line 845.  Then, [closeInbound()](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java#L1007) calls [shutdown()](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java#L359), which deallocates the OpenSSL data structures, before we have a chance for netty to grab the data.

The native Java [SSLEngine](https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html) works fine.  This [unwrap()](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L968)
 call returns an SSLEngineResult where `status == CLOSED`, `handshakeStatus == NEED_WRAP`. As a result, [wrapNonAppData(ctx, true)](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L1005) sets [needsFlush = true](https://github.com/netty/netty/blob/netty-4.1.6.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L609), which lets netty know to flush the close_notify out to the network.

### Steps to reproduce

* In terminal window 1, run a tcpdump to monitor TLS traffic: `sudo tcpdump -lilo0 port 8992 | fgrep -v 'length 0'`
* In terminal window 2, build the Secure Chat example, and run with `netty-tcnative-boringssl-static-1.1.33.Fork25.jar` in the CLASSPATH.  Test from terminal window 3: `openssl s_client -connect 127.0.0.1:8992 -tls1`.  Send a couple test messages, and then close the TLS connection by entering `Q` on a line by itself.  The tcpdump shows a close_notify sent from the client, but nothing coming back from the server.  This shows that close_notify goes unanswered when using OpenSSL.  Terminate the server.
* In terminal window 2, build the Secure Chat example, but run without `netty-tcnative-boringssl-static-1.1.33.Fork25.jar` in the CLASSPATH.  Test from terminal window 3: `openssl s_client -connect 127.0.0.1:8992 -tls1`.  Send a couple test messages, and then close the TLS connection by entering `Q` on a line by itself.  The tcpdump shows a close_notify sent from the client, and a response sent from the server.  This shows that close_notify works properly when using the native JDK implementation.  Terminate the server.

### Minimal yet complete reproducer code
Secure Chat example from netty-4.1.6.Final:
[SecureChatServer.java](https://github.com/netty/netty/blob/netty-4.1.6.Final/example/src/main/java/io/netty/example/securechat/SecureChatServer.java)
[SecureChatServerHandler.java](https://github.com/netty/netty/blob/netty-4.1.6.Final/example/src/main/java/io/netty/example/securechat/SecureChatServerHandler.java)
[SecureChatServerInitializer.java](https://github.com/netty/netty/blob/netty-4.1.6.Final/example/src/main/java/io/netty/example/securechat/SecureChatServerInitializer.java)

### Netty version

[netty-all-4.1.6.Final.jar](https://repo1.maven.org/maven2/io/netty/netty-all/4.1.6.Final/netty-all-4.1.6.Final.jar)
[netty-tcnative-boringssl-static-1.1.33.Fork25.jar](https://repo1.maven.org/maven2/io/netty/netty-tcnative-boringssl-static/1.1.33.Fork25/netty-tcnative-boringssl-static-1.1.33.Fork25.jar)

### JVM version

```
java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
```

### OS version

Mac OS X El Capitan v10.11.6 with latest updates

Issue comments:
@markdascher I think this is fixed in 4.1.7.Final-SNAPSHOT (current 4.1 branch). Can you please test and let us know ?
Wow, wasn't expecting an answer so fast.  👍

I just downloaded [netty-all-4.1.7.Final-20161230.120821-77.jar](https://oss.sonatype.org/content/repositories/snapshots/io/netty/netty-all/4.1.7.Final-SNAPSHOT/netty-all-4.1.7.Final-20161230.120821-77.jar), but the behavior is the same.
Ok thx will have a look
thanks for the detailed issue description @markdascher! seems reasonable to me and very helpful.
Yay!

The best workaround I've found, for the moment, is to add a listener to `SslHandler.sslCloseFuture()` to close the TCP connection immediately.  For example, replace [line 47](https://github.com/netty/netty/blob/netty-4.1.6.Final/example/src/main/java/io/netty/example/securechat/SecureChatServerInitializer.java#L47) of SecureChatServerInitializer.java with:

```
SslHandler sslHnd = sslCtx.newHandler(ch.alloc());
sslHnd.sslCloseFuture().addListener(
	new GenericFutureListener<Future<Channel>>() {
		@Override
		public void operationComplete(Future<Channel> future) throws Exception {
		if (future.isSuccess()) {
			future.getNow().close();
		}
	}
});
pipeline.addLast(sslHnd);
```

That at least prevents the connection from sitting idle while the client waits for an answer, but it'll still be better to have the close_notify go out first.  I'm pretty new to Netty, so please correct me if there's a better way to do it.  (e.g. Would `.disconnect()` be better than `.close()`?)
Looking now...
Stay tuned for a PR... I think I know the best way to fix this.
@markdascher https://github.com/netty/netty/pull/6189 should fix it... Can you check ?
Looks like that does the trick!  I built netty from the `fix_close_notify` branch, and now see the close_notify whether or not I use `netty-tcnative-boringssl-static-1.1.33.Fork25.jar`.

Thanks!
@markdascher boom! Thanks for reporting back :)
@markdascher and thanks again for the detailed description how to reproduce it easily. Without this it would have take a lot longer.
Fixed by https://github.com/netty/netty/pull/6189

Pull request review comments:
nit: revert to be consistent with other switch cases (and make merging easier :) )
futher -> further

or just `UNWRAP/WRAP are not expected after this point`
fixed
fixed

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
index 5594f3a4e66..23c666796ff 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -544,11 +544,14 @@ private SSLEngineResult readPendingBytesFromBIO(ByteBuffer dst, int bytesConsume
             final SSLEngineResult.Status rs;
 
             // If isOutboundDone, then the data from the network BIO
-            // was the close_notify message and all was consumed we are not required to wait
-            // for the receipt the peer's close_notify message -- shutdown.
+            // was the close_notify message, see if we also received the response yet.
             if (isOutboundDone()) {
                 rs = CLOSED;
-                shutdown();
+                if (isInboundDone()) {
+                    // If the inbound was done as well, we need to ensure we return NOT_HANDSHAKING to signal we are
+                    // done.
+                    hs = NOT_HANDSHAKING;
+                }
             } else {
                 rs = OK;
             }
@@ -1054,7 +1057,11 @@ public final synchronized void closeInbound() throws SSLException {
 
         isInboundDone = true;
 
-        shutdown();
+        if (isOutboundDone()) {
+            // Only call shutdown if there is no outbound data pending.
+            // See https://github.com/netty/netty/issues/6167
+            shutdown();
+        }
 
         if (handshakeState != HandshakeState.NOT_STARTED && !receivedShutdown) {
             throw new SSLException(


Test patch:
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 98d629206b9..140d95829f2 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -1214,4 +1214,113 @@ private static void testCloseInboundAfterBeginHandshake(SSLEngine engine) throws
             // expected
         }
     }
+
+    @Test
+    public void testCloseNotifySequence() throws Exception {
+        SelfSignedCertificate cert = new SelfSignedCertificate();
+
+        clientSslCtx = SslContextBuilder
+                .forClient()
+                .trustManager(cert.cert())
+                .sslProvider(sslClientProvider())
+                .build();
+        SSLEngine client = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
+
+        serverSslCtx = SslContextBuilder
+                .forServer(cert.certificate(), cert.privateKey())
+                .sslProvider(sslServerProvider())
+                .build();
+        SSLEngine server = serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
+
+        try {
+            ByteBuffer plainClientOut = ByteBuffer.allocate(client.getSession().getApplicationBufferSize());
+            ByteBuffer plainServerOut = ByteBuffer.allocate(server.getSession().getApplicationBufferSize());
+
+            ByteBuffer encryptedClientToServer = ByteBuffer.allocate(client.getSession().getPacketBufferSize());
+            ByteBuffer encryptedServerToClient = ByteBuffer.allocate(server.getSession().getPacketBufferSize());
+            ByteBuffer empty = ByteBuffer.allocate(0);
+
+            handshake(client, server);
+
+            // This will produce a close_notify
+            client.closeOutbound();
+
+            // Something still pending in the outbound buffer.
+            assertFalse(client.isOutboundDone());
+            assertFalse(client.isInboundDone());
+
+            // Now wrap and so drain the outbound buffer.
+            SSLEngineResult result = client.wrap(empty, encryptedClientToServer);
+            encryptedClientToServer.flip();
+
+            assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus());
+            // Need an UNWRAP to read the response of the close_notify
+            assertEquals(SSLEngineResult.HandshakeStatus.NEED_UNWRAP, result.getHandshakeStatus());
+
+            int produced = result.bytesProduced();
+            int consumed = result.bytesConsumed();
+            int closeNotifyLen = produced;
+
+            assertTrue(produced > 0);
+            assertEquals(0, consumed);
+            assertEquals(produced, encryptedClientToServer.remaining());
+            // Outbound buffer should be drained now.
+            assertTrue(client.isOutboundDone());
+            assertFalse(client.isInboundDone());
+
+            assertFalse(server.isOutboundDone());
+            assertFalse(server.isInboundDone());
+            result = server.unwrap(encryptedClientToServer, plainServerOut);
+            plainServerOut.flip();
+
+            assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus());
+            // Need a WRAP to respond to the close_notify
+            assertEquals(SSLEngineResult.HandshakeStatus.NEED_WRAP, result.getHandshakeStatus());
+
+            produced = result.bytesProduced();
+            consumed = result.bytesConsumed();
+            assertEquals(closeNotifyLen, consumed);
+            assertEquals(0, produced);
+            // Should have consumed the complete close_notify
+            assertEquals(0, encryptedClientToServer.remaining());
+            assertEquals(0, plainServerOut.remaining());
+
+            assertFalse(server.isOutboundDone());
+            assertTrue(server.isInboundDone());
+
+            result = server.wrap(empty, encryptedServerToClient);
+            encryptedServerToClient.flip();
+            assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus());
+            // UNWRAP/WRAP are not expected after this point
+            assertEquals(SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING, result.getHandshakeStatus());
+
+            produced = result.bytesProduced();
+            consumed = result.bytesConsumed();
+            assertEquals(closeNotifyLen, produced);
+            assertEquals(0, consumed);
+
+            assertEquals(produced, encryptedServerToClient.remaining());
+            assertTrue(server.isOutboundDone());
+            assertTrue(server.isInboundDone());
+
+            result = client.unwrap(encryptedServerToClient, plainClientOut);
+            plainClientOut.flip();
+            assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus());
+            // UNWRAP/WRAP are not expected after this point
+            assertEquals(SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING, result.getHandshakeStatus());
+
+            produced = result.bytesProduced();
+            consumed = result.bytesConsumed();
+            assertEquals(closeNotifyLen, consumed);
+            assertEquals(0, produced);
+            assertEquals(0, encryptedServerToClient.remaining());
+
+            assertTrue(client.isOutboundDone());
+            assertTrue(client.isInboundDone());
+        } finally {
+            cert.delete();
+            cleanupClientSslEngine(client);
+            cleanupServerSslEngine(server);
+        }
+    }
 }