From 412540a77ec4f43c563137a23b29167269f35ed9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 5 Jan 2021 10:24:23 -0500 Subject: [PATCH 1/3] WIP Signed-off-by: Alyssa Wilk --- test/integration/header_integration_test.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 9cdf7e0f7203..73dbe034383b 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -57,6 +57,11 @@ stat_prefix: header_test - header: key: "x-vhost-request" value: "vhost" + request_headers_to_add: + - header: + key: "host" + value: "FOO" + append: false request_headers_to_remove: ["x-vhost-request-remove"] response_headers_to_add: - header: @@ -444,6 +449,8 @@ class HeaderIntegrationTest headers.remove(Envoy::Http::LowerCaseString{"x-request-id"}); headers.remove(Envoy::Http::LowerCaseString{"x-envoy-internal"}); + std::cerr << expected_headers; + std::cerr << headers; EXPECT_EQ(expected_headers, headers); } From c128ec2aaf78665b7531dc302e697466e4fa2f2d Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 19 Jan 2021 11:30:09 -0500 Subject: [PATCH 2/3] http: treating authority and host consistently Signed-off-by: Alyssa Wilk --- .../http/http_conn_man/headers.rst | 2 +- docs/root/version_history/current.rst | 1 + source/common/http/header_utility.cc | 6 ++++ source/common/http/header_utility.h | 7 +++++ source/common/router/header_parser.cc | 6 ++-- source/common/runtime/runtime_features.cc | 1 + test/common/http/header_utility_test.cc | 9 ++++++ test/common/router/config_impl_test.cc | 31 ++++++++++++++++--- test/integration/header_integration_test.cc | 7 ----- 9 files changed, 56 insertions(+), 14 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/headers.rst b/docs/root/configuration/http/http_conn_man/headers.rst index 143def096e48..6763707f1aac 100644 --- a/docs/root/configuration/http/http_conn_man/headers.rst +++ b/docs/root/configuration/http/http_conn_man/headers.rst @@ -479,7 +479,7 @@ Custom request/response headers can be added to a request/response at the weight route, virtual host, and/or global route configuration level. See the :ref:`v3 ` API documentation. -No *:-prefixed* pseudo-header may be modified via this mechanism. The *:path* +Neither *:-prefixed* pseudo-headers nor the Host: header may be modified via this mechanism. The *:path* and *:authority* headers may instead be modified via mechanisms such as :ref:`prefix_rewrite `, :ref:`regex_rewrite `, and diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 65b7fb052e91..35af65fdb396 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,6 +23,7 @@ Bug Fixes * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. +* http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort. diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 4c2ff9904af2..24b817f170f4 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -314,5 +314,11 @@ bool HeaderUtility::isRemovableHeader(absl::string_view header) { !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get()); } +bool HeaderUtility::isModifiableHeader(absl::string_view header) { + return (header.empty() || header[0] != ':') && + (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.treat_host_like_authority") || + !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get())); +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 6ccda5423b3f..bcd60eaaecc4 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -193,6 +193,13 @@ class HeaderUtility { * may not be removed. */ static bool isRemovableHeader(absl::string_view header); + + /** + * Returns true if a header may be safely modified without causing additional + * problems. Currently header names beginning with ":" and the "host" header + * may not be modified. + */ + static bool isModifiableHeader(absl::string_view header); }; } // namespace Http } // namespace Envoy diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index 557a888c53fe..7b288bad148c 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -47,8 +47,10 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea // will cause us to have to worry about interaction with other aspects of the // RouteAction, e.g. prefix rewriting. We also reject other :-prefixed // headers, since it seems dangerous and there doesn't appear a use case. - if (key[0] == ':') { - throw EnvoyException(":-prefixed headers may not be modified"); + // Host is disallowed as it created confusing and inconsistent behaviors for + // HTTP/1 and HTTP/1. It could arugably be allowed on the response path. + if (!Http::HeaderUtility::isModifiableHeader(key)) { + throw EnvoyException(":-prefixed or host headers may not be modified"); } absl::string_view format(header_value.value()); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 66cdd486fe3e..a4968dd0a8b8 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -86,6 +86,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.stop_faking_paths", "envoy.reloadable_features.strict_1xx_and_204_response_headers", "envoy.reloadable_features.tls_use_io_handle_bio", + "envoy.reloadable_features.treat_host_like_authority", "envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure", "envoy.reloadable_features.upstream_host_weight_change_causes_rebuild", "envoy.reloadable_features.vhds_heartbeats", diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 4e60542f0be0..ad7bb071dd40 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -701,5 +701,14 @@ TEST(RequiredHeaders, IsRemovableHeader) { EXPECT_TRUE(HeaderUtility::isRemovableHeader("Content-Type")); } +TEST(RequiredHeaders, IsModifiableHeader) { + EXPECT_FALSE(HeaderUtility::isRemovableHeader(":path")); + EXPECT_FALSE(HeaderUtility::isRemovableHeader("host")); + EXPECT_FALSE(HeaderUtility::isRemovableHeader("Host")); + EXPECT_TRUE(HeaderUtility::isRemovableHeader("")); + EXPECT_TRUE(HeaderUtility::isRemovableHeader("hostname")); + EXPECT_TRUE(HeaderUtility::isRemovableHeader("Content-Type")); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 9e4747e06047..29789154e5c9 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1721,10 +1721,10 @@ most_specific_header_mutations_wins: true } } -// Validate that we can't add :-prefixed request headers. -TEST_F(RouteMatcherTest, TestRequestHeadersToAddNoPseudoHeader) { +// Validate that we can't add :-prefixed or Host request headers. +TEST_F(RouteMatcherTest, TestRequestHeadersToAddNoHostOrPseudoHeader) { for (const std::string& header : - {":path", ":authority", ":method", ":scheme", ":status", ":protocol"}) { + {":path", ":authority", ":method", ":scheme", ":status", ":protocol", "host"}) { const std::string yaml = fmt::format(R"EOF( virtual_hosts: - name: www2 @@ -1743,10 +1743,33 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddNoPseudoHeader) { parseRouteConfigurationFromYaml(yaml); EXPECT_THROW_WITH_MESSAGE(TestConfigImpl config(route_config, factory_context_, true), - EnvoyException, ":-prefixed headers may not be modified"); + EnvoyException, ":-prefixed or host headers may not be modified"); } } +TEST_F(RouteMatcherTest, TestRequestHeadersToAddLegacyHostHeader) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.treat_host_like_authority", "false"}}); + + const std::string yaml = R"EOF( +virtual_hosts: + - name: www2 + domains: ["*"] + request_headers_to_add: + - header: + key: "host" + value: vhost-www2 + append: false +)EOF"; + + NiceMock stream_info; + + envoy::config::route::v3::RouteConfiguration route_config = parseRouteConfigurationFromYaml(yaml); + + EXPECT_NO_THROW(TestConfigImpl config(route_config, factory_context_, true)); +} + // Validate that we can't remove :-prefixed request headers. TEST_F(RouteMatcherTest, TestRequestHeadersToRemoveNoPseudoHeader) { for (const std::string& header : diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 73dbe034383b..9cdf7e0f7203 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -57,11 +57,6 @@ stat_prefix: header_test - header: key: "x-vhost-request" value: "vhost" - request_headers_to_add: - - header: - key: "host" - value: "FOO" - append: false request_headers_to_remove: ["x-vhost-request-remove"] response_headers_to_add: - header: @@ -449,8 +444,6 @@ class HeaderIntegrationTest headers.remove(Envoy::Http::LowerCaseString{"x-request-id"}); headers.remove(Envoy::Http::LowerCaseString{"x-envoy-internal"}); - std::cerr << expected_headers; - std::cerr << headers; EXPECT_EQ(expected_headers, headers); } From aba4b0f6aa2a26c2d1b9c76cfe536078ca382f33 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 19 Jan 2021 12:32:33 -0500 Subject: [PATCH 3/3] typos Signed-off-by: Alyssa Wilk --- source/common/router/header_parser.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index 7b288bad148c..5bd40ba20796 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -48,7 +48,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea // RouteAction, e.g. prefix rewriting. We also reject other :-prefixed // headers, since it seems dangerous and there doesn't appear a use case. // Host is disallowed as it created confusing and inconsistent behaviors for - // HTTP/1 and HTTP/1. It could arugably be allowed on the response path. + // HTTP/1 and HTTP/2. It could arguably be allowed on the response path. if (!Http::HeaderUtility::isModifiableHeader(key)) { throw EnvoyException(":-prefixed or host headers may not be modified"); }