Conflicting `XFF` and `xff_trusted_cidr` documentation

[jaellio]

Conflicting XFF and xff_trusted_cidr documentation

Description:

The current documentation for using xff_trusted_cidrs for selecting the trusted original address is as follows:

• If the remote address is contained by an entry in xff_trusted_cidrs, and the last
  (rightmost) entry is also contained by an entry in xff_trusted_cidrs, the trusted client
  address is second-last IP address in XFF.

Based on the docs I'd expect the second to last IP address in the XFF header to be used as the original client address. The docs conflict with a later example:

 Example 7: Envoy as edge proxy, with one trusted CIDR
 Settings:
   | use_remote_address = false
   | xff_trusted_cidrs = 192.0.2.0/24

 Request details:
   | Downstream IP address = 192.0.2.5
   | XFF = "203.0.113.128, 203.0.113.10, 192.0.2.1"

 Result:
   | Trusted client address = 192.0.2.1
   | X-Envoy-External-Address is set to 192.0.2.1
   | XFF is changed to "203.0.113.128, 203.0.113.10, 192.0.2.1, 192.0.2.5"
   | X-Envoy-Internal is removed (if it was present in the incoming request)

Note the trusted client address is set to 192.0.2.1. Based on the documentation I would have expected the trusted client address to be set to 203.0.113.10. I thought maybe the second-last address was evaluated after appending the downstream IP address to the XFF but this also conflicts with a later example:

    Example 8: Envoy as edge proxy, with two trusted CIDRs
    Settings:
      | use_remote_address = false
      | xff_trusted_cidrs = 192.0.2.0/24, 198.51.100.0/24

    Request details:
      | Downstream IP address = 192.0.2.5
      | XFF = "203.0.113.128, 203.0.113.10, 198.51.100.1"

    Result:
      | Trusted client address = 203.0.113.10
      | X-Envoy-External-Address is set to 203.0.113.10
      | XFF is changed to "203.0.113.128, 203.0.113.10, 198.51.100.1, 192.0.2.5"
      | X-Envoy-Internal is removed (if it was present in the incoming request)

In example 8, the trusted client address is set to 203.0.113.10 which is the expected second-last XFF IP.

Could someone clarify the expected behavior and if this behavior verified in a unit test?

[grnmeira]

I just ran the following unit tests:

TEST_F(XffTrustedCidrsTest, Example7) {
  envoy::extensions::http::original_ip_detection::xff::v3::XffConfig config;

  config.mutable_xff_trusted_cidrs()->clear_cidrs();
  auto cidr1 = config.mutable_xff_trusted_cidrs()->add_cidrs();
  cidr1->set_address_prefix("192.0.2.0");
  cidr1->mutable_prefix_len()->set_value(24);

  xff_extension_ = *XffIPDetection::create(config);

  Envoy::Http::TestRequestHeaderMapImpl headers{
      {"x-forwarded-for", "203.0.113.128, 203.0.113.10, 192.0.2.1"}};

  auto remote_address = std::make_shared<Network::Address::Ipv4Instance>("192.0.2.5");
  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.1", result.detected_remote_address->ip()->addressAsString());
}

TEST_F(XffTrustedCidrsTest, Example8) {
  envoy::extensions::http::original_ip_detection::xff::v3::XffConfig config;

  config.mutable_xff_trusted_cidrs()->clear_cidrs();
  auto cidr1 = config.mutable_xff_trusted_cidrs()->add_cidrs();
  cidr1->set_address_prefix("192.0.2.0");
  cidr1->mutable_prefix_len()->set_value(24);
  auto cidr2 = config.mutable_xff_trusted_cidrs()->add_cidrs();
  cidr2->set_address_prefix("198.51.100.0");
  cidr2->mutable_prefix_len()->set_value(24);

  xff_extension_ = *XffIPDetection::create(config);

  Envoy::Http::TestRequestHeaderMapImpl headers{
      {"x-forwarded-for", "203.0.113.128, 203.0.113.10, 198.51.100.1"}};

  auto remote_address = std::make_shared<Network::Address::Ipv4Instance>("192.0.2.5");
  Envoy::Http::OriginalIPDetectionParams params = {headers, remote_address};
  auto result = xff_extension_->detect(params);
  ASSERT_NE(result.detected_remote_address, nullptr);
  EXPECT_EQ("203.0.113.10", result.detected_remote_address->ip()->addressAsString());
}

Example 7 fails to pass, as it presents the wrong detected address. Also, even though it uses use_remote_address=false, it's appending to the XFF header.

[grnmeira]

Happy to get this assigned to me once it's triaged. We could fix the documentation and make sure those examples are addressed by test cases.

[adisuissa]

