From 54c956e42b3dce65b36ee025f00deb5c102e4a7e Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Aug 2019 16:58:30 -0400 Subject: [PATCH 1/4] http: forwarding x-forwarded-proto from trusted proxies Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + source/common/http/conn_manager_utility.cc | 16 +++++- 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 | 49 +++++++++++++++++++ 6 files changed, 125 insertions(+), 2 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 c40a3454e4b1..a9520ce49345 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -25,6 +25,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`. * http: added the ability to :ref:`merge adjacent slashes` in the path. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 5f9d541693b6..b64112cc835a 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -85,6 +85,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(); + 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 @@ -107,8 +110,17 @@ 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) { + // Set x-forwarded-proto unless it already exists and was forwarded on by a trusted proxy. + 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 { + 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 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 c228f99e3c30..025a21ab1caf 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" @@ -226,6 +227,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..7fba4e4d9c4e --- /dev/null +++ b/test/test_common/test_runtime.h @@ -0,0 +1,49 @@ +// 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"}}); + +#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() { + envoy::config::bootstrap::v2::LayeredRuntime config; + config.add_layers()->mutable_admin_layer(); + + loader_ = std::make_unique(Runtime::LoaderPtr{ + new Runtime::LoaderImpl(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 From 02fce9f40bbb498618343af8ac24caff88b493df Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 21 Aug 2019 13:53:40 -0400 Subject: [PATCH 2/4] fixing asan test bug Signed-off-by: Alyssa Wilk --- test/test_common/test_runtime.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_common/test_runtime.h b/test/test_common/test_runtime.h index 7fba4e4d9c4e..0d109256cf56 100644 --- a/test/test_common/test_runtime.h +++ b/test/test_common/test_runtime.h @@ -25,7 +25,7 @@ namespace Envoy { // TODO(alyssawilk) move existing runtime tests over to using this. class TestScopedRuntime { public: - TestScopedRuntime() { + TestScopedRuntime() : api_(Api::createApiForTest()) { envoy::config::bootstrap::v2::LayeredRuntime config; config.add_layers()->mutable_admin_layer(); From 8dc7cbc3eadc74fff20fd9f8ae38bad54422e7c3 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 22 Aug 2019 10:38:21 -0400 Subject: [PATCH 3/4] reviewer comments Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 2 +- source/common/http/conn_manager_utility.cc | 12 ++++++++---- source/common/runtime/runtime_features.cc | 1 + test/test_common/test_runtime.h | 9 ++++++--- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index a9520ce49345..91addd15eb92 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -25,7 +25,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`. +* 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. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b64112cc835a..9c1b8a3b1ea6 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -85,7 +85,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest Network::Address::InstanceConstSharedPtr final_remote_address; bool single_xff_address; const uint32_t xff_num_trusted_hops = config.xffNumTrustedHops(); - bool trusted_forwarded_proto = + const bool trusted_forwarded_proto = Runtime::runtimeFeatureEnabled("envoy.reloadable_features.trusted_forwarded_proto"); if (config.useRemoteAddress()) { @@ -111,13 +111,17 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest } } if (trusted_forwarded_proto) { - // Set x-forwarded-proto unless it already exists and was forwarded on by a trusted proxy. + // 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); } @@ -130,8 +134,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/test_common/test_runtime.h b/test/test_common/test_runtime.h index 0d109256cf56..afbdd257babc 100644 --- a/test/test_common/test_runtime.h +++ b/test/test_common/test_runtime.h @@ -5,6 +5,9 @@ // 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 @@ -29,9 +32,9 @@ class TestScopedRuntime { envoy::config::bootstrap::v2::LayeredRuntime config; config.add_layers()->mutable_admin_layer(); - loader_ = std::make_unique(Runtime::LoaderPtr{ - new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_, - generator_, validation_visitor_, *api_)}); + loader_ = std::make_unique( + std::make_unique(dispatcher_, tls_, config, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_)); } private: From 6e3142d21ba0a896a60d15355de56001ab065ee6 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 22 Aug 2019 13:12:33 -0400 Subject: [PATCH 4/4] comment Signed-off-by: Alyssa Wilk --- test/test_common/test_runtime.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_common/test_runtime.h b/test/test_common/test_runtime.h index afbdd257babc..ba9900578fa7 100644 --- a/test/test_common/test_runtime.h +++ b/test/test_common/test_runtime.h @@ -30,6 +30,7 @@ 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(