The`xff_num_trusted_hops` in XffIPDetection is not aligned with the doc and the `xffNumTrustedHops` option in HCM

[zhaohuabing]

It seems that there is an inconsistency between the two approaches of getting remote IP from the XFF header.

The same request:

curl  --header "X-Forwarded-For: 10.0.0.4,10.0.2.1,10.0.0.5" http://172.18.255.203:80/protected2

With this configuration

"name": "envoy.filters.network.http_connection_manager",
"typedConfig": {
  "@type": "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager",
  // omitted for brevity
  ...

  "originalIpDetectionExtensions": [
    {
      "name": "envoy.extensions.http.original_ip_detection.xff",
      "typedConfig": {
        "@type": "type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig",
        "xffNumTrustedHops": 2
      }
    }
  ]

The remoteIP is the 10.0.0.4:0, the third rightmost IP, as the following log shows:

[2024-05-20 00:34:03.604][62][debug][rbac] [source/extensions/filters/http/rbac/rbac_filter.cc:131] checking request: requestedServerName: , sourceIP: 172.18.0.1:50822, directRemoteIP: 172.18.0.1:50822, remoteIP: 10.0.0.4:0,localAddress: 10.244.0.101:10080, ssl: none, headers: ':authority', 'www.example.com'

But with this configuration:

"name": "envoy.filters.network.http_connection_manager",
"typedConfig": {
  "@type": "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager",
  // omitted for brevity
  ...
   
  "useRemoteAddress": true,
  "xffNumTrustedHops": 2
}

The remoteIP is the 10.0.2.1, the second rightmost IP, as the following log shows:

[2024-05-20 00:33:18.581][98][debug][rbac] [source/extensions/filters/http/rbac/rbac_filter.cc:131] checking request: requestedServerName: , sourceIP: 172.18.0.1:46138, directRemoteIP: 172.18.0.1:46138, remoteIP: 10.0.2.1:0,localAddress: 10.244.0.101:10080, ssl: none, headers: ':authority', 'www.example.com'

According to the Envoy docs, the correct xxfNumTrustedHops should be 2 here.

Example 3: Envoy as edge proxy, with two trusted external proxies in front of it Settings: use_remote_address = true xff_num_trusted_hops = 2

The inconsistency seems comes from:

https://github.com/envoyproxy/envoy/blob/b65de1f56850326e1c6b74aa72cb1c9777441065/source/extensions/http/original_ip_detection/xff/xff.cc#L21

https://github.com/envoyproxy/envoy/blob/b65de1f56850326e1c6b74aa72cb1c9777441065/source/common/http/conn_manager_utility.cc#L128

[zuercher]

cc @alyssawilk

[alyssawilk]

are you setting use remote address in both cases? AFIK if you set num hops =2 through either method it should be using exactly the same code

[github-actions[bot]]

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 "no stalebot" or other activity occurs. Thank you for your contributions.

[zhaohuabing]

@alyssawilk

The inconsistency seems to come from the below code. Even though they call the same function, but the input parameters are different xff_num_trusted_hops_ vs xff_num_trusted_hops -1:

https://github.com/envoyproxy/envoy/blob/b65de1f56850326e1c6b74aa72cb1c9777441065/source/extensions/http/original_ip_detection/xff/xff.cc#L21

https://github.com/envoyproxy/envoy/blob/b65de1f56850326e1c6b74aa72cb1c9777441065/source/common/http/conn_manager_utility.cc#L128

[github-actions[bot]]

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 "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions[bot]]

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" or "no stalebot". Thank you for your contributions.

[rudrakhp]

@alyssawilk any comments on this disparity? Seems to similar to an issue I am facing #37140. Thanks!

[alyssawilk]

This one predates me, but it has always been the case that if use_remote_address is true we Utility::getLastAddressFromXFF(request_headers, xff_num_trusted_hops - 1).address_; and if use_remote_address is false we auto ret = Utility::getLastAddressFromXFF(request_headers, xff_num_trusted_hops);

It's easier to see before the refactor in this code:

https://github.com/envoyproxy/envoy/commit/beac1ece7512e6e39b4f1c29490e247996a0f51c#diff-03ebc558c95a058c0563535a9624a50261893ac80f88d6ad247a7a0c9dc05e57

If you look at the docs for use_remote_address

(BoolValue) If set to true, the connection manager will use the real remote address of the client connection when determining internal versus external origin

So I believe the -1 for that branch is because the remote address is treated as the last proxy in the hop, even if not in the XFF header. I think this is WAI if not working as expected?

[rudrakhp]

@alyssawilk Thanks for the response! It definitely makes sense based on what use_remote_address is set to, but if you check out the example configurations in #37140 we are forced to set use_remote_address to false to use original IP detection extension, which changes behaviour if usage was with use_remote_address set to true before. From a control plane perspective, how can we ensure the same behaviour when moving to extension from HCM? Also curious as to why use_remote_address has to be false for using the extension?

[alyssawilk]

My guess is the author only ever needed the one use case so didn't add hooks for the other. if you need cidr matching for both cases you could add it inline in ConnectionManagerUtility::mutateRequestHeaders or factor out the config.useRemoteAddress code to also use extensions. Either is fine as long as the result is no behavioral change for anyone else :-)

[rudrakhp]

We could decrement the number of hops to maintain backward compatibility. From the docs looks like the following configurations should behave the same as useRemoteAddress is used by HCM only for determining original IP, which will now be done by the extension. HCM:

useRemoteAddress: true
xffNumTrustedHops: 2

Extension:

useRemoteAddress: false
originalIpDetectionExtensions:
- name: envoy.extensions.http.original_ip_detection.xff
  typedConfig:
    '@type': type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig
    skipXffAppend: false
    xffNumTrustedHops: 1

@alyssawilk any side effects you can think of that we need to be concerned about? Thanks!

[alyssawilk]

sorry - don't recall the details without digging into the code :-(

[github-actions[bot]]

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 "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions[bot]]

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" or "no stalebot". Thank you for your contributions.