MEDIUM: mux-h1: Return an error on h2 upgrade attempts if not allowed

If h1 to h2 upgrades are not allowed, a 405-method-not-allowed error is now
returned from the H1 multiplexer itself instead of dealing with "PRI *
HTTP/2.0\r\n\r\n" request as a normal request.

Before this kind of request was caught by the HTTP analyzers and a
400-bad-request was returned. This was added before the multiplexers era to
protect backend apps against unexpected H1 to H2 uprade on server side.

Now, it is possible to handle the error in the H1 multiplexer. One benefit
is to be able to increment the glichtes counters. However, the error is
still handled in HTTP analyzers to be sure to detect unwanted upgrades that
can be hidden in H2 or H3 requests.

There is a special case. TCP > H1 > H2 upgrades. In that case, a H1 stream
exist. So we must report an error to the upper layer too.

A reg-test script was added to validate the feature. In addition,
tcp_to_http_upgrade.vtc was updated accordingly.

diff --git a/reg-tests/connection/tcp_to_http_upgrade.vtc b/reg-tests/connection/tcp_to_http_upgrade.vtc
index 304bf5f3e4a56b857e980e105d80e2be8a59e8f1..eec1a1083a9ef15d2e0b3df7018ab43c1456f6a7 100644 (file)
--- a/reg-tests/connection/tcp_to_http_upgrade.vtc
+++ b/reg-tests/connection/tcp_to_http_upgrade.vtc
@@ -145,7 +145,7 @@ client c3 -connect ${h1_li1h1_sock} {
 client c_err1 -connect ${h1_err1h1_sock} {
     send "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"
     rxresp
-    expect resp.status == 400
+    expect resp.status == 405
 } -run
 
 
@@ -153,7 +153,7 @@ client c_err1 -connect ${h1_err1h1_sock} {
 client c_err2 -connect ${h1_err2h1_sock} {
     send "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"
     rxresp
-    expect resp.status == 400
+    expect resp.status == 405
 } -run
 
 

diff --git a/reg-tests/http-messaging/h1_to_h2_upgrade.vtc b/reg-tests/http-messaging/h1_to_h2_upgrade.vtc
new file mode 100644 (file)
index 0000000..30f668f
--- /dev/null
+++ b/reg-tests/http-messaging/h1_to_h2_upgrade.vtc
@@ -0,0 +1,73 @@
+varnishtest "Tests of H1->H2 upgrades"
+
+feature ignore_unknown_macro
+
+server s1 {
+       rxreq
+       txresp \
+         -status 200 \
+} -start
+
+haproxy h1 -conf {
+    global
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+
+        # WT: limit false-positives causing "HTTP header incomplete" due to
+        # idle server connections being randomly used and randomly expiring
+        # under us.
+        tune.idle-pool.shared off
+
+    defaults
+       #log stdout format raw daemon
+       mode http
+       option http-buffer-request
+       timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+       timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+       timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    listen feh1
+       bind "fd@${feh1}"
+       bind "fd@${feh2}" proto h1
+       server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_feh1_sock} {
+       txpri
+       stream 0 {
+               txsettings
+               rxsettings
+               txsettings -ack
+               rxsettings
+               expect settings.ack == true
+       } -run
+
+       # first request is valid
+       stream 1 {
+               txreq \
+                 -req "GET" \
+                 -scheme "https" \
+                 -url "/"
+               rxresp
+               expect resp.status == 200
+       } -run
+
+       # Second request. hide something similar to a H2 preface (PRI method). it is invalid
+       stream 3 {
+               txreq \
+                 -req "PRI" \
+                 -scheme "https" \
+                 -url "*" \
+                 -body "SM\r\n\r\n"
+               rxresp
+               expect resp.status == 400
+       } -run
+
+} -run
+
+client c2 -connect ${h1_feh2_sock} {
+        send "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"
+        rxresp
+        expect resp.status == 405
+} -run

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 3e284aede2a02aaf94c0a80b3399a8894e105fc1..4a367210bae5719ce9967ac2f6603433b124eaa5 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -4179,13 +4179,15 @@ static int h1_process(struct h1c * h1c)
                if (h1c->flags & H1C_F_IS_BACK)
                        goto release;
 
-               /* First of all handle H1 to H2 upgrade (no need to create the H1 stream) */
-               if (h1c->state != H1_CS_DRAINING &&                /* Not draining message */
-                   !(h1c->flags & H1C_F_WAIT_NEXT_REQ) &&         /* First request */
-                   !(h1c->px->options2 & PR_O2_NO_H2_UPGRADE) &&  /* H2 upgrade supported by the proxy */
-                   !(conn->mux->flags & MX_FL_NO_UPG)) {          /* the current mux supports upgrades */
-                       /* Try to match H2 preface before parsing the request headers. */
-                       if (b_isteq(&h1c->ibuf, 0, b_data(&h1c->ibuf), ist(H2_CONN_PREFACE)) > 0) {
+               /* First of all handle H2 preface (except for DRAINING mode).
+                * If H1 to H2 upgrade is allowed, no need to create the H1
+                * stream. If not allowed, a 405 must be returned.
+                */
+               if (h1c->state != H1_CS_DRAINING &&
+                   b_isteq(&h1c->ibuf, 0, b_data(&h1c->ibuf), ist(H2_CONN_PREFACE)) > 0) {
+                       if (!(h1c->flags & H1C_F_WAIT_NEXT_REQ) &&         /* First request */
+                           !(h1c->px->options2 & PR_O2_NO_H2_UPGRADE) &&  /* H2 upgrade supported by the proxy */
+                           !(conn->mux->flags & MX_FL_NO_UPG)) {          /* the current mux supports upgrades */
                                h1c->flags |= H1C_F_UPG_H2C;
                                if (h1c->state == H1_CS_UPGRADING) {
                                        BUG_ON(!h1s);
@@ -4194,6 +4196,19 @@ static int h1_process(struct h1c * h1c)
                                TRACE_STATE("release h1c to perform H2 upgrade ", H1_EV_RX_DATA|H1_EV_H1C_WAKE);
                                goto release;
                        }
+                       else {
+                               if (h1c->state == H1_CS_UPGRADING) {
+                                       /* In case of TCP > H1 > H2 upgrade, h1s exist, so report an error to the SD */
+                                       BUG_ON(!h1s);
+                                       h1s->flags |= H1S_F_PARSING_ERROR;
+                                       se_fl_set(h1s->sd, SE_FL_ERROR); /* Set EOS here to release the SC */
+                               }
+                               h1c->errcode = 405;
+                               TRACE_ERROR("H2 update not allowed", H1_EV_H1C_WAKE|H1_EV_H1C_ERR);
+                               h1_report_glitch(h1c, 1, "H2 update not allowed");
+                               h1_handle_parsing_error(h1c);
+                               goto no_parsing;
+                       }
                }
 
                /* Create the H1 stream if not already there */