418892 - SSL session caching so unreliable it effectively does not

work.

Fixed by making sure that we completely decrypt read bytes.

Before the fix, it was possible that we returned after the decryption
of one TLS frame, while another was still present in the
_encryptedBuffer.
This lead to non-clean closes of the connection, which hampered the
capability of session reuse by clients.

Now we decrypt in a loop and only return if there is nothing more
that we can decrypt.

3 files changed, 220 insertions, 122 deletions

diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java
index e0130ddb19..4802db7338 100644
--- a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java
+++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java
@@ -84,6 +84,7 @@ public class SslBytesServerTest extends SslBytesTest
     private ExecutorService threadPool;
     private Server server;
     private SslContextFactory sslContextFactory;
+    private int serverPort;
     private SSLContext sslContext;
     private SimpleProxy proxy;
     private Runnable idleHook;
@@ -211,7 +212,7 @@ public class SslBytesServerTest extends SslBytesTest
             }
         });
         server.start();
-        int serverPort = connector.getLocalPort();
+        serverPort = connector.getLocalPort();
 
         sslContext = sslContextFactory.getSslContext();
 
@@ -293,6 +294,88 @@ public class SslBytesServerTest extends SslBytesTest
     }
 
     @Test
+    public void testHandshakeWithResumedSessionThenClose() throws Exception
+    {
+        // First socket will establish the SSL session
+        SSLSocket client1 = newClient();
+        SimpleProxy.AutomaticFlow automaticProxyFlow = proxy.startAutomaticFlow();
+        client1.startHandshake();
+        client1.close();
+        Assert.assertTrue(automaticProxyFlow.stop(5, TimeUnit.SECONDS));
+        int proxyPort = proxy.getPort();
+        proxy.stop();
+
+        proxy = new SimpleProxy(threadPool, proxyPort, "localhost", serverPort);
+        proxy.start();
+        logger.info("proxy:{} <==> server:{}", proxy.getPort(), serverPort);
+
+        final SSLSocket client2 = newClient(proxy);
+
+        Future<Object> handshake = threadPool.submit(new Callable<Object>()
+        {
+            @Override
+            public Object call() throws Exception
+            {
+                client2.startHandshake();
+                return null;
+            }
+        });
+
+        // Client Hello with SessionID
+        TLSRecord record = proxy.readFromClient();
+        Assert.assertNotNull(record);
+        proxy.flushToServer(record);
+
+        // Server Hello
+        record = proxy.readFromServer();
+        Assert.assertNotNull(record);
+        proxy.flushToClient(record);
+
+        // Change Cipher Spec
+        record = proxy.readFromServer();
+        Assert.assertNotNull(record);
+        proxy.flushToClient(record);
+
+        // Server Done
+        record = proxy.readFromServer();
+        Assert.assertNotNull(record);
+        proxy.flushToClient(record);
+
+        // Client Key Exchange
+        record = proxy.readFromClient();
+        Assert.assertNotNull(record);
+        // Client Done
+        TLSRecord doneRecord = proxy.readFromClient();
+        Assert.assertNotNull(doneRecord);
+        // Close
+        client2.close();
+        TLSRecord closeRecord = proxy.readFromClient();
+        Assert.assertNotNull(closeRecord);
+        Assert.assertEquals(TLSRecord.Type.ALERT, closeRecord.getType());
+        // Flush to server Client Key Exchange + Client Done + Close in one chunk
+        byte[] recordBytes = record.getBytes();
+        byte[] doneBytes = doneRecord.getBytes();
+        byte[] closeRecordBytes = closeRecord.getBytes();
+        byte[] chunk = new byte[recordBytes.length + doneBytes.length + closeRecordBytes.length];
+        System.arraycopy(recordBytes, 0, chunk, 0, recordBytes.length);
+        System.arraycopy(doneBytes, 0, chunk, recordBytes.length, doneBytes.length);
+        System.arraycopy(closeRecordBytes, 0, chunk, recordBytes.length + doneBytes.length, closeRecordBytes.length);
+        proxy.flushToServer(0, chunk);
+        // Close the raw socket
+        proxy.flushToServer(null);
+
+        // Expect the server to send a FIN as well
+        record = proxy.readFromServer();
+        Assert.assertNull(record);
+
+        // Check that we did not spin
+        TimeUnit.MILLISECONDS.sleep(500);
+        Assert.assertThat(sslFills.get(), Matchers.lessThan(20));
+        Assert.assertThat(sslFlushes.get(), Matchers.lessThan(20));
+        Assert.assertThat(httpParses.get(), Matchers.lessThan(20));
+    }
+
+    @Test
     public void testHandshakeWithSplitBoundary() throws Exception
     {
         final SSLSocket client = newClient();
@@ -1795,6 +1878,11 @@ public class SslBytesServerTest extends SslBytesTest
 
     private SSLSocket newClient() throws IOException, InterruptedException
     {
+        return newClient(proxy);
+    }
+
+    private SSLSocket newClient(SimpleProxy proxy) throws IOException, InterruptedException
+    {
         SSLSocket client = (SSLSocket)sslContext.getSocketFactory().createSocket("localhost", proxy.getPort());
         client.setUseClientMode(true);
         Assert.assertTrue(proxy.awaitClient(5, TimeUnit.SECONDS));
diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java
index b0f576995e..b24b4572e8 100644
--- a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java
+++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java
@@ -105,6 +105,7 @@ public abstract class SslBytesTest
     {
         private final CountDownLatch latch = new CountDownLatch(1);
         private final ExecutorService threadPool;
+        private final int proxyPort;
         private final String serverHost;
         private final int serverPort;
         private volatile ServerSocket serverSocket;
@@ -113,14 +114,20 @@ public abstract class SslBytesTest
 
         public SimpleProxy(ExecutorService threadPool, String serverHost, int serverPort)
         {
+            this(threadPool, 0, serverHost, serverPort);
+        }
+
+        public SimpleProxy(ExecutorService threadPool, int proxyPort, String serverHost, int serverPort)
+        {
             this.threadPool = threadPool;
+            this.proxyPort = proxyPort;
             this.serverHost = serverHost;
             this.serverPort = serverPort;
         }
 
         public void start() throws Exception
         {
-            serverSocket = new ServerSocket(0);
+            serverSocket = new ServerSocket(proxyPort);
             Thread acceptor = new Thread(this);
             acceptor.start();
             server = new Socket(serverHost, serverPort);
diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java
index 05e00c92f5..43a9969656 100644
--- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java
+++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java
@@ -189,12 +189,12 @@ public class SslConnection extends AbstractConnection
         // filling.
 
         if (DEBUG)
-            LOG.debug("onFillable enter {}", getEndPoint());
+            LOG.debug("onFillable enter {}", _decryptedEndPoint);
 
         // We have received a close handshake, close the end point to send FIN.
         if (_decryptedEndPoint.isInputShutdown())
-            getEndPoint().close();
-            
+            _decryptedEndPoint.close();
+
         // wake up whoever is doing the fill or the flush so they can
         // do all the filling, unwrapping, wrapping and flushing
         _decryptedEndPoint.getFillInterest().fillable();
@@ -210,7 +210,7 @@ public class SslConnection extends AbstractConnection
         }
 
         if (DEBUG)
-            LOG.debug("onFillable exit {}", getEndPoint());
+            LOG.debug("onFillable exit {}", _decryptedEndPoint);
     }
 
     @Override
@@ -312,7 +312,7 @@ public class SslConnection extends AbstractConnection
                         fail_filler = true;
                     }
                 }
