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.
(cherry picked from commit 39a0a1e12097a80d6707363792aee5caaa254f28)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 97c8baac9a8a065679cf8d039fc8f66252952c44)
Signed-off-by: Willy Tarreau <w@1wt.eu>
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 3bbaabe..58a5049 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -72,6 +72,8 @@
#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 {
@@ -2231,6 +2233,9 @@
}
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);
@@ -5146,6 +5151,26 @@
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))
@@ -5230,6 +5255,12 @@
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);