envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.67k stars 4.75k forks source link

QUIC rejects streams with `Host` header #35415

Closed ggreenway closed 3 days ago

ggreenway commented 1 month ago

QUIC streams with Host request header are rejected as invalid

[2024-07-24 15:31:45.998][12][error][quic] [com_github_google_quiche/quiche/quic/core/http/quic_spdy_stream.cc:1789] host header is not allowed

Envoy allows (but discards) the Host request header for http/2 (see #30005). This should be consistent between h2 and h3.

The failure happens in quiche, which has a hard-coded list of invalid headers checked in ValidateReceivedHeaders. The list of invalid headers is here.

Test case that currently fails and results in the above log message:

diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc
index f8f02758f9..b3729c9707 100644
--- a/test/common/quic/envoy_quic_server_stream_test.cc
+++ b/test/common/quic/envoy_quic_server_stream_test.cc
@@ -274,6 +274,27 @@ TEST_F(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
   quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/true);
 }

+TEST_F(EnvoyQuicServerStreamTest, HostHeaderAccepted) {
+  EXPECT_CALL(stream_decoder_, decodeHeaders_(_, /*end_stream=*/false))
+      .WillOnce(Invoke([this](const Http::RequestHeaderMapSharedPtr& headers, bool) {
+        EXPECT_EQ(host_, headers->getHostValue());
+        EXPECT_EQ("/", headers->getPathValue());
+        EXPECT_EQ(Http::Headers::get().MethodValues.Get, headers->getMethodValue());
+      }));
+  EXPECT_CALL(stream_decoder_, decodeData(BufferStringEqual(""), /*end_stream=*/true));
+  spdy::Http2HeaderBlock spdy_headers;
+  spdy_headers[":authority"] = host_;
+  spdy_headers[":method"] = "GET";
+  spdy_headers[":path"] = "/";
+  spdy_headers[":scheme"] = "https";
+  spdy_headers["host"] = host_;
+  std::string payload = spdyHeaderToHttp3StreamPayload(spdy_headers);
+  quic::QuicStreamFrame frame(stream_id_, true, 0, payload);
+  quic_stream_->OnStreamFrame(frame);
+  EXPECT_TRUE(quic_stream_->FinishedReadingHeaders());
+  quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/true);
+}
+
 TEST_F(EnvoyQuicServerStreamTest, PostRequestAndResponse) {
   EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions());
   receiveRequest(request_body_, true, request_body_.size() * 2);
yanavlasov commented 1 month ago

This is incorrect and inconsistent with the standard: https://datatracker.ietf.org/doc/html/rfc9113#name-request-pseudo-header-field

@danzh2010 @RyanTheOptimist for more comments

ggreenway commented 1 month ago

Some discussion on slack: https://envoyproxy.slack.com/archives/C9UGU858E/p1721771729606489

github-actions[bot] commented 2 weeks ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

ggreenway commented 2 weeks ago

Not stale; there is a forthcoming QUICHE change to address this

ggreenway commented 1 week ago

QUICHE change with fix: https://github.com/google/quiche/commit/c8f5996fc2b0b2bf1f1a59fa5276e51884e3d458

RyanTheOptimist commented 4 days ago

FYI, that QUICHE change is now in envoy as of #35951.

ggreenway commented 3 days ago

Thanks!