Request headers to add (#14747)

Being consistent about treating Host: and :authority the same way in Envoy header modification.

Risk Level: Medium (changes allowed modifiable headers)
Testing: new unit tests
Docs Changes: yes
Release Notes: inline

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

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 <envoy_v3_api_msg_config.route.v3.RouteConfiguration>` 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 <envoy_v3_api_field_config.route.v3.RouteAction.prefix_rewrite>`,
 :ref:`regex_rewrite <envoy_v3_api_field_config.route.v3.RouteAction.regex_rewrite>`, and

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.
 

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

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

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/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");
   }
 
   absl::string_view format(header_value.value());

source/common/runtime/runtime_features.cc

@@ -83,6 +83,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",

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

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<Envoy::StreamInfo::MockStreamInfo> 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 :