From b5d2b9e154d78e4075db163826c5e0f6d31b2ab1 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Wed, 11 Aug 2021 15:39:13 +0200
Subject: [PATCH] BUG/MEDIUM: h2: give :authority precedence over Host
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

The wording regarding Host vs :authority in RFC7540 is ambiguous as it
says that an intermediary must produce a host header from :authority if
Host is missing, but, contrary to HTTP/1.1, doesn't say anything regarding
the possibility that Host and :authority differ, which leaves Host with
higher precedence there. In addition it mentions that clients should use
:authority *instead* of Host, and that H1->H2 should use :authority only
if the original request was in authority form. This leaves some gray
area in the middle of the chain for fully valid H2 requests arboring a
Host header that are forwarded to the other side where it's possible to
drop the Host header and use the authority only after forwarding to a
second H2 layer, thus possibly seeing two different values of Host at
a different stage. There's no such issue when forwarding from H2 to H1
as the authority is dropped only only the Host is kept.

Note that the following request is sufficient to re-normalize such a
request:

   http-request set-header host %[req.hdr(host)]

The new spec in progress (draft-ietf-httpbis-http2bis-03) addresses
this trouble by being a bit is stricter on these rules. It clarifies
that :authority must always be used instead of Host and that Host ought
to be ignored. This is much saner as it avoids to convey two distinct
values along the chain. This becomes the protocol-level equivalent of:

   http-request set-uri %[url]

So this patch does exactly this, which we were initially a bit reluctant
to do initially by lack of visibility about other implementations'
expectations. In addition it slightly simplifies the Host header field
creation by always placing it first in the list of headers instead of
last; this could also speed up the look up a little bit.

This needs to be backported to 2.0. Non-HTX versions are safe regarding
this because they drop the URI during the conversion to HTTP/1.1 so
only Host is used and transmitted.

Thanks to Tim Düsterhus for reporting that one.
---
 src/h2.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/h2.c b/src/h2.c
index 280bbd7..bbc5853 100644
--- a/src/h2.c
+++ b/src/h2.c
@@ -456,10 +456,27 @@ int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *ms
 			if (!sl)
 				goto fail;
 			fields |= H2_PHDR_FND_NONE;
+
+			/* http2bis draft recommends to drop Host in favor of :authority when
+			 * the latter is present. This is required to make sure there is no
+			 * discrepancy between the authority and the host header, especially
+			 * since routing rules usually involve Host. Here we already know if
+			 * :authority was found so we can emit it right now and mark the host
+			 * as filled so that it's skipped later.
+			 */
+			if (fields & H2_PHDR_FND_AUTH) {
+				if (!htx_add_header(htx, ist("host"), phdr_val[H2_PHDR_IDX_AUTH]))
+					goto fail;
+				fields |= H2_PHDR_FND_HOST;
+			}
 		}
 
-		if (isteq(list[idx].n, ist("host")))
+		if (isteq(list[idx].n, ist("host"))) {
+			if (fields & H2_PHDR_FND_HOST)
+				continue;
+
 			fields |= H2_PHDR_FND_HOST;
+		}
 
 		if (isteq(list[idx].n, ist("content-length"))) {
 			ret = h2_parse_cont_len_header(msgf, &list[idx].v, body_len);
@@ -531,7 +548,9 @@ int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *ms
 	/* update the start line with last detected header info */
 	sl->flags |= sl_flags;
 
-	/* complete with missing Host if needed */
+	/* complete with missing Host if needed (we may validate this test if
+	 * no regular header was found).
+	 */
 	if ((fields & (H2_PHDR_FND_HOST|H2_PHDR_FND_AUTH)) == H2_PHDR_FND_AUTH) {
 		/* missing Host field, use :authority instead */
 		if (!htx_add_header(htx, ist("host"), phdr_val[H2_PHDR_IDX_AUTH]))
-- 
1.7.10.4