Netty factory: Close TCP connection when receive TLS close notify

vladimirlagunov · vladimirlagunov · commit 350a806f1e91 · 2019-07-15T16:11:15.000+07:00
TLS has special TLS Alert message "close_notify". After receiving such packet any conversation
should be immediately stopped. Some TLS implementations (OpenSSL for example) closes underlying TCP
connection when TLS connection being closed, some (Docker SSL server and docker-java before current
commit) does not close. AttachContainerCmd executes `onComplete` callback only when TCP connection
closes, so without this patch `onComplete` called after a long timeout, with this patch it called
when container exits.
diff --git a/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java b/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java
@@ -15,6 +15,8 @@
 
 import com.github.dockerjava.core.AbstractDockerCmdExecFactory;
 import com.github.dockerjava.core.WebTarget;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
 import org.apache.commons.lang.SystemUtils;
 
 import com.github.dockerjava.api.command.DockerCmdExecFactory;
@@ -232,12 +234,34 @@ public DuplexChannel connect(Bootstrap bootstrap) throws InterruptedException {
                 throw new RuntimeException("no port configured for " + host);
             }
 
-            DuplexChannel channel = (DuplexChannel) bootstrap.connect(host, port).sync().channel();
+            final DuplexChannel channel = (DuplexChannel) bootstrap.connect(host, port).sync().channel();
 
             final SslHandler ssl = initSsl(dockerClientConfig);
 
             if (ssl != null) {
                 channel.pipeline().addFirst(ssl);
+
+                // https://tools.ietf.org/html/rfc5246#section-7.2.1
+                // TLS has its own special message about connection termination. Because TLS is a
+                // session-level protocol, it can be covered by any transport-level protocol like
+                // TCP, UTP and so on. But we know exactly that data being transferred over TCP and
+                // that other side will never send any byte into this TCP connection, so this
+                // channel should be closed.
+                // RFC says that we must notify opposite side about closing. This could be done only
+                // in sun.security.ssl.SSLEngineImpl and unfortunately it does not send this
+                // message. On the other hand RFC does not enforce the opposite side to wait for
+                // such message.
+                ssl.sslCloseFuture().addListener(new GenericFutureListener<Future<? super Channel>>() {
+                    @Override
+                    public void operationComplete(Future<? super Channel> future) {
+                        channel.eventLoop().execute(new Runnable() {
+                            @Override
+                            public void run() {
+                                channel.close();
+                            }
+                        });
+                    }
+                });
             }
 
             return channel;
diff --git a/src/test/java/com/github/dockerjava/cmd/AttachContainerCmdIT.java b/src/test/java/com/github/dockerjava/cmd/AttachContainerCmdIT.java
@@ -13,11 +13,14 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.ByteArrayInputStream;
+import java.io.Closeable;
 import java.io.File;
 import java.io.InputStream;
 import java.io.PipedInputStream;
 import java.io.PipedOutputStream;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
 
 import static com.github.dockerjava.junit.DockerRule.DEFAULT_IMAGE;
 import static java.util.concurrent.TimeUnit.SECONDS;
@@ -28,6 +31,7 @@
 import static org.hamcrest.Matchers.isEmptyString;
 import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeThat;
 
 /**
@@ -205,6 +209,68 @@ public void onNext(Frame frame) {
         callback.close();
     }
 
+    /**
+     * {@link AttachContainerResultCallback#onComplete()} should be called immediately after
+     * container exit. It was broken for Netty and TLS connection.
+     */
+    @Test
+    public void attachContainerClosesStdoutWhenContainerExits() throws Exception {
+        DockerClient dockerClient = dockerRule.getClient();
+
+        CreateContainerResponse container = dockerClient.createContainerCmd(DEFAULT_IMAGE)
+                .withCmd("echo", "hello world")
+                .withTty(false)
+                .exec();
+        LOG.info("Created container: {}", container.toString());
+
+        final CountDownLatch started = new CountDownLatch(1);
+        final AtomicLong startedAtNanos = new AtomicLong();
+        final CountDownLatch gotLine = new CountDownLatch(1);
+        final CountDownLatch completed = new CountDownLatch(1);
+        final AtomicLong gotLineAtNanos = new AtomicLong();
+        AttachContainerTestCallback callback = new AttachContainerTestCallback() {
+            @Override
+            public void onStart(Closeable stream) {
+                startedAtNanos.set(System.nanoTime());
+                started.countDown();
+                super.onStart(stream);
+            }
+
+            @Override
+            public void onNext(Frame item) {
+                if (item.getStreamType() == StreamType.STDOUT) {
+                    gotLineAtNanos.set(System.nanoTime());
+                    gotLine.countDown();
+                }
+                super.onNext(item);
+            }
+
+            @Override
+            public void onComplete() {
+                completed.countDown();
+                super.onComplete();
+            }
+        };
+
+        try (Closeable ignored = callback) {
+            dockerClient.attachContainerCmd(container.getId())
+                    .withStdOut(true)
+                    .withFollowStream(true)
+                    .exec(callback);
+
+            dockerClient.startContainerCmd(container.getId()).exec();
+
+            assertTrue("Should start in a reasonable time", started.await(30, SECONDS));
+            assertTrue("Should get first line quickly after the start", gotLine.await(15, SECONDS));
+
+            long gotLineDurationSeconds = (gotLineAtNanos.get() - startedAtNanos.get()) / 1_000_000_000L;
+            LOG.info("Got line from {} for {} seconds", container.getId(), gotLineDurationSeconds);
+
+            boolean finished = completed.await(1L + gotLineDurationSeconds, SECONDS);
+            assertTrue("Should get EOF in a time close to time of getting the first line", finished);
+        }
+    }
+
     public static class AttachContainerTestCallback extends AttachContainerResultCallback {
         private StringBuffer log = new StringBuffer();
 
