Recent commit breaks Mutual TLS #6796
Assignees
Labels
Comments
|
@rkapsi thanks for reporting.... I will have a look but if we can not borrow it down it may be the easiest to just revert the change. @Scottmitch WDYT? |
|
@rkapsi apologies for the hijack 😜 but this issue looks interesting for our project @normanmaurer how's it looking time wise for the next tcnative / netty release to address the SSL fixes in particular the memory leak? |
|
@johnou very soon. That said the leak will only affect people who are creating and destroying |
Well, I am :) |
|
Thanks for reporting @rkapsi ... This interface is a bit fragile and I wouldn't expect calling 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 c05496466..6a5d28309 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -769,6 +769,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLException {
ByteBuf out = null;
ByteBufAllocator alloc = ctx.alloc();
+ boolean previousNeedUnwrap = false;
try {
// Only continue to loop if the handler was not removed in the meantime.
// See https://github.com/netty/netty/issues/5860
@@ -779,8 +780,13 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
// If this is not enough we will increase the buffer in wrap(...).
out = allocateOutNetBuf(ctx, 2048, 1);
}
+
SSLEngineResult result = wrap(alloc, engine, Unpooled.EMPTY_BUFFER, out);
+ if (previousNeedUnwrap) {
+ previousNeedUnwrap = false;
+ logger.info("previous status was unwrap, new status is {}", result);
+ }
if (result.bytesProduced() > 0) {
ctx.write(out);
if (inUnwrap) {
@@ -797,14 +803,11 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
runDelegatedTasks();
break;
case NEED_UNWRAP:
- if (inUnwrap) {
- // If we asked for a wrap, the engine requested an unwrap, and we are in unwrap there is
- // no use in trying to call wrap again because we have already attempted (or will after we
- // return) to feed more data to the engine.
- return;
+ previousNeedUnwrap = true;
+ if (!inUnwrap) {
+ unwrapNonAppData(ctx);
}
- unwrapNonAppData(ctx);
break;
case NEED_WRAP:
break;
|
|
@Scottmitch I'll test JDK and the patch tomorrow. |
|
@Scottmitch here are the results: 4.1.12-SNAPSHOT + OPENSSL: doesn't work 4.1.12-HEAD w/ patch + OPENSSL: works 4.1.12-HEAD w/ patch + JDK: works |
|
I should note that this is our Netty app (acting as client) trying to connect to something like Tomcat, Jetty using mutual TLS. |
|
thanks @rkapsi ... so looks like the behavior of OpenSslEngine is not consistent with the JDK engine ... let me investigate. @normanmaurer - if this is holding up a release let me know and I can revert and work out alternative means of investigation. |
|
@Scottmitch here are the results for #6803 openssl_wrap_status w/ OPENSSL: works openssl_wrap_status w/ JDK: works |
Scottmitch
added a commit
to Scottmitch/netty
that referenced
this issue
Jun 2, 2017
Motivation: If the destination buffer is completely filled during a call to OpenSslEngine#wrap(..) we may return NEED_UNWRAP because there is no data pending in the SSL buffers. However during a handshake if the SSL buffers were just drained, and filled up the destination buffer it is possible OpenSSL may produce more data on the next call to SSL_write. This means we should keep trying to call SSL_write as long as the destination buffer is filled and only return NEED_UNWRAP when the destination buffer is not full and there is no data pending in OpenSSL's buffers. Modifications: - If the handshake produces data in OpenSslEngine#wrap(..) we should return NEED_WRAP if the destination buffer is completely filled Result: OpenSslEngine returns the correct handshake status from wrap(). Fixes netty#6796.
Scottmitch
added a commit
that referenced
this issue
Jun 2, 2017
Motivation: If the destination buffer is completely filled during a call to OpenSslEngine#wrap(..) we may return NEED_UNWRAP because there is no data pending in the SSL buffers. However during a handshake if the SSL buffers were just drained, and filled up the destination buffer it is possible OpenSSL may produce more data on the next call to SSL_write. This means we should keep trying to call SSL_write as long as the destination buffer is filled and only return NEED_UNWRAP when the destination buffer is not full and there is no data pending in OpenSSL's buffers. Modifications: - If the handshake produces data in OpenSslEngine#wrap(..) we should return NEED_WRAP if the destination buffer is completely filled Result: OpenSslEngine returns the correct handshake status from wrap(). Fixes #6796.
liuzhengyang
pushed a commit
to liuzhengyang/netty
that referenced
this issue
Sep 10, 2017
Motivation: If the destination buffer is completely filled during a call to OpenSslEngine#wrap(..) we may return NEED_UNWRAP because there is no data pending in the SSL buffers. However during a handshake if the SSL buffers were just drained, and filled up the destination buffer it is possible OpenSSL may produce more data on the next call to SSL_write. This means we should keep trying to call SSL_write as long as the destination buffer is filled and only return NEED_UNWRAP when the destination buffer is not full and there is no data pending in OpenSSL's buffers. Modifications: - If the handshake produces data in OpenSslEngine#wrap(..) we should return NEED_WRAP if the destination buffer is completely filled Result: OpenSslEngine returns the correct handshake status from wrap(). Fixes netty#6796.
kiril-me
pushed a commit
to kiril-me/netty
that referenced
this issue
Feb 28, 2018
Motivation: If the destination buffer is completely filled during a call to OpenSslEngine#wrap(..) we may return NEED_UNWRAP because there is no data pending in the SSL buffers. However during a handshake if the SSL buffers were just drained, and filled up the destination buffer it is possible OpenSSL may produce more data on the next call to SSL_write. This means we should keep trying to call SSL_write as long as the destination buffer is filled and only return NEED_UNWRAP when the destination buffer is not full and there is no data pending in OpenSSL's buffers. Modifications: - If the handshake produces data in OpenSslEngine#wrap(..) we should return NEED_WRAP if the destination buffer is completely filled Result: OpenSslEngine returns the correct handshake status from wrap(). Fixes netty#6796.
pulllock
pushed a commit
to pulllock/netty
that referenced
this issue
Oct 19, 2023
Motivation: If the destination buffer is completely filled during a call to OpenSslEngine#wrap(..) we may return NEED_UNWRAP because there is no data pending in the SSL buffers. However during a handshake if the SSL buffers were just drained, and filled up the destination buffer it is possible OpenSSL may produce more data on the next call to SSL_write. This means we should keep trying to call SSL_write as long as the destination buffer is filled and only return NEED_UNWRAP when the destination buffer is not full and there is no data pending in OpenSSL's buffers. Modifications: - If the handshake produces data in OpenSslEngine#wrap(..) we should return NEED_WRAP if the destination buffer is completely filled Result: OpenSslEngine returns the correct handshake status from wrap(). Fixes netty#6796.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Commit f20063d appears to break Mutual TLS in the sense that the connection just hangs. There're no Exceptions as far as I can tell and the only failure I'm getting is when the events in SslHandler#channelInactive(...) fire.
Interestingly, the problem occurs only in our production environment (which has more load, latencies and RTTs between servers). The problem doesn't occur on my dev workstation (i.e. unable to reproduce in the form of a unit test).
Reverting the commit fixes the problem for me.
Expected behavior
Actual behavior
Steps to reproduce
Minimal yet complete reproducer code (or URL to code)
Netty version
4.1.12.Final-SNAPSHOT w/ TCN 2.0.2.Final-SNAPSHOT (openssl-static)
JVM version (e.g.
java -version)OS version (e.g.
uname -a)The text was updated successfully, but these errors were encountered: