diff --git a/changelogs/current.yaml b/changelogs/current.yaml index c8c73eaf0c90..fece25e236d3 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -5,6 +5,10 @@ behavior_changes: minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* +- area: quic + change: | + Enable QUICHE request and response headers validation. This behavior can be reverted by setting runtime flag + ``envoy.reloadable_features.FLAGS_envoy_quic_reloadable_flag_quic_act_upon_invalid_header`` to false. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 63439c1c4fa3..c383abc53eb6 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -222,6 +222,10 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, return; } quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list); + if (read_side_closed()) { + return; + } + if (!headers_decompressed() || header_list.empty()) { onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value(), quic::QUIC_BAD_APPLICATION_PAYLOAD); @@ -494,5 +498,9 @@ void EnvoyQuicClientStream::useCapsuleProtocol() { } #endif +void EnvoyQuicClientStream::OnInvalidHeaders() { + onStreamError(absl::nullopt, quic::QUIC_BAD_APPLICATION_PAYLOAD); +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index 7c40341b422f..e8a6272d07e1 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -64,6 +64,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, const quic::QuicHeaderList& header_list) override; void OnTrailingHeadersComplete(bool fin, size_t frame_len, const quic::QuicHeaderList& header_list) override; + void OnInvalidHeaders() override; // Http::MultiplexedStreamImplBase bool hasPendingData() override; diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 003ff29c5b77..aa5c8d57c587 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -222,6 +222,10 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len, return; } quic::QuicSpdyServerStreamBase::OnInitialHeadersComplete(fin, frame_len, header_list); + if (read_side_closed()) { + return; + } + if (!headers_decompressed() || header_list.empty()) { onStreamError(absl::nullopt); return; @@ -569,5 +573,7 @@ void EnvoyQuicServerStream::useCapsuleProtocol() { } #endif +void EnvoyQuicServerStream::OnInvalidHeaders() { onStreamError(absl::nullopt); } + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index ea9b4bf07ad8..35cef92dfe22 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -91,6 +91,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, void OnTrailingHeadersComplete(bool fin, size_t frame_len, const quic::QuicHeaderList& header_list) override; void OnHeadersTooLarge() override; + void OnInvalidHeaders() override; // Http::MultiplexedStreamImplBase void onPendingFlushTimer() override; diff --git a/source/common/quic/platform/quiche_flags_impl.cc b/source/common/quic/platform/quiche_flags_impl.cc index 38e105c545f0..187ce518720d 100644 --- a/source/common/quic/platform/quiche_flags_impl.cc +++ b/source/common/quic/platform/quiche_flags_impl.cc @@ -38,10 +38,6 @@ template <> constexpr bool maybeOverride(absl::string_view name, bool val) // Do not include 32-byte per-entry overhead while counting header size. return false; } - if (name == "quic_reloadable_flag_quic_act_upon_invalid_header") { - // This flag changes quiche behavior that's incompatible with current Envoy. - return false; - } return val; } diff --git a/test/common/quic/envoy_quic_server_session_test.cc b/test/common/quic/envoy_quic_server_session_test.cc index 9d2fa1762d34..7fc4a9aa7d8a 100644 --- a/test/common/quic/envoy_quic_server_session_test.cc +++ b/test/common/quic/envoy_quic_server_session_test.cc @@ -313,6 +313,7 @@ TEST_F(EnvoyQuicServerSessionTest, NewStream) { headers.OnHeader(":authority", host); headers.OnHeader(":method", "GET"); headers.OnHeader(":path", "/"); + headers.OnHeader(":scheme", "https"); headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0, /*compressed_header_bytes=*/0); // Request headers should be propagated to decoder. EXPECT_CALL(request_decoder, decodeHeaders_(_, /*end_stream=*/true)) @@ -548,6 +549,7 @@ TEST_F(EnvoyQuicServerSessionTest, WriteUpdatesDelayCloseTimer) { request_headers.OnHeader(":authority", host); request_headers.OnHeader(":method", "GET"); request_headers.OnHeader(":path", "/"); + request_headers.OnHeader(":scheme", "https"); request_headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0, /*compressed_header_bytes=*/0); // Request headers should be propagated to decoder. EXPECT_CALL(request_decoder, decodeHeaders_(_, /*end_stream=*/true)) @@ -647,6 +649,7 @@ TEST_F(EnvoyQuicServerSessionTest, FlushCloseNoTimeout) { std::string host("www.abc.com"); request_headers.OnHeader(":authority", host); request_headers.OnHeader(":method", "GET"); + request_headers.OnHeader(":scheme", "https"); request_headers.OnHeader(":path", "/"); request_headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0, /*compressed_header_bytes=*/0); // Request headers should be propagated to decoder. @@ -897,6 +900,7 @@ TEST_F(EnvoyQuicServerSessionTest, SendBufferWatermark) { request_headers.OnHeader(":authority", host); request_headers.OnHeader(":method", "GET"); request_headers.OnHeader(":path", "/"); + request_headers.OnHeader(":scheme", "https"); request_headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0, /*compressed_header_bytes=*/0); // Request headers should be propagated to decoder. EXPECT_CALL(request_decoder, decodeHeaders_(_, /*end_stream=*/true)) diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index d56e63ad0f76..5cdb1d9bb3a8 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -264,6 +264,7 @@ TEST_F(EnvoyQuicServerStreamTest, GetRequestAndResponse) { spdy_headers[":authority"] = host_; spdy_headers[":method"] = "GET"; spdy_headers[":path"] = "/"; + spdy_headers[":scheme"] = "https"; spdy_headers.AppendValueOrAddHeader("cookie", "a=b"); spdy_headers.AppendValueOrAddHeader("cookie", "c=d"); std::string payload = spdyHeaderToHttp3StreamPayload(spdy_headers); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index b4dbf27d6961..e81e9c66cbe1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -2101,6 +2101,7 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLength) { codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, {":path", "/test/long/url"}, {":authority", "sni.lyft.com"}, + {":scheme", "http"}, {"content-length", "-1"}}); auto response = std::move(encoder_decoder.second); @@ -2154,6 +2155,7 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, {":path", "/test/long/url"}, {":authority", "sni.lyft.com"}, + {":scheme", "http"}, {"content-length", "3,2"}}); auto response = std::move(encoder_decoder.second); @@ -2178,6 +2180,7 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, {":path", "/test/long/url"}, {":authority", "sni.lyft.com"}, + {":scheme", "http"}, {"content-length", "3,2"}}); auto response = std::move(encoder_decoder.second); diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 5597b44ef10f..11fb588cde26 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -914,10 +914,9 @@ TEST_P(QuicHttpIntegrationTest, ResetRequestWithoutAuthorityHeader) { request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); - ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->waitForReset()); + ASSERT_FALSE(response->complete()); codec_client_->close(); - ASSERT_TRUE(response->complete()); - EXPECT_EQ("400", response->headers().getStatusValue()); } TEST_P(QuicHttpIntegrationTest, ResetRequestWithInvalidCharacter) { @@ -1280,6 +1279,42 @@ TEST_P(QuicHttpIntegrationTest, DeferredLoggingWithQuicReset) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); + // omit required authority header to invoke EnvoyQuicServerStream::onStreamError + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, {":path", "/dynamo/url"}, {":scheme", "http"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(response->waitForReset()); + EXPECT_FALSE(response->complete()); + + std::string log = waitForAccessLog(access_log_name_); + std::vector metrics = absl::StrSplit(log, ','); + ASSERT_EQ(metrics.size(), 21); + EXPECT_EQ(/* PROTOCOL */ metrics.at(0), "HTTP/3"); + EXPECT_EQ(/* ROUNDTRIP_DURATION */ metrics.at(1), "-"); + EXPECT_EQ(/* REQUEST_DURATION */ metrics.at(2), "-"); + EXPECT_EQ(/* RESPONSE_DURATION */ metrics.at(3), "-"); + EXPECT_EQ(/* RESPONSE_CODE */ metrics.at(4), "0"); + EXPECT_EQ(/* BYTES_RECEIVED */ metrics.at(5), "0"); + EXPECT_EQ(/* request headers */ metrics.at(19), metrics.at(20)); +} + +TEST_P(QuicHttpIntegrationTest, DeferredLoggingWithEnvoyReset) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.FLAGS_envoy_quic_reloadable_flag_quic_act_upon_invalid_header", + "false"); + + useAccessLog( + "%PROTOCOL%,%ROUNDTRIP_DURATION%,%REQUEST_DURATION%,%RESPONSE_DURATION%,%RESPONSE_" + "CODE%,%BYTES_RECEIVED%,%ROUTE_NAME%,%VIRTUAL_CLUSTER_NAME%,%RESPONSE_CODE_DETAILS%,%" + "CONNECTION_TERMINATION_DETAILS%,%START_TIME%,%UPSTREAM_HOST%,%DURATION%,%BYTES_SENT%,%" + "RESPONSE_FLAGS%,%DOWNSTREAM_LOCAL_ADDRESS%,%UPSTREAM_CLUSTER%,%STREAM_ID%,%DYNAMIC_" + "METADATA(" + "udp.proxy.session:bytes_sent)%,%REQ(:path)%,%STREAM_INFO_REQ(:path)%"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + // omit required authority header to invoke EnvoyQuicServerStream::resetStream auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ {":method", "GET"}, {":path", "/dynamo/url"}, {":scheme", "http"}});