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 e0130ddb196..4802db7338a 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(); @@ -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 handshake = threadPool.submit(new Callable() + { + @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); 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 b0f576995e7..b24b4572e85 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; @@ -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); 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 05e00c92f5f..43a99696565 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(); + // 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 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; + HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); + HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus(); + Status unwrapResultStatus = unwrapResult.getStatus(); - case NEED_TASK: - // run the task - _sslEngine.getDelegatedTask().run(); - continue; + _underFlown = unwrapResultStatus == Status.BUFFER_UNDERFLOW; - 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(); - - 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); + if (_underFlown) + { + if (net_filled < 0) closeInbound(); - return -1; - } + if (net_filled <= 0) + return net_filled; + } - 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) + switch (unwrapResultStatus) + { + case CLOSED: { - if (app_in == buffer) - return unwrapResult.bytesProduced(); - return BufferUtil.flipPutFlip(_decryptedInput, buffer); - } - - // Dang! we have to care about the handshake state - switch (handshakeStatus) - { - 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(); }