cdb2api: fixing client invalid response type error

bbinc/ssl_io.h

@@ -25,6 +25,11 @@
 int SBUF2_FUNC(sslio_close)(SBUF2 *, int reuse);
 #define sslio_close SBUF2_FUNC(sslio_close)
 
+/* Does not shut down, simply frees. Use this for fatal SSL errors
+   such as SSL_ERROR_SYSCALL or SSL_ERROR_SSL */
+void SBUF2_FUNC(sslio_free)(SBUF2 *);
+#define sslio_free SBUF2_FUNC(sslio_free)
+
 int SBUF2_FUNC(sslio_read)(SBUF2 *, char *cc, int len);
 #define sslio_read SBUF2_FUNC(sslio_read)
 int SBUF2_FUNC(sslio_write)(SBUF2 *, const char *cc, int len);

bbinc/ssl_support.h

@@ -119,14 +119,13 @@ do {                                            \
             cb(fmt, ##__VA_ARGS__);                             \
     } while (0)
 
-#define ssl_sfliberrprint(err, n, cb, msg)                      \
-    do {                                                        \
-        if (err != NULL)                                        \
-            snprintf(err, n,                                    \
-                     "SSL Error: %s: (%lu) %s",                 \
-                     msg, ERR_get_error(), SSL_ERRSTR());       \
-        else                                                    \
-            PRINT_SSL_ERRSTR_MT(cb, msg);                       \
+#define ssl_sfliberrprint(err, n, cb, msg)                                                                             \
+    do {                                                                                                               \
+        unsigned long __err = ERR_get_error();                                                                         \
+        if (err != NULL)                                                                                               \
+            snprintf(err, n, "SSL Error: %s: (%lu) %s", msg, __err, ERR_reason_error_string(__err));                   \
+        else                                                                                                           \
+            PRINT_SSL_ERRSTR_MT(cb, msg);                                                                              \
     } while (0)
 
 /* XXX Don't change the order of the enum types */

cdb2api/cdb2api.c

@@ -2699,15 +2699,16 @@ static int try_ssl(cdb2_hndl_tp *hndl, SBUF2 *sb)
     /* An application may use different certificates.
        So we allocate an SSL context for each handle. */
     SSL_CTX *ctx;
-    int rc, dossl = 0;
+    int rc = 0, dossl = 0;
     cdb2_ssl_sess *p;
 
     if (SSL_IS_REQUIRED(hndl->c_sslmode)) {
         switch (hndl->s_sslmode) {
         case PEER_SSL_UNSUPPORTED:
             sprintf(hndl->errstr, "The database does not support SSL.");
             hndl->sslerr = 1;
-            return -1;
+            rc = -1;
+            break;
         case PEER_SSL_ALLOW:
         case PEER_SSL_REQUIRE:
             dossl = 1;
@@ -2716,7 +2717,8 @@ static int try_ssl(cdb2_hndl_tp *hndl, SBUF2 *sb)
             sprintf(hndl->errstr,
                     "Unrecognized peer SSL mode: %d", hndl->s_sslmode);
             hndl->sslerr = 1;
-            return -1;
+            rc = -1;
+            break;
         }
     } else if (SSL_IS_PREFERRED(hndl->c_sslmode)) {
         dossl = 1;
@@ -2733,32 +2735,38 @@ static int try_ssl(cdb2_hndl_tp *hndl, SBUF2 *sb)
             sprintf(hndl->errstr,
                     "Unrecognized peer SSL mode: %d", hndl->s_sslmode);
             hndl->sslerr = 1;
-            return -1;
+            rc = -1;
+            break;
         }
     }
 
+    if (rc != 0)
+        goto out;
+
     hndl->sslerr = 0;
 
     /* fast return if SSL is not needed. */
     if (!dossl)
-        return 0;
+        goto out;
 
     if ((rc = cdb2_init_ssl(1, 1)) != 0) {
         hndl->sslerr = 1;
-        return rc;
+        goto out;
     }
 
     /* If negotiation fails, let API retry. */
     struct newsqlheader hdr = {.type = ntohl(CDB2_REQUEST_TYPE__SSLCONN)};
     rc = sbuf2fwrite((char *)&hdr, sizeof(hdr), 1, sb);
-    if (rc != 1)
-        return -1;
+    if (rc != 1) {
+        rc = -1;
+        goto out;
+    }
     if ((rc = sbuf2flush(sb)) < 0 || (rc = sbuf2getc(sb)) < 0) {
         /* If SSL is optional (this includes ALLOW and PREFER), change
            my mode to ALLOW so that we can reconnect in plaintext. */
         if (SSL_IS_OPTIONAL(hndl->c_sslmode))
             hndl->c_sslmode = SSL_ALLOW;
-        return rc;
+        goto out;
     }
 
     /* The node does not agree with dbinfo. This usually happens
@@ -2768,14 +2776,16 @@ static int try_ssl(cdb2_hndl_tp *hndl, SBUF2 *sb)
             hndl->c_sslmode = SSL_ALLOW;
             /* if server sends back 'N', reuse this plaintext connection;
                force reconnecting for an unexpected byte */
-            return (rc == 'N') ? 0 : -1;
+            rc = (rc == 'N') ? 0 : -1;
+            goto out;
         }
 
         /* We reach here only if the server is mistakenly downgraded
            before the client. */
         sprintf(hndl->errstr, "The database does not support SSL.");
         hndl->sslerr = 1;
