http: Use last address in XFF when all are trusted

jamesog · jamesog · commit a016a6cf9416 · 2024-01-24T17:18:26.000Z
When parsing XFF using a list of trusted CIDRs, if all IP in XFF
match the trusted CIDRs (there are no untrusted IPs), return the first
IP in XFF (the last evaluated) - and a test to confirm the behaviour.

Signed-off-by: James O'Gorman &lt;james@netinertia.co.uk&gt;
diff --git a/api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto b/api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto
@@ -41,7 +41,8 @@ message XffConfig {
   //
   // If ``recurse`` is set the :ref:`config_http_conn_man_headers_x-forwarded-for`
   // header is also evaluated against the trusted CIDR list, and the last non-trusted
-  // address is used as the original client address.
+  // address is used as the original client address. If all addresses in ``x-forwarded-for``
+  // are within the trusted list, the first entry is used.
   //
   // This is typically used when requests are proxied by a
   // `CDN <https://en.wikipedia.org/wiki/Content_delivery_network>`_.
@@ -55,8 +56,9 @@ message XffTrustedCidrs {
   // connections are considered trusted.
   repeated config.core.v3.CidrRange cidrs = 1;
 
-  // If ``true``, recurse through the :ref:`config_http_conn_man_headers_x-forwarded-for`
-  // HTTP header to find the first (from the right) non-trusted IP address; otherwise
-  // the last address in ``x-forwarded-for`` is used.
+  // If ``true``, recurse backwards through the :ref:`config_http_conn_man_headers_x-forwarded-for`
+  // HTTP header to find the first (from the right) non-trusted IP address. If all addresses
+  // are trusted, the last (from the right) IP address is returned.
+  // Otherwise the last address in ``x-forwarded-for`` is used.
   google.protobuf.BoolValue recurse = 2;
 }
diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc
@@ -805,12 +805,14 @@ Utility::GetLastAddressFromXffInfo Utility::getLastNonTrustedAddressFromXFF(
   static constexpr absl::string_view separator(",");
 
   const auto xff_entries = StringUtil::splitToken(xff_string, separator, false, true);
+  Network::Address::InstanceConstSharedPtr last_valid_addr;
 
   for (auto it = xff_entries.rbegin(); it != xff_entries.rend(); it++) {
     auto addr = Network::Utility::parseInternetAddressNoThrow(std::string(*it));
     if (addr == nullptr) {
       continue;
     }
+    last_valid_addr = addr;
 
     bool trusted = false;
     for (const auto& cidr : trusted_cidrs) {
@@ -834,7 +836,9 @@ Utility::GetLastAddressFromXffInfo Utility::getLastNonTrustedAddressFromXFF(
     }
     return {addr, true};
   }
-  return {nullptr, false};
+  // If we reach this point all addresses in XFF were considered trusted, so return
+  // first IP in XFF (the last in the reverse-evaluated chain).
+  return {last_valid_addr, true};
 }
 
 Utility::GetLastAddressFromXffInfo
diff --git a/test/extensions/http/original_ip_detection/xff/xff_test.cc b/test/extensions/http/original_ip_detection/xff/xff_test.cc
@@ -129,6 +129,17 @@ TEST_F(XffTrustedCidrsRecurseTest, Recurse) {
   EXPECT_EQ("2.2.2.2", result.detected_remote_address->ip()->addressAsString());
 }
 
+TEST_F(XffTrustedCidrsRecurseTest, RecurseAllAddressesTrusted) {
+  Envoy::Http::TestRequestHeaderMapImpl headers{
+      {"x-forwarded-for", "192.0.2.4,192.0.2.5, 198.51.100.1"}};
+
+  auto remote_address = std::make_shared<Network::Address::Ipv4Instance>("192.0.2.11");
+  Envoy::Http::OriginalIPDetectionParams params = {headers, remote_address};
+  auto result = xff_extension_->detect(params);
+  ASSERT_NE(result.detected_remote_address, nullptr);
+  EXPECT_EQ("192.0.2.4", result.detected_remote_address->ip()->addressAsString());
+}
+
 } // namespace Xff
 } // namespace OriginalIPDetection
 } // namespace Http

Original file line number	Diff line number	Diff line change
`@@ -805,12 +805,14 @@ Utility::GetLastAddressFromXffInfo Utility::getLastNonTrustedAddressFromXFF(`
`805`	`805`	`static constexpr absl::string_view separator(",");`
`806`	`806`
`807`	`807`	`const auto xff_entries = StringUtil::splitToken(xff_string, separator, false, true);`
	`808`	`+ Network::Address::InstanceConstSharedPtr last_valid_addr;`
`808`	`809`
`809`	`810`	`for (auto it = xff_entries.rbegin(); it != xff_entries.rend(); it++) {`
`810`	`811`	`auto addr = Network::Utility::parseInternetAddressNoThrow(std::string(*it));`
`811`	`812`	`if (addr == nullptr) {`
`812`	`813`	`continue;`
`813`	`814`	`}`
	`815`	`+ last_valid_addr = addr;`
`814`	`816`
`815`	`817`	`bool trusted = false;`
`816`	`818`	`for (const auto& cidr : trusted_cidrs) {`
`@@ -834,7 +836,9 @@ Utility::GetLastAddressFromXffInfo Utility::getLastNonTrustedAddressFromXFF(`
`834`	`836`	`}`
`835`	`837`	`return {addr, true};`
`836`	`838`	`}`
`837`		`- return {nullptr, false};`
	`839`	`+ // If we reach this point all addresses in XFF were considered trusted, so return`
	`840`	`+ // first IP in XFF (the last in the reverse-evaluated chain).`
	`841`	`+ return {last_valid_addr, true};`
`838`	`842`	`}`
`839`	`843`
`840`	`844`	`Utility::GetLastAddressFromXffInfo`