http: forwarding x-forwarded-proto from trusted proxies (envoyproxy#7995

)

Trusting the x-forwarded-proto header from trusted proxies.
If Envoy is operating as an edge proxy but has a trusted hop in front, the trusted proxy should be allowed to set x-forwarded-proto and its x-forwarded-proto should be preserved.
Guarded by envoy.reloadable_features.trusted_forwarded_proto, default on.

Risk Level: Medium (L7 header changes) but guarded
Testing: new unit tests
Docs Changes: n/a
Release Notes: inline
Fixes envoyproxy#4496

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

docs/root/intro/version_history.rst

@@ -24,6 +24,7 @@ Version history
 * grpc-json: added support for :ref:`ignoring unknown query parameters<envoy_api_field_config.filter.http.transcoder.v2.GrpcJsonTranscoder.ignore_unknown_query_parameters>`.
 * header to metadata: added :ref:`PROTOBUF_VALUE <envoy_api_enum_value_config.filter.http.header_to_metadata.v2.Config.ValueType.PROTOBUF_VALUE>` and :ref:`ValueEncode <envoy_api_enum_config.filter.http.header_to_metadata.v2.Config.ValueEncode>` to support protobuf Value and Base64 encoding.
 * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
+* http: changed Envoy to forward existing x-forwarded-proto from upstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto` which defaults true.
 * http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.
 * http: remove h2c upgrade headers for HTTP/1 as h2c upgrades are currently not supported.
 * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out.

source/common/http/conn_manager_utility.cc

@@ -88,6 +88,9 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
   Network::Address::InstanceConstSharedPtr final_remote_address;
   bool single_xff_address;
   const uint32_t xff_num_trusted_hops = config.xffNumTrustedHops();
+  const bool trusted_forwarded_proto =
+      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.trusted_forwarded_proto");
+
   if (config.useRemoteAddress()) {
     single_xff_address = request_headers.ForwardedFor() == nullptr;
     // If there are any trusted proxies in front of this Envoy instance (as indicated by
@@ -110,8 +113,21 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
         Utility::appendXff(request_headers, *connection.remoteAddress());
       }
     }
-    request_headers.insertForwardedProto().value().setReference(
-        connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http);
+    if (trusted_forwarded_proto) {
+      // If the prior hop is not a trusted proxy, overwrite any x-forwarded-proto value it set as
+      // untrusted. Alternately if no x-forwarded-proto header exists, add one.
+      if (xff_num_trusted_hops == 0 || request_headers.ForwardedProto() == nullptr) {
+        request_headers.insertForwardedProto().value().setReference(
+            connection.ssl() ? Headers::get().SchemeValues.Https
+                             : Headers::get().SchemeValues.Http);
+      }
+    } else {
+      // Previously, before the trusted_forwarded_proto logic, Envoy would always overwrite the
+      // x-forwarded-proto header even if it was set by a trusted proxy. This code path is
+      // deprecated and will be removed.
+      request_headers.insertForwardedProto().value().setReference(
+          connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http);
+    }
   } else {
     // If we are not using remote address, attempt to pull a valid IPv4 or IPv6 address out of XFF.
     // If we find one, it will be used as the downstream address for logging. It may or may not be
@@ -121,8 +137,8 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
     single_xff_address = ret.single_address_;
   }
 
-  // If we didn't already replace x-forwarded-proto because we are using the remote address, and
-  // remote hasn't set it (trusted proxy), we set it, since we then use this for setting scheme.
+  // If the x-forwarded-proto header is not set, set it here, since Envoy uses it for determining
+  // scheme and communicating it upstream.
   if (!request_headers.ForwardedProto()) {
     request_headers.insertForwardedProto().value().setReference(
         connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http);

source/common/runtime/runtime_features.cc

@@ -26,6 +26,7 @@ constexpr const char* runtime_features[] = {
     // Enabled
     "envoy.reloadable_features.test_feature_true",
     "envoy.reloadable_features.buffer_filter_populate_content_length",
+    "envoy.reloadable_features.trusted_forwarded_proto",
 };
 
 // This is a list of configuration fields which are disallowed by default in Envoy

test/common/http/BUILD

@@ -233,6 +233,7 @@ envoy_cc_test(
         "//test/mocks/runtime:runtime_mocks",
         "//test/mocks/ssl:ssl_mocks",
         "//test/mocks/upstream:upstream_mocks",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",
     ],
 )

test/common/http/conn_manager_utility_test.cc

@@ -13,6 +13,7 @@
 #include "test/mocks/runtime/mocks.h"
 #include "test/mocks/ssl/mocks.h"
 #include "test/test_common/printers.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -227,6 +228,50 @@ TEST_F(ConnectionManagerUtilityTest, SkipXffAppendPassThruUseRemoteAddress) {
   EXPECT_EQ("198.51.100.1", headers.ForwardedFor()->value().getStringView());
 }
 
+TEST_F(ConnectionManagerUtilityTest, ForwardedProtoLegacyBehavior) {
+  TestScopedRuntime scoped_runtime;
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.trusted_forwarded_proto", "false"}});
+
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(1));
+  EXPECT_CALL(config_, skipXffAppend()).WillOnce(Return(true));
+  connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("12.12.12.12");
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  TestHeaderMapImpl headers{{"x-forwarded-proto", "https"}};
+
+  callMutateRequestHeaders(headers, Protocol::Http2);
+  EXPECT_EQ("http", headers.ForwardedProto()->value().getStringView());
+}
+
+TEST_F(ConnectionManagerUtilityTest, PreserveForwardedProtoWhenInternal) {
+  TestScopedRuntime scoped_runtime;
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.trusted_forwarded_proto", "true"}});
+
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(1));
+  EXPECT_CALL(config_, skipXffAppend()).WillOnce(Return(true));
+  connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("12.12.12.12");
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  TestHeaderMapImpl headers{{"x-forwarded-proto", "https"}};
+
+  callMutateRequestHeaders(headers, Protocol::Http2);
+  EXPECT_EQ("https", headers.ForwardedProto()->value().getStringView());
+}
+
+TEST_F(ConnectionManagerUtilityTest, OverwriteForwardedProtoWhenExternal) {
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(0));
+  connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1");
+  TestHeaderMapImpl headers{{"x-forwarded-proto", "https"}};
+  Network::Address::Ipv4Instance local_address("10.3.2.1");
+  ON_CALL(config_, localAddress()).WillByDefault(ReturnRef(local_address));
+
+  callMutateRequestHeaders(headers, Protocol::Http2);
+  EXPECT_EQ("http", headers.ForwardedProto()->value().getStringView());
+}
+
 // Verify internal request and XFF is set when we are using remote address and the address is
 // internal according to user configuration.
 TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWhenUserConfiguredRemoteAddress) {

test/test_common/BUILD

@@ -117,6 +117,21 @@ envoy_cc_test_library(
     ],
 )
 
+envoy_cc_test_library(
+    name = "test_runtime_lib",
+    hdrs = ["test_runtime.h"],
+    deps = [
+        "//source/common/runtime:runtime_lib",
+        "//source/common/stats:isolated_store_lib",
+        "//test/mocks/event:event_mocks",
+        "//test/mocks/init:init_mocks",
+        "//test/mocks/local_info:local_info_mocks",
+        "//test/mocks/protobuf:protobuf_mocks",
+        "//test/mocks/runtime:runtime_mocks",
+        "//test/mocks/thread_local:thread_local_mocks",
+    ],
+)
+
 envoy_cc_test_library(
     name = "thread_factory_for_test_lib",
     srcs = ["thread_factory_for_test.cc"],

test/test_common/test_runtime.h

@@ -0,0 +1,53 @@
+// A simple test utility to easily allow for runtime feature overloads in unit tests.
+//
+// As long as this class is in scope one can do runtime feature overrides:
+//
+//  TestScopedRuntime scoped_runtime;
+//  Runtime::LoaderSingleton::getExisting()->mergeValues(
+//      {{"envoy.reloadable_features.test_feature_true", "false"}});
+//
+//  As long as a TestScopedRuntime exists, Runtime::LoaderSingleton::getExisting()->mergeValues()
+//  can safely be called to override runtime values.
+
+#pragma once
+
+#include "common/runtime/runtime_impl.h"
+#include "common/stats/isolated_store_impl.h"
+
+#include "test/mocks/event/mocks.h"
+#include "test/mocks/init/mocks.h"
+#include "test/mocks/local_info/mocks.h"
+#include "test/mocks/protobuf/mocks.h"
+#include "test/mocks/runtime/mocks.h"
+#include "test/mocks/thread_local/mocks.h"
+
+#include "gmock/gmock.h"
+
+namespace Envoy {
+
+// TODO(alyssawilk) move existing runtime tests over to using this.
+class TestScopedRuntime {
+public:
+  TestScopedRuntime() : api_(Api::createApiForTest()) {
+    envoy::config::bootstrap::v2::LayeredRuntime config;
+    // The existence of an admin layer is required for mergeValues() to work.
+    config.add_layers()->mutable_admin_layer();
+
+    loader_ = std::make_unique<Runtime::ScopedLoaderSingleton>(
+        std::make_unique<Runtime::LoaderImpl>(dispatcher_, tls_, config, local_info_, init_manager_,
+                                              store_, generator_, validation_visitor_, *api_));
+  }
+
+private:
+  Event::MockDispatcher dispatcher_;
+  testing::NiceMock<ThreadLocal::MockInstance> tls_;
+  Stats::IsolatedStoreImpl store_;
+  Runtime::MockRandomGenerator generator_;
+  Api::ApiPtr api_;
+  testing::NiceMock<LocalInfo::MockLocalInfo> local_info_;
+  Init::MockManager init_manager_;
+  testing::NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_;
+  std::unique_ptr<Runtime::ScopedLoaderSingleton> loader_;
+};
+
+} // namespace Envoy