support multiple trusted proxies when processing XFF #2559

brian-pane · 2018-02-08T22:07:31Z

Description:
If the xff_num_trusted_hops config option is set to a number greater than zero, trust the specified number of additional addresses at the end of the X-Forwarded-For request header.

Risk Level: High
(because this change interacts with the internal/external request security model)

Testing: New test cases included

Docs Changes: #479

Release Notes: Included

Fixes: #2503

API Changes: #459

Signed-off-by: Brian Pane [email protected]

Signed-off-by: Brian Pane <[email protected]>

brian-pane · 2018-02-08T23:02:15Z

The CI errors for Mac and TSAN seem to be due to unrelated causes. I opened issue #2561 for the TSAN error.

…iest trusted remote address rather than immediate client address. Signed-off-by: Brian Pane <[email protected]>

mattklein123

In general looks great to me. Some random comments. Thanks for the careful change here. This code scares the shit out of me so would prefer to have multiple reviews. @alyssawilk can you give some attention to this on Monday?

mattklein123 · 2018-02-09T17:20:39Z

source/common/http/conn_manager_utility.cc

@@ -50,9 +50,24 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
  // our peer to have already properly set XFF, etc.
  Network::Address::InstanceConstSharedPtr final_remote_address;
  bool single_xff_address;
+  auto xff_num_trusted_hops = config.xffNumTrustedHops();


nit: prefer explicit type (not auto) for this type of code for clarity.

mattklein123 · 2018-02-09T17:24:32Z

source/common/http/conn_manager_utility.cc

    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.


nit: extend comment to say something like "do this before we append to XFF" (to explain the subtraction, etc.)

mattklein123 · 2018-02-09T17:29:14Z

source/common/http/conn_manager_utility.cc

+    // the xff_num_trusted_hops configuration option), get the trusted client address
+    // from the XFF.
+    if (xff_num_trusted_hops > 0) {
+      auto ret = Utility::getLastAddressFromXFF(request_headers, xff_num_trusted_hops - 1);


nit: I think you could just assign to final_remote_address directly via (...).address_

mattklein123 · 2018-02-09T17:31:13Z

source/common/http/utility.h

@@ -143,7 +143,8 @@ class Utility {
   * @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);


nit: update doc comments

mattklein123 · 2018-02-09T17:34:29Z

test/common/http/utility_test.cc

    TestHeaderMapImpl request_headers{
-        {"x-forwarded-for", fmt::format("{0}, {0}, {1}", first_address, second_address)}};
+        {"x-forwarded-for",
+         fmt::format("{0}, {1}, {2}", first_address, second_address, third_address)}};


nit: Can you just get rid of the fmt::format here and write out the header/strings directly? I think this just makes the test harder to read.

mattklein123 · 2018-02-09T17:39:37Z

source/common/http/utility.cc

@@ -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};


Q: Do you plan on enhancing the internal address detection behavior in the case of trusted proxies? I think the logic we have now is not-optimal. (Well, it was not optimal to begin with, now it's much less optimal). I point this out because if we change that behavior it will have to be configured and we will have to be super careful.

I can think of three approaches:

Add a configuration flag that changes the internal address detection to something like "the request is internal if and only if the trusted client address is an RFC1918/RFC4193 address."

When the new xff_num_trusted_hops is set, do the internal address detection based on the trusted client address. (And use the existing internal address logic if xff_num_trusted_hops is unset/zero.)

Leave the internal address detection unchanged.

Do you have a preference among those?

I would just do (3) unless you need the behavior to be otherwise. I was just pointing it out as an open issue that might need to be worked on in the future.

SGTM. Assuming the IP tagging filter operates on the trusted remote address, I'll just use that to differentiate internal from external.

Blerg, I'm torn on this.

I think if we're adding support for multiple xff headers, "the right thing to do" would be to consider it trusted if the direct connected host and N hops were trusted.

I was going to say given Brian's not using Envoy's notion of internal and touching auth code is scary, we could just leave it as a TODO. But it'd be uglier if we need to come around and clean this up later when someone wants it as it'd require yet another config option to avoid suddenly marking requests as internal for Envoy users.

I keep coming back to the fact the concept of "is this internal" is generally purpose useful but so far from my sample set of 3 (pinterest, lyft, google) the methods of determining "this is internal" and "what to do about it" are sufficiently user-specific I'm tempted to say we make everyone do their own filter, and move the current Lyft code to a reference implementation filter which could maybe even assert xff_num_trusted_hops was only 0 to show it was not supported. @mattklein123 WDYT? Is this more trouble than it's worth?

Signed-off-by: Brian Pane <[email protected]>

mattklein123 · 2018-02-09T22:32:32Z

test/common/http/utility_test.cc

    TestHeaderMapImpl request_headers{
-        {"x-forwarded-for", fmt::format("{0}, {0}, {1}", first_address, second_address)}};
+        {"x-forwarded-for", first_address + ", " + second_address + ", " + third_address}};


