BUG/MEDIUM: quic: avoid freezing 0RTT connections

This issue came with this commit:

	f627b92 BUG/MEDIUM: quic: always validate sender address on 0-RTT

and could be easily reproduced with picoquic QUIC client with -Q option
which splits a big ClientHello TLS message into two Initial datagrams.
A second condition must be fulfilled to reprodue this issue: picoquic
must not send the token provided by haproxy (NEW_TOKEN). To do that,
haproxy must be patched to prevent it to send such tokens.

Under these conditions, if haproxy has enough time to reply to the first Initial
datagrams, when it receives the second Initial datagram it sends a Retry paquet.
Then the client ignores the Retry paquet as mentionned by RFC 9000:

 17.2.5.2. Handling a Retry Packet
    A client MUST accept and process at most one Retry packet for each connection
    attempt. After the client has received and processed an Initial or Retry packet
    from the server, it MUST discard any subsequent Retry packets that it receives.

On its side, haproxy has closed the connection. When it receives the second
Initial datagram, it open a new connection but with Initial packets it
cannot decrypt (wrong ODCID) leaving the client without response.

To fix this, as the aim of the token (NEW_TOKEN) sent by haproxy is to validate
the peer address, in place of closing the connection when no token was received
for a 0RTT connection, one leaves this validation to the handshake process.
Indeed, the peer adress is validated during the handshake when a valid handshake
packet is received by the listener. But as one does not want haproxy to process
0RTT data when no token was received, one does not accept the connection before
the successful handshake completion. In addition to this, the 0RTT packets
are not released after successful handshake completion when no token was received
to leave a chance to haproxy to process these 0RTT data in such case (see
quic_conn_io_cb()).

Must be backported as far as 2.9.

diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h
index 6b5c6361b07ca340bd03abd7c6312d46e21d9943..7ed7156106f30afb0bbc6e8e306fbda02d5f662c 100644 (file)
--- a/include/haproxy/quic_conn-t.h
+++ b/include/haproxy/quic_conn-t.h
@@ -446,7 +446,6 @@ struct quic_conn_closed {
 #define QUIC_FL_CONN_HPKTNS_DCD                  (1U << 16) /* Handshake packet number space discarded  */
 #define QUIC_FL_CONN_PEER_VALIDATED_ADDR         (1U << 17) /* Peer address is considered as validated for this connection. */
 #define QUIC_FL_CONN_NO_TOKEN_RCVD               (1U << 18) /* Client dit not send any token */
-#define QUIC_FL_CONN_SEND_RETRY                  (1U << 19) /* A send retry packet must be sent */
 /* gap here */
 #define QUIC_FL_CONN_TO_KILL                     (1U << 24) /* Unusable connection, to be killed */
 #define QUIC_FL_CONN_TX_TP_RECEIVED              (1U << 25) /* Peer transport parameters have been received (used for the transmitting part) */

diff --git a/src/quic_conn.c b/src/quic_conn.c
index ea9e16315afa5a08311545c50c40026bf32e46a0..5c9577e639524d23eb80dda077fb6880a7d84917 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -811,11 +811,6 @@ struct task *quic_conn_io_cb(struct task *t, void *context, unsigned int state)
                goto out;
        }
 
-       if (qc->flags & QUIC_FL_CONN_TO_KILL) {
-               TRACE_DEVEL("connection to be killed", QUIC_EV_CONN_PHPKTS, qc);
-               goto out;
-       }
-
        if ((qc->flags & QUIC_FL_CONN_DRAINING) &&
            !(qc->flags & QUIC_FL_CONN_IMMEDIATE_CLOSE))
                goto out;
@@ -831,11 +826,20 @@ struct task *quic_conn_io_cb(struct task *t, void *context, unsigned int state)
                        qc_release_pktns_frms(qc, qc->hel->pktns);
                }
 
-               /* Release 0RTT packets still waiting for HP removal. These
+               /* Note: if no token for address validation was received
+                * for a 0RTT connection, some 0RTT packet could still be
+                * waiting for HP removal AFTER the successful handshake completion.
+                * Indeed a successful handshake completion implicitely valids
+                * the peer address. In this case, one wants to process
+                * these ORTT packets AFTER the succesful handshake completion.
+                *
+                * On the contrary, when a token for address validation was received,
+                * release 0RTT packets still waiting for HP removal. These
                 * packets are considered unneeded after handshake completion.
                 * They will be freed later from Rx buf via quic_rx_pkts_del().
                 */
