From 261145117eda40cb940f6bafc0408ea95b982959 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Wed, 7 Feb 2024 17:44:54 +0100 Subject: [PATCH 01/23] http: set TE header to `trailers` instead of removing it Signed-off-by: Nathanael DEMACON --- changelogs/current.yaml | 4 ++++ source/common/http/conn_manager_utility.cc | 2 +- test/integration/protocol_integration_test.cc | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index acff38659afc..178fe443bd61 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: | + Force the hop by hop TE header from downstream request headers to "trailers". 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..7e1c47c8f58f 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -93,7 +93,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m request_headers.removeConnection(); request_headers.removeUpgrade(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - request_headers.removeTE(); + request_headers.setTE(Http::Headers::get().TEValues.Trailers); } } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 03bc51ff801d..6f459d787170 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -806,7 +806,7 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitization) { auto upstream_headers = reinterpret_cast(fake_upstreams_[0].get())->lastRequestHeaders(); EXPECT_TRUE(upstream_headers != nullptr); - EXPECT_EQ("", upstream_headers->getTEValue()); + EXPECT_EQ("trailers", upstream_headers->getTEValue()); } // Regression test for https://github.com/envoyproxy/envoy/issues/10270 From cfe8dd4b0964eddb4869db374ea3157eafd916b8 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Thu, 8 Feb 2024 12:20:23 +0100 Subject: [PATCH 02/23] http: set TE header to `trailers` instead of removing it only in grpc Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 7 +++++- test/integration/protocol_integration_test.cc | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 7e1c47c8f58f..20b84725269f 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -92,8 +92,13 @@ 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.setTE(Http::Headers::get().TEValues.Trailers); + if (Grpc::Common::isGrpcRequestHeaders(request_headers)) { + request_headers.setTE(Http::Headers::get().TEValues.Trailers); + } else { + request_headers.removeTE(); + } } } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 6f459d787170..aa5a73557a3a 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -803,6 +803,30 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitization) { 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()); +} + +TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationGrpc) { + if (downstreamProtocol() != Http::CodecType::HTTP1) { + return; + } + + autonomous_upstream_ = true; + config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "true"); + + default_request_headers_.setTE("gzip"); + default_request_headers_.setContentType("application/grpc"); + + 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); From afc551258da8d538981e448575d920a2803794df Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 11:54:25 +0100 Subject: [PATCH 03/23] allow "trailers" Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 4 +--- test/integration/protocol_integration_test.cc | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 20b84725269f..edb4c1fa2b45 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -94,9 +94,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m request_headers.removeUpgrade(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - if (Grpc::Common::isGrpcRequestHeaders(request_headers)) { - request_headers.setTE(Http::Headers::get().TEValues.Trailers); - } else { + if (request_headers.TE() != Http::Headers::get().TEValues.Trailers) { request_headers.removeTE(); } } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index aa5a73557a3a..ef0a0c43ccf8 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -809,7 +809,7 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitization) { EXPECT_EQ("", upstream_headers->getTEValue()); } -TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationGrpc) { +TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationTrailers) { if (downstreamProtocol() != Http::CodecType::HTTP1) { return; } @@ -817,8 +817,7 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationGrpc) { autonomous_upstream_ = true; config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "true"); - default_request_headers_.setTE("gzip"); - default_request_headers_.setContentType("application/grpc"); + default_request_headers_.setTE("trailers"); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); From 6e96ea4de4ce917375c1fcfeed6a7115489dba1f Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 14:45:33 +0100 Subject: [PATCH 04/23] fix TE header get Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index edb4c1fa2b45..bc51a1448d65 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -94,7 +94,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m request_headers.removeUpgrade(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - if (request_headers.TE() != Http::Headers::get().TEValues.Trailers) { + if (request_headers.getTEValue() != Http::Headers::get().TEValues.Trailers) { request_headers.removeTE(); } } From 5afbedea25f6c0ff39bdb1dc1586c6905a362c68 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 17:48:30 +0100 Subject: [PATCH 05/23] handle multiple TE values Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 23 +++++++++++++++++-- test/integration/protocol_integration_test.cc | 23 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index bc51a1448d65..877ead3e60a7 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -94,8 +94,27 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m request_headers.removeUpgrade(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - if (request_headers.getTEValue() != Http::Headers::get().TEValues.Trailers) { - request_headers.removeTE(); + auto teHeader = request_headers.getTEValue(); + + if (!teHeader.empty()) { + auto hasTrailersTE = false; + + auto teValues = absl::StrSplit(, ","); + for (const auto& teValue : teValues) { + auto parts = absl::StrSplit(teValue, ";"); // Handles cases like "chunked, trailers;q=0.5" + auto value = absl::StripAsciiWhitespace(parts[0]); + + if (value == Http::Headers::get().TEValues.Trailers) { + hasTrailersTE = true; + break; + } + } + + if (hasTrailersTE) { + request_headers.setTE(Http::Headers::get().TEValues.Trailers); + } else { + request_headers.removeTE(); + } } } } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index ef0a0c43ccf8..dda1962ad5f1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -832,6 +832,29 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationTrailers) { 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 ;q=0.5,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 From 7f34b498992395571ee8797e2cc73506ff3213c5 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 18:48:29 +0100 Subject: [PATCH 06/23] add typing Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 877ead3e60a7..99e346ab4b0b 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -94,23 +94,23 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m request_headers.removeUpgrade(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - auto teHeader = request_headers.getTEValue(); + std::string te_header = request_headers.getTEValue(); - if (!teHeader.empty()) { - auto hasTrailersTE = false; + if (!te_header.empty()) { + bool has_trailers_te = false; - auto teValues = absl::StrSplit(, ","); - for (const auto& teValue : teValues) { - auto parts = absl::StrSplit(teValue, ";"); // Handles cases like "chunked, trailers;q=0.5" - auto value = absl::StripAsciiWhitespace(parts[0]); + std::vector te_values = absl::StrSplit(, ","); + for (const auto& teValue : te_values) { + std::vector parts = absl::StrSplit(teValue, ";"); // Handles cases like "chunked, trailers;q=0.5" + std::string value = absl::StripAsciiWhitespace(parts[0]); if (value == Http::Headers::get().TEValues.Trailers) { - hasTrailersTE = true; + has_trailers_te = true; break; } } - if (hasTrailersTE) { + if (has_trailers_te) { request_headers.setTE(Http::Headers::get().TEValues.Trailers); } else { request_headers.removeTE(); From c19ebc3916c03f8606f9279a8830eb2838efaac2 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 18:58:07 +0100 Subject: [PATCH 07/23] move logic inside a dedicated function Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 56 ++++++++++++---------- source/common/http/conn_manager_utility.h | 1 + 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 99e346ab4b0b..70ff1519c290 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -93,30 +93,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m request_headers.removeConnection(); request_headers.removeUpgrade(); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { - std::string te_header = request_headers.getTEValue(); - - if (!te_header.empty()) { - bool has_trailers_te = false; - - std::vector te_values = absl::StrSplit(, ","); - for (const auto& teValue : te_values) { - std::vector parts = absl::StrSplit(teValue, ";"); // Handles cases like "chunked, trailers;q=0.5" - std::string value = absl::StripAsciiWhitespace(parts[0]); - - if (value == Http::Headers::get().TEValues.Trailers) { - has_trailers_te = true; - break; - } - } - - if (has_trailers_te) { - request_headers.setTE(Http::Headers::get().TEValues.Trailers); - } else { - request_headers.removeTE(); - } - } - } + sanitizeTEHeader(request_headers); } // Clean proxy headers. @@ -314,6 +291,37 @@ 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; + } + + std::string te_header = request_headers.getTEValue(); + if (te_header.empty()) { + return; + } + + bool has_trailers_te = false; + + std::vector te_values = absl::StrSplit(, ","); + for (const auto& teValue : te_values) { + std::vector parts = + absl::StrSplit(teValue, ";"); // Handles cases like "chunked, trailers;q=0.5" + std::string value = absl::StripAsciiWhitespace(parts[0]); + + if (value == Http::Headers::get().TEValues.Trailers) { + has_trailers_te = true; + break; + } + } + + if (has_trailers_te) { + request_headers.setTE(Http::Headers::get().TEValues.Trailers); + } else { + 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); }; From fb94a2e1238b65d21c59f5ea5e86de19b3eee257 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:05:58 +0100 Subject: [PATCH 08/23] add unit tests Signed-off-by: Nathanael DEMACON --- test/common/http/conn_manager_utility_test.cc | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 6633650dd204..9a8e8f7d502b 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -2237,5 +2237,39 @@ 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) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); + + 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) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); + + TestRequestHeaderMapImpl headers{{"te", "chunked;q=0.8 , trailers ;q=0.5,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) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); + + TestRequestHeaderMapImpl headers{{"te", "gzip"}}; + callMutateRequestHeaders(headers, Protocol::Http2); + + EXPECT_EQ("", headers.getTEValue()); +} + } // namespace Http } // namespace Envoy From ae3dcbe23b5de0c5d514c0cd97eb3af645fe0ac7 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:42:21 +0100 Subject: [PATCH 09/23] simplify search Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 70ff1519c290..b52f21d9d17d 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -303,13 +303,9 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header bool has_trailers_te = false; - std::vector te_values = absl::StrSplit(, ","); + std::vector te_values = absl::StrSplit(te_header, ','); for (const auto& teValue : te_values) { - std::vector parts = - absl::StrSplit(teValue, ";"); // Handles cases like "chunked, trailers;q=0.5" - std::string value = absl::StripAsciiWhitespace(parts[0]); - - if (value == Http::Headers::get().TEValues.Trailers) { + if (absl::StripAsciiWhitespace(teValue) == Http::Headers::get().TEValues.Trailers) { has_trailers_te = true; break; } From 1bc9bb6b5a719bb39c4a143e06042cc68f117fa5 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:43:18 +0100 Subject: [PATCH 10/23] remove trailers weight from tests Signed-off-by: Nathanael DEMACON --- test/common/http/conn_manager_utility_test.cc | 2 +- test/integration/protocol_integration_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 9a8e8f7d502b..90e89542b87b 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -2253,7 +2253,7 @@ TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderMultipleValuesAndWeigth TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); - TestRequestHeaderMapImpl headers{{"te", "chunked;q=0.8 , trailers ;q=0.5,deflate "}}; + TestRequestHeaderMapImpl headers{{"te", "chunked;q=0.8 , trailers ,deflate "}}; callMutateRequestHeaders(headers, Protocol::Http2); EXPECT_EQ("trailers", headers.getTEValue()); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index dda1962ad5f1..f4e305689202 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -840,7 +840,7 @@ TEST_P(DownstreamProtocolIntegrationTest, TeSanitizationTrailersMultipleValuesAn autonomous_upstream_ = true; config_helper_.addRuntimeOverride("envoy.reloadable_features.sanitize_te", "true"); - default_request_headers_.setTE("chunked;q=0.8 , trailers ;q=0.5,deflate "); + default_request_headers_.setTE("chunked;q=0.8 , trailers ,deflate "); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); From a62aa02b9a8ddf8600f62a79bce2d52e62ba6c57 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:45:44 +0100 Subject: [PATCH 11/23] update changelog Signed-off-by: Nathanael DEMACON --- changelogs/current.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 178fe443bd61..5e6eb6bc7598 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -4,8 +4,8 @@ behavior_changes: # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* - area: http change: | - Force the hop by hop TE header from downstream request headers to "trailers". This change can be temporarily reverted - by setting ``envoy.reloadable_features.sanitize_te`` to false. + Remove the hop by hop TE header from downstream request headers if it's not set to `trailers`. 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* From 00918989f1ef203a9b26f1b14593ef6102cc1dee Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:46:15 +0100 Subject: [PATCH 12/23] default reloadable value is true Signed-off-by: Nathanael DEMACON --- test/common/http/conn_manager_utility_test.cc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 90e89542b87b..63c0f7c7f4f6 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -2239,9 +2239,6 @@ TEST_F(ConnectionManagerUtilityTest, DoNotOverwriteXForwardedPortFromUntrustedHo // Verify when TE header is present, the value should be preserved only if it's equal to "trailers". TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderSimple) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); - TestRequestHeaderMapImpl headers{{"te", "trailers"}}; callMutateRequestHeaders(headers, Protocol::Http2); @@ -2250,9 +2247,6 @@ TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderSimple) { // Verify when TE header is present, the value should be preserved only if it contains "trailers". TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderMultipleValuesAndWeigthted) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); - TestRequestHeaderMapImpl headers{{"te", "chunked;q=0.8 , trailers ,deflate "}}; callMutateRequestHeaders(headers, Protocol::Http2); @@ -2262,9 +2256,6 @@ TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderMultipleValuesAndWeigth // Verify when TE header is present, the value should be discarded if it doesn't contains // "trailers". TEST_F(ConnectionManagerUtilityTest, DiscardTEHeaderWithoutTrailers) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "true"}}); - TestRequestHeaderMapImpl headers{{"te", "gzip"}}; callMutateRequestHeaders(headers, Protocol::Http2); From e0c715663456450c906586256f97bdcfbae1ca3a Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:47:29 +0100 Subject: [PATCH 13/23] add test for `sanitize_te` reloadable feature Signed-off-by: Nathanael DEMACON --- test/common/http/conn_manager_utility_test.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 63c0f7c7f4f6..b2c5ded325c8 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -2262,5 +2262,17 @@ TEST_F(ConnectionManagerUtilityTest, DiscardTEHeaderWithoutTrailers) { 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, KeepTrailersTEHeaderSimple) { + 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 From a330a6be002165695e772d1a5b98437a6d0217bc Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 19:48:16 +0100 Subject: [PATCH 14/23] fix reloadable feature guard Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b52f21d9d17d..c0f235e0f882 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -292,7 +292,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m } void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_headers) { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) { return; } From da01b3adf632c2ef034a70d7adef913a35cdfc7b Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 22:10:20 +0100 Subject: [PATCH 15/23] simplify and add comments Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index c0f235e0f882..d6d7acb6e501 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -301,21 +301,19 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header return; } - bool has_trailers_te = false; - + // 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 auto& teValue : te_values) { - if (absl::StripAsciiWhitespace(teValue) == Http::Headers::get().TEValues.Trailers) { - has_trailers_te = true; - break; + bool has_trailers_te = absl::StripAsciiWhitespace(teValue) == Http::Headers::get().TEValues.Trailers; + + if (has_trailers_te) { + request_headers.setTE(Http::Headers::get().TEValues.Trailers); + return; } } - if (has_trailers_te) { - request_headers.setTE(Http::Headers::get().TEValues.Trailers); - } else { - request_headers.removeTE(); - } + // If the TE header does not contain the "trailers" value, remove the TE header. + request_headers.removeTE(); } void ConnectionManagerUtility::cleanInternalHeaders( From 8eebaef58c106a7320fc7e2ef149dd6d2c61f5c7 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 22:10:52 +0100 Subject: [PATCH 16/23] naming Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index d6d7acb6e501..05bdfc9528a0 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -303,8 +303,8 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header // 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 auto& teValue : te_values) { - bool has_trailers_te = absl::StripAsciiWhitespace(teValue) == Http::Headers::get().TEValues.Trailers; + for (const auto& te_value : te_values) { + bool has_trailers_te = absl::StripAsciiWhitespace(te_value) == Http::Headers::get().TEValues.Trailers; if (has_trailers_te) { request_headers.setTE(Http::Headers::get().TEValues.Trailers); From ad06416d1d7f976bed2c30d007e30d4cd929c9d3 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 22:10:58 +0100 Subject: [PATCH 17/23] explicit typing Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 05bdfc9528a0..280bc018f830 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -303,7 +303,7 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header // 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 auto& te_value : te_values) { + for (const std::string& te_value : te_values) { bool has_trailers_te = absl::StripAsciiWhitespace(te_value) == Http::Headers::get().TEValues.Trailers; if (has_trailers_te) { From 3c9d444f53827bce1c6cc369f0b2efef6feb2fed Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 22:11:59 +0100 Subject: [PATCH 18/23] is_trailer naming Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 280bc018f830..669752937f48 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -304,9 +304,9 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header // 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 std::string& te_value : te_values) { - bool has_trailers_te = absl::StripAsciiWhitespace(te_value) == Http::Headers::get().TEValues.Trailers; + bool is_trailers = absl::StripAsciiWhitespace(te_value) == Http::Headers::get().TEValues.Trailers; - if (has_trailers_te) { + if (is_trailers) { request_headers.setTE(Http::Headers::get().TEValues.Trailers); return; } From d8d5c19cb6628be4697f8fbfb02bcbc5268672da Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 22:13:34 +0100 Subject: [PATCH 19/23] improve explanation Signed-off-by: Nathanael DEMACON --- changelogs/current.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 5e6eb6bc7598..e79bb9a94d93 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -4,8 +4,8 @@ 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`. This change can be temporarily - reverted by setting ``envoy.reloadable_features.sanitize_te`` to false. + 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* From 143ecbc058b67762a7436e6fee96af5b553d8ae3 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 23:16:46 +0100 Subject: [PATCH 20/23] use `absl::string_view` instead of `std::string` Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 669752937f48..85e94f74d397 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -296,14 +296,14 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header return; } - std::string te_header = request_headers.getTEValue(); + 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 std::string& te_value : te_values) { + 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) { From 80fe87d71d2c96c54d0ee8453dd5207fd3f6c1e1 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Fri, 9 Feb 2024 23:33:52 +0100 Subject: [PATCH 21/23] format Signed-off-by: Nathanael DEMACON --- source/common/http/conn_manager_utility.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 85e94f74d397..08827b267127 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -304,7 +304,8 @@ void ConnectionManagerUtility::sanitizeTEHeader(RequestHeaderMap& request_header // 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; + bool is_trailers = + absl::StripAsciiWhitespace(te_value) == Http::Headers::get().TEValues.Trailers; if (is_trailers) { request_headers.setTE(Http::Headers::get().TEValues.Trailers); From 785642df04750e36d9011a21726fa7555d3a6f9d Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Sat, 10 Feb 2024 00:03:12 +0100 Subject: [PATCH 22/23] double backticks Signed-off-by: Nathanael DEMACON --- changelogs/current.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e79bb9a94d93..6307bd472c13 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -4,7 +4,7 @@ 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 + 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: From 71613ec1acd084a47cb0ade653c90263058347b3 Mon Sep 17 00:00:00 2001 From: Nathanael DEMACON Date: Sat, 10 Feb 2024 02:04:10 +0100 Subject: [PATCH 23/23] fix test name Signed-off-by: Nathanael DEMACON --- test/common/http/conn_manager_utility_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index b2c5ded325c8..e4ffd9d7c9b5 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -2264,7 +2264,7 @@ TEST_F(ConnectionManagerUtilityTest, DiscardTEHeaderWithoutTrailers) { // Verify when TE header is present, the value should be kept if the reloadable feature // "sanitize_te" is enabled. -TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderSimple) { +TEST_F(ConnectionManagerUtilityTest, KeepTrailersTEHeaderReloadableFeatureDisabled) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.sanitize_te", "false"}});