[bp/1.29] http: keep trailers TE header instead of removing it (#32255

) (#33018)

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 <ndemacon@scaleway.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
Co-authored-by: Nathanael Demacon <nathanael.dmc@outlook.fr>

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*

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<absl::string_view> 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<Http::LowerCaseString>& internal_only_headers) {

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<Http::LowerCaseString>& internal_only_headers);
 };

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

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<AutonomousUpstream*>(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<AutonomousUpstream*>(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