-        return -1;
+        rc = -1;
+        goto out;
     }
 
     rc = ssl_new_ctx(&ctx, hndl->c_sslmode, hndl->sslpath, &hndl->cert,
@@ -2790,15 +2800,25 @@ static int try_ssl(cdb2_hndl_tp *hndl, SBUF2 *sb)
     rc = sslio_connect(sb, ctx, hndl->c_sslmode, hndl->dbname, hndl->nid_dbname, ((p != NULL) ? p->sessobj : NULL));
 
     SSL_CTX_free(ctx);
+
     if (rc != 1) {
         hndl->sslerr = sbuf2lasterror(sb, hndl->errstr, sizeof(hndl->errstr));
         /* If SSL_connect() fails, invalidate the session. */
         if (p != NULL)
             p->sessobj = NULL;
-        return -1;
+        rc = -1;
+        goto out;
     }
     hndl->newsess = 1;
-    return 0;
+    rc = 0;
+out:
+    if (rc != 0) {
+        /* Don't leave an incomplete SSL connection here.
+         * Shut it down and reset sbuf. Caller will reconnect */
+        sbuf2close(hndl->sb);
+        hndl->sb = NULL;
+    }
+    return rc;
 }
 
 static int cdb2portmux_route(cdb2_hndl_tp *hndl, const char *remote_host,
@@ -2832,8 +2852,8 @@ static int cdb2portmux_route(cdb2_hndl_tp *hndl, const char *remote_host,
     sbuf2printf(ss, "rte %s\n", name);
     sbuf2flush(ss);
     res[0] = '\0';
-    sbuf2gets(res, sizeof(res), ss);
-    debugprint("rte '%s' returns res=%s", name, res);
+    int t_rc = sbuf2gets(res, sizeof(res), ss);
+    debugprint("rte '%s' returns rc %d res=%s errno=%s\n", name, t_rc, res, strerror(errno));
     if (res[0] != '0') { // character '0' is indication of success
         sbuf2close(ss);
         return -1;
@@ -2874,7 +2894,6 @@ static int newsql_connect_via_fd(cdb2_hndl_tp *hndl)
     sbuf2settimeout(sb, hndl->socket_timeout, hndl->socket_timeout);
 
     if (try_ssl(hndl, sb) != 0) {
-        sbuf2close(sb);
         rc = -1;
         goto after_callback;
     }
@@ -2976,7 +2995,6 @@ static int newsql_connect(cdb2_hndl_tp *hndl, int node_indx)
     sbuf2settimeout(sb, hndl->socket_timeout, hndl->socket_timeout);
 
     if (try_ssl(hndl, sb) != 0) {
-        sbuf2close(sb);
         rc = -1;
         goto after_callback;
     }
@@ -5996,6 +6014,11 @@ void cdb2_set_debug_trace(cdb2_hndl_tp *hndl)
     hndl->debug_trace = 1;
 }
 
+void cdb2_unset_debug_trace(cdb2_hndl_tp *hndl)
+{
+    hndl->debug_trace = 0;
+}
+
 void cdb2_dump_ports(cdb2_hndl_tp *hndl, FILE *out)
 {
     int i;

cdb2api/cdb2api.h

@@ -234,6 +234,7 @@ void *cdb2_column_value(cdb2_hndl_tp *hndl, int col);
 const char *cdb2_errstr(cdb2_hndl_tp *hndl);
 const char *cdb2_cnonce(cdb2_hndl_tp *hndl);
 void cdb2_set_debug_trace(cdb2_hndl_tp *hndl);
+void cdb2_unset_debug_trace(cdb2_hndl_tp *hndl);
 void cdb2_dump_ports(cdb2_hndl_tp *hndl, FILE *out);
 void cdb2_cluster_info(cdb2_hndl_tp *hndl, char **cluster, int *ports, int max, int *count);
 int cdb2_snapshot_file(cdb2_hndl_tp *hndl, int *file, int *offset);

db/sqlinterfaces.c

@@ -6700,8 +6700,8 @@ int check_appsock_limit(int pending, int is_admin)
         time_t now = time(NULL);
         if (now != last) {
             logmsg(LOGMSG_USER,
-                   "Exhausted appsock connections, total %d connections denied-connection count=%" PRId64 "\n",
-                   current, gbl_denied_appsock_connection_count);
+                   "Exhausted appsock connections, total %d connections denied-connection count=%" PRId64 "\n", current,
+                   gbl_denied_appsock_connection_count);
             last = now;
         }
         return -1;

db/ssl_bend.c

@@ -69,6 +69,7 @@ int gbl_nid_dbname = NID_commonName;
 double gbl_min_tls_ver = 0;
 /* (test-only) are connections from localhost always allowed? */
 int gbl_ssl_allow_localhost = 0;
+int gbl_ssl_print_io_errors = 0;
 
 /* number of full ssl handshakes */
 uint64_t gbl_ssl_num_full_handshakes = 0;
@@ -297,6 +298,9 @@ int ssl_process_lrl(char *line, size_t len)
         logmsg(LOGMSG_WARN, "Always allow connections from localhost. "
                             "This option is for testing only and should not be enabled on production.");
         gbl_ssl_allow_localhost = 1;
+    } else if (tokcmp(line, ltok, "ssl_print_io_errors") == 0) {
+        tok = segtok(line, len, &st, &ltok);
+        gbl_ssl_print_io_errors = (ltok <= 0) ? 1 : !!toknum(tok, ltok);
     }
     return 0;
 }

