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.

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();
 
@@ -292,6 +293,88 @@ public class SslBytesServerTest extends SslBytesTest
         closeClient(client);
     }
 
+    @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
     {
@@ -1794,6 +1877,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);

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;
@@ -112,15 +113,21 @@ public abstract class SslBytesTest
         private volatile Socket client;
 
         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);

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
+                    decryption: while (true)
-                    // 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)
                     {
-                        case BUFFER_OVERFLOW:
+                        // Let's unwrap even if we have no net data because in that
-                            throw new IllegalStateException();
+                        // 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 CLOSED:
+                        HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus();
-                            // Dang! we have to care about the handshake state specially for close
+                        HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus();
-                            switch (handshakeStatus)
+                        Status unwrapResultStatus = unwrapResult.getStatus();
-                            {
-                                case NOT_HANDSHAKING:
-                                    // We were not handshaking, so just tell the app we are closed
-                                    return -1;
 
-                                case NEED_TASK:
+                        _underFlown = unwrapResultStatus == Status.BUFFER_UNDERFLOW;
-                                    // run the task
-                                    _sslEngine.getDelegatedTask().run();
-                                    continue;
 
-                                case NEED_WRAP:
+                        if (_underFlown)
-                                    // 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
+                            if (net_filled < 0)
-                                    // The wrapping can be done by any output drivers doing flushing or shutdown output.
-                                    return -1;
-                            }
-                            throw new IllegalStateException();
-
-                        default:
-                            if (unwrapHandshakeStatus == HandshakeStatus.FINISHED && !_handshaken)
-                            {
-                                _handshaken = true;
-                                if (DEBUG)
-                                    LOG.debug("{} handshake completed client-side", SslConnection.this);
-                            }
-
-                            // Check whether renegotiation is allowed
-                            if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed())
-                            {
-                                if (DEBUG)
-                                    LOG.debug("{} renegotiation denied", SslConnection.this);
                                 closeInbound();
-                                return -1;
+                            if (net_filled <= 0)
-                            }
+                                return net_filled;
+                        }
 
-                            if (unwrapResultStatus == Status.BUFFER_UNDERFLOW)
+                        switch (unwrapResultStatus)
-                                _underFlown = true;
+                        {
-
+                            case CLOSED:
-                            // 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)
+                                switch (handshakeStatus)
-                                    return unwrapResult.bytesProduced();
+                                {
-                                return BufferUtil.flipPutFlip(_decryptedInput, buffer);
+                                    case NOT_HANDSHAKING:
-                            }
-
-                            // Dang! we have to care about the handshake state
-                            switch (handshakeStatus)
-                            {
-                                case NOT_HANDSHAKING:
-                                    // we just didn't read anything.
-                                    if (net_filled < 0)
                                     {
-                                        closeInbound();
+                                        // We were not handshaking, so just tell the app we are closed
                                         return -1;
                                     }
-                                    return 0;
+                                    case NEED_TASK:
-
-                                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))
                                     {
-                                        // the flush completed so continue
+                                        _sslEngine.getDelegatedTask().run();
-                                        _fillRequiresFlushToProgress = false;
                                         continue;
                                     }
-                                    return 0;
+                                    case NEED_WRAP:
-
-                                case NEED_UNWRAP:
-                                    // if we just filled some net data
-                                    if (net_filled < 0)
                                     {
-                                        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,
+                                            // The flush wrote all the encrypted bytes so continue to fill
-                                            // then we need to unwrap more, we don't
+                                            _fillRequiresFlushToProgress = false;
-                                            // care if net_filled is zero
                                             continue;
                                         }
-                                        // we need to wait for more net data
+                                        else
-                                        return 0;
+                                        {
+                                            // The flush did not complete, return from fill()
+                                            // and let the write completion mechanism to kick in.
+                                            return 0;
+                                        }
                                     }
-
+                                    case NEED_UNWRAP:
-                                case FINISHED:
+                                    {
-                                    throw new IllegalStateException();
+                                        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();
         }