Issue #464 - Improve reporting of SSLHandshakeException.

jetty-client/src/test/java/org/eclipse/jetty/client/HostnameVerificationTest.java

@@ -109,23 +109,10 @@ public class HostnameVerificationTest
         }
         catch (ExecutionException x)
         {
-            // The test may fail in 2 ways, since the CertificateException thrown because of the hostname
-            // verification failure is not rethrown immediately by the JDK SSL implementation, but only
-            // rethrown on the next read or write.
-            // Therefore this test may catch a SSLHandshakeException, or a ClosedChannelException.
-            // If it is the former, we verify that its cause is a CertificateException.
-
-            // ExecutionException wraps an SSLHandshakeException
             Throwable cause = x.getCause();
-            if (cause==null)
-            {
-                x.printStackTrace();
-                Assert.fail("No cause?");
-            }
-            if (cause instanceof SSLHandshakeException)
-                Assert.assertThat(cause.getCause().getCause(), Matchers.instanceOf(CertificateException.class));
-            else
-                Assert.assertThat(cause, Matchers.instanceOf(ClosedChannelException.class));
+            Assert.assertThat(cause, Matchers.instanceOf(SSLHandshakeException.class));
+            Throwable root = cause.getCause().getCause();
+            Assert.assertThat(root, Matchers.instanceOf(CertificateException.class));
         }
     }
 
@@ -134,7 +121,7 @@ public class HostnameVerificationTest
      * work fine.
      *
      * @throws Exception on test failure
-     * 
+     *
      */
     @Test
     public void simpleGetWithHostnameVerificationDisabledTest() throws Exception

jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesClientTest.java

@@ -342,6 +342,9 @@ public class SslBytesClientTest extends SslBytesTest
         Assert.assertEquals(TLSRecord.Type.HANDSHAKE, record.getType());
         proxy.flushToClient(record);
 
+        // Client sends close alert.
+        record = proxy.readFromClient();
+        Assert.assertEquals(TLSRecord.Type.ALERT, record.getType());
         record = proxy.readFromClient();
         Assert.assertNull(record);
 

jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java

@@ -360,7 +360,10 @@ public class SslBytesServerTest extends SslBytesTest
         // Close the raw socket
         proxy.flushToServer(null);
 
-        // Expect the server to send a FIN as well
+        // Expect the server to send a TLS Alert.
+        record = proxy.readFromServer();
+        Assert.assertNotNull(record);
+        Assert.assertEquals(TLSRecord.Type.ALERT, record.getType());
         record = proxy.readFromServer();
         Assert.assertNull(record);
 
@@ -1752,9 +1755,12 @@ public class SslBytesServerTest extends SslBytesTest
         // Instead of passing the Client Hello, we simulate plain text was passed in
         proxy.flushToServer(0, "GET / HTTP/1.1\r\n".getBytes(StandardCharsets.UTF_8));
 
-        // We expect that the server closes the connection immediately
+        // We expect that the server sends the TLS Alert.
         TLSRecord record = proxy.readFromServer();
-        Assert.assertNull(String.valueOf(record), record);
+        Assert.assertNotNull(record);
+        Assert.assertEquals(TLSRecord.Type.ALERT, record.getType());
+        record = proxy.readFromServer();
+        Assert.assertNull(record);
 
         // Check that we did not spin
         TimeUnit.MILLISECONDS.sleep(500);

jetty-http2/http2-alpn-tests/src/test/java/org/eclipse/jetty/http2/alpn/tests/ALPNNegotiationTest.java

@@ -31,6 +31,7 @@ import java.util.List;
 
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLEngineResult;
 import javax.net.ssl.SSLSocket;
 
 import org.eclipse.jetty.alpn.ALPN;
@@ -96,7 +97,8 @@ public class ALPNNegotiationTest extends AbstractALPNTest
             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);
+            Assert.assertTrue(read > 0);
+            Assert.assertEquals(21, encrypted.get(0));
         }
     }
 
@@ -146,11 +148,12 @@ public class ALPNNegotiationTest extends AbstractALPNTest
             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
+            // Now if we read more, we should read the TLS Close Alert.
             encrypted.clear();
             read = channel.read(encrypted);
-            // Since we have close the connection abruptly, the server also does so
-            Assert.assertTrue(read < 0);
+            encrypted.flip();
+            Assert.assertTrue(read > 0);
+            Assert.assertEquals(21, encrypted.get(0));
         }
     }
 

jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java

