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

feat: Support Client IP Detection using XFF on ClientTrafficPolicy #2535

Merged
merged 15 commits into from
Feb 3, 2024

Conversation

davidalger
Copy link
Contributor

@davidalger davidalger commented Jan 29, 2024

What type of PR is this?

feat: Support Client IP Detection using XFF on ClientTrafficPolicy

What this PR does / why we need it:

Implements client IP detection support on ClientTrafficPolicy using xff_num_trusted_hops allowing listeners to be configured to trust XFF headers such as x-forwarded-for and x-forwarded-proto

Which issue(s) this PR fixes:

Related to #2252

@davidalger davidalger requested a review from a team as a code owner January 29, 2024 21:10
@davidalger davidalger force-pushed the algerdev/http-conn-mgr-settings branch 2 times, most recently from a91f771 to 622273f Compare January 29, 2024 21:54
…Policy

Signed-off-by: David Alger <davidmalger@gmail.com>
@davidalger davidalger force-pushed the algerdev/http-conn-mgr-settings branch from 622273f to c3d18a4 Compare January 29, 2024 22:02
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (1754679) 64.50% compared to head (e23da95) 64.46%.

Files Patch % Lines
internal/ir/zz_generated.deepcopy.go 0.00% 18 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2535      +/-   ##
==========================================
- Coverage   64.50%   64.46%   -0.05%     
==========================================
  Files         116      116              
  Lines       17757    17790      +33     
==========================================
+ Hits        11455    11468      +13     
- Misses       5560     5579      +19     
- Partials      742      743       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arkodg
Copy link
Contributor

arkodg commented Jan 29, 2024

thanks for building this out @davidalger . Here's how I'm hoping we can make progress in this PR

  • get some consensus that ClientTrafficPolicy is the right home for this field vs in EnvoyProxy
    Youve already highlighted this - since EnvoyProxy attaches to a GatewayClass, it can affect multiple Gateways and corresponding Envoy Proxy fleets, so introducing the field in ClientTrafficPolicy does make sense, im a +1 for this
  • I'd prefer if we created a top level field called headers to hold all fields that affect proxy header transformations. Once we have consensus on this, we'd need to make that change first @liorokman
  • I'd prefer if we skipped introducing the use_remote_address field in this PR

@liorokman
Copy link
Contributor

  • I'd prefer if we created a top level field called headers to hold all fields that affect proxy header transformations. Once we have consensus on this, we'd need to make that change first @liorokman

I'm +1 for that. In fact, I've already proposed this in #2500 .

@davidalger
Copy link
Contributor Author

davidalger commented Jan 30, 2024

I'd prefer if we created a top level field called headers to hold all fields that affect proxy header transformations. Once we have consensus on this, we'd need to make that change first @liorokman

I like the idea of organizing things better, but not 100% sure headers is the right place to put it since XFF isn't just about header transformation and falls more into general IP detection territory.

@arkodg Since HCM supports either a combination of use_remote_address and xff_num_trusted_hops or using original_ip_detection_extensions for this, what are your thoughts on the following structure?

originalIpDetection:
  useRemoteAddress: true    # True by default, false if `extensions` is specified (not present in CRD, as suggested)
  xffNumTrustedHops: number # Mutually exclusive with `extensions` configures `xff_num_trusted_hops` HCM built-in
  extensions:
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: string
      rejectWithStatus: enum
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig
    xff:
      numTrustedHops: number

Supporting config for the custom headers IP detection extension will be of use to me in deployments of EG, so even if precluded from this MR for sake of keeping it single-purpose I would like to consider planning for it and can introduce it in a future change set.

…pDetection

Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
@davidalger davidalger force-pushed the algerdev/http-conn-mgr-settings branch from cabbb38 to e6de174 Compare January 30, 2024 15:26
…mgr-settings

Signed-off-by: David Alger <davidmalger@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Jan 30, 2024

