conn: fix connection reuse when SSL is optional

In curl 8.12 I tried to improve the logic on how we handle connections
that "upgrade" to TLS later, e.g. with a STARTTLS. I found the existing
code hard to read in this regard. But of course, the "improvements" blew
up in my face.

We fixed issues with imap, opo3, smtp in 8.12.1, but ftp was no longer
reusing existing, upgraded control connections as before. This PR adds
checks in our pytest FTP tests that verify reuse is happening as
intended.

I rewrote the logic in url.c again, so that the new test checks now pass.

Reported-by: Zenju on github
Fixes curl#16384
Closes curl#16392

lib/imap.c

@@ -473,7 +473,6 @@ static CURLcode imap_perform_upgrade_tls(struct Curl_easy *data,
       goto out;
     /* Change the connection handler */
     conn->handler = &Curl_handler_imaps;
-    conn->bits.tls_upgraded = TRUE;
   }
 
   DEBUGASSERT(!imapc->ssldone);
@@ -1747,14 +1746,8 @@ static CURLcode imap_setup_connection(struct Curl_easy *data,
                                       struct connectdata *conn)
 {
   /* Initialise the IMAP layer */
-  CURLcode result = imap_init(data);
-  if(result)
-    return result;
-
-  /* Clear the TLS upgraded flag */
-  conn->bits.tls_upgraded = FALSE;
-
-  return CURLE_OK;
+  (void)conn;
+  return imap_init(data);
 }
 
 /***********************************************************************

lib/openldap.c

@@ -530,9 +530,6 @@ static CURLcode oldap_connect(struct Curl_easy *data, bool *done)
     /* Initialize the SASL storage */
     Curl_sasl_init(&li->sasl, data, &saslldap);
 
-    /* Clear the TLS upgraded flag */
-    conn->bits.tls_upgraded = FALSE;
-
     result = oldap_parse_login_options(conn);
     if(result)
       return result;
@@ -797,7 +794,6 @@ static CURLcode oldap_connecting(struct Curl_easy *data, bool *done)
     if(result)
       result = oldap_map_error(code, CURLE_USE_SSL_FAILED);
     else if(ssl_installed(conn)) {
-      conn->bits.tls_upgraded = TRUE;
       if(li->sasl.prefmech != SASL_AUTH_NONE)
         result = oldap_perform_mechs(data);
       else if(data->state.aptr.user)

lib/pop3.c

@@ -419,7 +419,6 @@ static CURLcode pop3_perform_upgrade_tls(struct Curl_easy *data,
       goto out;
     /* Change the connection handler */
     conn->handler = &Curl_handler_pop3s;
-    conn->bits.tls_upgraded = TRUE;
   }
 
   DEBUGASSERT(!pop3c->ssldone);
@@ -1391,14 +1390,8 @@ static CURLcode pop3_setup_connection(struct Curl_easy *data,
                                       struct connectdata *conn)
 {
   /* Initialise the POP3 layer */
-  CURLcode result = pop3_init(data);
-  if(result)
-    return result;
-
-  /* Clear the TLS upgraded flag */
-  conn->bits.tls_upgraded = FALSE;
-
-  return CURLE_OK;
+  (void)conn;
+  return pop3_init(data);
 }
 
 /***********************************************************************

lib/smtp.c

@@ -401,7 +401,6 @@ static CURLcode smtp_perform_upgrade_tls(struct Curl_easy *data)
       goto out;
     /* Change the connection handler and SMTP state */
     conn->handler = &Curl_handler_smtps;
-    conn->bits.tls_upgraded = TRUE;
   }
 
   DEBUGASSERT(!smtpc->ssldone);
