MEDIUM: quic: Remove qc_conn_finalize() from the ClientHello TLS callbacks

This is a bad idea to make the TLS ClientHello callback call qc_conn_finalize().
If this latter fails, this would generate a TLS alert and make the connection
send packet whereas it is not functional. But qc_conn_finalize() job was to
install the transport parameters sent by the QUIC listener. This installation
cannot be done at any time. This must be done after having possibly negotiated
the QUIC version and before sending the first Handshake packets. It seems
the better moment to do that in when the Handshake TX secrets are derived. This
has been found inspecting the ngtcp2 code. Calling SSL_set_quic_transport_params()
too late would make the ServerHello to be sent without the transport parameters.

The code for the connection update which was done from qc_conn_finalize() has
been moved to quic_transport_params_store(). So, this update is done as soon as
possible.

Add QUIC_FL_CONN_TX_TP_RECEIVED to flag the connection as having received the
peer transport parameters. Indeed this is required when the ClientHello message
is splitted between packets.

Add QUIC_FL_CONN_FINALIZED to protect the connection from calling qc_conn_finalize()
more than one time. This latter is called only when the connection has received
the transport parameters and after returning from SSL_do_hanshake() which is the
function which trigger the TLS ClientHello callback call.

Remove the calls to qc_conn_finalize() from from the TLS ClientHello callbacks.

Must be backported to 2.6. and 2.7.

diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h
index fa4ff54a6d0e56bee159bea7d33b74a370b98997..f2c7680e47439e0c4bbaed8bba9faa95a00bc8ee 100644 (file)
--- a/include/haproxy/quic_conn-t.h
+++ b/include/haproxy/quic_conn-t.h
@@ -618,11 +618,14 @@ enum qc_mux_state {
 /* gap here */
 #define QUIC_FL_CONN_HALF_OPEN_CNT_DECREMENTED   (1U << 11) /* The half-open connection counter was decremented */
 #define QUIC_FL_CONN_HANDSHAKE_SPEED_UP          (1U << 12) /* Handshake speeding up was done */
+#define QUIC_FL_CONN_TX_TP_RECEIVED              (1U << 25) /* Peer transport parameters have been received (used for the transmitting part) */
+#define QUIC_FL_CONN_FINALIZED                   (1U << 26) /* QUIC connection finalized (functional, ready to send/receive) */
 #define QUIC_FL_CONN_NOTIFY_CLOSE                (1U << 27) /* MUX notified about quic-conn imminent closure (idle-timeout or CONNECTION_CLOSE emission/reception) */
 #define QUIC_FL_CONN_EXP_TIMER                   (1U << 28) /* timer has expired, quic-conn can be freed */
 #define QUIC_FL_CONN_CLOSING                     (1U << 29)
 #define QUIC_FL_CONN_DRAINING                    (1U << 30)
 #define QUIC_FL_CONN_IMMEDIATE_CLOSE             (1U << 31)
+
 struct quic_conn {
        const struct quic_version *original_version;
        const struct quic_version *negotiated_version;

diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h
index b5f25d865ceeb17bbeace8617f49a6f36632c2f6..5e92afbee8e470b0a42c21f38c0a92acbfe3f122 100644 (file)
--- a/include/haproxy/quic_conn.h
+++ b/include/haproxy/quic_conn.h
@@ -49,7 +49,6 @@
 
 extern struct pool_head *pool_head_quic_connection_id;
 
-int qc_conn_finalize(struct quic_conn *qc, int server);
 int ssl_quic_initial_ctx(struct bind_conf *bind_conf);
 
 /* Return the long packet type matching with <qv> version and <type> */

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 79c20137c74b049cec6c9d7d42660d9a5edb8b88..904e6699be1d60b43aab1630a91f06752d3d1373 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1002,6 +1002,22 @@ write:
                goto leave;
        }
 
+       if (level == ssl_encryption_handshake && qc_is_listener(qc)) {
+               qc->enc_params_len =
+                       quic_transport_params_encode(qc->enc_params,
+                                                    qc->enc_params + sizeof qc->enc_params,
+                                                    &qc->rx.params, ver, 1);
+               if (!qc->enc_params_len) {
+                       TRACE_ERROR("quic_transport_params_encode() failed", QUIC_EV_CONN_RWSEC);
+                       goto leave;
+               }
+
+               if (!SSL_set_quic_transport_params(qc->xprt_ctx->ssl, qc->enc_params, qc->enc_params_len)) {
+                       TRACE_ERROR("SSL_set_quic_transport_params() failed", QUIC_EV_CONN_RWSEC);
+                       goto leave;
+               }
+       }
+
        if (level == ssl_encryption_application) {
                struct quic_tls_kp *prv_rx = &qc->ku.prv_rx;
                struct quic_tls_kp *nxt_rx = &qc->ku.nxt_rx;
@@ -2216,6 +2232,41 @@ static forceinline void qc_ssl_dump_errors(struct connection *conn)
 int ssl_sock_get_alpn(const struct connection *conn, void *xprt_ctx,
                       const char **str, int *len);
 
+/* Finalize <qc> QUIC connection:
+ * - initialize the Initial QUIC TLS context for negotiated version,
+ * - derive the secrets for this context,
+ * - set them into the TLS stack,
+ *
+ * MUST be called after having received the remote transport parameters which
+ * are parsed when the TLS callback for the ClientHello message is called upon
+ * SSL_do_handshake() calls, not necessarily at the first time as this TLS
+ * message may be splitted between packets
+ * Return 1 if succeeded, 0 if not.
+ */
+static int qc_conn_finalize(struct quic_conn *qc, int server)
+{
+       int ret = 0;
+
+       TRACE_ENTER(QUIC_EV_CONN_NEW, qc);
+
+       if (qc->flags & QUIC_FL_CONN_FINALIZED)
+               goto finalized;
+
+       if (qc->negotiated_version &&
+           !qc_new_isecs(qc, &qc->negotiated_ictx, qc->negotiated_version,
+                         qc->odcid.data, qc->odcid.len, server))
+               goto out;
+
+       /* This connection is functional (ready to send/receive) */
+       qc->flags |= QUIC_FL_CONN_FINALIZED;
+
+ finalized:
+       ret = 1;
+ out:
+       TRACE_LEAVE(QUIC_EV_CONN_NEW, qc);
+       return ret;
+}
+
 /* Provide CRYPTO data to the TLS stack found at <data> with <len> as length
  * from <qel> encryption level with <ctx> as QUIC connection context.
  * Remaining parameter are there for debugging purposes.
@@ -2252,6 +2303,16 @@ static inline int qc_provide_cdata(struct quic_enc_level *el,
        state = qc->state;
        if (state < QUIC_HS_ST_COMPLETE) {
                ssl_err = SSL_do_handshake(ctx->ssl);
+
+               /* Finalize the connection as soon as possible if the peer transport parameters
+                * have been received. This may be useful to send packets even if this
+                * handshake fails.
+                */
+               if ((qc->flags & QUIC_FL_CONN_TX_TP_RECEIVED) && !qc_conn_finalize(qc, 1)) {
+                       TRACE_ERROR("connection finalization failed", QUIC_EV_CONN_IO_CB, qc, &state);
+                       goto leave;
+               }
+
                if (ssl_err != 1) {
                        ssl_err = SSL_get_error(ctx->ssl, ssl_err);
                        if (ssl_err == SSL_ERROR_WANT_READ || ssl_err == SSL_ERROR_WANT_WRITE) {
@@ -5877,71 +5938,6 @@ static int qc_ssl_sess_init(struct quic_conn *qc, SSL_CTX *ssl_ctx, SSL **ssl,
        goto leave;
 }
 
-/* Finalize <qc> QUIC connection:
- * - initialize the Initial QUIC TLS context for negotiated version,
- * - derive the secrets for this context,
- * - encode the transport parameters to be sent,
- * - set them into the TLS stack,
- * - initialize ->max_ack_delay and max_idle_timeout,
- *
- * MUST be called after having received the remote transport parameters.
- * Return 1 if succeeded, 0 if not.
- */
-int qc_conn_finalize(struct quic_conn *qc, int server)
-{
-       int ret = 0;
-       struct quic_transport_params *tx_tp = &qc->tx.params;
-       struct quic_transport_params *rx_tp = &qc->rx.params;
-       const struct quic_version *ver;
-
-       TRACE_ENTER(QUIC_EV_CONN_NEW, qc);
-
-       if (tx_tp->version_information.negotiated_version &&
-           tx_tp->version_information.negotiated_version != qc->original_version) {
-               qc->negotiated_version =
-                       qc->tx.params.version_information.negotiated_version;
-               if (!qc_new_isecs(qc, &qc->negotiated_ictx, qc->negotiated_version,
-                                                 qc->odcid.data, qc->odcid.len, !server))
-                       goto out;
-
-               ver = qc->negotiated_version;
-       }
-       else {
-               ver = qc->original_version;
-       }
-
-       qc->enc_params_len =
-               quic_transport_params_encode(qc->enc_params,
-                                            qc->enc_params + sizeof qc->enc_params,
-                                            &qc->rx.params, ver, 1);
-       if (!qc->enc_params_len) {
-               TRACE_ERROR("quic_transport_params_encode() failed", QUIC_EV_CONN_TXPKT);
-               goto out;
-       }
-
-       if (!SSL_set_quic_transport_params(qc->xprt_ctx->ssl, qc->enc_params, qc->enc_params_len)) {
-               TRACE_ERROR("SSL_set_quic_transport_params() failed", QUIC_EV_CONN_TXPKT);
-               goto out;
-       }
-
-       if (tx_tp->max_ack_delay)
-               qc->max_ack_delay = tx_tp->max_ack_delay;
-
-       if (tx_tp->max_idle_timeout && rx_tp->max_idle_timeout)
-               qc->max_idle_timeout =
-                       QUIC_MIN(tx_tp->max_idle_timeout, rx_tp->max_idle_timeout);
-       else
-               qc->max_idle_timeout =
-                       QUIC_MAX(tx_tp->max_idle_timeout, rx_tp->max_idle_timeout);
-
-       TRACE_PROTO("\nTX(remote) transp. params.", QUIC_EV_TRANSP_PARAMS, qc, tx_tp);
-
-       ret = 1;
- out:
-       TRACE_LEAVE(QUIC_EV_CONN_NEW, qc);
-       return ret;
-}
-
 /* Allocate the ssl_sock_ctx from connection <qc>. This creates the tasklet
  * used to process <qc> received packets. The allocated context is stored in
  * <qc.xprt_ctx>.

diff --git a/src/quic_tp.c b/src/quic_tp.c
index 31111f2ca1fdcef0014aa85146f4e052b35d1202..85e2870ee852d703f7dad99544a80289768130bf 100644 (file)
--- a/src/quic_tp.c
+++ b/src/quic_tp.c
@@ -625,6 +625,7 @@ int quic_transport_params_store(struct quic_conn *qc, int server,
                                 const unsigned char *end)
 {
        struct quic_transport_params *tx_params = &qc->tx.params;
+       struct quic_transport_params *rx_params = &qc->rx.params;
 
        /* initialize peer TPs to RFC default value */
        quic_dflt_transport_params_cpy(tx_params);
@@ -632,6 +633,23 @@ int quic_transport_params_store(struct quic_conn *qc, int server,
        if (!quic_transport_params_decode(tx_params, server, buf, end))
                return 0;
 
+       /* Update the connection from transport parameters received */
+       if (tx_params->version_information.negotiated_version &&
+           tx_params->version_information.negotiated_version != qc->original_version)
+               qc->negotiated_version =
+                       qc->tx.params.version_information.negotiated_version;
+
+       if (tx_params->max_ack_delay)
+               qc->max_ack_delay = tx_params->max_ack_delay;
+
+       if (tx_params->max_idle_timeout && rx_params->max_idle_timeout)
+               qc->max_idle_timeout =
+                       QUIC_MIN(tx_params->max_idle_timeout, rx_params->max_idle_timeout);
+       else
+               qc->max_idle_timeout =
+                       QUIC_MAX(tx_params->max_idle_timeout, rx_params->max_idle_timeout);
+       TRACE_PROTO("\nTX(remote) transp. params.", QUIC_EV_TRANSP_PARAMS, qc, tx_params);
+
        return 1;
 }
 

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 51d2d70464c0fca812d074c1d96088b0c54fc9f4..7c3c152a578304ed4441e556915f6b2800449e8c 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2361,9 +2361,10 @@ int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *arg)
                }
 
                if (!quic_transport_params_store(qc, 0, extension_data,
-                                                extension_data + extension_len) ||
-                   !qc_conn_finalize(qc, 0))
+                                                extension_data + extension_len))
                        goto abort;
+
+               qc->flags |= QUIC_FL_CONN_TX_TP_RECEIVED;
        }
 #endif /* USE_QUIC */
 
@@ -2657,10 +2658,10 @@ int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *priv)
                }
 
                if (!quic_transport_params_store(qc, 0, extension_data,
-                                                extension_data + extension_len) ||
-                   !qc_conn_finalize(qc, 0)) {
+                                                extension_data + extension_len))
                        return SSL_TLSEXT_ERR_NOACK;
-               }
+
+               qc->flags |= QUIC_FL_CONN_TX_TP_RECEIVED;
        }
 #endif /* USE_QUIC */