Repository: netty/netty
Issue: OpenSslEngine should not call SSL_set_state

OpenSslEngine currently calls `SSL_set_state(SSL_ST_ACCEPT)` during the renegotiation process. The OpenSSL team has advised against this [1]. We should investigate and avoid calling `SSL_ST_ACCEPT` if possible.

[1] https://github.com/openssl/openssl/issues/2551#issuecomment-277644153


Issue comments:
OpenSSL 1.1.0e cannot be used in netty-tcnative, because SSL_set_state no longer exists in OpenSSL since 1.1.0 (https://github.com/netty/netty-tcnative/issues/263).
Given that OpenSSL 1.1.1 (hopefully) supports TLS 1.3, it seems worth to remove usage of SSL_set_state.
I don't know if this helps, but this is how apache tcnative does the renegotiation (in an older version): https://github.com/apache/tomcat-native/blob/043f95087f712595598901b7c5f915ca5fbf86d5/native/src/sslnetwork.c#L650
In the trunk they removed the `SSL_set_state` call completely: https://github.com/apache/tomcat-native/blob/trunk/native/src/sslnetwork.c#L632
@Scottmitch I wonder if we should just remove the support for `renegotiation` as its risky business anyway. This would also allow us to just drop `set_state` usage and so support openssl 1.1.x.

WDYT ?

@carl-mastrangelo @ejona86 @nmittler WDYT as well?
+1
I honestly don't know enough to determine whether renegotiation is necessary or alternative approaches.

Pull request review comments:

Patch that resolved the issue:
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 360d9fd9ab2..d235653cfd4 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -163,7 +163,6 @@ private enum HandshakeState {
     }
 
     private HandshakeState handshakeState = HandshakeState.NOT_STARTED;
-    private boolean renegotiationPending;
     private boolean receivedShutdown;
     private volatile int destroyed;
     private volatile String applicationProtocol;
@@ -654,17 +653,6 @@ public final SSLEngineResult wrap(
 
                     status = handshake();
 
-                    if (renegotiationPending && status == FINISHED) {
-                        // If renegotiationPending is true that means when we attempted to start renegotiation
-                        // the BIO buffer didn't have enough space to hold the HelloRequest which prompts the
-                        // client to initiate a renegotiation. At this point the HelloRequest has been written
-                        // so we can actually start the handshake process.
-                        renegotiationPending = false;
-                        SSL.setState(ssl, SSL.SSL_ST_ACCEPT);
-                        handshakeState = HandshakeState.STARTED_EXPLICITLY;
-                        status = handshake();
-                    }
-
                     // Handshake may have generated more data, for example if the internal SSL buffer is small
                     // we may have freed up space by flushing above.
                     bytesProduced = bioLengthBefore - SSL.bioLengthByteBuffer(networkBIO);
@@ -1510,43 +1498,7 @@ public final synchronized void beginHandshake() throws SSLException {
                 // Nothing to do as the handshake is not done yet.
                 break;
             case FINISHED:
-                if (clientMode) {
-                    // Only supported for server mode at the moment.
-                    throw RENEGOTIATION_UNSUPPORTED;
-                }
-                // For renegotiate on the server side we need to issue the following command sequence with openssl:
-                //
-                // SSL_renegotiate(ssl)
-                // SSL_do_handshake(ssl)
-                // ssl->state = SSL_ST_ACCEPT
-                // SSL_do_handshake(ssl)
-                //
-                // Because of this we fall-through to call handshake() after setting the state, as this will also take
-                // care of updating the internal OpenSslSession object.
-                //
-                // See also:
-                // https://github.com/apache/httpd/blob/2.4.16/modules/ssl/ssl_engine_kernel.c#L812
-                // http://h71000.www7.hp.com/doc/83final/ba554_90007/ch04s03.html
-                int status;
-                if ((status = SSL.renegotiate(ssl)) != 1 || (status = SSL.doHandshake(ssl)) != 1) {
-                    int err = SSL.getError(ssl, status);
-                    if (err == SSL.SSL_ERROR_WANT_READ || err == SSL.SSL_ERROR_WANT_WRITE) {
-                        // If the internal SSL buffer is small it is possible that doHandshake may "fail" because
-                        // there is not enough room to write, so we should wait until the renegotiation has been.
-                        renegotiationPending = true;
-                        handshakeState = HandshakeState.STARTED_EXPLICITLY;
-                        lastAccessed = System.currentTimeMillis();
-                        return;
-                    } else {
-                        throw shutdownWithError("renegotiation failed");
-                    }
-                }
-
-                SSL.setState(ssl, SSL.SSL_ST_ACCEPT);
-
-                lastAccessed = System.currentTimeMillis();
-
-                // fall-through
+                throw RENEGOTIATION_UNSUPPORTED;
             case NOT_STARTED:
                 handshakeState = HandshakeState.STARTED_EXPLICITLY;
                 handshake();


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateSmallBIOTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateSmallBIOTest.java
deleted file mode 100644
index 3959e646998..00000000000
--- a/handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateSmallBIOTest.java
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Copyright 2017 The Netty Project
- *
- * The Netty Project licenses this file to you under the Apache License,
- * version 2.0 (the "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at:
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations
- * under the License.
- */
-package io.netty.handler.ssl;
-
-public class OpenSslRenegotiateSmallBIOTest extends OpenSslRenegotiateTest {
-    @Override
-    protected void initSslServerContext(SslContext context) {
-        ((ReferenceCountedOpenSslContext) context).setBioNonApplicationBufferSize(1);
-    }
-}
diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateTest.java
deleted file mode 100644
index 8f3dfeea7f2..00000000000
--- a/handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateTest.java
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * Copyright 2015 The Netty Project
- *
- * The Netty Project licenses this file to you under the Apache License,
- * version 2.0 (the "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at:
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations
- * under the License.
- */
-package io.netty.handler.ssl;
-
-import org.junit.BeforeClass;
-
-import static org.junit.Assume.assumeFalse;
-import static org.junit.Assume.assumeTrue;
-
-public class OpenSslRenegotiateTest extends RenegotiateTest {
-
-    @BeforeClass
-    public static void checkOpenSsl() {
-        assumeTrue(OpenSsl.isAvailable());
-        // BoringSSL does not support renegotiation intentionally.
-        assumeFalse("BoringSSL".equals(OpenSsl.versionString()));
-    }
-
-    @Override
-    protected SslProvider serverSslProvider() {
-        return SslProvider.OPENSSL;
-    }
-}