From 212b2dbf7db946861dc6ff00a51bdb921d603bd2 Mon Sep 17 00:00:00 2001 From: Nathanael Demacon Date: Mon, 12 Feb 2024 19:44:20 +0100 Subject: [PATCH] http: keep `trailers` TE header instead of removing it (#32255) Official grpc library does not allow empty TE header. This causes an issue when proxying requests through Envoy which currently deletes the TE header since #30535. I opened a related MR on the grpc library to allow this header to be empty as the HTTP/2 RFC allows it but it might cause issues with other libraries. Risk Level: medium (behavioral change) Testing: modified an existing test Docs Changes: n/a Release Notes: inline Platform Specific Features: Runtime guard: keep the previous one Signed-off-by: Nathanael DEMACON Signed-off-by: Ryan Northey --- changelogs/current.yaml | 4 ++ source/common/http/conn_manager_utility.cc | 31 +++++++++++-- source/common/http/conn_manager_utility.h | 1 + test/common/http/conn_manager_utility_test.cc | 37 +++++++++++++++ test/integration/protocol_integration_test.cc | 46 +++++++++++++++++++ 5 files changed, 116 insertions(+), 3 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce..cd3ee3b3bbd8 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -2,6 +2,10 @@ date: Pending behavior_changes: # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* +- area: http + change: | + Remove the hop by hop TE header from downstream request headers if it's not set to ``trailers``, else keep it. This change can be + temporarily reverted by setting ``envoy.reloadable_features.sanitize_te`` to false. minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b9fb637485db..08827b267127 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -92,9 +92,8 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m if (!Utility::isUpgrade(request_headers)) { request_headers.removeConnection(); request_headers.removeUpgrade(); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - request_headers.removeTE(); - } + + sanitizeTEHeader(request_headers); } // Clean proxy headers. @@ -292,6 +291,32 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m return {final_remote_address, absl::nullopt}; } +void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_headers) { + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { + return; + } + + absl::string_view te_header = request_headers.getTEValue(); + if (te_header.empty()) { + return; + } + + // If the TE header contains the "trailers" value, set the TE header to "trailers" only. + std::vector te_values = absl::StrSplit(te_header, ','); + for (const absl::string_view& te_value : te_values) { + bool is_trailers = + absl::StripAsciiWhitespace(te_value) == Http::Headers::get().TEValues.Trailers; + + if (is_trailers) { + request_headers.setTE(Http::Headers::get().TEValues.Trailers); + return; + } + } + + // If the TE header does not contain the "trailers" value, remove the TE header. + request_headers.removeTE(); +} + void ConnectionManagerUtility::cleanInternalHeaders( RequestHeaderMap& request_headers, bool edge_request, const std::list& internal_only_headers) { diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index a93d53706914..934b08132437 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -141,6 +141,7 @@ class ConnectionManagerUtility { static void mutateXfccRequestHeader(RequestHeaderMap& request_headers, Network::Connection& connection, ConnectionManagerConfig& config); + static void sanitizeTEHeader(RequestHeaderMap& request_headers); static void cleanInternalHeaders(RequestHeaderMap& request_headers, bool edge_request, const std::list& internal_only_headers); }; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 6633650dd204..e4ffd9d7c9b5 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -2237,5 +2237,42 @@ TEST_F(ConnectionManagerUtilityTest, DoNotOverwriteXForwardedPortFromUntrustedHo EXPECT_EQ("80", headers.getForwardedPortValue()); } +// Verify when TE header is present, the value should be preserved only if it's equal to "trailers". +TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderSimple) { + TestRequestHeaderMapImpl headers{{"te", "trailers"}}; + callMutateRequestHeaders(headers, Protocol::Http2); + + EXPECT_EQ("trailers", headers.getTEValue()); +} + +// Verify when TE header is present, the value should be preserved only if it contains "trailers". +TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderMultipleValuesAndWeigthted) { + TestRequestHeaderMapImpl headers{{"te", "chunked;q=0.8 , trailers ,deflate "}}; + callMutateRequestHeaders(headers, Protocol::Http2); + + EXPECT_EQ("trailers", headers.getTEValue()); +} + +// Verify when TE header is present, the value should be discarded if it doesn't contains +// "trailers". +TEST_F(ConnectionManagerUtilityTest, DiscardTEHeaderWithoutTrailers) { + TestRequestHeaderMapImpl headers{{"te", "gzip"}}; + callMutateRequestHeaders(headers, Protocol::Http2); + + EXPECT_EQ("", headers.getTEValue()); +} + +// Verify when TE header is present, the value should be kept if the reloadable feature +// "sanitize_te" is enabled. +TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderReloadableFeatureDisabled) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "false"}}); + + TestRequestHeaderMapImpl headers{{"te", "gzip"}}; + callMutateRequestHeaders(headers, Protocol::Http2); + + EXPECT_EQ("gzip", headers.getTEValue()); +} + } // namespace Http } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 271c020b15ac..f9554f393e47 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -809,6 +809,52 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitization) { EXPECT_EQ("", upstream_headers->getTEValue()); } +TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationTrailers) { + if (downstreamProtocol() != Http::CodecType::HTTP1) { + return; + } + + autonomous_upstream_ = true; + config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "true"); + + default_request_headers_.setTE("trailers"); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + + auto upstream_headers = + reinterpret_cast(fake_upstreams_[0].get())->lastRequestHeaders(); + EXPECT_TRUE(upstream_headers != nullptr); + EXPECT_EQ("trailers", upstream_headers->getTEValue()); +} + +TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationTrailersMultipleValuesAndWeigthted) { + if (downstreamProtocol() != Http::CodecType::HTTP1) { + return; + } + + autonomous_upstream_ = true; + config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "true"); + + default_request_headers_.setTE("chunked;q=0.8 , trailers ,deflate "); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + + auto upstream_headers = + reinterpret_cast(fake_upstreams_[0].get())->lastRequestHeaders(); + EXPECT_TRUE(upstream_headers != nullptr); + EXPECT_EQ("trailers", upstream_headers->getTEValue()); +} + // Regression test for https://github.com/envoyproxy/envoy/issues/10270 TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that