Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fixes #3787 - Jetty client throws EOFException instead of SSLHandshakeException on certificate errors. 

Now exceptions thrown by fill() or flush() are stored in a field.
Further fill() operations will rethrow the original exception rather
than returning -1.

Returning -1 to application was causing them to close() with a generic
failure that was triggering the EOFException reported in this issue.

Now applications see the original exception and can close() with the
proper cause.

Re-enabled HostnameVerificationTest that was reproducing this issue
reliably but was @Disabled a while back and never re-enabled.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
This commit is contained in:
Simone Bordet 2019-09-26 18:30:43 +02:00
parent d12df30df7
commit 2c75e876a3
2 changed files with 79 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
jetty-client/src/test/java/org/eclipse/jetty/client/HostnameVerificationTest.java
View File

@ -22,7 +22,6 @@ import java.io.IOException;
import java.security.cert.CertificateException;
import java.util.concurrent.ExecutionException;
import javax.net.ssl.SSLHandshakeException;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@ -36,7 +35,6 @@ import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import static org.hamcrest.MatcherAssert.assertThat;
@ -47,7 +45,6 @@ import static org.junit.jupiter.api.Assertions.fail;
* This test class runs tests to make sure that hostname verification (http://www.ietf.org/rfc/rfc2818.txt
* section 3.1) is configurable in SslContextFactory and works as expected.
*/
@Disabled
public class HostnameVerificationTest
{
private SslContextFactory clientSslContextFactory = new SslContextFactory.Client();
@ -70,7 +67,7 @@ public class HostnameVerificationTest
server.setHandler(new DefaultHandler()
{
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
{
baseRequest.setHandled(true);
response.getWriter().write("foobar");
@ -94,30 +91,33 @@ public class HostnameVerificationTest
{
client.stop();
server.stop();
server.join();
}
/**
* This test is supposed to verify that hostname verification works as described in:
* http://www.ietf.org/rfc/rfc2818.txt section 3.1. It uses a certificate with a common name different to localhost
* and sends a request to localhost. This should fail with an SSLHandshakeException.
*
* @throws Exception on test failure
*/
@Test
public void simpleGetWithHostnameVerificationEnabledTest() throws Exception
public void simpleGetWithHostnameVerificationEnabledTest()
{
clientSslContextFactory.setEndpointIdentificationAlgorithm("HTTPS");
String uri = "https://localhost:" + connector.getLocalPort() + "/";
ExecutionException x = assertThrows(ExecutionException.class, () ->
{
client.GET(uri);
});
ExecutionException x = assertThrows(ExecutionException.class, () -> client.GET(uri));
Throwable cause = x.getCause();
assertThat(cause, Matchers.instanceOf(SSLHandshakeException.class));
Throwable root = cause.getCause().getCause();
assertThat(root, Matchers.instanceOf(CertificateException.class));
// Search for the CertificateException.
Throwable certificateException = cause.getCause();
while (certificateException != null)
{
if (certificateException instanceof CertificateException)
break;
certificateException = certificateException.getCause();
}
assertThat(certificateException, Matchers.instanceOf(CertificateException.class));
}
/**

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

@ -146,7 +146,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
@Override
public InvocationType getInvocationType()
{
return getDecryptedEndPoint().getFillInterest().getCallbackInvocationType();
return _decryptedEndPoint.getFillInterest().getCallbackInvocationType();
}
};
@ -364,6 +364,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
public class DecryptedEndPoint extends AbstractEndPoint
{
private final Callback _incompleteWriteCallback = new IncompleteWriteCallback();
private Throwable _failure;
public DecryptedEndPoint()
{
@ -529,12 +530,12 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
case NEED_WRAP:
if (_flushState == FlushState.IDLE && flush(BufferUtil.EMPTY_BUFFER))
{
rethrow(_failure);
if (_sslEngine.isInboundDone())
// TODO this is probably a JVM bug, work around it by -1
return -1;
return filled = -1;
continue;
}
// handle in needsFillInterest
// Handle in needsFillInterest().
return filled = 0;
default:
@ -598,6 +599,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
switch (unwrap)
{
case CLOSED:
rethrow(_failure);
return filled = -1;
case BUFFER_UNDERFLOW:
@ -607,6 +609,8 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
if (netFilled < 0 && _sslEngine.getUseClientMode())
{
closeInbound();
if (_flushState == FlushState.WAIT_FOR_FILL)
throw new SSLHandshakeException("Abruptly closed by peer");
return filled = -1;
}
return filled = netFilled;
@ -639,15 +643,14 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
}
catch (Throwable x)
{
handshakeFailed(x);
Throwable failure = handleException(x, "fill");
handshakeFailed(failure);
if (_flushState == FlushState.WAIT_FOR_FILL)
{
_flushState = FlushState.IDLE;
getExecutor().execute(() -> _decryptedEndPoint.getWriteFlusher().onFail(x));
getExecutor().execute(() -> _decryptedEndPoint.getWriteFlusher().onFail(failure));
}
throw x;
throw failure;
}
finally
{
@ -676,10 +679,9 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
}
catch (Throwable x)
{
if (LOG.isDebugEnabled())
LOG.debug(SslConnection.this.toString(), x);
close(x);
throw x;
rethrow(x);
return -1;
}
}
@ -694,15 +696,18 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
synchronized (_decryptedEndPoint)
{
if (LOG.isDebugEnabled())
{
LOG.debug(">needFillInterest uf={} {}", _underflown, SslConnection.this);
LOG.debug("ei={} di={}", BufferUtil.toDetailString(_encryptedInput), BufferUtil.toDetailString(_decryptedInput));
}
LOG.debug(">needFillInterest s={}/{} uf={} ei={} di={} {}",
_flushState,
_fillState,
_underflown,
BufferUtil.toDetailString(_encryptedInput),
BufferUtil.toDetailString(_decryptedInput),
SslConnection.this);
if (_fillState != FillState.IDLE)
return;
// Fillable if we have decrypted Input OR encrypted input that has not yet been underflown.
// Fillable if we have decrypted input OR enough encrypted input.
fillable = BufferUtil.hasContent(_decryptedInput) || (BufferUtil.hasContent(_encryptedInput) && !_underflown);
HandshakeStatus status = _sslEngine.getHandshakeStatus();
@ -966,8 +971,9 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
}
catch (Throwable x)
{
handshakeFailed(x);
throw x;
Throwable failure = handleException(x, "flush");
handshakeFailed(failure);
throw failure;
}
finally
{
@ -979,10 +985,9 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
}
catch (Throwable x)
{
if (LOG.isDebugEnabled())
LOG.debug(SslConnection.this.toString(), x);
close(x);
throw x;
rethrow(x);
return false;
}
}
@ -1092,7 +1097,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
boolean ishut = endp.isInputShutdown();
boolean oshut = endp.isOutputShutdown();
if (LOG.isDebugEnabled())
LOG.debug("shutdownOutput: {} oshut={}, ishut={} {}", SslConnection.this, oshut, ishut);
LOG.debug("shutdownOutput: {} oshut={}, ishut={}", SslConnection.this, oshut, ishut);
closeOutbound();
@ -1110,15 +1115,11 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
{
if (!flush(BufferUtil.EMPTY_BUFFER) && !close)
{
Thread.yield();
// if we still can't flush, but we are not closing the endpoint,
// If we still can't flush, but we are not closing the endpoint,
// let's just flush the encrypted output in the background.
// and continue as if we are closed. The assumption here is that
// the encrypted buffer will contain the entire close handshake
// and that a call to flush(EMPTY_BUFFER) is not needed.
endp.write(Callback.from(() ->
{
}, t -> endp.close()), _encryptedOutput);
ByteBuffer write = _encryptedOutput;
if (BufferUtil.hasContent(write))
endp.write(Callback.from(Callback.NOOP::succeeded, t -> endp.close()), write);
}
}
@ -1282,6 +1283,39 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
return TLS_1_3.equals(protocol);
}
private Throwable handleException(Throwable x, String context)
{
synchronized (_decryptedEndPoint)
{
if (_failure == null)
{
_failure = x;
if (LOG.isDebugEnabled())
LOG.debug(this + " stored " + context + " exception", x);
}
else if (x != _failure)
{
_failure.addSuppressed(x);
if (LOG.isDebugEnabled())
LOG.debug(this + " suppressed " + context + " exception", x);
}
return _failure;
}
}
private void rethrow(Throwable x) throws IOException
{
if (x == null)
return;
if (x instanceof RuntimeException)
throw (RuntimeException)x;
if (x instanceof Error)
throw (Error)x;
if (x instanceof IOException)
throw (IOException)x;
throw new IOException(x);
}
@Override
public String toString()
{