Sorry I actually meant remove the first_address, etc. variables and literally do:

{"x-forwarded-for", "129.0.2.10, 192.0.2.1, 10.0.0.1"}

:) But just a nit/preference if you feel otherwise.

mattklein123 · 2018-02-09T22:32:55Z

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;
+  uint32_t xff_num_trusted_hops = config.xffNumTrustedHops();


mattklein123

LGTM other than nits. Will defer to @alyssawilk for further review/merge.

Signed-off-by: Brian Pane <[email protected]>

Signed-off-by: Brian Pane <[email protected]> Signed-off-by: Brian Pane <[email protected]>

alyssawilk

Code LGTM insofar as it does what it says it does, and it's crazy well documented (yay!)

My concerns is I think we're adding more complexity to an already complex system, and even with great docs between the various combinations and permutations of use_remote_address and xff_number_trusted_hops, and inconsistency with how "trusted" works, I feel like any addition of complexity should come with some clean up :-)

Chatted with Brian offline and he had one thought for simplifying, which was only supporting xff_num_trusted_hops for nodes with use_remote_address==true which would at least remove the N vs N+1 hops confusion.

alyssawilk · 2018-02-12T18:20:57Z

source/common/http/utility.cc

@@ -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};


Blerg, I'm torn on this.

I think if we're adding support for multiple xff headers, "the right thing to do" would be to consider it trusted if the direct connected host and N hops were trusted.

I was going to say given Brian's not using Envoy's notion of internal and touching auth code is scary, we could just leave it as a TODO. But it'd be uglier if we need to come around and clean this up later when someone wants it as it'd require yet another config option to avoid suddenly marking requests as internal for Envoy users.

I keep coming back to the fact the concept of "is this internal" is generally purpose useful but so far from my sample set of 3 (pinterest, lyft, google) the methods of determining "this is internal" and "what to do about it" are sufficiently user-specific I'm tempted to say we make everyone do their own filter, and move the current Lyft code to a reference implementation filter which could maybe even assert xff_num_trusted_hops was only 0 to show it was not supported. @mattklein123 WDYT? Is this more trouble than it's worth?

mattklein123 · 2018-02-12T19:49:24Z

Chatted with Brian offline and he had one thought for simplifying, which was only supporting xff_num_trusted_hops for nodes with use_remote_address==true which would at least remove the N vs N+1 hops confusion.

TBH I could easily see someone asking for this to work even when use_remote_address is false. My inclination is to keep it as is, but I don't feel that strongly about it.

mattklein123 · 2018-02-12T19:52:19Z

I keep coming back to the fact the concept of "is this internal" is generally purpose useful but so far from my sample set of 3 (pinterest, lyft, google) the methods of determining "this is internal" and "what to do about it" are sufficiently user-specific I'm tempted to say we make everyone do their own filter, and move the current Lyft code to a reference implementation filter which could maybe even assert xff_num_trusted_hops was only 0 to show it was not supported. @mattklein123 WDYT? Is this more trouble than it's worth?

I think this is a great idea and something we should definitely consider. Please open a ticket to track. With that said, we absolutely cannot change the current behavior with causing chaos to not only Lyft but who knows who else (I think we all agree on that but just throwing it out there).

As you point out, this is a) ridiculously complicated and b) everyone has a slightly different definition of what internal/external means. I think my vote is to move forward with @brian-pane change as-is and then consider any further changes as potentially moving into some type of filter/plug-in system. Thoughts?

alyssawilk

