From 9dbf0119c69fc2295d972059e3489a56a258bfbe Mon Sep 17 00:00:00 2001 From: Yuchen Wu Date: Mon, 14 Aug 2023 16:58:33 -0700 Subject: [PATCH] Feat: Add a feature flag to ignore connection headers According to the RFC, when encountering connection headers, H2 should treat them as protocol errors. However, in reality there are servers just setting these headers but only for informational purpose. This feature allow the receiving end to just ignore these headers without bailing the entire stream. --- Cargo.toml | 1 + src/frame/headers.rs | 19 ++++++-- tests/h2-support/Cargo.toml | 3 ++ tests/h2-tests/Cargo.toml | 3 ++ tests/h2-tests/tests/server.rs | 89 ++++++++++++++++++++++++++++------ 5 files changed, 95 insertions(+), 20 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c815ff0e7..f45d5c16d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,6 +24,7 @@ rust-version = "1.63" [features] # Enables `futures::Stream` implementations for various types. stream = [] +ignore_connection_header = [] # Enables **unstable** APIs. Any API exposed by this feature has no backwards # compatibility guarantees. In other words, you should not use this feature for diff --git a/src/frame/headers.rs b/src/frame/headers.rs index 9d5c8cefe..170ea1b8e 100644 --- a/src/frame/headers.rs +++ b/src/frame/headers.rs @@ -871,16 +871,25 @@ impl HeaderBlock { match header { Field { name, value } => { // Connection level header fields are not supported and must - // result in a protocol error. + // result in a protocol error. However in reality there are + // servers setting these headers. Some of these headers are + // purely informational and can be simply ignored without + // affecting the integrity of the stream. - if name == header::CONNECTION - || name == header::TRANSFER_ENCODING + if name == header::TRANSFER_ENCODING { + tracing::trace!("load_hpack; connection level header"); + malformed = true; + } else if name == header::CONNECTION || name == header::UPGRADE || name == "keep-alive" || name == "proxy-connection" { - tracing::trace!("load_hpack; connection level header"); - malformed = true; + if cfg!(feature = "ignore_connection_header") { + tracing::trace!("load_hpack; connection level header ignored"); + } else { + tracing::trace!("load_hpack; connection level header"); + malformed = true; + } } else if name == header::TE && value != "trailers" { tracing::trace!( "load_hpack; TE header not set to trailers; val={:?}", diff --git a/tests/h2-support/Cargo.toml b/tests/h2-support/Cargo.toml index f178178eb..a41f8e34f 100644 --- a/tests/h2-support/Cargo.toml +++ b/tests/h2-support/Cargo.toml @@ -4,6 +4,9 @@ version = "0.1.0" authors = ["Carl Lerche "] edition = "2018" +[features] +ignore_connection_header = ["h2/ignore_connection_header"] + [dependencies] h2 = { path = "../..", features = ["stream", "unstable"] } diff --git a/tests/h2-tests/Cargo.toml b/tests/h2-tests/Cargo.toml index 6afdf9053..6ab863e02 100644 --- a/tests/h2-tests/Cargo.toml +++ b/tests/h2-tests/Cargo.toml @@ -5,6 +5,9 @@ authors = ["Carl Lerche "] publish = false edition = "2018" +[features] +ignore_connection_header = ["h2-support/ignore_connection_header"] + [dependencies] [dev-dependencies] diff --git a/tests/h2-tests/tests/server.rs b/tests/h2-tests/tests/server.rs index 6075c7dcf..21cac6f40 100644 --- a/tests/h2-tests/tests/server.rs +++ b/tests/h2-tests/tests/server.rs @@ -528,25 +528,84 @@ async fn recv_connection_header() { }; let client = async move { - let settings = client.assert_server_handshake().await; - assert_default_settings!(settings); - client.send_frame(req(1, "connection", "foo")).await; - client.send_frame(req(3, "keep-alive", "5")).await; - client.send_frame(req(5, "proxy-connection", "bar")).await; - client - .send_frame(req(7, "transfer-encoding", "chunked")) - .await; - client.send_frame(req(9, "upgrade", "HTTP/2")).await; - client.recv_frame(frames::reset(1).protocol_error()).await; - client.recv_frame(frames::reset(3).protocol_error()).await; - client.recv_frame(frames::reset(5).protocol_error()).await; - client.recv_frame(frames::reset(7).protocol_error()).await; - client.recv_frame(frames::reset(9).protocol_error()).await; + if cfg!(feature = "ignore_connection_header") { + let settings = client.assert_server_handshake().await; + assert_default_settings!(settings); + client + .send_frame( + frames::headers(1) + .request("GET", "https://example.com/") + .field("connection", "foo") + .eos(), + ) + .await; + client + .send_frame( + frames::headers(3) + .request("GET", "https://example.com/") + .field("keep-alive", "5") + .eos(), + ) + .await; + client + .send_frame( + frames::headers(5) + .request("GET", "https://example.com/") + .field("proxy-connection", "bar") + .eos(), + ) + .await; + client + .send_frame( + frames::headers(7) + .request("GET", "https://example.com/") + .field("upgrade", "HTTP/2") + .eos(), + ) + .await; + client + .recv_frame(frames::headers(1).response(200).eos()) + .await; + client + .recv_frame(frames::headers(3).response(200).eos()) + .await; + client + .recv_frame(frames::headers(5).response(200).eos()) + .await; + client + .recv_frame(frames::headers(7).response(200).eos()) + .await; + } else { + let settings = client.assert_server_handshake().await; + assert_default_settings!(settings); + client.send_frame(req(1, "connection", "foo")).await; + client.send_frame(req(3, "keep-alive", "5")).await; + client.send_frame(req(5, "proxy-connection", "bar")).await; + client + .send_frame(req(7, "transfer-encoding", "chunked")) + .await; + client.send_frame(req(9, "upgrade", "HTTP/2")).await; + client.recv_frame(frames::reset(1).protocol_error()).await; + client.recv_frame(frames::reset(3).protocol_error()).await; + client.recv_frame(frames::reset(5).protocol_error()).await; + client.recv_frame(frames::reset(7).protocol_error()).await; + client.recv_frame(frames::reset(9).protocol_error()).await; + } }; let srv = async move { let mut srv = server::handshake(io).await.expect("handshake"); - assert!(srv.next().await.is_none()); + if cfg!(feature = "ignore_connection_header") { + while let Some(stream) = srv.next().await { + let (req, mut stream) = stream.unwrap(); + // the headers are ignored + assert_eq!(req.headers().len(), 0); + let rsp = http::Response::builder().status(200).body(()).unwrap(); + stream.send_response(rsp, true).unwrap(); + } + } else { + assert!(srv.next().await.is_none()); + } }; join(client, srv).await;