so we have two options here, (please ignore the exact names of the fields)

  1. add this config under a newly created headers field
headers:
  xForwardedFor:
    numTrustedHops:
  1. add this config under a dedicated field around IP detection / network topology
ipDetection:
  numTrustedHops

cc @envoyproxy/gateway-maintainers
will bring this up in the community meeting this week

Signed-off-by: David Alger <davidmalger@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Jan 31, 2024

discussed this in the community meeting today and folks prefer 2 over 1 (generic headers section)

  • brainstorming some names - what should the top level field be - remoteIP or originalIP or remoteIPDetection or originalIPDetection or originatingIP.... or clientIP..... Whichever term we decide will need to be used in other APIs (access control and RateLimit)

  • sidenote (not related to this PR) - should proxy protocol config be moved under this field ? cc @envoyproxy/gateway-maintainers

@davidalger
Copy link
Contributor Author

brainstorming some names - what should the top level field be - remoteIP or originalIP or remoteIPDetection or originalIPDetection or originatingIP.... or clientIP..... Whichever term we decide will need to be used in other APIs (#2250 and RateLimit)

clientIPDetection makes sense to me given the term 'client IP' lines up with what's used used to document sourceCIDR on RateLimit already and also has widespread use across the broader industry (Google LBs, CloudFlare, Nginx, etc) and it's meaning should be widely understood

@arkodg
Copy link
Contributor

arkodg commented Jan 31, 2024

brainstorming some names - what should the top level field be - remoteIP or originalIP or remoteIPDetection or originalIPDetection or originatingIP.... or clientIP..... Whichever term we decide will need to be used in other APIs (#2250 and RateLimit)

clientIPDetection makes sense to me given the term 'client IP' lines up with what's used used to document sourceCIDR on RateLimit already and also has widespread use across the broader industry (Google LBs, CloudFlare, Nginx, etc) and it's meaning should be widely understood

sg @davidalger, lets go with clientIpDetection !

…mgr-settings

Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
@davidalger davidalger force-pushed the algerdev/http-conn-mgr-settings branch from aed86d9 to 76f64a9 Compare February 1, 2024 16:22
@davidalger
Copy link
Contributor Author

@arkodg Implemented first pass here with the following structure:

clientIPDetection:
  xffNumTrustedHops: number # Mutually exclusive with `extensions` configures `xff_num_trusted_hops` HCM built-in
  extensions:
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: string
      rejectWithStatus: number
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig
    xff:
      numTrustedHops: number

use_remote_address maintains it's default of true while being set to false if original IP detection extensions are used since the two are mutually exclusive

Once envoyproxy/envoy#31831 is merged, I would see there being an additional field to control the trusted CIDRs:

clientIPDetection:
  extensions:
    xff:
      trustedCIDRs:
      - "130.211.0.0/22"
      - "35.191.0.0/16"

Would it be better to keep things a little more flat given this won't support arbitrary extensions?

clientIPDetection:
    xffNumTrustedHops: 2
    customHeaderExtension:
      // ...
    xffExtension:
      // ...

@davidalger davidalger changed the title feat: HTTPConnectionManager settings for XFF headers on ClientTrafficPolicy feat: Support Client IP Detection settings on ClientTrafficPolicy Feb 1, 2024
Signed-off-by: David Alger <davidmalger@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Feb 1, 2024

@davidalger curious if you know the difference between the numTrustedHops field in the top level vs extension setting in envoy ?

@davidalger
Copy link
Contributor Author

curious if you know the difference between the numTrustedHops field in the top level vs extension setting in envoy?

At the moment the only difference I believe is the extensions were marked incompatible with use_remote_address, but otherwise the XFF extension is identical in function currently.

It's been asked by maintainers to implement additions to XFF handling in original_ip_detection_extensions and in the coming days it's looking like sourceCIDRs support will be added to the XFF extension (envoyproxy/envoy#31831) and notably it will evaluate the x-forwarded-for header either by a fixed number of trusted hops, or by evaluating the client IP address against a list of trusted addresses.

So something that could be considered is supporting numTrustedHops at the top-level and sourceCIDRs (potential future feature) would use the extension but without any plans to support the seemingly duplicate numTrustedHops field.

Something like this I imagine:

clientIPDetection:
    xForwardedFor:
      # Top-level xff_num_trusted_hops field with no support for same field in XFF extension
      numTrustedHops: 2

      # Future ability to skip appending to xff header
      skipAppend: true

      # Future ability to evaluate xff header based on trusted CIDRs rather than number of hops (uses extension)
      sourceCIDRs:
      - "130.211.0.0/22"
      - "35.191.0.0/16"

    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: x-client-ip-address
      rejectWithStatus: 403

Anything other than numTrustedHops would result in use_remote_address being set to false due to incompatibility with extensions, otherwise the current default of true would apply.

I'll update this MR to only add the ability to set numTrustedHops and tackle customHeader separately after this is complete.

@arkodg
Copy link
Contributor

arkodg commented Feb 1, 2024

curious if you know the difference between the numTrustedHops field in the top level vs extension setting in envoy?

At the moment the only difference I believe is the extensions were marked incompatible with use_remote_address, but otherwise the XFF extension is identical in function currently.

It's been asked by maintainers to implement additions to XFF handling in original_ip_detection_extensions and in the coming days it's looking like sourceCIDRs support will be added to the XFF extension (envoyproxy/envoy#31831) and notably it will evaluate the x-forwarded-for header either by a fixed number of trusted hops, or by evaluating the client IP address against a list of trusted addresses.

So something that could be considered is supporting numTrustedHops at the top-level and sourceCIDRs (potential future feature) would use the extension but without any plans to support the seemingly duplicate numTrustedHops field.

Something like this I imagine:

clientIPDetection:
    xForwardedFor:
      # Top-level xff_num_trusted_hops field with no support for same field in XFF extension
      numTrustedHops: 2

      # Future ability to skip appending to xff header
      skipAppend: true

      # Future ability to evaluate xff header based on trusted CIDRs rather than number of hops (uses extension)
      sourceCIDRs:
      - "130.211.0.0/22"
      - "35.191.0.0/16"

    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: x-client-ip-address
      rejectWithStatus: 403

Anything other than numTrustedHops would result in use_remote_address being set to false due to incompatibility with extensions, otherwise the current default of true would apply.

I'll update this MR to only add the ability to set numTrustedHops and tackle customHeader separately after this is complete.

sounds like a good plan ! this keeps any envoyisms out of the API increasing its ability to be moved into the upstream gateway api in the future

Signed-off-by: David Alger <davidmalger@gmail.com>
…mgr-settings

Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
@davidalger davidalger changed the title feat: Support Client IP Detection settings on ClientTrafficPolicy feat: Support Client IP Detection using XFF on ClientTrafficPolicy Feb 2, 2024
@davidalger
Copy link
Contributor Author

@arkodg updated and should be ready for review

@arkodg
Copy link
Contributor

arkodg commented Feb 2, 2024

PR looks good @davidalger, added a minor comment around optional fields

Signed-off-by: David Alger <davidmalger@gmail.com>
@davidalger davidalger force-pushed the algerdev/http-conn-mgr-settings branch from 4d7472c to 518157d Compare February 2, 2024 14:34
Signed-off-by: David Alger <davidmalger@gmail.com>
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team February 2, 2024 19:06
@arkodg arkodg merged commit e3994ec into envoyproxy:main Feb 3, 2024
19 of 20 checks passed
@zirain
Copy link
Contributor

zirain commented Feb 3, 2024

need a user doc for this?

@arkodg
Copy link
Contributor

arkodg commented Feb 3, 2024

need a user doc for this?

was part of this PR https://gateway.envoyproxy.io/latest/user/client-traffic-policy/#configure-client-ip-detection

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