diff --git a/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NextProtoNegoServerConnection.java b/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnection.java similarity index 52% rename from jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NextProtoNegoServerConnection.java rename to jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnection.java index 6f32a197a66..8d2d8a8bb64 100644 --- a/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NextProtoNegoServerConnection.java +++ b/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnection.java @@ -20,7 +20,6 @@ package org.eclipse.jetty.spdy.server; import java.io.IOException; import java.util.List; - import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLEngineResult; @@ -34,7 +33,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -public class NextProtoNegoServerConnection extends AbstractConnection implements NextProtoNego.ServerProvider +public class NPNServerConnection extends AbstractConnection implements NextProtoNego.ServerProvider { private final Logger LOG = Log.getLogger(getClass()); private final Connector connector; @@ -43,7 +42,7 @@ public class NextProtoNegoServerConnection extends AbstractConnection implements private final String defaultProtocol; private String nextProtocol; // No need to be volatile: it is modified and read by the same thread - public NextProtoNegoServerConnection(EndPoint endPoint, SSLEngine engine, Connector connector, Listprotocols, String defaultProtocol) + public NPNServerConnection(EndPoint endPoint, SSLEngine engine, Connector connector, List protocols, String defaultProtocol) { super(endPoint, connector.getExecutor()); this.connector = connector; @@ -63,33 +62,58 @@ public class NextProtoNegoServerConnection extends AbstractConnection implements @Override public void onFillable() { - while (true) - { - int filled = fill(); - if (filled == 0 && nextProtocol == null) - fillInterested(); - if (filled <= 0 || nextProtocol != null) - break; - } + int filled = fill(); - if (nextProtocol == null && engine.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) + if (filled == 0) { - // The client sent the NPN extension, but did not send the NextProtocol - // message with the chosen protocol so we need to close - LOG.debug("{} missing next protocol. SSLEngine: {}", this, engine); + if (nextProtocol == null) + { + if (engine.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) + { + // Here the SSL handshake is finished, but while the client sent + // the NPN extension, the server application did not select the + // protocol; we need to close as the protocol cannot be negotiated. + LOG.debug("{} missing next protocol. SSLEngine: {}", this, engine); + close(); + } + else + { + // Here the SSL handshake is not finished yet but we filled 0 bytes, + // so we need to read more. + fillInterested(); + } + } + else + { + ConnectionFactory connectionFactory = connector.getConnectionFactory(nextProtocol); + if (connectionFactory == null) + { + LOG.debug("{} application selected protocol '{}', but no correspondent {} has been configured", + this, nextProtocol, ConnectionFactory.class.getName()); + close(); + } + else + { + EndPoint endPoint = getEndPoint(); + Connection oldConnection = endPoint.getConnection(); + Connection newConnection = connectionFactory.newConnection(connector, endPoint); + LOG.debug("{} switching from {} to {}", this, oldConnection, newConnection); + oldConnection.onClose(); + endPoint.setConnection(newConnection); + getEndPoint().getConnection().onOpen(); + } + } + } + else if (filled < 0) + { + // Something went bad, we need to close. + LOG.debug("{} closing on client close", this); close(); } - - if (nextProtocol != null) + else { - ConnectionFactory connectionFactory = connector.getConnectionFactory(nextProtocol); - EndPoint endPoint = getEndPoint(); - Connection oldConnection = endPoint.getConnection(); - oldConnection.onClose(); - Connection connection = connectionFactory.newConnection(connector, endPoint); - LOG.debug("{} switching from {} to {}", this, oldConnection, connection); - endPoint.setConnection(connection); - getEndPoint().getConnection().onOpen(); + // Must never happen, since we fill using an empty buffer + throw new IllegalStateException(); } } @@ -102,8 +126,7 @@ public class NextProtoNegoServerConnection extends AbstractConnection implements catch (IOException x) { LOG.debug(x); - NextProtoNego.remove(engine); - getEndPoint().close(); + close(); return -1; } } @@ -127,4 +150,13 @@ public class NextProtoNegoServerConnection extends AbstractConnection implements nextProtocol = protocol != null ? protocol : defaultProtocol; NextProtoNego.remove(engine); } + + @Override + public void close() + { + NextProtoNego.remove(engine); + EndPoint endPoint = getEndPoint(); + endPoint.shutdownOutput(); + endPoint.close(); + } } diff --git a/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnectionFactory.java b/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnectionFactory.java index b370e392860..14b17219fc1 100644 --- a/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnectionFactory.java +++ b/jetty-spdy/spdy-server/src/main/java/org/eclipse/jetty/spdy/server/NPNServerConnectionFactory.java @@ -22,7 +22,6 @@ package org.eclipse.jetty.spdy.server; import java.util.Arrays; import java.util.Iterator; import java.util.List; - import javax.net.ssl.SSLEngine; import org.eclipse.jetty.io.Connection; @@ -110,7 +109,7 @@ public class NPNServerConnectionFactory extends AbstractConnectionFactory ep=null; } - return configure(new NextProtoNegoServerConnection(endPoint, engine, connector,protocols,_defaultProtocol),connector,endPoint); + return configure(new NPNServerConnection(endPoint, engine, connector,protocols,_defaultProtocol),connector,endPoint); } @Override diff --git a/jetty-spdy/spdy-server/src/test/java/org/eclipse/jetty/spdy/server/SSLSynReplyTest.java b/jetty-spdy/spdy-server/src/test/java/org/eclipse/jetty/spdy/server/SSLSynReplyTest.java index f19672206dc..8af6083928b 100644 --- a/jetty-spdy/spdy-server/src/test/java/org/eclipse/jetty/spdy/server/SSLSynReplyTest.java +++ b/jetty-spdy/spdy-server/src/test/java/org/eclipse/jetty/spdy/server/SSLSynReplyTest.java @@ -19,14 +19,22 @@ package org.eclipse.jetty.spdy.server; +import java.net.InetSocketAddress; +import java.nio.ByteBuffer; +import java.nio.channels.SocketChannel; +import java.util.List; import java.util.concurrent.Executor; +import javax.net.ssl.SSLEngine; import org.eclipse.jetty.npn.NextProtoNego; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.spdy.api.server.ServerSessionFrameListener; import org.eclipse.jetty.spdy.client.SPDYClient; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.junit.Assert; import org.junit.Before; +import org.junit.Test; public class SSLSynReplyTest extends SynReplyTest { @@ -51,4 +59,119 @@ public class SSLSynReplyTest extends SynReplyTest { NextProtoNego.debug = true; } + + @Test + public void testGentleCloseDuringHandshake() throws Exception + { + InetSocketAddress address = startServer(version, null); + SslContextFactory sslContextFactory = newSslContextFactory(); + sslContextFactory.start(); + SSLEngine sslEngine = sslContextFactory.newSSLEngine(address); + sslEngine.setUseClientMode(true); + NextProtoNego.put(sslEngine, new NextProtoNego.ClientProvider() + { + @Override + public boolean supports() + { + return true; + } + + @Override + public void unsupported() + { + } + + @Override + public String selectProtocol(List protocols) + { + return null; + } + }); + sslEngine.beginHandshake(); + + ByteBuffer encrypted = ByteBuffer.allocate(sslEngine.getSession().getPacketBufferSize()); + sslEngine.wrap(BufferUtil.EMPTY_BUFFER, encrypted); + encrypted.flip(); + + try (SocketChannel channel = SocketChannel.open(address)) + { + // Send ClientHello, immediately followed by TLS Close Alert and then by FIN + channel.write(encrypted); + sslEngine.closeOutbound(); + encrypted.clear(); + sslEngine.wrap(BufferUtil.EMPTY_BUFFER, encrypted); + encrypted.flip(); + channel.write(encrypted); + channel.shutdownOutput(); + + // Read ServerHello from server + encrypted.clear(); + int read = channel.read(encrypted); + encrypted.flip(); + Assert.assertTrue(read > 0); + // Cannot decrypt, as the SSLEngine has been already closed + + // Now if we read more, we should either read the TLS Close Alert, or directly -1 + encrypted.clear(); + read = channel.read(encrypted); + // Sending a TLS Close Alert during handshake results in an exception when + // unwrapping that the server react to by closing the connection abruptly. + Assert.assertTrue(read < 0); + } + } + + @Test + public void testAbruptCloseDuringHandshake() throws Exception + { + InetSocketAddress address = startServer(version, null); + SslContextFactory sslContextFactory = newSslContextFactory(); + sslContextFactory.start(); + SSLEngine sslEngine = sslContextFactory.newSSLEngine(address); + sslEngine.setUseClientMode(true); + NextProtoNego.put(sslEngine, new NextProtoNego.ClientProvider() + { + @Override + public boolean supports() + { + return true; + } + + @Override + public void unsupported() + { + } + + @Override + public String selectProtocol(List protocols) + { + return null; + } + }); + sslEngine.beginHandshake(); + + ByteBuffer encrypted = ByteBuffer.allocate(sslEngine.getSession().getPacketBufferSize()); + sslEngine.wrap(BufferUtil.EMPTY_BUFFER, encrypted); + encrypted.flip(); + + try (SocketChannel channel = SocketChannel.open(address)) + { + // Send ClientHello, immediately followed by FIN (no TLS Close Alert) + channel.write(encrypted); + channel.shutdownOutput(); + + // Read ServerHello from server + encrypted.clear(); + int read = channel.read(encrypted); + encrypted.flip(); + Assert.assertTrue(read > 0); + ByteBuffer decrypted = ByteBuffer.allocate(sslEngine.getSession().getApplicationBufferSize()); + sslEngine.unwrap(encrypted, decrypted); + + // Now if we read more, we should either read the TLS Close Alert, or directly -1 + encrypted.clear(); + read = channel.read(encrypted); + // Since we have close the connection abruptly, the server also does so + Assert.assertTrue(read < 0); + } + } }