-                
+
                 final boolean filler_failed=fail_filler;
 
                 getExecutor().execute(new Runnable()
@@ -508,142 +508,142 @@ public class SslConnection extends AbstractConnection
                     int net_filled = getEndPoint().fill(_encryptedInput);
                     if (DEBUG)
                         LOG.debug("{} filled {} encrypted bytes", SslConnection.this, net_filled);
-                    if (net_filled > 0)
-                        _underFlown = false;
-
-                    // Let's try the SSL thang even if we have no net data because in that
-                    // case we want to fall through to the handshake handling
-                    int pos = BufferUtil.flipToFill(app_in);
-
-                    SSLEngineResult unwrapResult = _sslEngine.unwrap(_encryptedInput, app_in);
 
-                    BufferUtil.flipToFlush(app_in, pos);
-                    if (DEBUG)
-                        LOG.debug("{} unwrap {}", SslConnection.this, unwrapResult);
-
-                    Status unwrapResultStatus = unwrapResult.getStatus();
-                    HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus();
-                    HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus();
-
-                    // and deal with the results
-                    switch (unwrapResultStatus)
+                    decryption: while (true)
                     {
-                        case BUFFER_OVERFLOW:
-                            throw new IllegalStateException();
-
-                        case CLOSED:
-                            // Dang! we have to care about the handshake state specially for close
-                            switch (handshakeStatus)
-                            {
-                                case NOT_HANDSHAKING:
-                                    // We were not handshaking, so just tell the app we are closed
-                                    return -1;
-
-                                case NEED_TASK:
-                                    // run the task
-                                    _sslEngine.getDelegatedTask().run();
-                                    continue;
+                        // Let's unwrap even if we have no net data because in that
+                        // case we want to fall through to the handshake handling
+                        int pos = BufferUtil.flipToFill(app_in);
+                        SSLEngineResult unwrapResult = _sslEngine.unwrap(_encryptedInput, app_in);
+                        BufferUtil.flipToFlush(app_in, pos);
+                        if (DEBUG)
+                            LOG.debug("{} unwrap {}", SslConnection.this, unwrapResult);
 
-                                case NEED_WRAP:
-                                    // we need to send some handshake data (probably to send a close handshake).
-                                    // but that will not enable any extra data to fill, so we just return -1
-                                    // The wrapping can be done by any output drivers doing flushing or shutdown output.
-                                    return -1;
-                            }
-                            throw new IllegalStateException();
+                        HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus();
+                        HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus();
+                        Status unwrapResultStatus = unwrapResult.getStatus();
 
-                        default:
-                            if (unwrapHandshakeStatus == HandshakeStatus.FINISHED && !_handshaken)
-                            {
-                                _handshaken = true;
-                                if (DEBUG)
-                                    LOG.debug("{} handshake completed client-side", SslConnection.this);
-                            }
+                        _underFlown = unwrapResultStatus == Status.BUFFER_UNDERFLOW;
 
-                            // Check whether renegotiation is allowed
-                            if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed())
-                            {
-                                if (DEBUG)
-                                    LOG.debug("{} renegotiation denied", SslConnection.this);
+                        if (_underFlown)
+                        {
+                            if (net_filled < 0)
                                 closeInbound();
-                                return -1;
-                            }
-
-                            if (unwrapResultStatus == Status.BUFFER_UNDERFLOW)
-                                _underFlown = true;
-
-                            // If bytes were produced, don't bother with the handshake status;
-                            // pass the decrypted data to the application, which will perform
-                            // another call to fill() or flush().
-                            if (unwrapResult.bytesProduced() > 0)
-                            {
-                                if (app_in == buffer)
-                                    return unwrapResult.bytesProduced();
-                                return BufferUtil.flipPutFlip(_decryptedInput, buffer);
-                            }
+                            if (net_filled <= 0)
+                                return net_filled;
+                        }
 
-                            // Dang! we have to care about the handshake state
-                            switch (handshakeStatus)
+                        switch (unwrapResultStatus)
+                        {
+                            case CLOSED:
                             {
-                                case NOT_HANDSHAKING:
-                                    // we just didn't read anything.
-                                    if (net_filled < 0)
+                                switch (handshakeStatus)
+                                {
+                                    case NOT_HANDSHAKING:
                                     {
-                                        closeInbound();
+                                        // We were not handshaking, so just tell the app we are closed
                                         return -1;
                                     }
-                                    return 0;
-
-                                case NEED_TASK:
-                                    // run the task
-                                    _sslEngine.getDelegatedTask().run();
-                                    continue;
-
-                                case NEED_WRAP:
-                                    // we need to send some handshake data
-
-                                    // if we are called from flush
-                                    if (buffer == __FLUSH_CALLED_FILL)
-                                        return 0; // let it do the wrapping
-
-                                    _fillRequiresFlushToProgress = true;
-                                    flush(__FILL_CALLED_FLUSH);
-                                    if (BufferUtil.isEmpty(_encryptedOutput))
+                                    case NEED_TASK:
                                     {
-                                        // the flush completed so continue
-                                        _fillRequiresFlushToProgress = false;
+                                        _sslEngine.getDelegatedTask().run();
                                         continue;
                                     }
-                                    return 0;
-
-                                case NEED_UNWRAP:
-                                    // if we just filled some net data
-                                    if (net_filled < 0)
+                                    case NEED_WRAP:
                                     {
-                                        closeInbound();
+                                        // We need to send some handshake data (probably the close handshake).
+                                        // We return -1 so that the application can drive the close by flushing
+                                        // or shutting down the output.
                                         return -1;
                                     }
-                                    else if (net_filled > 0)
+                                    default:
                                     {
-                                        // maybe we will fill some more on a retry
+                                        throw new IllegalStateException();
+                                    }
+                                }
+                            }
+                            case BUFFER_UNDERFLOW:
+                            case OK:
+                            {
+                                if (unwrapHandshakeStatus == HandshakeStatus.FINISHED && !_handshaken)
+                                {
+                                    _handshaken = true;
+                                    if (DEBUG)
+                                        LOG.debug("{} {} handshake completed", SslConnection.this,
+                                                _sslEngine.getUseClientMode() ? "client-side" : "resumed session server-side");
+                                }
+
+                                // Check whether renegotiation is allowed
+                                if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed())
+                                {
+                                    if (DEBUG)
+                                        LOG.debug("{} renegotiation denied", SslConnection.this);
+                                    closeInbound();
+                                    return -1;
+                                }
+
+                                // If bytes were produced, don't bother with the handshake status;
+                                // pass the decrypted data to the application, which will perform
+                                // another call to fill() or flush().
+                                if (unwrapResult.bytesProduced() > 0)
+                                {
+                                    if (app_in == buffer)
+                                        return unwrapResult.bytesProduced();
+                                    return BufferUtil.flipPutFlip(_decryptedInput, buffer);
+                                }
+
+                                switch (handshakeStatus)
+                                {
+                                    case NOT_HANDSHAKING:
+                                    {
+                                        if (_underFlown)
+                                            break decryption;
                                         continue;
                                     }
-                                    else
+                                    case NEED_TASK:
                                     {
-                                        if (_encryptedInput.hasRemaining())
+                                        _sslEngine.getDelegatedTask().run();
+                                        continue;
+                                    }
+                                    case NEED_WRAP:
+                                    {
+                                        // If we are called from flush()
+                                        // return to let it do the wrapping.
+                                        if (buffer == __FLUSH_CALLED_FILL)
+                                            return 0;
+
+                                        _fillRequiresFlushToProgress = true;
+                                        flush(__FILL_CALLED_FLUSH);
+                                        if (BufferUtil.isEmpty(_encryptedOutput))
                                         {
-                                            // if there are more encrypted bytes,
-                                            // then we need to unwrap more, we don't
-                                            // care if net_filled is zero
+                                            // The flush wrote all the encrypted bytes so continue to fill
+                                            _fillRequiresFlushToProgress = false;
                                             continue;
                                         }
-                                        // we need to wait for more net data
-                                        return 0;
+                                        else
+                                        {
+                                            // The flush did not complete, return from fill()
+                                            // and let the write completion mechanism to kick in.
+                                            return 0;
+                                        }
                                     }
-
-                                case FINISHED:
-                                    throw new IllegalStateException();
+                                    case NEED_UNWRAP:
+                                    {
+                                        if (_underFlown)
+                                            break decryption;
+                                        continue;
+                                    }
+                                    default:
+                                    {
+                                        throw new IllegalStateException();
+                                    }
+                                }
                             }
+                            default:
+                            {
+                                throw new IllegalStateException();
+                            }
+                        }
                     }
                 }
             }
