From d4ed8d4f6e071d0022a9f342110335d8a3c16ba5 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 30 Nov 2023 16:40:45 -0500 Subject: [PATCH] http: clear hop by hop TE header (#30535) Changes Envoy to clear TE header by default, as it is a hop by hop header and should not be forwarded on. Risk Level: medium (behavioral change) Testing: new e2e test Docs Changes: n/a Release Notes: inline. [Optional Runtime guard:] yes. Fixes #30362 Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 4 ++++ source/common/http/conn_manager_utility.cc | 3 +++ source/common/runtime/runtime_features.cc | 1 + test/integration/header_integration_test.cc | 6 +++-- test/integration/protocol_integration_test.cc | 23 +++++++++++++++++++ 5 files changed, 35 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 2075df8a3393..cdad84fe88d1 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. This change can be temporarily reverted + by setting ``envoy.reloadable_features.sanitize_te`` to false. - area: http change: | Flip runtime flag ``envoy.reloadable_features.no_downgrade_to_canonical_name`` to true. Name downgrading in the diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b6f450af5547..b9fb637485db 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -92,6 +92,9 @@ 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(); + } } // Clean proxy headers. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 5b3f34c34de4..cc80ee728269 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -73,6 +73,7 @@ RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout); RUNTIME_GUARD(envoy_reloadable_features_overload_manager_error_unknown_action); RUNTIME_GUARD(envoy_reloadable_features_proxy_status_upstream_request_timeout); RUNTIME_GUARD(envoy_reloadable_features_quic_fix_filter_manager_uaf); +RUNTIME_GUARD(envoy_reloadable_features_sanitize_te); RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value); RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests); RUNTIME_GUARD(envoy_reloadable_features_ssl_transport_failure_reason_format); diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index b9905ff94054..20c31de71483 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -1341,8 +1341,9 @@ TEST_P(HeaderIntegrationTest, PathWithEscapedSlashesRedirected) { }); } -// Validates TE header is forwarded if it contains a supported value +// Validates legacy TE handling: TE header is forwarded if it contains a supported value TEST_P(HeaderIntegrationTest, TestTeHeaderPassthrough) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "false"); initializeFilter(HeaderMode::Append, false); performRequest( Http::TestRequestHeaderMapImpl{ @@ -1375,8 +1376,9 @@ TEST_P(HeaderIntegrationTest, TestTeHeaderPassthrough) { }); } -// Validates TE header is stripped if it contains an unsupported value +// Validates legacy TE handling: that TE header stripped if it contains an unsupported value. TEST_P(HeaderIntegrationTest, TestTeHeaderSanitized) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "false"); initializeFilter(HeaderMode::Append, false); performRequest( Http::TestRequestHeaderMapImpl{ diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e554cedad5ea..010d27ca9ac4 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -784,6 +784,29 @@ TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyWithBodyBytesC BytesCountExpectation(7, 10, 7, 8)); } +TEST_P(DownstreamProtocolIntegrationTest, TeSanitization) { + if (downstreamProtocol() != Http::CodecType::HTTP1) { + return; + } + + autonomous_upstream_ = true; + config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "true"); + + default_request_headers_.setTE("gzip"); + + 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("", 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