-               if (qc->eel && !LIST_ISEMPTY(&qc->eel->rx.pqpkts)) {
+               if (qc->eel && !LIST_ISEMPTY(&qc->eel->rx.pqpkts) &&
+                   !(qc->flags & QUIC_FL_CONN_NO_TOKEN_RCVD)) {
                        struct quic_rx_packet *pqpkt, *pkttmp;
                        list_for_each_entry_safe(pqpkt, pkttmp, &qc->eel->rx.pqpkts, list) {
                                LIST_DEL_INIT(&pqpkt->list);
@@ -896,25 +900,7 @@ struct task *quic_conn_io_cb(struct task *t, void *context, unsigned int state)
                quic_nictx_free(qc);
        }
 
-       if (qc->flags & QUIC_FL_CONN_SEND_RETRY) {
-               struct quic_counters *prx_counters;
-               struct proxy *prx = qc->li->bind_conf->frontend;
-               struct quic_rx_packet pkt = {
-                       .scid = qc->dcid,
-                       .dcid = qc->odcid,
-               };
-
-               prx_counters = EXTRA_COUNTERS_GET(prx->extra_counters_fe, &quic_stats_module);
-               if (send_retry(qc->li->rx.fd, &qc->peer_addr, &pkt, qc->original_version)) {
-                       TRACE_ERROR("Error during Retry generation",
-                                   QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qc->original_version);
-               }
-               else
-                       HA_ATOMIC_INC(&prx_counters->retry_sent);
-       }
-
-       if ((qc->flags & (QUIC_FL_CONN_CLOSING|QUIC_FL_CONN_TO_KILL)) &&
-           qc->mux_state != QC_MUX_READY) {
+       if ((qc->flags & QUIC_FL_CONN_CLOSING) && qc->mux_state != QC_MUX_READY) {
                quic_conn_release(qc);
                qc = NULL;
        }

diff --git a/src/quic_ssl.c b/src/quic_ssl.c
index 03631c0acda740c08bb58f6ccc01b2cca1272b69..a8c3c2adbefec803ce0ba1601d79009a719e5dd2 100644 (file)
--- a/src/quic_ssl.c
+++ b/src/quic_ssl.c
@@ -223,12 +223,21 @@ static int ha_quic_set_encryption_secrets(SSL *ssl, enum ssl_encryption_level_t
        }
 
        /* Enqueue this connection asap if we could derive O-RTT secrets as
-        * listener. Note that a listener derives only RX secrets for this
-        * level.
+        * listener and if a token was received. Note that a listener derives only RX
+        * secrets for this level.
         */
        if (qc_is_listener(qc) && level == ssl_encryption_early_data) {
-               TRACE_DEVEL("pushing connection into accept queue", QUIC_EV_CONN_RWSEC, qc);
-               quic_accept_push_qc(qc);
+               if (qc->flags & QUIC_FL_CONN_NO_TOKEN_RCVD) {
+                       /* Leave a chance to the address validation to be completed by the
+                        * handshake without starting the mux: one does not want to process
+                        * the 0RTT data in this case.
+                        */
+                       TRACE_PROTO("0RTT session without token", QUIC_EV_CONN_RWSEC, qc);
+               }
+               else {
+                       TRACE_DEVEL("pushing connection into accept queue", QUIC_EV_CONN_RWSEC, qc);
+                       quic_accept_push_qc(qc);
+               }
        }
 
 write:
@@ -355,21 +364,6 @@ static int ha_quic_add_handshake_data(SSL *ssl, enum ssl_encryption_level_t leve
 
        TRACE_PROTO("ha_quic_add_handshake_data() called", QUIC_EV_CONN_IO_CB, qc, NULL, NULL, ssl);
 
-#ifdef HAVE_SSL_0RTT_QUIC
-       /* Detect asap if some 0-RTT data were accepted for this connection.
-        * If this is the case and no token was provided, interrupt the useless
-        * secrets derivations. A Retry packet must be sent, and this connection
-        * must be killed.
-        * Note that QUIC_FL_CONN_NO_TOKEN_RCVD is possibly set only for when 0-RTT is
-        * enabled for the connection.
-        */
-       if ((qc->flags & QUIC_FL_CONN_NO_TOKEN_RCVD) && qc_ssl_eary_data_accepted(ssl)) {
-               TRACE_PROTO("connection to be killed", QUIC_EV_CONN_ADDDATA, qc);
-               qc->flags |= QUIC_FL_CONN_TO_KILL|QUIC_FL_CONN_SEND_RETRY;
-               goto leave;
-       }
-#endif
-
        if (qc->flags & QUIC_FL_CONN_TO_KILL) {
                TRACE_PROTO("connection to be killed", QUIC_EV_CONN_ADDDATA, qc);
                goto out;