BUG/MINOR: quic-be: handshake errors without connection stream closure

This bug was revealed on backend side by reg-tests/ssl/del_ssl_crt-list.vtc when
run wich QUIC connections. As expected by the test, a TLS alert is generated on
servsr side. This latter sands a CONNECTION_CLOSE frame with a CRYPTO error
(>= 0x100). In this case the client closes its QUIC connection. But
the stream connection was not informed. This leads the connection to
be closed after the server timeout expiration. It shouls be closed asap.
This is the reason why reg-tests/ssl/del_ssl_crt-list.vtc could succeeds
or failed, but only after a 5 seconds delay.

To fix this, mimic the ssl_sock_io_cb() for TCP/SSL connections. Call
the same code this patch implements with ssl_sock_handle_hs_error()
to correctly handle the handshake errors. Note that some SSL counters
were not incremented for both the backends and frontends. After such
errors, ssl_sock_io_cb() start the mux after the connection has been
flagged in error. This has as side effect to close the stream
in conn_create_mux().

Must be backported to 3.3 only for backends. This is not sure at this time
if this bug may impact the frontends.

diff --git a/include/haproxy/ssl_sock.h b/include/haproxy/ssl_sock.h
index f974110fda60df0d2b07abbc23681d04522d108a..53375c917281e4322b0cd99e0d9abe322e2ccd87 100644 (file)
--- a/include/haproxy/ssl_sock.h
+++ b/include/haproxy/ssl_sock.h
@@ -30,6 +30,7 @@
 #include <haproxy/proxy-t.h>
 #include <haproxy/quic_conn-t.h>
 #include <haproxy/ssl_sock-t.h>
+#include <haproxy/stats.h>
 #include <haproxy/thread.h>
 
 extern struct list tlskeys_reference;
@@ -89,6 +90,7 @@ unsigned int ssl_sock_get_verify_result(struct connection *conn);
 void ssl_sock_update_counters(SSL *ssl,
                               struct ssl_counters *counters,
                               struct ssl_counters *counters_px, int backend);
+void ssl_sock_handle_hs_error(struct connection *conn);
 #if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0)
 int ssl_sock_update_tlskey_ref(struct tls_keys_ref *ref,
                                struct buffer *tlskey);
@@ -241,6 +243,30 @@ static inline struct connection *ssl_sock_get_conn(const SSL *s, struct ssl_sock
        return ret;
 }
 
+/* Set at <counters> and <counters_px> addresses the SSL statistical counters */
+static inline void ssl_sock_get_stats_counters(struct connection *conn,
+                                               struct ssl_counters **counters,
+                                               struct ssl_counters **counters_px)
+{
+       switch (obj_type(conn->target)) {
+       case OBJ_TYPE_LISTENER: {
+               struct listener *li = __objt_listener(conn->target);
+               *counters = EXTRA_COUNTERS_GET(li->extra_counters, &ssl_stats_module);
+               *counters_px = EXTRA_COUNTERS_GET(li->bind_conf->frontend->extra_counters_fe,
+                                                &ssl_stats_module);
+               break;
+       }
+       case OBJ_TYPE_SERVER: {
+               struct server *srv = __objt_server(conn->target);
+               *counters = EXTRA_COUNTERS_GET(srv->extra_counters, &ssl_stats_module);
+               *counters_px = EXTRA_COUNTERS_GET(srv->proxy->extra_counters_be,
+                                                &ssl_stats_module);
+               break;
+       }
+       default:
+               break;
+       }
+}
 
 #endif /* USE_OPENSSL */
 #endif /* _HAPROXY_SSL_SOCK_H */

diff --git a/src/quic_rx.c b/src/quic_rx.c
index c01cc9d338aa3c3bc6f7850d3000eaf2b1d03d0b..797844476d419cec856567fc3a2cdce9ca27501f 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -1099,6 +1099,16 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                        /* TODO */
                        break;
                case QUIC_FT_CONNECTION_CLOSE:
+                       /* Nothing to do for non crypto errors */
+                       if ((frm->connection_close.error_code & QC_ERR_CRYPTO_ERROR) && qc->conn) {
+                               ssl_sock_handle_hs_error(qc->conn);
+                               if (objt_server(qc->conn->target) && !qc->conn->mux) {
+                                       /* This has as side effect to close the connection stream */
+                                       if (conn_create_mux(qc->conn, NULL) >= 0)
+                                               qc->conn->mux->wake(qc->conn);
+                               }
+                       }
+                       __fallthrough;
                case QUIC_FT_CONNECTION_CLOSE_APP:
                        /* Increment the error counters */
                        quic_conn_closed_err_count_inc(qc, frm);

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index b549ddd2edb276099fc89faa7c6c98f912201a6e..efcb40b4eb48b523d5a58e1558d26b12c372d9e8 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5963,6 +5963,45 @@ void ssl_sock_update_counters(SSL *ssl,
        }
 }
 
