From 9c065e17a852cfb0a1f73fb714a03fc02ee554e6 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 22 Aug 2019 15:07:01 -0400 Subject: [PATCH] http: forwarding x-forwarded-proto from trusted proxies (#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 #4496 Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + source/common/http/conn_manager_utility.cc | 24 +++++++-- source/common/runtime/runtime_features.cc | 1 + test/common/http/BUILD | 1 + test/common/http/conn_manager_utility_test.cc | 45 ++++++++++++++++ test/test_common/BUILD | 15 ++++++ test/test_common/test_runtime.h | 53 +++++++++++++++++++ 7 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 test/test_common/test_runtime.h diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index f0bac13068d8..6ba1faeed272 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -24,6 +24,7 @@ Version history * grpc-json: added support for :ref:`ignoring unknown query parameters`. * header to metadata: added :ref:`PROTOBUF_VALUE ` and :ref:`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` 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 ` to configure whether a listener will still create a connection when listener filters time out. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index c2b4337068ec..3a29db583cc7 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/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); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index e1dc2a53bc91..db42217c8233 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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 diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 56eefc2b25a6..b1db36183cd5 100644 --- a/test/common/http/BUILD +++ b/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", ], ) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index b2179a4d42e5..2f3d27ae5d85 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/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("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("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("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) { diff --git a/test/test_common/BUILD b/test/test_common/BUILD index c1d4c4f692bf..0d37bc707004 100644 --- a/test/test_common/BUILD +++ b/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"], diff --git a/test/test_common/test_runtime.h b/test/test_common/test_runtime.h new file mode 100644 index 000000000000..ba9900578fa7 --- /dev/null +++ b/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( + std::make_unique(dispatcher_, tls_, config, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_)); + } + +private: + Event::MockDispatcher dispatcher_; + testing::NiceMock tls_; + Stats::IsolatedStoreImpl store_; + Runtime::MockRandomGenerator generator_; + Api::ApiPtr api_; + testing::NiceMock local_info_; + Init::MockManager init_manager_; + testing::NiceMock validation_visitor_; + std::unique_ptr loader_; +}; + +} // namespace Envoy