MEDIUM: h2/hpack: emit a Dynamic Table Size Update after settings change

As reported by @jinsubsim in github issue #1498, there is an
interoperability issue between nghttp2 as a client and a few servers
among which haproxy (in fact likely all those which do not make use
of the dynamic headers table in responses or which do not intend to
use a larger table), when reducing the header table size below 4096.
These are easily testable this way:

  nghttp -v -H":method: HEAD" --header-table-size=0 https://$SITE

It will result in a compression error for those which do not start
with an HPACK dynamic table size update opcode.

There is a possible interpretation of the H2 and HPACK specs that
says that an HPACK encoder must send an HPACK headers table update
confirming the new size it will be using after having acknowledged
it, because since it's possible for a decoder to advertise a late
SETTINGS and change it after transfers have begun, the initially
advertised value might very well be seen as a first change from the
initial setting, and the HPACK spec doesn't specify the side which
causes the change that triggers a DTSU update, which was essentially
summed up in this question from nghttp2's author when this issue
was already raised 6 years ago, but which didn't really find a solid
response by then:

  https://lists.w3.org/Archives/Public/ietf-http-wg/2015OctDec/0107.html

The ongoing consensus based on what some servers are doing and that aims
at limiting interoperability issues seems to be that a DTSU is expected
for each reduction from the current size, which should be reflected in
the next revision of the H2 spec:

  https://github.com/httpwg/http2-spec/pull/1005

Given that we do not make use of this table we can emit a DTSU of zero
before encoding any HPACK frame. However, some clients do not support
receiving DTSU with such values (e.g. VTest) so we cannot do it
inconditionnally!

The current patch aims at sticking as close to the spec as possible by
proceeding this way:
  - when a SETTINGS_HEADER_TABLE_SIZE is received, a flag is set
    indicating that the value changed
  - before sending any HPACK frame, this flag is checked to see if
    an update is wanted and if none was sent
  - in this case a DTSU of size zero is emitted and a flag is set
    to mention it was emitted so that it never has to be sent again

This addresses the problem with nghttp2 without affecting VTest.

More context is available here:
  https://github.com/nghttp2/nghttp2/issues/1660
  https://lists.w3.org/Archives/Public/ietf-http-wg/2021OctDec/0235.html

Many thanks to @jinsubsim for this report and participating to the issue
that led to an improvement of the H2 spec.

This should be backported to stable releases in a timely manner, ideally
as far as 2.4 once the h2spec update is merged, then to other versions
after a few months of observation or in case an issue around this is
reported.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index ddc19c176958ede15a17ae49456d10ae81945e3a..3b055b44e713f5cd72fedbe531d665db5b74296e 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -73,6 +73,8 @@ static const struct h2s *h2_idle_stream;
 #define H2_CF_END_REACHED       0x00040000  // pending data too short with RCVD_SHUT present
 
 #define H2_CF_RCVD_RFC8441      0x00100000  // settings from RFC8441 has been received indicating support for Extended CONNECT
+#define H2_CF_SHTS_UPDATED      0x00200000  // SETTINGS_HEADER_TABLE_SIZE updated
+#define H2_CF_DTSU_EMITTED      0x00400000  // HPACK Dynamic Table Size Update opcode emitted
 
 /* H2 connection state, in h2c->st0 */
 enum h2_cs {
@@ -2232,6 +2234,9 @@ static int h2c_handle_settings(struct h2c *h2c)
                        }
                        h2c->mfs = arg;
                        break;
+               case H2_SETTINGS_HEADER_TABLE_SIZE:
+                       h2c->flags |= H2_CF_SHTS_UPDATED;
+                       break;
                case H2_SETTINGS_ENABLE_PUSH:
                        if (arg < 0 || arg > 1) { // RFC7540#6.5.2
                                TRACE_ERROR("ENABLE_PUSH out of range", H2_EV_RX_FRAME|H2_EV_RX_SETTINGS, h2c->conn);
@@ -5147,6 +5152,26 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, struct htx *htx)
        write_n32(outbuf.area + 5, h2s->id); // 4 bytes
        outbuf.data = 9;
 
+       if ((h2c->flags & (H2_CF_SHTS_UPDATED|H2_CF_DTSU_EMITTED)) == H2_CF_SHTS_UPDATED) {
+               /* SETTINGS_HEADER_TABLE_SIZE changed, we must send an HPACK
+                * dynamic table size update so that some clients are not
+                * confused. In practice we only need to send the DTSU when the
+                * advertised size is lower than the current one, and since we
+                * don't use it and don't care about the default 4096 bytes,
+                * we only ack it with a zero size thus we at most have to deal
+                * with this once. See RFC7541#4.2 and #6.3 for the spec, and
+                * below for the whole context and interoperability risks:
+                * https://lists.w3.org/Archives/Public/ietf-http-wg/2021OctDec/0235.html
+                */
+               if (b_room(&outbuf) < 1)
+                       goto full;
+               outbuf.area[outbuf.data++] = 0x20; // HPACK DTSU 0 bytes
+
+               /* let's not update the flags now but only once the buffer is
+                * really committed.
+                */
+       }
+
        /* encode status, which necessarily is the first one */
        if (!hpack_encode_int_status(&outbuf, h2s->status)) {
                if (b_space_wraps(mbuf))
@@ -5231,6 +5256,12 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, struct htx *htx)
        if (h2s->status >= 200)
                h2s->flags |= H2_SF_HEADERS_SENT;
 
+       if (h2c->flags & H2_CF_SHTS_UPDATED) {
+               /* was sent above */
+               h2c->flags |= H2_CF_DTSU_EMITTED;
+               h2c->flags &= H2_CF_SHTS_UPDATED;
+       }
+
        if (es_now) {
                h2s->flags |= H2_SF_ES_SENT;
                TRACE_PROTO("setting ES on HEADERS frame", H2_EV_TX_FRAME|H2_EV_TX_HDR, h2c->conn, h2s, htx);