Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable configuration of the number of trusted addresses in XFF #459

Merged
merged 2 commits into from
Feb 6, 2018
Merged

enable configuration of the number of trusted addresses in XFF #459

merged 2 commits into from
Feb 6, 2018

Conversation

brian-pane
Copy link
Contributor

For envoyproxy/envoy issue #2503
Signed-off-by: Brian Pane [email protected]

@mattklein123
Copy link
Member

High level question: Do we want this on the HTTP connection manager level? Or the route level? I was thinking the former, but happy to discuss.

@wora
Copy link
Contributor

wora commented Feb 5, 2018 via email

@brian-pane
Copy link
Contributor Author

@mattklein123 I have a use case where the setting needs to be different for different virtual hosts within the same Envoy instance. To make the configuration simpler in the general case, what do you think about having this setting available at both the HTTP connection manager level and the virtual host level?

@mattklein123
Copy link
Member

If you need it to be different for different virtual hosts, I think it's reasonable to put it on the virtual host. Here is my concrete concern though:

We will consume this setting inside conn_man_impl when we first select the route and before request header transformations. It is possible that a filter could change the route before the request gets to the router, etc., which would nullify this setting. As I think about it more, I think this is probably fine, since the same is true potentially of other things that filters do, but it would probably be worth documenting. Thoughts?

Also, In the final docs can you please add a full description to https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/headers.html#config-http-conn-man-headers-x-forwarded-for? (Also feel free to edit/improve those docs as this is an area of confusion for people).

// Specifies the number of proxies between the origin client and this Envoy instance that
// are known to append an accurate source IP address to the X-Forwarded-For HTTP request
// header. The default is 1.
uint32 num_trusted_ingress_proxies = 12;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this defaults to 1, I think we should use the wrapped type, and put a constraint that it is >= 1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, per @wora I might try to better clarify what this is doing, and potentially but "xff" in the name somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on how the field is defined, we'll need to support a value of zero to handle the most common case. The default of 1 is just for backward compatibility with what appears to be a bug in how Envoy handles XFF (depending on how I interpret the current documentation).

The XFF docs docs say that "only the last address (furthest to the right) can be trusted." Read in isolation, that suggests that Envoy defaults to assuming a trusted HTTP proxy in front of it. However, other text in that section mentions that "Envoy uses the final XFF contents to determine whether a request originated externally or internally," where "final" seems to mean after Envoy has appended its immediate client's address. So it's not altogether clear from that section whether the current behavior is for Envoy to:

  1. Treat the unmodified ingress XFF as untrusted, but trust the immediate client's address that Envoy appends to XFF (which would match how other HTTP implementors handle incoming XFF)

Or:

  1. Treat the last non-RFC1918 in the unmodified ingress XFF (before Envoy has modified the XFF at all) as trusted (which would match my reading of the current docs, but it seems like a bug compared to how other implementations handle XFF).

If (1) is the right interpretation, then I'll make the default for this new field zero (and rewrite the XFF docs to clarify whether they're talking about the pre-modification or post-modification XFF).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read in isolation, that suggests that Envoy defaults to assuming a trusted HTTP proxy in front of it.

This depends on the "use_remote_address" setting. Can you take a look at that and see if that makes sense? (Basically if "use_remote_address" is set, Envoy will append to XFF and use that to determine trusted status, if it's not set, it won't append, and use XFF directly).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After mapping out the various potential configurations, I think it makes sense to have this new setting default to 0.

Let's call this new setting xff_num_trusted_hops (I'll update the PR to match). The semantics for determining the trusted origin client address would be:

  1. First, either do or don't append the immediate client's IP address to XFF, depending on the use_remote_address setting.
  2. Find the first non-RFC1918 address from the end of the XFF.
  3. (New step) Move forward xff_num_trusted_hops hops to the left from that location, and the result is the trusted origin client address.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow step (2), given that addresses cannot be implicitly trusted just because they are RFC1918. IMO isn't just (1) and (3) sufficient with a default of 0? Basically, assume that the address furthest to the right is the trusted address, unless xff_num_trusted_hops is set and then we go to the left?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My motivation in step (2) is to preserve the current semantics from the XFF docs, "More specifically, the first external (non RFC1918) address from the right is the only trustable addresses."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a bug in the docs effectively. Can you review the code to check me and make sure we are on the same page? This is some of the code that is most scary (and confusing) from a security perspective so want to make sure we are on the same page:
https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_utility.cc#L48

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, the current implementation doesn't seem to implement the semantics described in the docs. The implementation does this:

  1. If use_remote_address is set, the trusted remote address is the address of Envoy's immediate client, regardless of whether it's RFC1918.
  2. If use_remote_address is not set, the trusted remote address is the last address in XFF, again regardless of whether it's RFC1918. If there's no XFF, though, the trusted remote address is the address of Envoy's immediate client.

Which should I preserve, the semantics described in the docs or the current behavior of the current implementation? The implementation is more consistent with how I've seen other HTTP implementors handle XFF, but the documentation is so specific about differentiating public and RFC1918 addresses that I can't tell which one should be considered authoritative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 I just saw your reply after I posted mine. I'll build the new feature on top of the implementation semantics, and change the docs to match.

@brian-pane
Copy link
Contributor Author

Two notes:

  • For now, I've just put the new option at the http_connection_manager level, to keep the semantics from getting even more complicated. I'll defer the route-level override capability to a later issue+PR.
  • I just created docs: clarify the x-forwarded-for documentation #463 to try to make the current docs match the current implementation, in hopes that the result will be a cleaner starting point for documenting this new feature.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for working through this with me. @alyssawilk can you take a look at this and the long discussion thread to make sure we are all on the same page?

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. My request is when we unhide this and add full docs to http_conn_man, can we have actual examples of the configuration options and the headers which are trusted? After reading the long thread my main take-away is that this is super confusing, it's easy to get wrong, and there's security implications to getting it wrong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants