support multiple trusted proxies when processing XFF by brian-pane · Pull Request #2559 · envoyproxy/envoy

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 bpane@pinterest.com

Signed-off-by: Brian Pane <bpane@pinterest.com>

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 <bpane@pinterest.com>

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

  // 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

   */
-  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

    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 <bpane@pinterest.com>

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

test/common/http/utility_test.cc

+    const std::string third_address = "10.0.0.1";
    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

  // 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 <bpane@pinterest.com>

Signed-off-by: Brian Pane <bpane@pinterest.com> Signed-off-by: Brian Pane <bpane@pinterest.com>

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

    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 <lizan@tetrate.io> * fix due to ABI change * remove compile db Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix for envoyproxy/envoy-wasm#216 Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix ci * roll forward Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Signed-off-by: Snow Pettersen <snowp@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

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 <bpane@pinterest.com>

Update release notes

4aad5d8

Signed-off-by: Brian Pane <bpane@pinterest.com>

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 <bpane@pinterest.com>

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 <bpane@pinterest.com>

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 <bpane@pinterest.com>

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

f182cf9

Signed-off-by: Brian Pane <bpane@pinterest.com> Signed-off-by: Brian Pane <bpane@pinterest.com>

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 <snowp@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

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 <snowp@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jamesog mentioned this pull request Jan 25, 2024

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

Merged

Conversation

brian-pane commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-pane commented Feb 8, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Feb 12, 2018

Uh oh!

mattklein123 commented Feb 12, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

brian-pane commented Feb 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brian-pane commented Feb 8, 2018 •

edited

Loading