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

[improve][broker] Support X-Forwarded-For and HA Proxy Protocol for resolving original client IP of http/https requests #22524

Merged
merged 6 commits into from
Apr 22, 2024

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Apr 17, 2024

Fixes #22512

Motivation

See #22512. In some environments, there's a HTTP reverse proxy in front of Pulsar Proxy or Pulsar Broker and there's a desire to log the actual client IP addresses instead of the reverse proxy's IP address. For this purpose, it's now possible to configure the broker or proxy to trust the value of the X-Forwarded-For header.

X-Forwarded-For support is also useful when Pulsar Proxy is in front of Pulsar Broker. Pulsar Proxy already adds the X-Forwarded-For headers to the request. This change will allow propagating the original client IP from the Pulsar Proxy to the Pulsar Broker as well.

For supporting Layer 4 (TCP) proxies, this PR also includes HA Proxy Protocol v1 and v2 support for resolving the original client IP of http/https requests. This is added to ensure that client IP propagation can be configured in all types of configurations.

In the cloud, HA Proxy Protocol is used in Layer 4 (TCP) proxies such as AWS Classic LB or AWS NLB. X-Forwarded-For/Forwarded headers are used in Layer 7 (http reverse proxy) proxies such as AWS ALB.

Security implications

Enabling webServiceTrustXForwardedFor=true or webServiceHaProxyProtocolEnabled=true should be only done in environments where only a trusted proxy and trusted clients that can connect to the server with http/https. In other words, network perimeter security should be in place. The reason for this is that any client can set the X-Forwarded-For header or the Proxy Protocol prefix and that value would be logged as the client IP.

Modifications

  • Configure ForwardedRequestCustomizer for jetty httpConfiguration to handle the X-Forwarded-For header when webServiceTrustXForwardedFor=true.
  • Configure ProxyConnectionFactory to handle the HA Proxy Protocol v1 and v2 prefix when webServiceHaProxyProtocolEnabled=true.
  • Add configuration webServiceLogDetailedAddresses to enable logging detailed remote addresses (original and real) and local addresses (real and original destination).
    • example: [R:99.22.33.44:1234 via 127.0.0.1:56873]->[L:127.0.0.1:56871 dst 5.4.3.1:4321] is appended to the log entry, such as: 2024-04-19T14:59:15,896 - INFO - [prometheus-stats-32-1:RequestLog] - 99.22.33.44 - - [19/Apr/2024:14:59:15 +0300] "GET /metrics/ HTTP/1.1" 200 5061 "-" "Jetty/9.4.54.v20240208" 6 [R:99.22.33.44:1234 via 127.0.0.1:56873]->[L:127.0.0.1:56871 dst 5.4.3.1:4321]
  • Add support for all webservers in Pulsar: broker, proxy, function worker and websocket proxy

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 3.3.0 milestone Apr 17, 2024
@lhotari lhotari self-assigned this Apr 17, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 17, 2024
@lhotari lhotari force-pushed the lh-xff-header-support branch from 0a260c2 to df4e7c9 Compare April 17, 2024 16:50
@lhotari lhotari requested review from merlimat, nodece and dao-jun April 17, 2024 17:21
@lhotari lhotari force-pushed the lh-xff-header-support branch from df4e7c9 to 0292dcd Compare April 18, 2024 10:01
@lhotari lhotari changed the title [improve][broker] Support trusting X-Forwarded-For header for resolving the client IP of http/https requests [improve][broker] Support X-Forwarded-For and HA Proxy Protocol for resolving original client IP of http/https requests Apr 18, 2024
…esolving the client IP of http/https requests
@lhotari lhotari force-pushed the lh-xff-header-support branch from 0292dcd to eed42c6 Compare April 18, 2024 10:29
@lhotari
Copy link
Member Author

lhotari commented Apr 18, 2024

proposal to cherry-pick this to maintenance branches: https://lists.apache.org/thread/9rqh5rmzfcl8lf6rmd1rr6h0t4kp6kpc

@lhotari lhotari force-pushed the lh-xff-header-support branch from 7df1724 to 62050fd Compare April 18, 2024 17:52
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.35714% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 73.86%. Comparing base (bbc6224) to head (62050fd).
Report is 165 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22524      +/-   ##
============================================
+ Coverage     73.57%   73.86%   +0.28%     
- Complexity    32624    33003     +379     
============================================
  Files          1877     1885       +8     
  Lines        139502   140185     +683     
  Branches      15299    15379      +80     
============================================
+ Hits         102638   103545     +907     
+ Misses        28908    28647     -261     
- Partials       7956     7993      +37     
Flag Coverage Δ
inttests 27.13% <55.35%> (+2.54%) ⬆️
systests 24.42% <37.50%> (+0.09%) ⬆️
unittests 73.14% <75.00%> (+0.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 99.39% <100.00%> (+<0.01%) ⬆️
...apache/pulsar/proxy/server/ProxyConfiguration.java 97.22% <100.00%> (+0.06%) ⬆️
.../java/org/apache/pulsar/broker/web/WebService.java 94.11% <93.75%> (-0.04%) ⬇️
...java/org/apache/pulsar/proxy/server/WebServer.java 87.70% <93.75%> (+0.43%) ⬆️
...pache/pulsar/proxy/server/ProxyServiceStarter.java 68.91% <43.75%> (-1.78%) ⬇️

... and 241 files with indirect coverage changes

@lhotari lhotari marked this pull request as draft April 19, 2024 04:33
@lhotari lhotari marked this pull request as ready for review April 19, 2024 10:58
@lhotari lhotari merged commit 4a88721 into apache:master Apr 22, 2024
51 of 54 checks passed
lhotari added a commit that referenced this pull request Apr 22, 2024
…esolving original client IP of http/https requests (#22524)

(cherry picked from commit 4a88721)
lhotari added a commit that referenced this pull request Apr 22, 2024
…esolving original client IP of http/https requests (#22524)

(cherry picked from commit 4a88721)
lhotari added a commit that referenced this pull request Apr 22, 2024
…esolving original client IP of http/https requests (#22524)

(cherry picked from commit 4a88721)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
…esolving original client IP of http/https requests (apache#22524)

(cherry picked from commit 4a88721)
(cherry picked from commit 7d52dd7)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
…esolving original client IP of http/https requests (apache#22524)

(cherry picked from commit 4a88721)
(cherry picked from commit 7d52dd7)
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Apr 24, 2024
…esolving original client IP of http/https requests (apache#22524)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[improve] Parse the XFF header to get real client IP when behind reverse proxy
5 participants