diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java index 686dcfe9ed..626a3a217d 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java @@ -278,6 +278,7 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted logger.debug("request of key failed", cause); SSLHandshakeException e = new SSLHandshakeException("General OpenSslEngine problem"); e.initCause(cause); + assert engine.handshakeException == null; engine.handshakeException = e; } } diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java index c6dafe8144..3430257912 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java @@ -664,6 +664,7 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen logger.debug("verification of certificate failed", cause); SSLHandshakeException e = new SSLHandshakeException("General OpenSslEngine problem"); e.initCause(cause); + assert engine.handshakeException == null; engine.handshakeException = e; // Try to extract the correct error code that should be used. 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 a45921c578..2d49e22b6d 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -733,7 +733,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. @@ -742,7 +742,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(); @@ -885,7 +887,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 @@ -1172,7 +1175,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 { @@ -1318,11 +1322,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. @@ -1680,27 +1684,31 @@ 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; + assert exception != null; + 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) { + return handshakeException(); } // Adding the OpenSslEngine to the OpenSslEngineMap so it can be used in the AbstractCertificateVerifier. @@ -1711,24 +1719,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/ReferenceCountedOpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java index f3ee8e2a0f..1f9f455bc4 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java @@ -210,6 +210,7 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted logger.debug("Failed to set the server-side key material", cause); SSLHandshakeException e = new SSLHandshakeException("General OpenSslEngine problem"); e.initCause(cause); + assert engine.handshakeException == null; engine.handshakeException = e; } } 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 7c105143c1..aeee540309 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -984,6 +984,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; } @@ -1651,7 +1653,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; @@ -1660,6 +1662,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();