From 0ddba58243795b6f0a21056a1f296ddd795329d1 Mon Sep 17 00:00:00 2001 From: David Papp Date: Tue, 29 Sep 2026 10:14:49 +0200 Subject: [PATCH] fix: the header-list bound is a limit, so it belongs in the reuse hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups to #35, which merged before they were addressed. The doc contract was wrong in the case the setting exists for. It said h2 "allows a multiple of this value before failing the stream outright"; measured on this branch with a bound of 8KiB, 2x over drops the headers and errors the stream with the connection still usable, but 32x over raises a *connection* error — ENHANCE_YOUR_CALM, `header_list_way_too_large` — and the connection is finished. On a pooled, multiplexed upstream that fails every other request in flight on it. That blast radius is the trade the setting makes and it was neither documented nor tested; now it is both, including a case for the stream-local band the old comment described and nothing covered. `h2_max_header_list_size` now takes part in `reuse_hash`. Excluding it alongside the window sizes was wrong: those are tuning parameters, this is a limit, and sharing a pool across different values breaks it in both directions. A peer that asked for a bound gets none when an unbounded peer established the connection, and a peer that asked for none inherits a sibling's cap — which, past the abuse multiplier, takes the shared connection down with it. A limit that applies or not depending on who connected first is not a limit. `Some(0)` is rejected at handshake, as the window sizes already are. It does not mean "no bound": it makes the abuse limit zero too, so the first response on every connection to that peer is a connection error, with nothing pointing at the misconfiguration. The test server now `expect`s its send instead of swallowing the error. If a future h2 enforced the peer's advertised limit on send, the server would quietly send nothing and the oversize test would still pass on the resulting error — testing nothing. --- pingora-core/src/connectors/http/v2.rs | 91 ++++++++++++++++++++++++-- pingora-core/src/upstreams/peer.rs | 43 ++++++++++-- 2 files changed, 124 insertions(+), 10 deletions(-) diff --git a/pingora-core/src/connectors/http/v2.rs b/pingora-core/src/connectors/http/v2.rs index 5f5f940b3..a8bd8597c 100644 --- a/pingora-core/src/connectors/http/v2.rs +++ b/pingora-core/src/connectors/http/v2.rs @@ -570,9 +570,19 @@ pub struct H2HandshakeSettings { /// If `None`, h2's default of 16MB applies. The bound is enforced on /// receive, not merely advertised: h2 applies it to its own decoder once /// the peer ACKs the settings, and it also derives h2's CONTINUATION - /// frame budget. Note that h2 allows a multiple of this value before - /// failing the stream outright, so it is a bound on the order of - /// magnitude rather than an exact ceiling. + /// frame budget. + /// + /// **How a violation fails depends on how far over it is, and the second + /// case is not stream-local.** Up to h2's abuse multiplier (4x) the + /// headers are dropped and the *stream* errors, leaving the connection + /// usable. Past that, h2 raises a *connection* error + /// (`ENHANCE_YOUR_CALM`, `header_list_way_too_large`) and the connection + /// is finished — so on a pooled, multiplexed upstream one hostile + /// response also fails every other request in flight on that connection. + /// + /// That is the trade this setting makes: a bounded blast radius on a + /// shared connection, instead of an unbounded header list. Set it with + /// that in mind rather than as a tight fit around expected traffic. pub max_header_list_size: Option, } @@ -622,6 +632,16 @@ pub async fn handshake(stream: Stream, settings: H2HandshakeSettings) -> Result< ); } + // Zero is not "no bound": it makes the abuse limit zero too, so the very + // first response on every connection to this peer is a connection error. + // Rejected here for the same reason the window sizes are. + if settings.max_header_list_size == Some(0) { + return Error::e_explain( + H2Error, + "max_header_list_size must be greater than 0; omit it for no bound", + ); + } + let id = stream.id(); let digest = Digest { // NOTE: this field is always false because the digest is shared across all streams @@ -1100,9 +1120,14 @@ mod tests { .header("x-fat", "a".repeat(header_bytes)) .body(()) .unwrap(); - if let Ok(mut stream) = respond.send_response(resp, false) { - let _ = stream.send_data(Bytes::from("ok"), true); - } + // Not `if let Ok(..)`: if a future h2 enforced the peer's + // advertised limit on send, the server would quietly send + // nothing and the oversize test would still see an error and + // pass, testing nothing. + let mut stream = respond + .send_response(resp, false) + .expect("the test server must be able to send the oversized response"); + let _ = stream.send_data(Bytes::from("ok"), true); } while let Some(_res) = server_conn.accept().await {} } @@ -1177,6 +1202,60 @@ mod tests { assert_eq!(stream.response_header().unwrap().status, 200); } + /// Within h2's abuse multiplier the failure is stream-local: the + /// headers are dropped and the stream errors, but the connection keeps + /// working. Pins the first half of the contract the doc comment states — + /// the other tests only ever drive the connection-error path, so without + /// this the documented distinction is untested. + #[tokio::test] + async fn test_h2_upstream_a_modest_overshoot_does_not_kill_the_connection() { + let mut settings = H2HandshakeSettings::new(); + settings.max_streams = 100; + settings.max_header_list_size = Some(8 * 1024); + + let (client, server) = tokio::io::duplex(1024 * 1024); + // 2x the bound: over it, but inside the 4x abuse multiplier. + tokio::spawn(serve_one_fat_response(server, 16 * 1024)); + + let conn = handshake(Box::new(client), settings).await.unwrap(); + let mut stream = conn.spawn_stream().await.unwrap().unwrap(); + let mut request = RequestHeader::build("GET", b"/", None).unwrap(); + request + .insert_header(http::header::HOST, "example.com") + .unwrap(); + stream + .write_request_header(Box::new(request), true) + .unwrap(); + + assert!( + stream.read_response_header().await.is_err(), + "the oversized response must still be refused" + ); + assert!( + conn.spawn_stream().await.is_ok(), + "a modest overshoot must not take the shared connection down with it" + ); + } + + /// Zero is not "no bound" — it would make h2's abuse limit zero too, so + /// the first response on every connection to the peer becomes a + /// connection error. Rejected at handshake, like the window sizes. + #[tokio::test] + async fn test_h2_handshake_rejects_a_zero_header_list_size() { + let mut settings = H2HandshakeSettings::new(); + settings.max_streams = 100; + settings.max_header_list_size = Some(0); + let (client, _server) = tokio::io::duplex(65536); + match handshake(Box::new(client), settings).await { + Err(e) => assert!( + e.to_string() + .contains("max_header_list_size must be greater"), + "unexpected error: {e}" + ), + Ok(_) => panic!("expected max_header_list_size = 0 to be rejected"), + } + } + /// `spawn_stream()` must return `Ok(None)` when the server sends /// GOAWAY(NO_ERROR) before any streams are opened. #[tokio::test] diff --git a/pingora-core/src/upstreams/peer.rs b/pingora-core/src/upstreams/peer.rs index 708b14a01..da9cabc31 100644 --- a/pingora-core/src/upstreams/peer.rs +++ b/pingora-core/src/upstreams/peer.rs @@ -533,10 +533,9 @@ pub struct PeerOptions { /// advertised as SETTINGS_MAX_HEADER_LIST_SIZE. `None` leaves h2's 16MB /// default in place. /// - /// Like the window sizes, this is deliberately excluded from - /// `reuse_hash`: it is a connection-level setting, so two peers that - /// differ only here share a pooled connection carrying whichever value - /// established it. + /// Part of `reuse_hash`, unlike the window sizes: peers with different + /// bounds must not share a pooled connection, or the bound would apply + /// or not depending on which peer happened to establish it. pub h2_max_header_list_size: Option, /// Allow a single invalid Content-Length in HTTP/1 responses (non-RFC compliant). /// @@ -815,6 +814,17 @@ impl Hash for HttpPeer { // from the reuse hash for now. These are per-connection settings applied at handshake // time and may be revisited alongside other h2 settings that could be dynamically // adjusted over the lifetime of a connection. + // + // h2_max_header_list_size is NOT excluded, though it is a + // per-connection handshake setting like those two. It is a limit + // rather than a tuning parameter, and sharing a pool across + // different values breaks it in both directions: a peer that asked + // for a bound silently gets none when an unbounded peer established + // the connection, and a peer that asked for none inherits a + // sibling's cap — which, past h2's abuse multiplier, kills the + // shared connection for everyone on it. A limit that applies or not + // depending on who connected first is not a limit. + self.options.h2_max_header_list_size.hash(state); self.options.curves.hash(state); self.options.second_keyshare.hash(state); } @@ -918,6 +928,31 @@ impl Display for Proxy { #[cfg(test)] mod tests { + + /// A bound that applies or not depending on which peer happened to + /// establish the pooled connection is not a bound. Peers differing only + /// in `h2_max_header_list_size` must not share one. + #[test] + fn peers_with_different_header_list_bounds_do_not_share_a_connection() { + let mut bounded = HttpPeer::new("1.1.1.1:443", true, "one.one.one.one".into()); + bounded.options.h2_max_header_list_size = Some(64 * 1024); + let mut unbounded = HttpPeer::new("1.1.1.1:443", true, "one.one.one.one".into()); + unbounded.options.h2_max_header_list_size = None; + + assert_ne!( + bounded.reuse_hash(), + unbounded.reuse_hash(), + "a bounded peer must not reuse a connection established without the bound" + ); + + let mut same = HttpPeer::new("1.1.1.1:443", true, "one.one.one.one".into()); + same.options.h2_max_header_list_size = Some(64 * 1024); + assert_eq!( + bounded.reuse_hash(), + same.reuse_hash(), + "identical peers must still pool together" + ); + } use super::*; #[test]