@@ -1614,10 +1613,8 @@ static CURLcode smtp_setup_connection(struct Curl_easy *data,
 {
   CURLcode result;
 
-  /* Clear the TLS upgraded flag */
-  conn->bits.tls_upgraded = FALSE;
-
   /* Initialise the SMTP layer */
+  (void)conn;
   result = smtp_init(data);
   CURL_TRC_SMTP(data, "smtp_setup_connection() -> %d", result);
   return result;

lib/url.c

@@ -950,13 +950,17 @@ static bool url_match_conn(struct connectdata *conn, void *userdata)
     return FALSE;
 #endif
 
-  if((!(needle->handler->flags&PROTOPT_SSL) !=
-      !Curl_conn_is_ssl(conn, FIRSTSOCKET)) &&
-     !(get_protocol_family(conn->handler) == needle->handler->protocol &&
-       conn->bits.tls_upgraded))
-    /* Deny `conn` if it is not fit for `needle`'s SSL needs,
-     * UNLESS `conn` is the same protocol family and was upgraded to SSL. */
+  if(needle->handler->flags&PROTOPT_SSL) {
+    /* We are looking for SSL, if `conn` does not do it, not a match. */
+    if(!Curl_conn_is_ssl(conn, FIRSTSOCKET))
       return FALSE;
+  }
+  else if(Curl_conn_is_ssl(conn, FIRSTSOCKET)) {
+    /* We are not *requiring* SSL, however `conn` has it. If the
+     * protocol *family* is not the same, not a match. */
+    if(get_protocol_family(conn->handler) != needle->handler->protocol)
+      return FALSE;
+  }
 
 #ifndef CURL_DISABLE_PROXY
   if(needle->bits.httpproxy != conn->bits.httpproxy ||
@@ -1084,12 +1088,21 @@ static bool url_match_conn(struct connectdata *conn, void *userdata)
      || !needle->bits.httpproxy || needle->bits.tunnel_proxy
 #endif
     ) {
-    /* Talking the same protocol scheme or a TLS upgraded protocol in the
-     * same protocol family? */
-    if(!strcasecompare(needle->handler->scheme, conn->handler->scheme) &&
-       (get_protocol_family(conn->handler) !=
-        needle->handler->protocol || !conn->bits.tls_upgraded))
-      return FALSE;
+    if(!strcasecompare(needle->handler->scheme, conn->handler->scheme)) {
+      /* `needle` and `conn` do not have the same scheme... */
+      if(get_protocol_family(conn->handler) != needle->handler->protocol) {
+        /* and `conn`s protocol family is not the protocol `needle` wants.
+         * IMAPS would work for IMAP, but no vice versa. */
+        return FALSE;
+      }
+      /* We are in an IMAPS vs IMAP like case. We expect `conn` to have SSL */
+      if(!Curl_conn_is_ssl(conn, FIRSTSOCKET)) {
+        DEBUGF(infof(data,
+          "Connection #%" FMT_OFF_T " has compatible protocol famiy, "
+          "but no SSL, no match", conn->connection_id));
+        return FALSE;
+      }
+    }
 
     /* If needle has "conn_to_*" set, conn must match this */
     if((needle->bits.conn_to_host && !strcasecompare(
@@ -3590,18 +3603,25 @@ static CURLcode create_conn(struct Curl_easy *data,
      * `existing` and thus we need to cleanup the one we just
      * allocated before we can move along and use `existing`.
      */
+    bool tls_upgraded = (!(conn->given->flags & PROTOPT_SSL) &&
+                         Curl_conn_is_ssl(conn, FIRSTSOCKET));
+
     reuse_conn(data, conn, existing);
     conn = existing;
     *in_connect = conn;
 
 #ifndef CURL_DISABLE_PROXY
-    infof(data, "Re-using existing connection with %s %s",
+    infof(data, "Re-using existing %s: connection%s with %s %s",
+          conn->given->scheme,
+          tls_upgraded ? " (upgraded to SSL)" : "",
           conn->bits.proxy ? "proxy" : "host",
           conn->socks_proxy.host.name ? conn->socks_proxy.host.dispname :
           conn->http_proxy.host.name ? conn->http_proxy.host.dispname :
           conn->host.dispname);
 #else
-    infof(data, "Re-using existing connection with host %s",
+    infof(data, "Re-using existing %s: connection%s with host %s",
+          conn->given->scheme,
+          tls_upgraded ? " (upgraded to SSL)" : "",
           conn->host.dispname);
 #endif
   }

lib/urldata.h

@@ -513,7 +513,6 @@ struct ConnectBits {
 #ifdef USE_UNIX_SOCKETS
   BIT(abstract_unix_socket);
 #endif
-  BIT(tls_upgraded);
   BIT(sock_accepted); /* TRUE if the SECONDARYSOCKET was created with
                          accept() */
   BIT(parallel_connect); /* set TRUE when a parallel connect attempt has

tests/http/Makefile.am

@@ -65,6 +65,7 @@ test_19_shutdown.py    \
 test_20_websockets.py  \
 test_30_vsftpd.py      \
 test_31_vsftpds.py     \
+test_32_ftps_vsftpd.py \
 $(TESTENV)
 
 clean-local:

tests/http/test_30_vsftpd.py

@@ -103,6 +103,7 @@ def test_30_03_download_10_serial(self, env: Env, vsftpd: VsFTPD, docname):
         r = curl.ftp_get(urls=[url], with_stats=True)
         r.check_stats(count=count, http_status=226)
         self.check_downloads(curl, srcfile, count)
+        assert r.total_connects == count + 1, 'should reuse the control conn'
 
     @pytest.mark.parametrize("docname", [
         'data-1k', 'data-1m', 'data-10m'
@@ -117,6 +118,7 @@ def test_30_04_download_10_parallel(self, env: Env, vsftpd: VsFTPD, docname):
         ])
         r.check_stats(count=count, http_status=226)
         self.check_downloads(curl, srcfile, count)
+        assert r.total_connects > count + 1, 'should have used several control conns'
 
     @pytest.mark.parametrize("docname", [
         'upload-1k', 'upload-100k', 'upload-1m'

tests/http/test_31_vsftpds.py

@@ -110,6 +110,7 @@ def test_31_03_download_10_serial(self, env: Env, vsftpds: VsFTPD, docname):
         r = curl.ftp_ssl_get(urls=[url], with_stats=True)
         r.check_stats(count=count, http_status=226)
         self.check_downloads(curl, srcfile, count)
+        assert r.total_connects == count + 1, 'should reuse the control conn'
 
     @pytest.mark.parametrize("docname", [
         'data-1k', 'data-1m', 'data-10m'
@@ -124,6 +125,7 @@ def test_31_04_download_10_parallel(self, env: Env, vsftpds: VsFTPD, docname):
         ])
         r.check_stats(count=count, http_status=226)
         self.check_downloads(curl, srcfile, count)
+        assert r.total_connects > count + 1, 'should have used several control conns'
 
     @pytest.mark.parametrize("docname", [
         'upload-1k', 'upload-100k', 'upload-1m'