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 fa4ff54..f2c7680 100644
--- a/include/haproxy/quic_conn-t.h
+++ b/include/haproxy/quic_conn-t.h
@@ -618,11 +618,14 @@
/* 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 b5f25d8..5e92afb 100644
--- 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 79c2013..904e669 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1002,6 +1002,22 @@
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 @@
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 @@
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 @@
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 31111f2..85e2870 100644
--- a/src/quic_tp.c
+++ b/src/quic_tp.c
@@ -625,6 +625,7 @@
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 @@
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 51d2d70..7c3c152 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2361,9 +2361,10 @@
}
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 @@
}
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 */