Skip to content

Commit

Permalink
BUG/MINOR: h1: do not forward h2c upgrade header token
Browse files Browse the repository at this point in the history
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 9bf9573
  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.
  • Loading branch information
a-denoyelle committed Aug 1, 2024
1 parent a7a2db4 commit 7b89aa5
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 2 deletions.
1 change: 1 addition & 0 deletions include/haproxy/h1.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
27 changes: 27 additions & 0 deletions reg-tests/http-messaging/protocol_upgrade.vtc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
5 changes: 4 additions & 1 deletion src/h1.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down
9 changes: 8 additions & 1 deletion src/h1_htx.c
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand All @@ -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;
Expand Down

0 comments on commit 7b89aa5

Please sign in to comment.