From 316dd982848eae7baa8acbe181b5157fbde00031 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Sun, 24 Mar 2019 09:03:27 +0100 Subject: [PATCH] Allow to offload certificate validation when using BoringSSL (#8908) Motivation: BoringSSL supports offloading certificate validation to a different thread. This is useful as it may need to do blocking operations and so may block the EventLoop. Modification: - Adjust ReferenceCountedOpenSslEngine to correctly handle offloaded certificate validation (just as we already have code for certificate selection). Result: Be able to offload certificate validation when using BoringSSL. --- .../ssl/ReferenceCountedOpenSslEngine.java | 75 +++++++++++-------- .../java/io/netty/handler/ssl/SslHandler.java | 5 +- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index 4627d9502b..3cf6b0056f 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -737,7 +737,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc // Flush any data that may have been written implicitly during the handshake by OpenSSL. bytesProduced = SSL.bioFlushByteBuffer(networkBIO); - if (bytesProduced > 0 && handshakeException != null) { + if (handshakeException != null) { // TODO(scott): It is possible that when the handshake failed there was not enough room in the // non-application buffers to hold the alert. We should get all the data before progressing on. // However I'm not aware of a way to do this with the OpenSSL APIs. @@ -746,7 +746,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc // We produced / consumed some data during the handshake, signal back to the caller. // If there is a handshake exception and we have produced data, we should send the data before // we allow handshake() to throw the handshake exception. - return newResult(NEED_WRAP, 0, bytesProduced); + // + // unwrap(...) will then ensure we propagate the handshake error back to the user. + return newResult(NEED_UNWRAP, 0, bytesProduced); } status = handshake(); @@ -889,7 +891,8 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc // to write encrypted data to. This is an OVERFLOW condition. // [1] https://www.openssl.org/docs/manmaster/ssl/SSL_write.html return newResult(BUFFER_OVERFLOW, status, bytesConsumed, bytesProduced); - } else if (sslError == SSL.SSL_ERROR_WANT_X509_LOOKUP) { + } else if (sslError == SSL.SSL_ERROR_WANT_X509_LOOKUP || + sslError == SSL.SSL_ERROR_WANT_CERTIFICATE_VERIFY) { return newResult(NEED_TASK, bytesConsumed, bytesProduced); } else { // Everything else is considered as error @@ -1178,7 +1181,8 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } return newResultMayFinishHandshake(isInboundDone() ? CLOSED : OK, status, bytesConsumed, bytesProduced); - } else if (sslError == SSL.SSL_ERROR_WANT_X509_LOOKUP) { + } else if (sslError == SSL.SSL_ERROR_WANT_X509_LOOKUP + || sslError == SSL.SSL_ERROR_WANT_CERTIFICATE_VERIFY) { return newResult(isInboundDone() ? CLOSED : OK, NEED_TASK, bytesConsumed, bytesProduced); } else { @@ -1324,11 +1328,11 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc return new Runnable() { @Override public void run() { + if (isDestroyed()) { + // The engine was destroyed in the meantime, just return. + return; + } try { - if (isDestroyed()) { - // The engine was destroyed in the meantime, just return. - return; - } task.run(); } finally { // The task was run, reset needTask to false so getHandshakeStatus() returns the correct value. @@ -1679,27 +1683,35 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc return cert == null || cert.length == 0; } + private SSLEngineResult.HandshakeStatus handshakeException() throws SSLException { + if (SSL.bioLengthNonApplication(networkBIO) > 0) { + // There is something pending, we need to consume it first via a WRAP so we don't loose anything. + return NEED_WRAP; + } + + SSLHandshakeException exception = handshakeException; + handshakeException = null; + shutdown(); + throw exception; + } + private SSLEngineResult.HandshakeStatus handshake() throws SSLException { + if (needTask) { + return NEED_TASK; + } if (handshakeState == HandshakeState.FINISHED) { return FINISHED; } + checkEngineClosed(HANDSHAKE_ENGINE_CLOSED); - // Check if we have a pending handshakeException and if so see if we need to consume all pending data from the - // BIO first or can just shutdown and throw it now. - // This is needed so we ensure close_notify etc is correctly send to the remote peer. - // See https://github.com/netty/netty/issues/3900 - SSLHandshakeException exception = handshakeException; - if (exception != null) { - if (SSL.bioLengthNonApplication(networkBIO) > 0) { - // There is something pending, we need to consume it first via a WRAP so we don't loose anything. - return NEED_WRAP; - } - // No more data left to send to the remote peer, so null out the exception field, shutdown and throw - // the exception. - handshakeException = null; - shutdown(); - throw exception; + if (handshakeException != null) { + // If we there was an handshake exception we need to call SSL.doHandshake(...) again as it may produce + // some alert or advance the internal state machine. Also clear the error queue as this call may put + // something on the queue. + SSL.doHandshake(ssl); + SSL.clearError(); + return handshakeException(); } // Adding the OpenSslEngine to the OpenSslEngineMap so it can be used in the AbstractCertificateVerifier. @@ -1710,24 +1722,21 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc int code = SSL.doHandshake(ssl); if (code <= 0) { - // Check if we have a pending exception that was created during the handshake and if so throw it after - // shutdown the connection. - if (handshakeException != null) { - exception = handshakeException; - handshakeException = null; - shutdown(); - throw exception; - } - int sslError = SSL.getError(ssl, code); if (sslError == SSL.SSL_ERROR_WANT_READ || sslError == SSL.SSL_ERROR_WANT_WRITE) { return pendingStatus(SSL.bioLengthNonApplication(networkBIO)); } - if (sslError == SSL.SSL_ERROR_WANT_X509_LOOKUP) { + if (sslError == SSL.SSL_ERROR_WANT_X509_LOOKUP || sslError == SSL.SSL_ERROR_WANT_CERTIFICATE_VERIFY) { return NEED_TASK; } + // Check if we have a pending exception that was created during the handshake and if so throw it after + // shutdown the connection. + if (handshakeException != null) { + return handshakeException(); + } + // Everything else is considered as error throw shutdownWithError("SSL_do_handshake", sslError); } diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java index c3f74af4ef..b7a392f106 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -989,6 +989,8 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH throw new IllegalStateException("Unknown handshake status: " + result.getHandshakeStatus()); } + // Check if did not produce any bytes and if so break out of the loop, but only if we did not process + // a task as last action. It's fine to not produce any data as part of executing a task. if (result.bytesProduced() == 0 && status != HandshakeStatus.NEED_TASK) { break; } @@ -1657,7 +1659,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } // Flush now as we may have written some data as part of the wrap call. - forceFlush(ctx); + forceFlush(ctx); } catch (Throwable e) { taskError(e); return; @@ -1666,6 +1668,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH // Now try to feed in more data that we have buffered. tryDecodeAgain(); break; + default: // Should never reach here as we handle all cases. throw new AssertionError();