From 7b89aa5b1943c1537a844cc375f7e8a86bac7942 Mon Sep 17 00:00:00 2001
From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Thu, 1 Aug 2024 15:35:06 +0200
Subject: [PATCH] BUG/MINOR: h1: do not forward h2c upgrade header token

haproxy supports tunnel establishment through HTTP Upgrade mechanism.
Since the following commit, extended CONNECT is also supported for
HTTP/2 both on frontend and backend side.

  commit 9bf957335e2c385b74901481f7a89c9565dfce53
  MEDIUM: mux_h2: generate Extended CONNECT from htx upgrade

As specified by HTTP/2 rfc, "h2c" can be used by an HTTP/1.1 client to
request an upgrade to HTTP/2. In haproxy, this is not supported so it
silently ignores this. However, Connection and Upgrade headers are
forwarded as-is on the backend side.

If using HTTP/1 on the backend side and the server supports this upgrade
mechanism, haproxy won't be able to parse the HTTP response. If using
HTTP/2, mux backend tries to incorrectly convert the request to an
Extended CONNECT with h2c protocol, which may also prevent the response
to be transmitted.

To fix this, flag HTTP/1 request with "h2c" or "h2" token in an upgrade
header. On converting the header list to HTX, the upgrade header is
skipped if any of this token is present and the H1_MF_CONN_UPG flag is
removed.

This issue can easily be reproduced using curl --http2 argument to
connect to an HTTP/1 frontend.

This must be backported up to 2.4 after a period of observation.
---
 include/haproxy/h1.h                          |  1 +
 reg-tests/http-messaging/protocol_upgrade.vtc | 27 +++++++++++++++++++
 src/h1.c                                      |  5 +++-
 src/h1_htx.c                                  |  9 ++++++-
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/include/haproxy/h1.h b/include/haproxy/h1.h
index 283bbad5b1f10..b4e66908e03f7 100644
--- a/include/haproxy/h1.h
+++ b/include/haproxy/h1.h
@@ -98,6 +98,7 @@ enum h1m_state {
 #define H1_MF_UPG_WEBSOCKET     0x00008000 // Set for a Websocket upgrade handshake
 #define H1_MF_TE_CHUNKED        0x00010000 // T-E "chunked"
 #define H1_MF_TE_OTHER          0x00020000 // T-E other than supported ones found (only "chunked" is supported for now)
+#define H1_MF_UPG_H2C           0x00040000 // "h2c" or "h2" used as upgrade token
 
 /* Mask to use to reset H1M flags when we restart headers parsing.
  *
diff --git a/reg-tests/http-messaging/protocol_upgrade.vtc b/reg-tests/http-messaging/protocol_upgrade.vtc
index ebb6328c3014d..be291f18c50a3 100644
--- a/reg-tests/http-messaging/protocol_upgrade.vtc
+++ b/reg-tests/http-messaging/protocol_upgrade.vtc
@@ -85,6 +85,15 @@ server srv_h2_no_ws2 {
 	} -run
 } -start
 
+# http/1.1 server
+server srv_h1_h2c {
+	rxreq
+	expect req.method == "GET"
+	expect req.http.upgrade != "h2c"
+	txresp \
+	  -status 200
+} -start
+
 haproxy hap -conf {
 	defaults
 	mode http
@@ -121,6 +130,11 @@ haproxy hap -conf {
 	listen frt_h2_h1
 	bind "fd@${frt_h2_h1}" proto h2
 	server frt_h1 ${hap_frt_h1_addr}:${hap_frt_h1_port}
+
+	# h1 frontend to handle "h2c" token
+	listen frt_h1_h2c
+	bind "fd@${frt_h1_h2c}"
+	server srv_h1_h2c ${srv_h1_h2c_addr}:${srv_h1_h2c_port}
 } -start
 
 ## connect to h1 translation frontend
@@ -226,3 +240,16 @@ client c6 -connect ${hap_frt_h1_no_ws2_sock} {
 	rxresp
 	expect resp.status == 502
 } -run
+
+# connect as http/1 with invalid "h2c" protocol
+client c7_h2c -connect ${hap_frt_h1_h2c_sock} {
+	txreq \
+	  -req "GET" \
+	  -url "/" \
+	  -hdr "host: 127.0.0.1" \
+	  -hdr "connection: upgrade" \
+	  -hdr "upgrade: h2c"
+
+	rxresp
+	expect resp.status == 200
+} -run
diff --git a/src/h1.c b/src/h1.c
index 2cc15040fbbc8..e1a7028fbc5c7 100644
--- a/src/h1.c
+++ b/src/h1.c
@@ -365,13 +365,14 @@ void h1_parse_connection_header(struct h1m *h1m, struct ist *value)
 
 /* Parse the Upgrade: header of an HTTP/1 request.
  * If "websocket" is found, set H1_MF_UPG_WEBSOCKET flag
+ * If "h2c" or "h2" found, set H1_MF_UPG_H2C flag.
  */
 void h1_parse_upgrade_header(struct h1m *h1m, struct ist value)
 {
 	char *e, *n;
 	struct ist word;
 
-	h1m->flags &= ~H1_MF_UPG_WEBSOCKET;
+	h1m->flags &= ~(H1_MF_UPG_WEBSOCKET|H1_MF_UPG_H2C);
 
 	word.ptr = value.ptr - 1; // -1 for next loop's pre-increment
 	e = istend(value);
@@ -390,6 +391,8 @@ void h1_parse_upgrade_header(struct h1m *h1m, struct ist value)
 
 		if (isteqi(word, ist("websocket")))
 			h1m->flags |= H1_MF_UPG_WEBSOCKET;
+		else if (isteqi(word, ist("h2c")) || isteqi(word, ist("h2")))
+			h1m->flags |= H1_MF_UPG_H2C;
 
 		word.ptr = n;
 	}
diff --git a/src/h1_htx.c b/src/h1_htx.c
index 562c0f2b6fdfb..472169aec5507 100644
--- a/src/h1_htx.c
+++ b/src/h1_htx.c
@@ -180,7 +180,13 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx
 
 
 	flags |= h1m_htx_sl_flags(h1m);
-	if ((flags & (HTX_SL_F_CONN_UPG|HTX_SL_F_BODYLESS)) == HTX_SL_F_CONN_UPG) {
+
+	/* Remove Upgrade header in problematic cases :
+	 * - body present
+	 * - "h2c" or "h2" token specified as token
+	 */
+	if (((flags & (HTX_SL_F_CONN_UPG|HTX_SL_F_BODYLESS)) == HTX_SL_F_CONN_UPG) ||
+	    ((h1m->flags & (H1_MF_CONN_UPG|H1_MF_UPG_H2C)) == (H1_MF_CONN_UPG|H1_MF_UPG_H2C))) {
 		int i;
 
 		for (i = 0; hdrs[i].n.len; i++) {
@@ -190,6 +196,7 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx
 		h1m->flags &=~ H1_MF_CONN_UPG;
 		flags &= ~HTX_SL_F_CONN_UPG;
 	}
+
 	sl = htx_add_stline(htx, HTX_BLK_REQ_SL, flags, meth, uri, vsn);
 	if (!sl || !htx_add_all_headers(htx, hdrs))
 		goto error;