MB-42607 [2/2]: Handle SSL_accept returning SSL_ERROR_WANT_WRITE

During TLS handshake with the Data Service, if the node certificate
requires more than 8192 bytes to transmit then the handshake can fail
with the following error:

    WARNING 634: ERROR: SSL_accept returned -1 with error 3
    INFO 634 Closing connection [ 1.2.3.4:55555 - 5.6.7.8:11207 (not authenticated) ] due to read error: Connection reset by peer

This is caused by KV-Engine SSL handshake code failig to handle one of
the possible temporary status codes from SSL_accept(), namely
SSL_ERROR_WANT_WRITE which occurs when OpenSSL has consumed the BIO
send buffer but still has more data it wishes to write. Given the BIO
buffer size is 8192 bytes, if sending the node certificate requires
more than 8192B then SSL_ERROR_WANT_WRITE is returned by OpenSSL.

Node certificates which are in excess of 8kB - for example those which
contain a large number of Subject Alternative Names (SANs) - can
encounter this problem.

Fix by checking for SSL_ERROR_WANT_WRITE being returned from
SSL_accept(), and flushing the read/write BIOs and retrying.

Change-Id: Ic248c47a1bb22f6de64263a3edeb85818d2fc35f
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/139826
Tested-by: Build Bot <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>
Reviewed-by: James Harrison <[email protected]>
Well-Formed: Build Bot <[email protected]>

Author: daverigby

daemon/connection.cc

@@ -791,8 +791,48 @@ void Connection::logSslErrorInfo(const std::string& method, int rval) {
     }
 }
 
+int Connection::sslAcceptWithRetry() {
+    while (true) {
+        int r = ssl.accept();
+        if (r == 1) {
+            // handshake completed.
+            return r;
+        }
+
+        auto sslError = ssl.getError(r);
+        if (sslError == SSL_ERROR_WANT_READ ||
+            sslError == SSL_ERROR_WANT_WRITE) {
+            // Drain send and receive pipes.
+            // Note: This is somewhat of a naive implementation - ideally we
+            // would only drain the specific pipe direction based on the status
+            // code, repeating any drains until there is no more data ready
+            // to transfer. However that requires a more expressive interface on
+            // drainBio{Send,Recv}Pipe. Given SSL_accept() only occurs once
+            // per connect at the start, having a simpler (but technically
+            // sub-optimal) handing of errors here seems reasonable.
+            ssl.drainBioSendPipe(socketDescriptor);
+            if (ssl.hasError()) {
+                cb::net::set_econnreset();
+                return -1;
+            }
+            ssl.drainBioRecvPipe(socketDescriptor);
+            if (ssl.hasError()) {
+                cb::net::set_econnreset();
+                return -1;
+            }
+            // Continue SSL accept handshake.
+            continue;
+        } else {
+            logSslErrorInfo("SSL_accept", r);
+            cb::net::set_econnreset();
+            return -1;
+        }
+    }
+    folly::assume_unreachable();
+}
+
 int Connection::sslPreConnection() {
-    int r = ssl.accept();
+    int r = sslAcceptWithRetry();
     if (r == 1) {
         ssl.drainBioSendPipe(socketDescriptor);
         ssl.setConnected();
@@ -841,22 +881,10 @@ int Connection::sslPreConnection() {
             return -1;
         }
 
-         LOG_INFO("{}: Using SSL cipher:{}",
-                 getId(),
-                 ssl.getCurrentCipherName());
-    } else {
-        if (ssl.getError(r) == SSL_ERROR_WANT_READ) {
-            ssl.drainBioSendPipe(socketDescriptor);
-            cb::net::set_ewouldblock();
-            return -1;
-        } else {
-            logSslErrorInfo("SSL_accept", r);
-            cb::net::set_econnreset();
-            return -1;
-        }
+        LOG_INFO(
+                "{}: Using SSL cipher:{}", getId(), ssl.getCurrentCipherName());
     }
-
-    return 0;
+    return r;
 }
 
 int Connection::recv(char* dest, size_t nbytes) {

daemon/connection.h

@@ -1049,6 +1049,13 @@ class Connection : public dcp_message_producers {
      */
     int sslPreConnection();
 
+    /**
+     * Helper method for sslPreConnection, performs SSL accept, retrying
+     * any temporary errors.
+     * @return
+     */
+    int sslAcceptWithRetry();
+
     // Shared DCP_DELETION write function for the v1/v2 commands.
     ENGINE_ERROR_CODE deletionInner(const item_info& info,
                                     cb::const_byte_buffer packet,

tests/testapp/testapp_tls.cc

@@ -26,6 +26,10 @@ class TlsTests : public TestappClientTest {
 protected:
     void SetUp() override {
         TestappTest::SetUp();
+        // MB-42607: Reduce BIO buffer size from default 8192 to 64 bytes, to
+        // exercise SSL_ERROR_WANT_READ / SSL_ERROR_WANT_WRITE code paths.
+        memcached_cfg["bio_drain_buffer_sz"] = 64;
+
         memcached_cfg["ssl_cipher_list"]["tls 1.2"] = "HIGH";
         memcached_cfg["ssl_cipher_list"]["tls 1.3"] =
                 "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_"