envoyproxy · alyssawilk · Feb 12, 2018 · Feb 7, 2018 · Feb 8, 2018 · Feb 8, 2018
diff --git 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
@@ -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
@@ -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
@@ -239,14 +239,24 @@ 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};
   }
 
   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
@@ -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
@@ -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
@@ -90,6 +90,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   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<Http::ClientCertDetailsType>& setCurrentClientCertDetails() const override {
     return set_current_client_cert_details_;
@@ -114,6 +115,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   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<Http::ClientCertDetailsType> 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
@@ -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
@@ -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<Http::ClientCertDetailsType>& 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
@@ -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<Network::Address::Ipv4Instance>("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<Network::Address::Ipv4Instance>("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<Network::Address::Ipv4Instance>("10.0.0.1");

diff --git 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
@@ -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<Http::ClientCertDetailsType>&());