Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38471 +/- ##
==========================================
+ Coverage 65.97% 65.99% +0.01%
==========================================
Files 2408 2409 +1
Lines 192369 192464 +95
Branches 8501 8501
==========================================
+ Hits 126925 127017 +92
- Misses 53891 53893 +2
- Partials 11553 11554 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce a configurable client IP extraction strategy for the fleet server. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant Middleware as publicIPMiddleware
participant Strategy as ClientIPStrategy
participant Context as Request Context
Client->>Handler: HTTP Request
Handler->>Middleware: Forward Request
Middleware->>Strategy: ClientIP(headers, remoteAddr)
alt trustedProxies = "" (Legacy)
Strategy->>Strategy: Check True-Client-IP Header
Strategy->>Strategy: Check X-Real-IP Header
Strategy->>Strategy: Check X-Forwarded-For Header
Strategy->>Strategy: Fallback to RemoteAddr
else trustedProxies = "none"
Strategy->>Strategy: Use RemoteAddr only
else trustedProxies = Known Header (e.g., CF-Connecting-IP)
Strategy->>Strategy: Extract from Header
Strategy->>Strategy: Fallback to RemoteAddr
else trustedProxies = Hop Count (e.g., "2")
Strategy->>Strategy: Parse X-Forwarded-For
Strategy->>Strategy: Select rightmost N IPs
Strategy->>Strategy: Fallback to RemoteAddr
else trustedProxies = IP Ranges (e.g., 10.0.0.0/8)
Strategy->>Strategy: Parse IP Ranges
Strategy->>Strategy: Find first non-trusted IP
Strategy->>Strategy: Fallback to RemoteAddr
end
Strategy-->>Middleware: Return extracted IP
Middleware->>Context: Attach IP to ctx
Middleware-->>Handler: Continue Request
Handler-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/config/config.go`:
- Around line 1179-1180: Update the help text passed to man.addConfigString for
the "server.trusted_proxies" config to explicitly list the header names the
implementation accepts (e.g., "X-Forwarded-For, Forwarded, X-Real-IP,
True-Client-IP") instead of implying arbitrary header names are allowed; ensure
the wording in the man.addConfigString call matches the actual allowed headers
in the parsing logic so operators aren't misled and include the accepted header
list and examples along with the existing hop-count and CIDR options.
sgress454
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Hopping into a meeting but I've reviewed everything except the first ~200 lines of tests.
|
|
||
| // singleIPHeaderNames are header names that contain a single IP address, | ||
| // typically set by CDNs or reverse proxies. | ||
| var singleIPHeaderNames = map[string]bool{ |
There was a problem hiding this comment.
IIRC we usually do map[string]struct{} for items like this?
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Unreviewed is down to first 120 lines of the test. Will finish after this call.
|
Addressed feedback! |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| ip := ipStrategy.ClientIP(r.Header, r.RemoteAddr) | ||
| if ip != "" { | ||
| r.RemoteAddr = ip | ||
| } | ||
| handler.ServeHTTP(w, r.WithContext(publicip.NewContext(r.Context(), ip))) | ||
| }) |
There was a problem hiding this comment.
Putting this inline to avoid having to declare an interface solely for the purpose of sending strategy as a param to a "create the IP extraction middleware" utility function that gets called once when the server starts.
Adds documentation for the new config setting added in #38471. Adding to 4.8.0 (with callout that this works in 4.80.1) on guidance from @rachaelshaw. cc: @iansltx if you want to double-check my math on the examples 😆
|
To test: The general pattern is to start the fleet server with and hit the login API with: You should get an authn failed message and in the activity feed see: For the actual tests to do, you can refer to the automated tests for reference, as well as the trusted proxies config. |
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves # Adds a new `FLEET_SERVER_TRUSTED_PROXIES` config, allowing more fine-grained control over how the client IP is determined for requests. Uses the [realclientip-go](https://github.com/realclientip/realclientip-go) library as the engine for parsing headers and using rules to determine the IP. If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Introduced FLEET_SERVER_TRUSTED_PROXIES configuration option to specify trusted proxy IPs and hosts. The server now supports flexible client IP detection strategies that respect your proxy configuration, with support for multiple formats including single IP header names, hop counts, and IP address ranges, adapting to various infrastructure setups and deployment scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
Details
Adds a new
FLEET_SERVER_TRUSTED_PROXIESconfig, allowing more fine-grained control over how the client IP is determined for requests. Uses the realclientip-go library as the engine for parsing headers and using rules to determine the IP.Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.