Improper HTTP/2 enforcement of a reduced session window size

[bneradt]

While adding test coverage for #9085, I noticed that we have issues handling multiplexed HTTP/2 DATA frames on the client side as we handle translating the transactions toward the origin. I forced the client to send DATA continuation frames by reducing the stream window to 10 bytes. The client (Proxy Verifier) multiplexes three transactions per this window. The client and ATS handle this fine. Toward the origin, I notice that ATS sends the first request headers and the first 10 byte body as a chunked body. This 10 byte body corresponds to the first DATA frame content. The problem is that ATS does not continue to stream this first request until chunk termination. Instead, ATS then sends the headers for the second request. This is, naturally, not acceptable for HTTP/1 since there is no concept of multiplexing like that for HTTP/1.

I have an AuTest that reproduces this. I'll attach to this issue:

1. The AuTest.
2. The Proxy Verifier replay file used by the test to generate the HTTP traffic.
3. A tcpdump I captured while running the test. Port 2007 is client-side HTTP/2, port 2002 is the client side HTTP/1.1 traffic.
4. A TLS keylog file to decrypt the tcpdump on the client side (the origin side is plaintext HTTP/1.1).

Looking at the pcap file:

• Note that the transaction has an initial GET request for path /zero-request. Ignore this.
• Frame 55 contains the headers for the first relevant POST request. Wireshark doesn't parse it because the stream is ultimately corrupted per the bug recorded in this issue.
• Frame 57 contains the 10 byte chunk for the first request. It correctly has no chunk terminator because further bytes should be forthcoming.
• Frame 61 incorrectly contains the header bytes for the second POST request. This is malformed HTTP/1.1 as an HTTP/1.1 parser would be expecting more chunk bytes, not header bytes of another request.

tls_session_keys.txt
h2_to_h1_malformed.pcap.gz
http2_flow_control.replay.yaml.gz
http2_flow_control.test.py.gz

[maskit]

There are 4 TCP connections captured, and frame 57 and frame 61 are on different TCP connections.

0. Client to ATS (HTTP/2)
1. ATS to Origin (HTTP/1.1, which contains GET /zero-request and POST /first-request)
2. ATS to Origin (HTTP/1.1, which contains POST /second-request)
3. ATS to Origin (HTTP/1.1, which contains POST /third-request)

It seems like just none of the POST requests are completed before the TCP connections close. I don't think two (or more) requests are incorrectly mixed up on a connection. Are you saying the requests should not be processed in parallel on multiple connections?

[bneradt]

Thank you @maskit for taking the time to double check my observations. This is helpful. I did not realize that the origin-side transactions were on separate connections.

So this issue is entirely different than my previous interpretation of things. Replaying the test traffic again and reading through the logs, it turns out that the problem isn't on the origin side at all. Rather the problem has to do with how ATS handles the HTTP/2 session window calculation on the client side. The test sets the proxy.config.http2.initial_window_size_in value to 10 bytes. The ATS implementation expects this to apply to both stream and session windows. ATS correctly adjusts the stream window size for the client via a SETTINGS frame. The HTTP/2 client (implemented via nghttp2 in Proxy Verifier) correctly respects this new stream setting in all of its DATA frames and correctly awaits for WINDOW_UPDATE frames before sending more data. But the HTTP/2 protocol has no way to explicitly reduce the session window size like it does the stream window size. A receiver is simply expected to let the session window decline over time without incrementing it via WINDOW_UPDATE frames for the session. ATS, on the other hand, incorrectly enforces the reduced session window size (which the client can know nothing about) and replies with a GOAWAY frame to the client when it exceeds a 10 byte session window. That happens here:

trafficserver/proxy/http2/Http2ConnectionState.cc

Lines 158 to 162 in 48d358f

	// Check whether Window Size is acceptable
	if (this->server_rwnd() < payload_length) {
	return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_FLOW_CONTROL_ERROR,
	"recv data cstate.server_rwnd < payload_length");
	}

Locally, I modified that log message to emit the window sizes. ATS is emitting this error:

[Sep 30 16:54:35.983] [ET_NET 0] ERROR: HTTP/2 connection error code=0x03 client_ip=127.0.0.1 session_id=0 stream_id=5 recv data cstate.server_rwnd (0) < payload_length (10)

Note that ATS's internal accounting of the session window has incorrectly dropped to 10 bytes. The test only sends 4,800 bytes across all streams in the test session, so if ATS properly started the window at 65,535 bytes, this would be impossible. Looking at the code, we indiscriminately reduce the session window to the configured value here:

trafficserver/proxy/http2/Http2ConnectionState.cc

Line 1043 in 48d358f

this->_server_rwnd = Http2::initial_window_size;