+/* Handle the handshake error for <conn> connection.
+ * Also used by QUIC.
+ */
+void ssl_sock_handle_hs_error(struct connection *conn)
+{
+       struct ssl_counters *counters = NULL;
+       struct ssl_counters *counters_px = NULL;
+
+       /* get counters */
+       ssl_sock_get_stats_counters(conn, &counters, &counters_px);
+
+       /* free resumed session if exists */
+       if (objt_server(conn->target)) {
+               struct server *s = __objt_server(conn->target);
+               /* RWLOCK: only rdlock the SSL cache even when writing in it because there is
+                * one cache per thread, it only prevents to flush it from the CLI in
+                * another thread */
+
+               HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
+               if (s->ssl_ctx.reused_sess[tid].ptr)
+                       ha_free(&s->ssl_ctx.reused_sess[tid].ptr);
+               HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
+       }
+
+       if (counters) {
+               HA_ATOMIC_INC(&counters->failed_handshake);
+               HA_ATOMIC_INC(&counters_px->failed_handshake);
+       }
+
+       /* Report an HS error only on SSL error */
+       if (!(conn->flags & (CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH)))
+               conn_report_term_evt(conn, tevt_loc_hs, hs_tevt_type_truncated_rcv_err);
+
+       /* Fail on all other handshake errors */
+       conn->flags |= CO_FL_ERROR;
+       if (!conn->err_code)
+               conn->err_code = CO_ER_SSL_HANDSHAKE;
+}
+
 /* This is the callback which is used when an SSL handshake is pending. It
  * updates the FD status if it wants some polling before being called again.
  * It returns 0 if it fails in a fatal way or needs to poll to go further,
@@ -5975,8 +6014,6 @@ static int ssl_sock_handshake(struct connection *conn, unsigned int flag)
        int ret;
        struct ssl_counters *counters = NULL;
        struct ssl_counters *counters_px = NULL;
-       struct listener *li;
-       struct server *srv = NULL;
        socklen_t lskerr;
        int skerr;
 
@@ -5985,26 +6022,6 @@ static int ssl_sock_handshake(struct connection *conn, unsigned int flag)
        if (!conn_ctrl_ready(conn))
                return 0;
 
-       /* get counters */
-       switch (obj_type(conn->target)) {
-       case OBJ_TYPE_LISTENER:
-               li = __objt_listener(conn->target);
-               counters = EXTRA_COUNTERS_GET(li->extra_counters, &ssl_stats_module);
-               counters_px = EXTRA_COUNTERS_GET(li->bind_conf->frontend->extra_counters_fe,
-                                                &ssl_stats_module);
-               break;
-
-       case OBJ_TYPE_SERVER:
-               srv = __objt_server(conn->target);
-               counters = EXTRA_COUNTERS_GET(srv->extra_counters, &ssl_stats_module);
-               counters_px = EXTRA_COUNTERS_GET(srv->proxy->extra_counters_be,
-                                                &ssl_stats_module);
-               break;
-
-       default:
-               break;
-       }
-
        if (!ctx)
                goto out_error;
 
@@ -6294,8 +6311,10 @@ reneg_ok:
        if (global_ssl.async)
                SSL_clear_mode(ctx->ssl, SSL_MODE_ASYNC);
 #endif
+       ssl_sock_get_stats_counters(conn, &counters, &counters_px);
        /* Handshake succeeded */
-       ssl_sock_update_counters(ctx->ssl, counters, counters_px, !!srv);
+       ssl_sock_update_counters(ctx->ssl, counters, counters_px,
+                                !!objt_server(conn->target));
 
        TRACE_LEAVE(SSL_EV_CONN_HNDSHK, conn, ctx->ssl);
 
@@ -6307,33 +6326,7 @@ reneg_ok:
        /* Clear openssl global errors stack */
        ssl_sock_dump_errors(conn, NULL);
        ERR_clear_error();
-
-       /* free resumed session if exists */
-       if (objt_server(conn->target)) {
-               struct server *s = __objt_server(conn->target);
-               /* RWLOCK: only rdlock the SSL cache even when writing in it because there is
-                * one cache per thread, it only prevents to flush it from the CLI in
-                * another thread */
-
-               HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
-               if (s->ssl_ctx.reused_sess[tid].ptr)
-                       ha_free(&s->ssl_ctx.reused_sess[tid].ptr);
-               HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
-       }
-
-       if (counters) {
-               HA_ATOMIC_INC(&counters->failed_handshake);
-               HA_ATOMIC_INC(&counters_px->failed_handshake);
-       }
-
-       /* Report an HS error only on SSL error */
-       if (!(conn->flags & (CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH)))
-               conn_report_term_evt(conn, tevt_loc_hs, hs_tevt_type_truncated_rcv_err);
-
-       /* Fail on all other handshake errors */
-       conn->flags |= CO_FL_ERROR;
-       if (!conn->err_code)
-               conn->err_code = CO_ER_SSL_HANDSHAKE;
+       ssl_sock_handle_hs_error(conn);
 
        TRACE_ERROR("handshake error", SSL_EV_CONN_HNDSHK|SSL_EV_CONN_ERR, conn, (ctx ? ctx->ssl : NULL), &conn->err_code, (ctx ? &ctx->error_code : NULL));
        return 0;