diff --git a/RAW_RELEASE_NOTES.md b/RAW_RELEASE_NOTES.md index fa5a7c505a115..de3ae4e5e49b6 100644 --- a/RAW_RELEASE_NOTES.md +++ b/RAW_RELEASE_NOTES.md @@ -61,3 +61,4 @@ final version. * Added support for abstract unix domain sockets on linux. The abstract namespace can be used by prepending '@' to a socket path. * Added `GEORADIUS_RO` and `GEORADIUSBYMEMBER_RO` to the Redis command splitter whitelist. +* Added support for trusting additional hops in the X-Forwarded-For request header. diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 080c737f59b0b..17d16ed501772 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -244,6 +244,12 @@ class ConnectionManagerConfig { */ virtual bool useRemoteAddress() PURE; + /** + * @return uint32_t the number of trusted proxy hops in front of this Envoy instance, for + * the purposes of XFF processing. + */ + virtual uint32_t xffNumTrustedHops() const PURE; + /** * @return ForwardClientCertType the configuration of how to forward the client cert information. */ diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 959684a83da70..1a5bb35371882 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -50,9 +50,22 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest // our peer to have already properly set XFF, etc. Network::Address::InstanceConstSharedPtr final_remote_address; bool single_xff_address; + const uint32_t xff_num_trusted_hops = config.xffNumTrustedHops(); if (config.useRemoteAddress()) { - final_remote_address = connection.remoteAddress(); single_xff_address = request_headers.ForwardedFor() == nullptr; + // If there are any trusted proxies in front of this Envoy instance (as indicated by + // the xff_num_trusted_hops configuration option), get the trusted client address + // from the XFF before we append to XFF. + if (xff_num_trusted_hops > 0) { + final_remote_address = + Utility::getLastAddressFromXFF(request_headers, xff_num_trusted_hops - 1).address_; + } + // If there aren't any trusted proxies in front of this Envoy instance, or there + // are but they didn't populate XFF properly, the trusted client address is the + // source address of the immediate downstream's connection to us. + if (final_remote_address == nullptr) { + final_remote_address = connection.remoteAddress(); + } if (Network::Utility::isLoopbackAddress(*connection.remoteAddress())) { Utility::appendXff(request_headers, config.localAddress()); } else { @@ -64,7 +77,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest // 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 // used for determining internal/external status (see below). - auto ret = Utility::getLastAddressFromXFF(request_headers); + auto ret = Utility::getLastAddressFromXFF(request_headers, xff_num_trusted_hops); final_remote_address = ret.address_; single_xff_address = ret.single_address_; } @@ -140,9 +153,9 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest // If we are an external request, AND we are "using remote address" (see above), we set // x-envoy-external-address since this is our first ingress point into the trusted network. - if (edge_request && connection.remoteAddress()->type() == Network::Address::Type::Ip) { + if (edge_request && final_remote_address->type() == Network::Address::Type::Ip) { request_headers.insertEnvoyExternalAddress().value( - connection.remoteAddress()->ip()->addressAsString()); + final_remote_address->ip()->addressAsString()); } // Generate x-request-id for all edge requests, or if there is none. diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 7ea0e3d60f948..110bc95fc7a2c 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -239,7 +239,7 @@ void Utility::sendLocalReply( } Utility::GetLastAddressFromXffInfo -Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers) { +Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers, uint32_t num_to_skip) { const auto xff_header = request_headers.ForwardedFor(); if (xff_header == nullptr) { return {nullptr, false}; @@ -247,6 +247,16 @@ Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers) { absl::string_view xff_string(xff_header->value().c_str(), xff_header->value().size()); static const std::string seperator(", "); + // Ignore the last num_to_skip addresses at the end of XFF. + for (uint32_t i = 0; i < num_to_skip; i++) { + std::string::size_type last_comma = xff_string.rfind(seperator); + if (last_comma == std::string::npos) { + return {nullptr, false}; + } + xff_string = xff_string.substr(0, last_comma); + } + // The text after the last remaining comma, or the entirety of the string if there + // is no comma, is the requested IP address. std::string::size_type last_comma = xff_string.rfind(seperator); if (last_comma != std::string::npos && last_comma + seperator.size() < xff_string.size()) { xff_string = xff_string.substr(last_comma + seperator.size()); @@ -259,7 +269,7 @@ Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers) { // TODO(mattklein123 PERF: Avoid the copy here. return { Network::Utility::parseInternetAddress(std::string(xff_string.data(), xff_string.size())), - last_comma == std::string::npos}; + last_comma == std::string::npos && num_to_skip == 0}; } catch (const EnvoyException&) { return {nullptr, false}; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 378d1c1eb301e..54e848c68bb2e 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -140,10 +140,13 @@ class Utility { /** * Retrieves the last IPv4/IPv6 address in the x-forwarded-for header. * @param request_headers supplies the request headers. + * @param num_to_skip specifies the number of addresses at the end of the XFF header + * to ignore when identifying the "last" address. * @return GetLastAddressFromXffInfo information about the last address in the XFF header. * @see GetLastAddressFromXffInfo for more information. */ - static GetLastAddressFromXffInfo getLastAddressFromXFF(const Http::HeaderMap& request_headers); + static GetLastAddressFromXffInfo getLastAddressFromXFF(const Http::HeaderMap& request_headers, + uint32_t num_to_skip = 0); /** * Get the string for the given http protocol. diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index e5d789463a51a..d563c2a39b94c 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -120,6 +120,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats(stats_prefix_, context_.scope())), use_remote_address_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_remote_address, false)), + xff_num_trusted_hops_(config.xff_num_trusted_hops()), route_config_provider_manager_(route_config_provider_manager), http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())), diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 5d0cba9517539..5dd71a1eb0704 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -90,6 +90,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerStats& stats() override { return stats_; } Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } bool useRemoteAddress() override { return use_remote_address_; } + uint32_t xffNumTrustedHops() const override { return xff_num_trusted_hops_; } Http::ForwardClientCertType forwardClientCert() override { return forward_client_cert_; } const std::vector& setCurrentClientCertDetails() const override { return set_current_client_cert_details_; @@ -114,6 +115,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerStats stats_; Http::ConnectionManagerTracingStats tracing_stats_; const bool use_remote_address_{}; + const uint32_t xff_num_trusted_hops_; Http::ForwardClientCertType forward_client_cert_; std::vector set_current_client_cert_details_; Router::RouteConfigProviderManager& route_config_provider_manager_; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index eb24f6ec2806b..d45715ff7506b 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -76,6 +76,7 @@ class AdminImpl : public Admin, Http::ConnectionManagerStats& stats() override { return stats_; } Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } bool useRemoteAddress() override { return true; } + uint32_t xffNumTrustedHops() const override { return 0; } Http::ForwardClientCertType forwardClientCert() override { return Http::ForwardClientCertType::Sanitize; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 2642dd9e8d39f..8febb68402fe6 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -235,6 +235,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi ConnectionManagerStats& stats() override { return stats_; } ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } bool useRemoteAddress() override { return use_remote_address_; } + uint32_t xffNumTrustedHops() const override { return 0; } Http::ForwardClientCertType forwardClientCert() override { return forward_client_cert_; } const std::vector& setCurrentClientCertDetails() const override { return set_current_client_cert_details_; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 521dc595fe9bc..8a2ee8ff9fe4f 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -96,6 +96,28 @@ TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWhenLocalHostRemoteAddress) EXPECT_EQ(local_address.ip()->addressAsString(), headers.get_(Headers::get().ForwardedFor)); } +// Verify that we trust Nth address from XFF when using remote address with xff_num_trusted_hops. +TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWithXFFTrustedHops) { + connection_.remote_address_ = std::make_shared("203.0.113.128"); + ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true)); + ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(1)); + TestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"}}; + EXPECT_EQ((MutateRequestRet{"198.51.100.1:0", false}), + callMutateRequestHeaders(headers, Protocol::Http2)); + EXPECT_EQ(headers.EnvoyExternalAddress()->value(), "198.51.100.1"); +} + +// Verify that xff_num_trusted_hops works when not using remote address. +TEST_F(ConnectionManagerUtilityTest, UseXFFTrustedHopsWithoutRemoteAddress) { + connection_.remote_address_ = std::make_shared("127.0.0.1"); + ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(false)); + ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(1)); + TestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.2, 198.51.100.1"}}; + EXPECT_EQ((MutateRequestRet{"198.51.100.2:0", false}), + callMutateRequestHeaders(headers, Protocol::Http2)); + EXPECT_EQ(headers.EnvoyExternalAddress(), nullptr); +} + // Verify that we don't set user agent when it is already set. TEST_F(ConnectionManagerUtilityTest, UserAgentDontSet) { connection_.remote_address_ = std::make_shared("10.0.0.1"); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 6e78771f4beed..c21b5b97d10d8 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -126,13 +126,22 @@ TEST(HttpUtility, parseHttp2Settings) { TEST(HttpUtility, getLastAddressFromXFF) { { - const std::string first_address = "34.0.0.1"; - const std::string second_address = "10.0.0.1"; - TestHeaderMapImpl request_headers{ - {"x-forwarded-for", fmt::format("{0}, {0}, {1}", first_address, second_address)}}; + const std::string first_address = "192.0.2.10"; + const std::string second_address = "192.0.2.1"; + const std::string third_address = "10.0.0.1"; + TestHeaderMapImpl request_headers{{"x-forwarded-for", "192.0.2.10, 192.0.2.1, 10.0.0.1"}}; auto ret = Utility::getLastAddressFromXFF(request_headers); + EXPECT_EQ(third_address, ret.address_->ip()->addressAsString()); + EXPECT_FALSE(ret.single_address_); + ret = Utility::getLastAddressFromXFF(request_headers, 1); EXPECT_EQ(second_address, ret.address_->ip()->addressAsString()); EXPECT_FALSE(ret.single_address_); + ret = Utility::getLastAddressFromXFF(request_headers, 2); + EXPECT_EQ(first_address, ret.address_->ip()->addressAsString()); + EXPECT_FALSE(ret.single_address_); + ret = Utility::getLastAddressFromXFF(request_headers, 3); + EXPECT_EQ(nullptr, ret.address_); + EXPECT_FALSE(ret.single_address_); } { TestHeaderMapImpl request_headers{{"x-forwarded-for", ""}}; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index f5ef41e149eb0..c5082c1c08ce0 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -53,6 +53,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(stats, ConnectionManagerStats&()); MOCK_METHOD0(tracingStats, ConnectionManagerTracingStats&()); MOCK_METHOD0(useRemoteAddress, bool()); + MOCK_CONST_METHOD0(xffNumTrustedHops, uint32_t()); MOCK_METHOD0(forwardClientCert, Http::ForwardClientCertType()); MOCK_CONST_METHOD0(setCurrentClientCertDetails, const std::vector&());