Ok, my one holding point was I liked the simplicity of not having all 4 combinations of permutations of remote / xff but if you think it's worth keeping I'm otherwise fine.

@brian-pane can you open a issue ticket for clean up? You don't have to own it but we should at least note this is an area where help is wanted :-)

brian-pane · 2018-02-12T21:50:41Z

Ok, I've created issue 2590 for the follow-up.

* Update envoy-wasm sha Signed-off-by: Lizan Zhou <[email protected]> * fix due to ABI change * remove compile db Signed-off-by: Lizan Zhou <[email protected]> * fix for envoyproxy/envoy-wasm#216 Signed-off-by: Lizan Zhou <[email protected]> * fix ci * roll forward Signed-off-by: Lizan Zhou <[email protected]>

Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: JP Simard <[email protected]>

Brian Pane added 2 commits February 8, 2018 21:54

Compute trusted client address based on number of trusted front proxies

7911f30

Signed-off-by: Brian Pane <[email protected]>

Update release notes

4aad5d8

Signed-off-by: Brian Pane <[email protected]>

brian-pane changed the title ~~Trusted proxy/2503~~ support multiple trusted proxies when processing XFF Feb 8, 2018

When using xff_num_trusted_hops, set X-Envoy-External-Address to earl…

39ab59a

…iest trusted remote address rather than immediate client address. Signed-off-by: Brian Pane <[email protected]>

brian-pane mentioned this pull request Feb 9, 2018

Document the new xff_num_trusted_hops feature envoyproxy/data-plane-api#479

Merged

mattklein123 reviewed Feb 9, 2018

View reviewed changes

mattklein123 assigned mattklein123 and alyssawilk Feb 9, 2018

clean up based on code review feedback

07e2edc

Signed-off-by: Brian Pane <[email protected]>

mattklein123 reviewed Feb 9, 2018

View reviewed changes

Brian Pane added 2 commits February 9, 2018 23:02

additional cleanups

106d22f

Signed-off-by: Brian Pane <[email protected]>

Merge remote-tracking branch 'upstream/master' into trusted-proxy/2503

f182cf9

Signed-off-by: Brian Pane <[email protected]> Signed-off-by: Brian Pane <[email protected]>

alyssawilk reviewed Feb 12, 2018

View reviewed changes

alyssawilk approved these changes Feb 12, 2018

View reviewed changes

alyssawilk merged commit 40a2ee5 into envoyproxy:master Feb 12, 2018

brian-pane mentioned this pull request Feb 12, 2018

Improve the logic for XFF and internal/external client detection #2590

Closed

jpsim pushed a commit that referenced this pull request Nov 28, 2022

prefer using onEngineRunning over setLogger in test (#2559)

d792bdc

Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

prefer using onEngineRunning over setLogger in test (#2559)

88dadad

Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: JP Simard <[email protected]>

jamesog mentioned this pull request Jan 25, 2024

xff: add support for configuring a list of trusted CIDRs #31831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support multiple trusted proxies when processing XFF #2559

support multiple trusted proxies when processing XFF #2559

brian-pane commented Feb 8, 2018 •

edited

Loading

brian-pane commented Feb 8, 2018

mattklein123 left a comment

mattklein123 Feb 9, 2018

mattklein123 Feb 9, 2018

mattklein123 Feb 9, 2018

mattklein123 Feb 9, 2018

mattklein123 Feb 9, 2018

mattklein123 Feb 9, 2018

brian-pane Feb 9, 2018

mattklein123 Feb 9, 2018

brian-pane Feb 9, 2018

alyssawilk Feb 12, 2018

mattklein123 Feb 9, 2018

mattklein123 Feb 9, 2018

mattklein123 left a comment

alyssawilk left a comment

alyssawilk Feb 12, 2018

mattklein123 commented Feb 12, 2018

mattklein123 commented Feb 12, 2018

alyssawilk left a comment

brian-pane commented Feb 12, 2018

support multiple trusted proxies when processing XFF #2559

support multiple trusted proxies when processing XFF #2559

Conversation

brian-pane commented Feb 8, 2018 • edited Loading

brian-pane commented Feb 8, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Feb 12, 2018

mattklein123 commented Feb 12, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

brian-pane commented Feb 12, 2018

brian-pane commented Feb 8, 2018 •

edited

Loading