QUIC ACKM: RFC 9000 s. 13.2.1: max_ack_delay taken as 0 in INITIAL/HANDSHAKE

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21349)

diff --git a/ssl/quic/quic_ackm.c b/ssl/quic/quic_ackm.c
index 025b7cc7bd30e022298879f14cda4b98a9f581a0..facd34176a0552a2c15c338435fc52258fec9707 100644 (file)
--- a/ssl/quic/quic_ackm.c
+++ b/ssl/quic/quic_ackm.c
@@ -1466,6 +1466,8 @@ static void ackm_on_rx_ack_eliciting(OSSL_ACKM *ackm,
                                      OSSL_TIME rx_time, int pkt_space,
                                      int was_missing)
 {
+    OSSL_TIME tx_max_ack_delay;
+
     if (ackm->rx_ack_desired[pkt_space])
         /* ACK generation already requested so nothing to do. */
         return;
@@ -1507,15 +1509,24 @@ static void ackm_on_rx_ack_eliciting(OSSL_ACKM *ackm,
      * Not emitting an ACK yet.
      *
      * Update the ACK flush deadline.
+     *
+     * RFC 9000 s. 13.2.1: "An endpoint MUST acknowledge all ack-eliciting
+     * Initial and Handshake packets immediately"; don't delay ACK generation if
+     * we are using the Initial or Handshake PN spaces.
      */
+    tx_max_ack_delay = ackm->tx_max_ack_delay;
+    if (pkt_space == QUIC_PN_SPACE_INITIAL
+        || pkt_space == QUIC_PN_SPACE_HANDSHAKE)
+        tx_max_ack_delay = ossl_time_zero();
+
     if (ossl_time_is_infinite(ackm->rx_ack_flush_deadline[pkt_space]))
         ackm_set_flush_deadline(ackm, pkt_space,
-                                ossl_time_add(rx_time, ackm->tx_max_ack_delay));
+                                ossl_time_add(rx_time, tx_max_ack_delay));
     else
         ackm_set_flush_deadline(ackm, pkt_space,
                                 ossl_time_min(ackm->rx_ack_flush_deadline[pkt_space],
                                               ossl_time_add(rx_time,
-                                                            ackm->tx_max_ack_delay)));
+                                                            tx_max_ack_delay)));
 }
 
 int ossl_ackm_on_rx_packet(OSSL_ACKM *ackm, const OSSL_ACKM_RX_PKT *pkt)

