Possible discrepancy between docs and implementation for X-Envoy-Internal

[mpuncel]

Title
Possible discrepancy between docs and implementation for X-Envoy-Internal

Description
I've been taking a look at how Envoy determines whether HTTP requests are internal, and I think there might be a discrepancy between what the docs say and what actually happens, particularly when use_remote_address is false on the HTTP connection manager.

The docs for X-Forwarded-For say:

If use_remote_address is false and an XFF containing at least one IP address is present in the request, the trusted client address is the last (rightmost) IP address in XFF.
Otherwise, the trusted client address is the source IP address of the immediate downstream node’s connection to Envoy.

That seems different than what was described in #2232 which says that X-Envoy-Internal is set if:

1. XFF is present (either via use_remote_address or in request headers).
2. XFF contains a single address after any appending from use_remote_address.
3. The address is valid.

In other words, it seems like there is no way to consider a request internal unless X-Forwarded-For is set. The docs, however, suggest that even if use_remote_address: is false, and X-Forwarded-For is empty, the remote address will be used to determine whether the request is internal. This is most clearly demonstrated in Example 5 from the x-forwarded-for doc:

Example 5: Envoy as an internal proxy, receiving a request from an internal client
Settings:
use_remote_address = false
xff_num_trusted_hops = 0
Request details:
Downstream IP address = 10.20.30.40 (address of the internal client)
XFF is not present
Result:
Trusted client address = 10.20.30.40
X-Envoy-External-Address remains unset
X-Envoy-Internal is set to “true”

A bit about our use case: we are currently not using Envoy at the edge at all, just for an internal service-to-service mesh. Currently we do not set use_remote_address and leave xff_num_trusted_hops at zero. We believe that leaving use_remote_address to false makes sense because Envoy is not at the edge and the docs say:

In general, use_remote_address should be set to true when Envoy is deployed as an edge node (aka a front proxy), whereas it may need to be set to false when Envoy is used as an internal service node in a mesh deployment.

It seems like we have the following options:

1. set use_remote_address and skip_xff_append everywhere. That means Envoy will use the remote address to determine whether a request is internal but will not append to XFF. We will leave xff_num_trusted_hops to zero as well. This approach does well as long as every host in the mesh has internal IP addresses
2. add a config option like always_internal to HTTP connection manager. This would make sense for setups (like ours) where the listener associated with the HTTP connection manager is validating TLS certs against an internal CA, therefore we can always trust requests we get as internal (provided we do the right sanitizing of headers at our edge).

I'm interested in thoughts on the docs here as well as suggestions for how we might set up our internal Envoy setup to consider requests internal, either based on remote IP or via a flag to set internal at the listener/conn manager level

[htuch]

I'd need to refresh my mental cache of how this all works. IMHO it's quite complicated and this issue points to the fact we could stand to simplify. I know @PiotrSikora has been poking around at possible changes in this space, any thoughts here Piotr?

[mattklein123]

As @htuch said this is ridiculously complicated, but as I just mentioned in another issue, it's very difficult to outright change this behavior without regressing behavior that someone is depending on. We also need to be super careful about how we do it.

@mpuncel I'm not sure I fully follow the exact problem you think there is either with the code or docs. Can you clarify? Here is the code in question if you want to follow along: https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_utility.cc#L105

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.