Thanks for raising this issue.
cc @jamesog as the feature was introduced in #31831.
cc @alyssawilk @mattklein123 as codeowners

[krinkinmu]

@grnmeira

I just ran the following unit tests:

TEST_F(XffTrustedCidrsTest, Example7) {
  envoy::extensions::http::original_ip_detection::xff::v3::XffConfig config;

  config.mutable_xff_trusted_cidrs()->clear_cidrs();
  auto cidr1 = config.mutable_xff_trusted_cidrs()->add_cidrs();
  cidr1->set_address_prefix("192.0.2.0");
  cidr1->mutable_prefix_len()->set_value(24);

  xff_extension_ = *XffIPDetection::create(config);

  Envoy::Http::TestRequestHeaderMapImpl headers{
      {"x-forwarded-for", "203.0.113.128, 203.0.113.10, 192.0.2.1"}};

  auto remote_address = std::make_shared<Network::Address::Ipv4Instance>("192.0.2.5");
  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.1", result.detected_remote_address->ip()->addressAsString());
}

TEST_F(XffTrustedCidrsTest, Example8) {
  envoy::extensions::http::original_ip_detection::xff::v3::XffConfig config;

  config.mutable_xff_trusted_cidrs()->clear_cidrs();
  auto cidr1 = config.mutable_xff_trusted_cidrs()->add_cidrs();
  cidr1->set_address_prefix("192.0.2.0");
  cidr1->mutable_prefix_len()->set_value(24);
  auto cidr2 = config.mutable_xff_trusted_cidrs()->add_cidrs();
  cidr2->set_address_prefix("198.51.100.0");
  cidr2->mutable_prefix_len()->set_value(24);

  xff_extension_ = *XffIPDetection::create(config);

  Envoy::Http::TestRequestHeaderMapImpl headers{
      {"x-forwarded-for", "203.0.113.128, 203.0.113.10, 198.51.100.1"}};

  auto remote_address = std::make_shared<Network::Address::Ipv4Instance>("192.0.2.5");
  Envoy::Http::OriginalIPDetectionParams params = {headers, remote_address};
  auto result = xff_extension_->detect(params);
  ASSERT_NE(result.detected_remote_address, nullptr);
  EXPECT_EQ("203.0.113.10", result.detected_remote_address->ip()->addressAsString());
}

Example 7 fails to pass, as it presents the wrong detected address. Also, even though it uses use_remote_address=false, it's appending to the XFF header.

I think that's actually working as intended, basically we have two options:

1. use_remote_address=true
2. use_remote_address=false + xff extension.

We looked at the code today with @jaellio, and when it comes to the option 2 (when we use xff extension), xff is always updated unless you explicitly tell it not to. And that behavior makes sense to me for the edge proxy use cases.

The examples still seem incorrect though, particularly, as you pointed out the trusted client IP address in example 7 is wrong, both from the point of view of what is actually implemented and from the point of view of what we want the trusted client IP address to be.

My understanding of the behavior we want here is that if the downstream address is trusted, then the trusted IP client should be the rightmost untrusted IP in XFF. The logic is based on the idea that trusted IP addresses represent trusted proxies that correctly update XFF. Each trusted proxy appends to XFF the downstream address it got the request from.

If we got request from a trusted downstream address, then we can trust that the proxy before us appended a correct address to XFF, so the last entry in XFF is correct. If the last entry in XFF is itself a trusted IP, then we can be sure that entry it added to XFF is also correct, thus we can trust a second to last entry in XFF, and so on and so forth, until we find an entry we cannot actually trust (the entry that was added to XFF by the first trusted proxy in the chain of trust).

Returning to the example 7, I think, the trusted client IP address should be 203.0.113.10 and not 192.0.2.1, because both downstream IP address (192.0.2.5) and the last entry in XFF (192.0.2.1) are trusted.

[jaellio]

Perhaps it would be helpful to rename getLastNonTrustedAddressFromXFF to getTrustedClientAddressFromXFF or at least add a comment clarifying this functionality.

[grnmeira]

/assign @grnmeira

[grnmeira]

I think that's actually working as intended, basically we have two options:

1. use_remote_address=true
2. use_remote_address=false + xff extension.

We looked at the code today with @jaellio, and when it comes to the option 2 (when we use xff extension), xff is always updated unless you explicitly tell it not to. And that behavior makes sense to me for the edge proxy use cases.

Oops my bad. I made a poor judgement based only on the XFF extension without considering the full implementation in "conn_manager_utility_test".

[jamesog]

The analysis here looks spot on, I did indeed mess up example 7 and @jaellio is correct about what it should actually say. Sorry for the mixup! #31831 went on for months and had a lot of refactoring so I'm not surprised I ended up confusing myself with the examples 😞