net/ssl_evbuffer.c

@@ -36,6 +36,8 @@ extern ssl_mode gbl_client_ssl_mode;
 extern uint64_t gbl_ssl_num_full_handshakes;
 extern uint64_t gbl_ssl_num_partial_handshakes;
 
+extern int gbl_ssl_print_io_errors;
+
 struct ssl_data {
     struct event *ev;
     int fd;
@@ -72,7 +74,7 @@ static void ssl_handshake_evbuffer(int fd, short what, void *data)
             ATOMIC_ADD64(gbl_ssl_num_full_handshakes, 1);
         }
         ssl_data->cert = SSL_get_peer_certificate(ssl);
-        ssl_data->do_shutdown = 1;
+        ssl_data->do_shutdown = 0;
         arg->success_cb(arg->data); /* newsql_accept_ssl_success, net_accept_ssl_success, net_connect_ssl_success */
         free(arg);
         return;
@@ -88,12 +90,17 @@ static void ssl_handshake_evbuffer(int fd, short what, void *data)
         event_add(ssl_data->ev, NULL);
         return;
     case SSL_ERROR_SYSCALL:
-        logmsg(LOGMSG_ERROR, "%s:%d SSL_do_handshake fd:%d rc:%d err:%d errno:%d [%s]\n",
-               __func__, __LINE__, fd, rc, err, errno, strerror(errno));
+        if (gbl_ssl_print_io_errors) {
+            if (errno == 0) /* openssl 1.x bug: an errno 0 reported under SSL_ERROR_SYSCALL means EOF from peer */
+                errno = ECONNRESET;
+            logmsg(LOGMSG_ERROR, "%s:%d SSL_do_handshake fd:%d rc:%d err:%d errno:%d [%s]\n", __func__, __LINE__, fd,
+                   rc, err, errno, strerror(errno));
+        }
         break;
     default:
