From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Tue, 24 Jan 2023 16:42:21 +0000 (+0100)
Subject: MEDIUM: h3: enforce GOAWAY by resetting higher unhandled stream
X-Git-Tag: v2.8-dev5~165
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3d550848beb694b566aad05f6d7c549cf35d9f00;p=thirdparty%2Fhaproxy.git

MEDIUM: h3: enforce GOAWAY by resetting higher unhandled stream

When a GOAWAY has been emitted, an ID is announced to represent handled
streams. H3 RFC suggests that higher streams should be resetted with the
error code H3_REQUEST_CANCELLED. This allows the peer to replay requests
on another connection.

For the moment, the impact of this change is limitted as GOAWAY is only
used on connection shutdown just before the MUX is freed. However, for
soft-stop support, a GOAWAY can be emitted in anticipation while keeping
the MUX to finish the active streams. In this case, new streams opened
by the client are resetted.

As a consequence of this change, app_ops.attach() operation has been
delayed at the very end of qcs_new(). This ensure that all qcs members
are initialized to support RESET_STREAM sending.

This should be backported up to 2.7.
---

diff --git a/src/h3.c b/src/h3.c
index 1572ac1b65..c912c0a15f 100644
--- a/src/h3.c
+++ b/src/h3.c
@@ -114,6 +114,7 @@ INITCALL1(STG_REGISTER, trace_register_source, TRACE_SOURCE);
 #define H3_CF_UNI_CTRL_SET      0x00000004  /* Remote H3 Control stream opened */
 #define H3_CF_UNI_QPACK_DEC_SET 0x00000008  /* Remote QPACK decoder stream opened */
 #define H3_CF_UNI_QPACK_ENC_SET 0x00000010  /* Remote QPACK encoder stream opened */
+#define H3_CF_GOAWAY_SENT       0x00000020  /* GOAWAY sent on local control stream */
 
 /* Default settings */
 static uint64_t h3_settings_qpack_max_table_capacity = 0;
@@ -1661,13 +1662,38 @@ static int h3_close(struct qcs *qcs, enum qcc_app_ops_close_side side)
 
 static int h3_attach(struct qcs *qcs, void *conn_ctx)
 {
-	struct h3s *h3s;
+	struct h3c *h3c = conn_ctx;
+	struct h3s *h3s = NULL;
 
 	TRACE_ENTER(H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
 
+	/* RFC 9114 5.2. Connection Shutdown
+	 *
+	 * Upon sending
+	 * a GOAWAY frame, the endpoint SHOULD explicitly cancel (see
+	 * Sections 4.1.1 and 7.2.3) any requests or pushes that have
+	 * identifiers greater than or equal to the one indicated, in
+	 * order to clean up transport state for the affected streams.
+	 * The endpoint SHOULD continue to do so as more requests or
+	 * pushes arrive.
+	 */
+	if (h3c->flags & H3_CF_GOAWAY_SENT && qcs->id >= h3c->id_goaway &&
+	    quic_stream_is_bidi(qcs->id)) {
+		/* Reject request and do not allocate a h3s context.
+		 * TODO support push uni-stream rejection.
+		 */
+		TRACE_STATE("reject stream higher than goaway", H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
+		qcc_abort_stream_read(qcs);
+		qcc_reset_stream(qcs, H3_REQUEST_REJECTED);
+		goto done;
+	}
+
 	h3s = pool_alloc(pool_head_h3s);
-	if (!h3s)
-		return 1;
+	if (!h3s) {
+		TRACE_ERROR("h3s allocation failure", H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
+		qcc_emit_cc_app(qcs->qcc, H3_INTERNAL_ERROR, 1);
+		goto err;
+	}
 
 	qcs->ctx = h3s;
 	h3s->h3c = conn_ctx;
@@ -1689,8 +1715,13 @@ static int h3_attach(struct qcs *qcs, void *conn_ctx)
 		h3s->type = H3S_T_UNKNOWN;
 	}
 
+ done:
 	TRACE_LEAVE(H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
 	return 0;
+
+ err:
+	TRACE_DEVEL("leaving in error", H3_EV_H3S_NEW, qcs->qcc->conn, qcs);
+	return 1;
 }
 
 static void h3_detach(struct qcs *qcs)
@@ -1758,10 +1789,15 @@ static int h3_send_goaway(struct h3c *h3c)
 	b_force_xfer(res, &pos, b_data(&pos));
 	qcc_send_stream(qcs, 1);
 
+	h3c->flags |= H3_CF_GOAWAY_SENT;
 	TRACE_LEAVE(H3_EV_H3C_END, h3c->qcc->conn);
 	return 0;
 
  err:
+	/* Consider GOAWAY as sent even if not really the case. This will
+	 * block future stream opening using H3_REQUEST_REJECTED reset.
+	 */
+	h3c->flags |= H3_CF_GOAWAY_SENT;
 	TRACE_DEVEL("leaving in error", H3_EV_H3C_END, h3c->qcc->conn);
 	return 1;
 }
diff --git a/src/mux_quic.c b/src/mux_quic.c
index d68abbeee3..8577debd3f 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -135,13 +135,6 @@ static struct qcs *qcs_new(struct qcc *qcc, uint64_t id, enum qcs_type type)
 		}
 	}
 
-	if (qcc->app_ops->attach) {
-		if (qcc->app_ops->attach(qcs, qcc->ctx)) {
-			TRACE_ERROR("app proto failure", QMUX_EV_QCS_NEW, qcc->conn, qcs);
-			goto err;
-		}
-	}
-
 	/* If stream is local, use peer remote-limit, or else the opposite. */
 	if (quic_stream_is_bidi(id)) {
 		qcs->tx.msd = quic_stream_is_local(qcc, id) ? qcc->rfctl.msd_bidi_r :
@@ -174,6 +167,11 @@ static struct qcs *qcs_new(struct qcc *qcc, uint64_t id, enum qcs_type type)
 
 	qcs->err = 0;
 
+	if (qcc->app_ops->attach && qcc->app_ops->attach(qcs, qcc->ctx)) {
+		TRACE_ERROR("app proto failure", QMUX_EV_QCS_NEW, qcc->conn, qcs);
+		goto err;
+	}
+
  out:
 	TRACE_LEAVE(QMUX_EV_QCS_NEW, qcc->conn, qcs);
 	return qcs;