diff --git a/test/quic_ackm_test.c b/test/quic_ackm_test.c
index b92e153d1a1cee85baf6e44d8e01960f42707855..7e7c35816b771d3a2906d576b55a857bf4eeff17 100644 (file)
--- a/test/quic_ackm_test.c
+++ b/test/quic_ackm_test.c
@@ -618,7 +618,8 @@ enum {
     RX_OPK_CHECK_STATE,      /* check is_desired/deadline */
     RX_OPK_CHECK_ACKS,       /* check ACK ranges */
     RX_OPK_TX,               /* TX packet */
-    RX_OPK_RX_ACK            /* RX ACK frame */
+    RX_OPK_RX_ACK,           /* RX ACK frame */
+    RX_OPK_SKIP_IF_PN_SPACE  /* skip for a given PN space */
 };
 
 struct rx_test_op {
@@ -685,6 +686,12 @@ struct rx_test_op {
       0, 0, NULL, 0, 0                                              \
     },
 
+#define RX_OP_SKIP_IF_PN_SPACE(pn_space)                            \
+    {                                                               \
+      RX_OPK_SKIP_IF_PN_SPACE, 0, (pn_space), 0,                    \
+      0, 0, NULL, 0, 0                                              \
+    },
+
 #define RX_OP_END                                                   \
     { RX_OPK_END }
 
@@ -715,7 +722,7 @@ static const struct rx_test_op rx_script_1[] = {
     RX_OP_END
 };
 
-/* RX 2. Simple Test with ACK Not Yet Desired (Packet Threshold) */
+/* RX 2. Simple Test with ACK Not Yet Desired (Packet Threshold) (1-RTT) */
 static const OSSL_QUIC_ACK_RANGE rx_ack_ranges_2a[] = {
     { 0, 0 }
 };
@@ -725,6 +732,14 @@ static const OSSL_QUIC_ACK_RANGE rx_ack_ranges_2b[] = {
 };
 
 static const struct rx_test_op rx_script_2[] = {
+    /*
+     * We skip this for INITIAL/HANDSHAKE and use a separate version
+     * (rx_script_4) for those spaces as those spaces should not delay ACK
+     * generation, so a different RX_OP_CHECK_STATE test is needed.
+     */
+    RX_OP_SKIP_IF_PN_SPACE(QUIC_PN_SPACE_INITIAL)
+    RX_OP_SKIP_IF_PN_SPACE(QUIC_PN_SPACE_HANDSHAKE)
+
     RX_OP_CHECK_STATE   (0, 0, 0)   /* no threshold yet */
     RX_OP_CHECK_PROC    (0, 0, 3)
 
@@ -826,10 +841,57 @@ static const struct rx_test_op rx_script_3[] = {
     RX_OP_END
 };
 
+/*
+ * RX 4. Simple Test with ACK Not Yet Desired (Packet Threshold)
+ * (Initial/Handshake)
+ */
+static const OSSL_QUIC_ACK_RANGE rx_ack_ranges_4a[] = {
+    { 0, 1 }
+};
+
+static const struct rx_test_op rx_script_4[] = {
+    /* The application PN space is tested in rx_script_2. */
+    RX_OP_SKIP_IF_PN_SPACE(QUIC_PN_SPACE_APP)
+
+    RX_OP_CHECK_STATE   (0, 0, 0)   /* no threshold yet */
+    RX_OP_CHECK_PROC    (0, 0, 3)
+
+    /* First packet always generates an ACK so get it out of the way. */
+    RX_OP_PKT           (0, 0, 1)
+    RX_OP_CHECK_UNPROC  (0, 0, 1)
+    RX_OP_CHECK_PROC    (0, 1, 1)
+    RX_OP_CHECK_STATE   (0, 1, 0)   /* first packet always causes ACK */
+    RX_OP_CHECK_ACKS    (0, rx_ack_ranges_2a) /* clears packet counter */
+    RX_OP_CHECK_STATE   (0, 0, 0)   /* desired state should have been cleared */
+
+    /*
+     * Second packet should cause ACK-desired state because we are
+     * INITIAL/HANDSHAKE (RFC 9000 s. 13.2.1)
+     */
+    RX_OP_PKT           (0, 1, 1)   /* just one packet, threshold is 2 */
+    RX_OP_CHECK_UNPROC  (0, 0, 2)
+    RX_OP_CHECK_PROC    (0, 2, 1)
+    RX_OP_CHECK_STATE   (0, 1, 1)
+    RX_OP_CHECK_ACKS    (0, rx_ack_ranges_4a)
+    RX_OP_CHECK_STATE   (0, 0, 0)   /* desired state should have been cleared */
+
+    /* At this point we would generate e.g. a packet with an ACK. */
+    RX_OP_TX            (0, 0, 1)   /* ACKs all */
+    RX_OP_CHECK_ACKS    (0, rx_ack_ranges_4a) /* not provably ACKed yet */
+    RX_OP_RX_ACK        (0, 0, 1)   /* TX'd packet is ACK'd */
+
+    RX_OP_CHECK_NO_ACKS (0)         /* nothing more to ACK */
+    RX_OP_CHECK_UNPROC  (0, 0, 2)   /* still unprocessable */
+    RX_OP_CHECK_PROC    (0, 2, 1)   /* still processable */
+
+    RX_OP_END
+};
+
 static const struct rx_test_op *const rx_test_scripts[] = {
     rx_script_1,
     rx_script_2,
-    rx_script_3
+    rx_script_3,
+    rx_script_4
 };
 
 static void on_ack_deadline_callback(OSSL_TIME deadline,
@@ -850,6 +912,7 @@ static int test_rx_ack_actual(int tidx, int space)
     struct pkt_info *pkts = NULL;
     OSSL_ACKM_TX_PKT *txs = NULL, *tx;
     OSSL_TIME ack_deadline[QUIC_PN_SPACE_NUM];
+    size_t opn = 0;
 
     for (i = 0; i < QUIC_PN_SPACE_NUM; ++i)
         ack_deadline[i] = ossl_time_infinite();
@@ -880,7 +943,7 @@ static int test_rx_ack_actual(int tidx, int space)
         goto err;
 
     /* Run script. */
-    for (s = script; s->kind != RX_OPK_END; ++s) {
+    for (s = script; s->kind != RX_OPK_END; ++s, ++opn) {
         fake_time = ossl_time_add(fake_time,
                                   ossl_ticks2time(s->time_advance));
         switch (s->kind) {
@@ -992,6 +1055,14 @@ static int test_rx_ack_actual(int tidx, int space)
 
             break;
 
+        case RX_OPK_SKIP_IF_PN_SPACE:
+            if (space == s->pn) {
+                testresult = 1;
+                goto err;
+            }
+
+            break;
+
         default:
             goto err;
         }
@@ -999,6 +1070,9 @@ static int test_rx_ack_actual(int tidx, int space)
 
     testresult = 1;
 err:
+    if (!testresult)
+        TEST_error("error in ACKM RX script %d, op %zu", tidx + 1, opn + 1);
+
     helper_destroy(&h);
     OPENSSL_free(pkts);
     OPENSSL_free(txs);