-        logmsg(LOGMSG_ERROR, "%s:%d SSL_do_handshake fd:%d rc:%d err:%d error_string:%s]\n",
-               __func__, __LINE__, fd, rc, err, ERR_error_string(err, NULL));
+        if (gbl_ssl_print_io_errors)
+            logmsg(LOGMSG_ERROR, "%s:%d SSL_do_handshake fd:%d rc:%d err:%d error_string:%s]\n", __func__, __LINE__, fd,
+                   rc, err, ERR_error_string(ERR_get_error(), NULL));
         break;
     }
     arg->error_cb(arg->data); /* ssl_error_cb, accept_ssl_error_cb */
@@ -157,21 +164,28 @@ int rd_ssl_evbuffer(struct evbuffer *rd_buf, struct ssl_data *ssl_data, int *eof
     }
     int err = SSL_get_error(ssl, rc);
     switch (err) {
-    case SSL_ERROR_ZERO_RETURN: *eof = 1; /* fallthrough */
+    case SSL_ERROR_ZERO_RETURN:
+        *eof = 1;
+        /* Do not perform shutdown unless client has done a clean shutdown first */
+        ssl_data->do_shutdown = 1;
+        /* fallthrough */
     case SSL_ERROR_WANT_READ: return 1;
     case SSL_ERROR_WANT_WRITE:
         logmsg(LOGMSG_ERROR, "%s:%d SSL_read rc:%d err:%d SSL_ERROR_WANT_WRITE]\n",
                __func__, __LINE__, rc, err);
         break;
     case SSL_ERROR_SYSCALL:
-        if (errno && errno != ECONNRESET) {
+        if (gbl_ssl_print_io_errors) {
+            if (errno == 0)
+                errno = ECONNRESET;
             logmsg(LOGMSG_ERROR, "%s:%d SSL_read rc:%d err:%d errno:%d [%s]\n",
                    __func__, __LINE__, rc, err, errno, strerror(errno));
         }
         break;
     default:
-        logmsg(LOGMSG_ERROR, "%s:%d SSL_read rc:%d err:%d [%s]\n",
-               __func__, __LINE__, rc, err, ERR_error_string(err, NULL));
+        if (gbl_ssl_print_io_errors)
+            logmsg(LOGMSG_ERROR, "%s:%d SSL_read rc:%d err:%d [%s]\n", __func__, __LINE__, rc, err,
+                   ERR_error_string(ERR_get_error(), NULL));
         break;
     }
     return rc;
@@ -199,14 +213,17 @@ int wr_ssl_evbuffer(struct ssl_data *ssl_data, struct evbuffer *wr_buf)
                __func__, __LINE__, rc, err);
         break;
     case SSL_ERROR_SYSCALL:
-        if (errno && errno != ECONNRESET) {
+        if (gbl_ssl_print_io_errors) {
+            if (errno == 0)
+                errno = ECONNRESET;
             logmsg(LOGMSG_ERROR, "%s:%d SSL_write rc:%d err:%d errno:%d [%s]\n",
                    __func__, __LINE__, rc, err, errno, strerror(errno));
         }
         break;
     default:
-        logmsg(LOGMSG_ERROR, "%s:%d SSL_write rc:%d err:%d [%s]\n",
-               __func__, __LINE__, rc, err, ERR_error_string(err, NULL));
+        if (gbl_ssl_print_io_errors)
+            logmsg(LOGMSG_ERROR, "%s:%d SSL_write rc:%d err:%d [%s]\n", __func__, __LINE__, rc, err,
+                   ERR_error_string(ERR_get_error(), NULL));
         break;
     }
     return rc;
@@ -243,7 +260,11 @@ void ssl_data_free(struct ssl_data *ssl_data)
 {
     if (!ssl_data) return;
     if (ssl_data->ev) event_free(ssl_data->ev);
-    if (ssl_data->do_shutdown) SSL_shutdown(ssl_data->ssl);
+    if (!ssl_data->do_shutdown) /* fail fast and make session reusable */
+        SSL_set_shutdown(ssl_data->ssl, SSL_SENT_SHUTDOWN);
+    else if (SSL_shutdown(ssl_data->ssl) == 0)
+        SSL_shutdown(ssl_data->ssl);
+
     SSL_free(ssl_data->ssl);
     X509_free(ssl_data->cert);
     free(ssl_data);

plugins/newsql/newsql_evbuffer.c

@@ -879,6 +879,7 @@ static void process_ssl_request(struct newsql_appdata_evbuffer *appdata)
         evtimer_once(appdata->base, rd_hdr, appdata);
         return;
     }
+
     appdata->ssl_data = ssl_data_new(appdata->fd, clnt->origin);
     accept_ssl_evbuffer(appdata->ssl_data, appdata->base, newsql_accept_ssl_error, newsql_accept_ssl_success, appdata);
     return;

tests/setup

@@ -105,6 +105,7 @@ setup_db() {
 
     if [[ -z "${SKIPSSL}" && $DBNAME != *"nossl"* ]] ; then
         echo -e "ssl_client_mode REQUIRE\nssl_cert_path $TESTDIR" >> ${LRL}
+        echo "ssl_print_io_errors 1" >> ${LRL}
     fi
 
     if [[ -z "${SKIPDEBUG}" ]] ; then
@@ -144,6 +145,16 @@ setup_db() {
     echo "comdb2_config:ssl_cert_path=$TESTDIR" >>$CDB2_CONFIG
     echo "comdb2_config:allow_pmux_route:true" >> $CDB2_CONFIG
 
+    # Be a little more generous on connect timeout if running a stress test
+    if [ "${STRESS_TEST}" = "1" ]; then
+        if [ "${SKIPSSL}" = "1" ]; then
+            echo "comdb2_config:connect_timeout:1000" >> $CDB2_CONFIG
+        else
+            # SSL handshakes are expensive!
+            echo "comdb2_config:connect_timeout:10000" >> $CDB2_CONFIG
+        fi
+    fi
+
     set +e
 
     # If PMUXPORT is defined we will overwrite the default pmux port with that

tests/tools/CMakeLists.txt

@@ -83,6 +83,7 @@ add_exe(test_compare_semver test_compare_semver.c)
 add_exe(test_str_util test_str_util.c)
 add_exe(test_consistent_hash test_consistent_hash.c)
 add_exe(test_consistent_hash_bench test_consistent_hash_bench.c)
+add_exe(undrained_connection_test undrained_connection_test.c)
 add_exe(updater updater.c testutil.c)
 add_exe(utf8 utf8.c)
 add_exe(verify_atomics_work verify_atomics_work.c)