WPB-20209 Forward IP address by federator #4787
Conversation
zebot
added
the
ok-to-test
battermann
force-pushed
the
WPB-20209-forward-ip-address-of-the-remote-from-federator-to-component
branch
2 times, most recently
from
b887080 to
c47d89b
Compare
battermann
changed the base branch from
develop
to
WPB-20208-add-audit-logging-to-asset-download
battermann
force-pushed
the
WPB-20208-add-audit-logging-to-asset-download
branch
from
c825167 to
890a52a
Compare
battermann
force-pushed
the
WPB-20209-forward-ip-address-of-the-remote-from-federator-to-component
branch
from
c47d89b to
e2c53da
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements IP address forwarding by the federator service to enable better audit logging and forensic capabilities in federated communications.
Key changes:
- Nginx proxy configuration updated to forward client IP headers to upstream services
- Federator service modified to capture and forward remote peer IP addresses when making federated calls
- Cargohold service enhanced to include IP addresses in audit logs for asset downloads
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
services/nginz/integration-test/conf/nginz/nginx.conf |
Adds X-Forwarded-For and X-Real-IP header forwarding with IPv4/IPv6 normalization |
services/nginz/integration-test/conf/nginz/common_response.conf |
Forwards client IP headers to all upstream services |
services/federator/test/unit/Test/Federator/ExternalServer.hs |
Updates tests to verify Wire-Origin-IP header forwarding |
services/federator/src/Federator/Remote.hs |
Captures remote peer IP during federation calls and adds to response headers |
services/federator/src/Federator/ExternalServer.hs |
Extracts IP from X-Forwarded-For/X-Real-IP headers and forwards as Wire-Origin-IP |
services/cargohold/src/CargoHold/Federation.hs |
Updates federation streaming to handle IP addresses for audit logging |
services/cargohold/src/CargoHold/API/Federation.hs |
Passes remote IP to audit logging functions |
services/cargohold/src/CargoHold/API/AuditLog.hs |
Adds IP address fields to audit log entries |
libs/wire-api-federation/src/Wire/API/Federation/Endpoint.hs |
Defines new header types and streaming endpoint with IP support |
libs/http2-manager/src/HTTP2/Client/Manager/Internal.hs |
Adds hook mechanism to capture peer socket address |
| Various build files | Add network and bytestring-conversion dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Use X-Forwarded-For as the real client IP when present | ||
| real_ip_header X-Forwarded-For; | ||
| set_real_ip_from 0.0.0.0/0; |
There was a problem hiding this comment.
Using set_real_ip_from 0.0.0.0/0 allows any client to spoof the X-Forwarded-For header. This should be restricted to trusted proxy IP ranges only, such as load balancers or known upstream proxies.
| set_real_ip_from 0.0.0.0/0; | |
| set_real_ip_from 127.0.0.1; | |
| set_real_ip_from ::1; | |
| # If running in Docker, also trust the default bridge network: | |
| set_real_ip_from 172.17.0.0/16; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
this is for testing so it's fine, IMO
| (either (throw . federationErrorToWai . FederationCallFailure) (flip unSourceT k)) | ||
| ( either | ||
| (throw . federationErrorToWai . FederationCallFailure) | ||
| (\(mIp, src) -> (runAppT appEnv $ (runExceptT (onRemoteIp mIp))) *> unSourceT src k) |
There was a problem hiding this comment.
The nested parentheses and monad transformer usage is complex and hard to read. Consider simplifying: \\(mIp, src) -> runAppT appEnv (runExceptT (onRemoteIp mIp)) *> unSourceT src k
| (\(mIp, src) -> (runAppT appEnv $ (runExceptT (onRemoteIp mIp))) *> unSourceT src k) | |
| (handleFederatedStreamingResult appEnv onRemoteIp k) |
Copilot uses AI. Check for mistakes.
| instance (RoutesToPaths api) => RoutesToPaths (OriginIpHeader :> api) where | ||
| getRoutes = getRoutes @api | ||
|
|
||
| type instance SpecialiseToVersion v (OriginIpHeader :> api) = OriginIpHeader :> SpecialiseToVersion v api |
There was a problem hiding this comment.
I think it is not needed
There was a problem hiding this comment.
I guess it's not needed because SpecialiseToVersion isn't used for the federation API? Doesn't hurt to keep it though, I'd say.
| instance (RoutesToPaths (StreamPost framing ct a)) => RoutesToPaths (StreamPostWithRemoteIp framing ct a) where | ||
| getRoutes = getRoutes @(StreamPost framing ct a) | ||
|
|
||
| type instance SpecialiseToVersion v (StreamPostWithRemoteIp framing ct a) = StreamPostWithRemoteIp framing ct (SpecialiseToVersion v a) |
There was a problem hiding this comment.
I think this is not needed either.
| instance (RoutesToPaths api) => RoutesToPaths (OriginIpHeader :> api) where | ||
| getRoutes = getRoutes @api | ||
|
|
||
| type instance SpecialiseToVersion v (OriginIpHeader :> api) = OriginIpHeader :> SpecialiseToVersion v api |
There was a problem hiding this comment.
I guess it's not needed because SpecialiseToVersion isn't used for the federation API? Doesn't hurt to keep it though, I'd say.
get the IP address by hooking into the http2 manager
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
battermann
force-pushed
the
WPB-20209-forward-ip-address-of-the-remote-from-federator-to-component
branch
from
c8f7e66 to
a02b1ac
Compare
battermann
deleted the
WPB-20209-forward-ip-address-of-the-remote-from-federator-to-component
branch
Checklist
changelog.d