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

Adds configuration to retain client IP information #2284

Merged
merged 20 commits into from
Sep 6, 2024

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Jul 24, 2024

Proposed changes

Problem: As a user, I want to be able to retain client IP information when a requests comes in to NGF.

Solution: Introduces configuration that allows retaining client IP information using the real_ip module.

Testing: Manual testing with example and TLS-Passthrough combined

  1. ClientIP settings with Proxy Protocol and Trusted Addresses allows all IPs

nginx-proxy-config

    rewriteClientIP:
      mode: "ProxyProtocol"
      # -- The trusted addresses field needs to be replaced with the load balancer's IP address.
      trustedAddresses: ["0.0.0.0/0"]
      setIPRecursively: true

nginx.conf

server {
    listen 80 default_server proxy_protocol;
    listen [::]:80 default_server proxy_protocol;
    set_real_ip_from 0.0.0.0/0;
    real_ip_header proxy_protocol;
    real_ip_recursive on;
    default_type text/html;
    return 404;
}

server {
    listen 80 proxy_protocol;
    listen [::]:80 proxy_protocol;

    server_name echo.example.com;
    set_real_ip_from 0.0.0.0/0;
    real_ip_header proxy_protocol;
    real_ip_recursive on;

    location /headers/ {
        proxy_http_version 1.1;
        proxy_set_header Accept-Encoding "${accept_encoding_header_var}compress";
        proxy_set_header My-cool-header "${my_cool_header_header_var}this-is-an-appended-value";
        proxy_set_header My-Overwrite-Header "this-is-the-only-value";
        proxy_set_header User-Agent "";
        proxy_set_header Host "$gw_api_compliant_host";
        proxy_set_header X-Forwarded-For "$proxy_add_x_forwarded_for";
        proxy_set_header Upgrade "$http_upgrade";
        proxy_set_header Connection "$connection_upgrade";
        proxy_set_header X-Real-IP "$remote_addr";
        proxy_set_header X-Forwarded-Proto "$scheme";
        proxy_set_header X-Forwarded-Host "$host";
        proxy_set_header X-Forwarded-Port "$server_port";
        proxy_pass http://default_headers_80$request_uri;
    }

curl requests sent to load balance with the expectation that NGINX will preserve the original client IP(my system's IP)

curl -v --haproxy-protocol  --resolve echo.example.com:80:34.94.229.134 http://echo.example.com:80/headers
* Added echo.example.com:80:34.94.229.134 to DNS cache
* Hostname echo.example.com was found in DNS cache
*   Trying 34.94.229.134:80...
* Connected to echo.example.com (34.94.229.134) port 80
> GET /headers HTTP/1.1
> Host: echo.example.com
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Server: nginx
< Date: Wed, 28 Aug 2024 16:55:12 GMT
< Content-Type: text/plain
< Content-Length: 469
< Connection: keep-alive
<
Headers:
  header 'Accept-Encoding' is 'compress'
  header 'My-cool-header' is 'this-is-an-appended-value'
  header 'My-Overwrite-Header' is 'this-is-the-only-value'
  header 'Host' is 'echo.example.com'
  header 'X-Forwarded-For' is '10.0.0.229'
  header 'Connection' is 'close'
  header 'X-Real-IP' is '10.0.0.229'.  <-- this is my `en0` (ethernet) IP 
  header 'X-Forwarded-Proto' is 'http'
  header 'X-Forwarded-Host' is 'echo.example.com'
  header 'X-Forwarded-Port' is '80'
  header 'Accept' is '*/*'
* Connection #0 to host echo.example.com left intact
    1. ClientIP settings with Proxy Protocol and Trusted Addresses allows a random CIDR, not in the valid range of that of LoadBalancer (Control plane authorized networks | (76.25.67.31/32)

nginx-proxy-config

    rewriteClientIP:
      mode: "ProxyProtocol"
      # -- The trusted addresses field needs to be replaced with the load balancer's IP address.
      trustedAddresses: ["1.1.1.1/32"]
      setIPRecursively: true

curl requests sent to load balance with the expectation that NGINX will NOT preserve the original client IP(my system's IP) since trusted addresses list does not contain load balancers IP

curl -v --haproxy-protocol  --resolve echo.example.com:80:34.94.229.134 http://echo.example.com:80/headers
* Added echo.example.com:80:34.94.229.134 to DNS cache
* Hostname echo.example.com was found in DNS cache
*   Trying 34.94.229.134:80...
* Connected to echo.example.com (34.94.229.134) port 80
> GET /headers HTTP/1.1
> Host: echo.example.com
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Server: nginx
< Date: Wed, 28 Aug 2024 19:29:11 GMT
< Content-Type: text/plain
< Content-Length: 471
< Connection: keep-alive
<
Headers:
  header 'Accept-Encoding' is 'compress'
  header 'My-cool-header' is 'this-is-an-appended-value'
  header 'My-Overwrite-Header' is 'this-is-the-only-value'
  header 'Host' is 'echo.example.com'
  header 'X-Forwarded-For' is '76.25.67.31'  <--- LB's IP
  header 'Connection' is 'close'
  header 'X-Real-IP' is '76.25.67.31'
  header 'X-Forwarded-Proto' is 'http'
  header 'X-Forwarded-Host' is 'echo.example.com'
  header 'X-Forwarded-Port' is '80'
  header 'Accept' is '*/*'
    1. ClientIP settings with XForwardedFor and Trusted Addresses allows LB IP's

nginx-proxy-config

    rewriteClientIP:
      mode: "XForwardedFor"
      # -- The trusted addresses field needs to be replaced with the load balancer's IP address.
      trustedAddresses: ["76.25.67.31/32"]
      setIPRecursively: true

nginx.conf

server {
    listen 80 default_server;
    listen [::]:80 default_server;
    set_real_ip_from 76.25.67.31/32;
    real_ip_header X-Forwarded-For;
    real_ip_recursive on;
    default_type text/html;
    return 404;
}

server {
    listen 80;
    listen [::]:80;

    server_name echo.example.com;
    set_real_ip_from 76.25.67.31/32;
    real_ip_header X-Forwarded-For;
    real_ip_recursive on;

curl request being sent with the assumption that XforwardedFor left most IP will be returned if LB IP is in X-Fowarded-For headers

curl -s -v -H "X-Forwarded-For: 1.1.1.1, 76.25.67.31"  --resolve echo.example.com:80:34.94.229.134 http://echo.example.com:80/headers
* Added echo.example.com:80:34.94.229.134 to DNS cache
* Hostname echo.example.com was found in DNS cache
*   Trying 34.94.229.134:80...
* Connected to echo.example.com (34.94.229.134) port 80
> GET /headers HTTP/1.1
> Host: echo.example.com
> User-Agent: curl/8.4.0
> Accept: */*
> X-Forwarded-For: 1.1.1.1, 76.25.67.31
>
< HTTP/1.1 200 OK
< Server: nginx
< Date: Wed, 28 Aug 2024 19:36:20 GMT
< Content-Type: text/plain
< Content-Length: 485
< Connection: keep-alive
<
Headers:
  header 'Accept-Encoding' is 'compress'
  header 'My-cool-header' is 'this-is-an-appended-value'
  header 'My-Overwrite-Header' is 'this-is-the-only-value'
  header 'Host' is 'echo.example.com'
  header 'X-Forwarded-For' is '1.1.1.1, 76.25.67.31, 1.1.1.1'
  header 'Connection' is 'close'
  header 'X-Real-IP' is '1.1.1.1'
  header 'X-Forwarded-Proto' is 'http'
  header 'X-Forwarded-Host' is 'echo.example.com'
  header 'X-Forwarded-Port' is '80'
  header 'Accept' is '*/*'

curl being sent with recursive turned off with the expectation X-Real-IP will return LB's IP and not recursively search for the IP before.

curl -s -v -H "X-Forwarded-For: 1.1.1.1, 76.25.67.31"  --resolve echo.example.com:80:34.94.229.134 http://echo.example.com:80/headers
* Added echo.example.com:80:34.94.229.134 to DNS cache
* Hostname echo.example.com was found in DNS cache
*   Trying 34.94.229.134:80...
* Connected to echo.example.com (34.94.229.134) port 80
> GET /headers HTTP/1.1
> Host: echo.example.com
> User-Agent: curl/8.4.0
> Accept: */*
> X-Forwarded-For: 1.1.1.1, 76.25.67.31
>
< HTTP/1.1 200 OK
< Server: nginx
< Date: Wed, 28 Aug 2024 19:37:51 GMT
< Content-Type: text/plain
< Content-Length: 493
< Connection: keep-alive
<
Headers:
  header 'Accept-Encoding' is 'compress'
  header 'My-cool-header' is 'this-is-an-appended-value'
  header 'My-Overwrite-Header' is 'this-is-the-only-value'
  header 'Host' is 'echo.example.com'
  header 'X-Forwarded-For' is '1.1.1.1, 76.25.67.31, 76.25.67.31'
  header 'Connection' is 'close'
  header 'X-Real-IP' is '76.25.67.31'
  header 'X-Forwarded-Proto' is 'http'
  header 'X-Forwarded-Host' is 'echo.example.com'
  header 'X-Forwarded-Port' is '80'
  header 'Accept' is '*/*'

NOTE Without the --haproxy-protocol flag, nginx will throw an error since no proxy details from client is passed

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #1406

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Adds configuration to retain client IP information. 

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jul 24, 2024
@salonichf5 salonichf5 changed the title add proxy_protocol to server configuration Add proxy_protocol to server configuration Jul 24, 2024
@github-actions github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels Jul 24, 2024
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.34%. Comparing base (6b9ad3a) to head (121cfeb).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2284      +/-   ##
==========================================
+ Coverage   89.21%   89.34%   +0.12%     
==========================================
  Files         100      100              
  Lines        7539     7628      +89     
  Branches       50       50              
==========================================
+ Hits         6726     6815      +89     
  Misses        756      756              
  Partials       57       57              

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

@salonichf5 salonichf5 force-pushed the feat/proxy-protocol branch 2 times, most recently from 58e4b21 to 7fb59de Compare July 25, 2024 00:59
@sjberman
Copy link
Contributor

Be sure to link the issue associated with this PR in the description.

Copy link
Contributor

github-actions bot commented Aug 9, 2024

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Aug 9, 2024
@salonichf5 salonichf5 force-pushed the feat/proxy-protocol branch 2 times, most recently from 2a818c9 to 513b580 Compare August 13, 2024 21:24
@github-actions github-actions bot removed the stale Pull requests/issues with no activity label Aug 14, 2024
@salonichf5 salonichf5 force-pushed the feat/proxy-protocol branch 3 times, most recently from 8835f3a to 7bfedbe Compare August 20, 2024 00:38
@salonichf5 salonichf5 marked this pull request as ready for review August 20, 2024 00:39
@salonichf5 salonichf5 requested review from a team as code owners August 20, 2024 00:39
@salonichf5 salonichf5 force-pushed the feat/proxy-protocol branch 2 times, most recently from 3599ee2 to a531369 Compare August 20, 2024 04:14
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

@salonichf5 I'm not done reviewing yet, but here's a partial review to get you started.

apis/v1alpha1/nginxproxy_types.go Outdated Show resolved Hide resolved
apis/v1alpha1/nginxproxy_types.go Outdated Show resolved Hide resolved
apis/v1alpha1/nginxproxy_types.go Show resolved Hide resolved
apis/v1alpha1/nginxproxy_types.go Outdated Show resolved Hide resolved
charts/nginx-gateway-fabric/values.yaml Outdated Show resolved Hide resolved
internal/mode/static/state/graph/nginxproxy_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/nginxproxy_test.go Outdated Show resolved Hide resolved
site/content/how-to/monitoring/troubleshooting.md Outdated Show resolved Hide resolved
site/content/how-to/monitoring/troubleshooting.md Outdated Show resolved Hide resolved
@kate-osborn
Copy link
Contributor

We also want to be setting the following headers for grpc and http:

I would add these headers to the http and grpc base headers here:

var httpBaseHeaders = []http.Header{

@salonichf5 salonichf5 force-pushed the feat/proxy-protocol branch 3 times, most recently from e1c0421 to 64f1278 Compare August 22, 2024 19:50
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

Just a couple nits, otherwise looks good!

charts/nginx-gateway-fabric/values.yaml Outdated Show resolved Hide resolved
charts/nginx-gateway-fabric/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Sep 6, 2024

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@salonichf5
Copy link
Contributor Author

recheck

@salonichf5
Copy link
Contributor Author

I have hereby read the F5 CLA and agree to its terms

@salonichf5 salonichf5 enabled auto-merge (squash) September 6, 2024 19:24
@salonichf5 salonichf5 merged commit bf17bd5 into nginxinc:main Sep 6, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart release-notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Enable Proxy Protocol
4 participants