@@ -778,7 +778,7 @@ public class SslConnection extends AbstractConnection
                             {
                                 _handshaken = true;
                                 if (DEBUG)
-                                    LOG.debug("{} handshake completed server-side", SslConnection.this);
+                                    LOG.debug("{} {} handshake completed", SslConnection.this, "server-side");
                             }
 
                             HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus();
@@ -824,7 +824,7 @@ public class SslConnection extends AbstractConnection
                                         if (handshakeStatus == HandshakeStatus.NEED_WRAP)
                                             continue;
                                     }
-                                    return allConsumed&&BufferUtil.isEmpty(_encryptedOutput);
+                                    return allConsumed && BufferUtil.isEmpty(_encryptedOutput);
 
                                 case FINISHED:
                                     throw new IllegalStateException();
@@ -860,17 +860,18 @@ public class SslConnection extends AbstractConnection
         public void shutdownOutput()
         {
             boolean ishut = isInputShutdown();
+            boolean oshut = isOutputShutdown();
             if (DEBUG)
-                LOG.debug("{} shutdownOutput: oshut={}, ishut={}", SslConnection.this, isOutputShutdown(), ishut);
+                LOG.debug("{} shutdownOutput: oshut={}, ishut={}", SslConnection.this, oshut, ishut);
             if (ishut)
             {
                 // Aggressively close, since inbound close alert has already been processed
                 // and the TLS specification allows to close the connection directly, which
                 // is what most other implementations expect: a FIN rather than a TLS close
-                // reply. If a TLS close reply is sent, most implementation send a RST.
+                // reply. If a TLS close reply is sent, most implementations send a RST.
                 getEndPoint().close();
             }
-            else
+            else if (!oshut)
             {
                 try
                 {
@@ -895,6 +896,8 @@ public class SslConnection extends AbstractConnection
         @Override
         public void close()
         {
+            // First send the TLS Close Alert, then the FIN
+            shutdownOutput();
             getEndPoint().close();
         }