@@ -107,19 +107,18 @@ public abstract class AbstractEndPoint extends IdleTimeout implements EndPoint
     }
 
     @Override
-    public void onClose()
+    public void close()
     {
-        super.onClose();
-        if (LOG.isDebugEnabled())
-            LOG.debug("onClose {}",this);
+        onClose();
         _writeFlusher.onClose();
         _fillInterest.onClose();
     }
 
-    @Override
-    public void close()
+    protected void close(Throwable failure)
     {
         onClose();
+        _writeFlusher.onFail(failure);
+        _fillInterest.onFail(failure);
     }
 
     @Override

jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java

@@ -495,8 +495,6 @@ abstract public class WriteFlusher
 
     public void onClose()
     {
-        if (_state.get()==__IDLE)
-            return;
         onFail(new ClosedChannelException());
     }
 

jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java

@@ -28,6 +28,7 @@ import javax.net.ssl.SSLEngineResult;
 import javax.net.ssl.SSLEngineResult.HandshakeStatus;
 import javax.net.ssl.SSLEngineResult.Status;
 import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLHandshakeException;
 
 import org.eclipse.jetty.io.AbstractConnection;
 import org.eclipse.jetty.io.AbstractEndPoint;
@@ -655,17 +656,22 @@ public class SslConnection extends AbstractConnection
                     }
                 }
             }
-            catch (IllegalStateException e)
+            catch (SSLHandshakeException x)
             {
-                // Some internal error in SSLEngine
-                LOG.debug(e);
-                close();
-                throw new EofException(e);
+                close(x);
+                throw x;
             }
-            catch (Exception e)
+            catch (SSLException x)
             {
-                close();
-                throw e;
+                if (!_handshaken)
+                    x = (SSLException)new SSLHandshakeException(x.getMessage()).initCause(x);
+                close(x);
+                throw x;
+            }
+            catch (Throwable x)
+            {
+                close(x);
+                throw x;
             }
             finally
             {
@@ -854,6 +860,16 @@ public class SslConnection extends AbstractConnection
                     }
                 }
             }
+            catch (SSLHandshakeException x)
+            {
+                close(x);
+                throw x;
+            }
+            catch (Throwable x)
+            {
+                close(x);
+                throw x;
+            }
             finally
             {
                 releaseEncryptedOutputBuffer();
@@ -878,30 +894,24 @@ public class SslConnection extends AbstractConnection
             boolean oshut = isOutputShutdown();
             if (LOG.isDebugEnabled())
                 LOG.debug("{} shutdownOutput: oshut={}, ishut={}", SslConnection.this, oshut, ishut);
-            if (ishut)
+            try
             {
-                // 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 implementations send a RST.
-                getEndPoint().close();
-            }
-            else if (!oshut)
-            {
-                try
+                synchronized (this)
                 {
-                    synchronized (this) // TODO review synchronized boundary
+                    if (!oshut)
                     {
                         _sslEngine.closeOutbound();
-                        flush(BufferUtil.EMPTY_BUFFER); // Send close handshake
-                        ensureFillInterested();
+                        // Send the TLS close message.
+                        flush(BufferUtil.EMPTY_BUFFER);
+                        if (!ishut)
+                            ensureFillInterested();
                     }
                 }
-                catch (Exception e)
-                {
-                    LOG.ignore(e);
-                    getEndPoint().close();
-                }
+            }
+            catch (Throwable x)
+            {
+                LOG.ignore(x);
+                getEndPoint().close();
             }
         }
 
@@ -920,12 +930,20 @@ public class SslConnection extends AbstractConnection
         @Override
         public void close()
         {
-            // First send the TLS Close Alert, then the FIN
+            // First send the TLS Close Alert, then the FIN.
             shutdownOutput();
             getEndPoint().close();
             super.close();
         }
 
+        protected void close(Throwable failure)
+        {
+            // First send the TLS Close Alert, then the FIN.
+            shutdownOutput();
+            getEndPoint().close();
+            super.close(failure);
+        }
+
         @Override
         public Object getTransport()
         {

jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java

@@ -151,8 +151,8 @@ public class SslConnectionFactoryTest
         {
             out.write("Rubbish".getBytes());
             out.flush();
-
-            Assert.assertThat(socket.getInputStream().read(),Matchers.equalTo(-1));
+            // Expect TLS message type == 21: Alert
+            Assert.assertThat(socket.getInputStream().read(),Matchers.equalTo(21));
         }
     }