From 7a5f48073c9e1a0fde0c2af90d667c4520415218 Mon Sep 17 00:00:00 2001 From: Leif Battermann Date: Wed, 1 Oct 2025 07:28:30 +0000 Subject: [PATCH 1/5] reapply WPB-20209 Forward IP address by federator Revert "Revert "WPB-20209 Forward IP address by federator (#4787)" (#4798)" This reverts commit 2a308cecf687ae37089e4be94ce0fd7a3866cd61. --- changelog.d/2-features/WPB-20207 | 2 +- charts/nginz/templates/conf/_nginx.conf.tpl | 4 + .../http2-manager/src/HTTP2/Client/Manager.hs | 1 + .../src/HTTP2/Client/Manager/Internal.hs | 29 ++++- .../src/Wire/API/Federation/Endpoint.hs | 109 +++++++++++++++++- services/cargohold/cargohold.cabal | 17 +-- services/cargohold/default.nix | 1 + .../cargohold/src/CargoHold/API/AuditLog.hs | 15 ++- .../cargohold/src/CargoHold/API/Federation.hs | 9 +- .../cargohold/src/CargoHold/Federation.hs | 24 ++-- .../unit/Test/CargoHold/API/AuditLogTest.hs | 19 +-- .../test/unit/Test/CargoHold/API/LogJSON.hs | 16 --- services/federator/default.nix | 2 + services/federator/federator.cabal | 1 + .../federator/src/Federator/ExternalServer.hs | 14 ++- services/federator/src/Federator/Remote.hs | 22 +++- .../unit/Test/Federator/ExternalServer.hs | 10 +- .../conf/nginz/common_response.conf | 3 + .../integration-test/conf/nginz/nginx.conf | 23 ++++ 19 files changed, 264 insertions(+), 57 deletions(-) diff --git a/changelog.d/2-features/WPB-20207 b/changelog.d/2-features/WPB-20207 index 0c9b6ed8f6..7ead251916 100644 --- a/changelog.d/2-features/WPB-20207 +++ b/changelog.d/2-features/WPB-20207 @@ -1 +1 @@ -cargohold: add asset audit logging (#4782 ,#4784) +cargohold: add asset audit logging (#4782, #4784, #4787) diff --git a/charts/nginz/templates/conf/_nginx.conf.tpl b/charts/nginz/templates/conf/_nginx.conf.tpl index b3dce8f465..9538ca0352 100644 --- a/charts/nginz/templates/conf/_nginx.conf.tpl +++ b/charts/nginz/templates/conf/_nginx.conf.tpl @@ -322,6 +322,10 @@ http { proxy_pass http://{{ $name }}{{ if hasKey $.Values.nginx_conf.upstream_namespace $name }}.{{ get $.Values.nginx_conf.upstream_namespace $name }}{{end}}; proxy_http_version 1.1; + # Forward client IP information to upstreams + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Real-IP $remote_addr; + {{- if ($location.disable_request_buffering) }} proxy_request_buffering off; {{ end -}} diff --git a/libs/http2-manager/src/HTTP2/Client/Manager.hs b/libs/http2-manager/src/HTTP2/Client/Manager.hs index b163b7fdf0..bd578f9129 100644 --- a/libs/http2-manager/src/HTTP2/Client/Manager.hs +++ b/libs/http2-manager/src/HTTP2/Client/Manager.hs @@ -12,6 +12,7 @@ module HTTP2.Client.Manager http2ManagerWithSSLCtx, withHTTP2Request, withHTTP2RequestOnSingleUseConn, + withHTTP2RequestOnSingleUseConnWithHook, connectIfNotAlreadyConnected, ConnectionAlreadyClosed (..), disconnectTarget, diff --git a/libs/http2-manager/src/HTTP2/Client/Manager/Internal.hs b/libs/http2-manager/src/HTTP2/Client/Manager/Internal.hs index 5ec47c2fff..bac83f0ee2 100644 --- a/libs/http2-manager/src/HTTP2/Client/Manager/Internal.hs +++ b/libs/http2-manager/src/HTTP2/Client/Manager/Internal.hs @@ -168,9 +168,19 @@ withHTTP2Request mgr target req k = do -- | Temporary workaround for https://github.com/kazu-yamamoto/http2/issues/102 withHTTP2RequestOnSingleUseConn :: Http2Manager -> Target -> HTTP2.Request -> (HTTP2.Response -> IO a) -> IO a -withHTTP2RequestOnSingleUseConn Http2Manager {..} target req k = do +withHTTP2RequestOnSingleUseConn mgr target req k = + withHTTP2RequestOnSingleUseConnWithHook mgr target req (\_ -> pure ()) k + +withHTTP2RequestOnSingleUseConnWithHook :: + Http2Manager -> + Target -> + HTTP2.Request -> + (NS.SockAddr -> IO ()) -> + (HTTP2.Response -> IO a) -> + IO a +withHTTP2RequestOnSingleUseConnWithHook Http2Manager {..} target req onConnect k = do sendReqMVar <- newEmptyMVar - thread <- liftIO . async $ startPersistentHTTP2Connection sslContext target cacheLimit sslRemoveTrailingDot tcpConnectionTimeout sendReqMVar + thread <- liftIO . async $ startPersistentHTTP2ConnectionWithHook sslContext target cacheLimit sslRemoveTrailingDot tcpConnectionTimeout onConnect sendReqMVar let newConn = HTTP2Conn thread (putMVar sendReqMVar CloseConnection) sendReqMVar sendRequestWithConnection newConn req $ \resp -> do k resp <* disconnect newConn @@ -276,6 +286,17 @@ disconnectTargetWithTimeout mgr target microSeconds = do `finally` (atomically . modifyTVar' (connections mgr) $ Map.delete target) startPersistentHTTP2Connection :: + SSL.SSLContext -> + Target -> + Int -> + Bool -> + Int -> + MVar ConnectionAction -> + IO () +startPersistentHTTP2Connection ctx target cl removeTrailingDot tcpConnectTimeout sendReqMVar = + startPersistentHTTP2ConnectionWithHook ctx target cl removeTrailingDot tcpConnectTimeout (\_ -> pure ()) sendReqMVar + +startPersistentHTTP2ConnectionWithHook :: SSL.SSLContext -> Target -> -- cacheLimit @@ -284,12 +305,13 @@ startPersistentHTTP2Connection :: Bool -> -- | TCP connect timeout in microseconds Int -> + (NS.SockAddr -> IO ()) -> -- MVar used to communicate requests or the need to close the connection. (We could use a -- queue here to queue several requests, but since the requestor has to wait for the -- response, it might as well block before sending off the request.) MVar ConnectionAction -> IO () -startPersistentHTTP2Connection ctx (tlsEnabled, hostname, port) cl removeTrailingDot tcpConnectTimeout sendReqMVar = do +startPersistentHTTP2ConnectionWithHook ctx (tlsEnabled, hostname, port) cl removeTrailingDot tcpConnectTimeout onConnect sendReqMVar = do liveReqs <- newIORef mempty let clientConfig = HTTP2.defaultClientConfig @@ -332,6 +354,7 @@ startPersistentHTTP2Connection ctx (tlsEnabled, hostname, port) cl removeTrailin handle cleanupThreadsWith $ bracket connectTCPWithTimeout NS.close $ \sock -> do + onConnect =<< NS.getPeerName sock bracket (mkTransport sock transportConfig) cleanupTransport $ \transport -> bracket (allocHTTP2Config transport) HTTP2.freeSimpleConfig $ \http2Cfg -> do let runAction = HTTP2.run clientConfig http2Cfg $ \sendReq _aux -> do diff --git a/libs/wire-api-federation/src/Wire/API/Federation/Endpoint.hs b/libs/wire-api-federation/src/Wire/API/Federation/Endpoint.hs index cc565259e4..f93c1ed6ae 100644 --- a/libs/wire-api-federation/src/Wire/API/Federation/Endpoint.hs +++ b/libs/wire-api-federation/src/Wire/API/Federation/Endpoint.hs @@ -21,10 +21,21 @@ module Wire.API.Federation.Endpoint ) where +import Control.Lens ((?~)) +import Data.ByteString.Conversion (fromByteString) import Data.Kind +import Data.Metrics.Servant +import Data.Misc (IpAddr) +import Data.OpenApi qualified as S +import Data.Sequence qualified as Seq import GHC.TypeLits import Imports -import Servant.API +import Network.HTTP.Types qualified as HTTP +import Servant +import Servant.Client +import Servant.Client.Core +import Servant.OpenApi (HasOpenApi (toOpenApi)) +import Servant.Server.Internal (MkContextWithErrorFormatter) import Wire.API.ApplyMods import Wire.API.Federation.API.Common import Wire.API.Federation.Domain @@ -72,8 +83,9 @@ type StreamingFedEndpoint name input output = name ( name :> OriginDomainHeader + :> OriginIpHeader :> ReqBody '[JSON] input - :> StreamPost NoFraming OctetStream output + :> StreamPostWithRemoteIp NoFraming OctetStream output ) type family @@ -90,3 +102,96 @@ type instance type instance MkNotificationFedEndpoint m s ('Just v) p = NotificationFedEndpointWithMods m (Versioned v s) s p + +type OriginIpHeaderName = "Wire-Origin-IP" :: Symbol + +-- | The remote backend's origin IP is best-effort forensic metadata only. +-- Do not use for auth, policy, or attribution; cert identity is authoritative. +-- IP reflects only the socket peer at our edge (often LB/NAT/egress) and may be inaccurate. +data OriginIpHeader + +instance (RoutesToPaths api) => RoutesToPaths (OriginIpHeader :> api) where + getRoutes = getRoutes @api + +instance (HasClient m api) => HasClient m (OriginIpHeader :> api) where + type Client m (OriginIpHeader :> api) = Client m api + clientWithRoute pm _ req = clientWithRoute pm (Proxy @api) req + hoistClientMonad pm _ = hoistClientMonad pm (Proxy @api) + +type OriginIpHeaderHasServer = Header' '[Strict] OriginIpHeaderName IpAddr + +instance + ( HasServer api context, + HasContextEntry (MkContextWithErrorFormatter context) ErrorFormatters + ) => + HasServer (OriginIpHeader :> api) context + where + type ServerT (OriginIpHeader :> api) m = Maybe IpAddr -> ServerT api m + route _pa = route (Proxy @(OriginIpHeaderHasServer :> api)) + hoistServerWithContext _ pc nt s = hoistServerWithContext (Proxy :: Proxy api) pc nt . s + +originIpHeaderName :: (IsString a) => a +originIpHeaderName = fromString $ symbolVal (Proxy @OriginIpHeaderName) + +-- Header carrying the best-effort remote peer IP observed by our internal +-- federator when calling a remote backend. Forensic metadata only; do not use +-- for auth/policy or attribution. +type RemoteIpHeaderName = "Wire-Remote-IP" :: Symbol + +remoteIpHeaderName :: (IsString a) => a +remoteIpHeaderName = fromString $ symbolVal (Proxy @RemoteIpHeaderName) + +instance (HasOpenApi api) => HasOpenApi (OriginIpHeader :> api) where + toOpenApi _ = desc $ toOpenApi (Proxy @api) + where + desc = + S.allOperations + . S.description + ?~ ("Federated endpoints may include optional origin IP header: `" <> originIpHeaderName <> "`") + +-- | A streaming POST combinator that behaves like Servant's 'StreamPost', +-- but whose client additionally returns an optional remote IP parsed from +-- the 'Wire-Remote-IP' response header. +data StreamPostWithRemoteIp framing (ct :: Type) a + +-- Server-side simply delegates to the standard 'StreamPost' implementation. +instance + ( HasServer (StreamPost framing ct a) context + ) => + HasServer (StreamPostWithRemoteIp framing ct a) context + where + type ServerT (StreamPostWithRemoteIp framing ct a) m = ServerT (StreamPost framing ct a) m + route _ = route (Proxy @(StreamPost framing ct a)) + hoistServerWithContext _ = hoistServerWithContext (Proxy @(StreamPost framing ct a)) + +-- OpenAPI, metrics and path routing can delegate to the underlying StreamPost +instance RoutesToPaths (StreamPostWithRemoteIp (framing :: Type) (ct :: Type) (a :: Type)) where + getRoutes = getRoutes @(StreamPost framing ct a) + +instance (HasOpenApi (StreamPost framing ct a)) => HasOpenApi (StreamPostWithRemoteIp framing ct a) where + toOpenApi _ = toOpenApi (Proxy @(StreamPost framing ct a)) + +-- Client-side: make the streaming request and return the body together with an +-- optional 'IpAddr' parsed from the 'Wire-Remote-IP' header. +instance + ( RunStreamingClient m, + Accept ct, + FromSourceIO ByteString a + ) => + HasClient m (StreamPostWithRemoteIp framing ct a) + where + type Client m (StreamPostWithRemoteIp framing ct a) = m (Maybe IpAddr, a) + + clientWithRoute _ _ req = + withStreamingRequest + req + { requestMethod = HTTP.methodPost, + requestAccept = Seq.singleton (contentType (Proxy @ct)) + } + $ \resp -> do + let hdrs = toList (responseHeaders resp) + mIp = lookup remoteIpHeaderName hdrs >>= fromByteString + body <- fromSourceIO (responseBody resp) + pure (mIp, body) + + hoistClientMonad _ _ f c = f c diff --git a/services/cargohold/cargohold.cabal b/services/cargohold/cargohold.cabal index c02b23050e..e84c9fad29 100644 --- a/services/cargohold/cargohold.cabal +++ b/services/cargohold/cargohold.cabal @@ -356,21 +356,22 @@ test-suite cargohold-tests -Wredundant-constraints -Wunused-packages -Wno-x-partial build-depends: - aeson >=2.0.1.0 - , base >=4 && <5 - , bytestring >=0.10 + aeson >=2.0.1.0 + , base >=4 && <5 + , bytestring >=0.10 + , bytestring-conversion , cargohold , cargohold-types , extended , imports - , mime >=0.4 - , tasty >=1.0 - , tasty-quickcheck >=0.10 - , text >=1.1 + , mime >=0.4 + , tasty >=1.0 + , tasty-quickcheck >=0.10 + , text >=1.1 , types-common , unix , unliftio - , uri-bytestring >=0.2 + , uri-bytestring >=0.2 , wire-api default-language: Haskell2010 diff --git a/services/cargohold/default.nix b/services/cargohold/default.nix index 8d91b931bd..3883bf46b0 100644 --- a/services/cargohold/default.nix +++ b/services/cargohold/default.nix @@ -177,6 +177,7 @@ mkDerivation { aeson base bytestring + bytestring-conversion cargohold-types extended imports diff --git a/services/cargohold/src/CargoHold/API/AuditLog.hs b/services/cargohold/src/CargoHold/API/AuditLog.hs index c105255771..df869c6b4c 100644 --- a/services/cargohold/src/CargoHold/API/AuditLog.hs +++ b/services/cargohold/src/CargoHold/API/AuditLog.hs @@ -28,6 +28,7 @@ import qualified CargoHold.Types.V3 as V3 import Codec.MIME.Type (showType) import Data.ByteString.Conversion.To (toByteString) import Data.Id (UserId, botUserId) +import Data.Misc (IpAddr) import Data.Qualified (Local, Qualified, Remote, qDomain, qUnqualified, tDomain, tUnqualified) import Data.Text.Encoding (decodeUtf8) import Imports @@ -63,12 +64,13 @@ logUpload lwon mMeta pathTxt = ~~ "uploader.id" .= toByteString p principalDomain = "uploader.domain" .= toByteString (tDomain lwon) -logDownload :: (Log.MonadLogger m) => Maybe (Qualified V3.Principal) -> S3AssetMeta -> Text -> m () -logDownload mqDownloader s3 pathTxt = +logDownload :: (Log.MonadLogger m) => Maybe (Qualified V3.Principal) -> Maybe IpAddr -> S3AssetMeta -> Text -> m () +logDownload mqDownloader mIp s3 pathTxt = Log.info $ auditTrue ~~ "event" .= ("file-download" :: Text) ~~ downloaderFields mqDownloader + ~~ ipaddr "downloader.backend.ip" mIp ~~ auditMetaFields (v3AssetAuditLogMetadata s3) ~~ "download.path" .= pathTxt ~~ msg (val "Asset audit log: download") @@ -88,8 +90,8 @@ logSignedURLCreation mqCreator mMeta uri = renderHost = maybe ("" :: Text) (decodeUtf8 . hostBS . authorityHost) hostBS (Host h) = h -logDownloadRemoteAsset :: (Log.MonadLogger m) => Local UserId -> Remote () -> m () -logDownloadRemoteAsset luid remote = do +logDownloadRemoteAsset :: (Log.MonadLogger m) => Local UserId -> Remote () -> Maybe IpAddr -> m () +logDownloadRemoteAsset luid remote mIp = do Log.info $ auditTrue ~~ "event" .= ("file-download" :: Text) @@ -97,6 +99,7 @@ logDownloadRemoteAsset luid remote = do ~~ "downloader.id" .= toByteString (tUnqualified luid) ~~ "downloader.domain" .= toByteString (tDomain luid) ~~ "remote.domain" .= toByteString (tDomain remote) + ~~ ipaddr "remote.backend.ip" mIp ~~ msg (val "Asset audit log: remote download") ------------------------------------------------------------------------------ @@ -146,3 +149,7 @@ notAvailable = "N/A" notAvailableInternalAccess :: Text notAvailableInternalAccess = "N/A internal access" + +ipaddr :: ByteString -> Maybe IpAddr -> Log.Msg -> Log.Msg +ipaddr field Nothing = field .= notAvailable +ipaddr field (Just ip) = field .= toByteString ip diff --git a/services/cargohold/src/CargoHold/API/Federation.hs b/services/cargohold/src/CargoHold/API/Federation.hs index 3bb80e4684..6f41992b18 100644 --- a/services/cargohold/src/CargoHold/API/Federation.hs +++ b/services/cargohold/src/CargoHold/API/Federation.hs @@ -32,6 +32,7 @@ import CargoHold.Types.V3 (Principal (UserPrincipal)) import Control.Error import Data.ByteString.Conversion (toByteString') import Data.Domain +import Data.Misc (IpAddr) import Data.Qualified (Qualified (Qualified)) import Data.Text.Encoding (decodeLatin1) import Imports @@ -54,12 +55,12 @@ checkAsset remote ga = runMaybeT $ checkMetadata (Qualified (UserPrincipal ga.user) remote) (F.key ga) (F.token ga) -streamAsset :: Domain -> F.GetAsset -> Handler AssetSource -streamAsset remote ga = do - meta <- checkAsset remote ga >>= maybe (throwE assetNotFound) pure +streamAsset :: Domain -> Maybe IpAddr -> F.GetAsset -> Handler AssetSource +streamAsset remoteDomain remoteIp ga = do + meta <- checkAsset remoteDomain ga >>= maybe (throwE assetNotFound) pure whenM (asks (.options.settings.assetAuditLogEnabled)) $ do let pathTxt = decodeLatin1 (toByteString' (S3.mkKey (F.key ga))) - logDownload (Just $ Qualified (UserPrincipal ga.user) remote) meta pathTxt + logDownload (Just $ Qualified (UserPrincipal ga.user) remoteDomain) remoteIp meta pathTxt AssetSource <$> S3.downloadV3 (F.key ga) getAsset :: Domain -> F.GetAsset -> Handler F.GetAssetResponse diff --git a/services/cargohold/src/CargoHold/Federation.hs b/services/cargohold/src/CargoHold/Federation.hs index 16b0d93d89..ec9ce8a555 100644 --- a/services/cargohold/src/CargoHold/Federation.hs +++ b/services/cargohold/src/CargoHold/Federation.hs @@ -17,13 +17,14 @@ module CargoHold.Federation where -import CargoHold.API.AuditLog (logDownloadRemoteAsset) +import CargoHold.API.AuditLog import CargoHold.App import CargoHold.Options import Control.Error import Control.Exception (throw) import Control.Monad.Codensity import Data.Id +import Data.Misc import Data.Qualified import Imports hiding (head) import Servant.API @@ -66,14 +67,15 @@ downloadRemoteAsset usr rkey tok = do fedClient @'Cargohold @"get-asset" ga if exists then do - whenM (asks (.options.settings.assetAuditLogEnabled)) $ - logDownloadRemoteAsset usr (void rkey) + auditEnabled <- asks (.options.settings.assetAuditLogEnabled) Just <$> executeFederatedStreaming rkey - ( toSourceIO - <$> fedClient @'Cargohold @"stream-asset" ga + ( do + (mIp, resp) <- fedClient @'Cargohold @"stream-asset" ga + pure (mIp, toSourceIO resp) ) + (\mIp -> when auditEnabled $ logDownloadRemoteAsset usr (void rkey) mIp) else pure Nothing mkFederatorClientEnv :: Remote x -> Handler FederatorClientEnv @@ -101,10 +103,13 @@ executeFederated remote c = do executeFederatedStreaming :: Remote x -> - FederatorClient 'Cargohold (SourceIO ByteString) -> + FederatorClient 'Cargohold (Maybe IpAddr, SourceIO ByteString) -> + -- hook to run when the optional remote IP is available (used for audit logging) + (Maybe IpAddr -> Handler ()) -> Handler (SourceIO ByteString) -executeFederatedStreaming remote c = do +executeFederatedStreaming remote c onRemoteIp = do env <- mkFederatorClientEnv remote + appEnv <- ask -- To clean up resources correctly, we exploit the Codensity wrapper around -- StepT to embed the result of @runFederatorClientToCodensity@. This works, but -- using this within a Servant handler has the effect of delaying exceptions to @@ -118,4 +123,7 @@ executeFederatedStreaming remote c = do SourceT $ \k -> runCodensity (runFederatorClientToCodensity @'Cargohold env c) - (either (throw . federationErrorToWai . FederationCallFailure) (flip unSourceT k)) + ( either + (throw . federationErrorToWai . FederationCallFailure) + (\(mIp, src) -> (runAppT appEnv $ (runExceptT (onRemoteIp mIp))) *> unSourceT src k) + ) diff --git a/services/cargohold/test/unit/Test/CargoHold/API/AuditLogTest.hs b/services/cargohold/test/unit/Test/CargoHold/API/AuditLogTest.hs index 6ed6f921ec..73baf693d0 100644 --- a/services/cargohold/test/unit/Test/CargoHold/API/AuditLogTest.hs +++ b/services/cargohold/test/unit/Test/CargoHold/API/AuditLogTest.hs @@ -23,10 +23,13 @@ import qualified CargoHold.Types.V3 as V3 import qualified Codec.MIME.Type as MIME import Data.Aeson ((.=)) import qualified Data.Aeson as Aeson +import Data.ByteString.Conversion (toByteString') import Data.Domain (Domain (..), domainText) import Data.Id (UserId, botUserId) +import Data.Misc (IpAddr) import Data.Qualified import qualified Data.Text as T +import Data.Text.Encoding (decodeUtf8) import Imports import System.Logger.Extended (LoggerT, runWithLogger) import Test.CargoHold.API.LogJSON @@ -71,8 +74,8 @@ propLogUpload dom princ meta pathTxt = QC.ioProperty $ do in obj QC.=== expected _ -> counterexample "No logs emitted" False -propLogDownload :: Domain -> V3.Principal -> V3.Principal -> AssetAuditLogMetadata -> T.Text -> QC.Property -propLogDownload dom owner downloader meta pathTxt = QC.ioProperty $ do +propLogDownload :: Domain -> V3.Principal -> V3.Principal -> AssetAuditLogMetadata -> T.Text -> Maybe IpAddr -> QC.Property +propLogDownload dom owner downloader meta pathTxt mIp = QC.ioProperty $ do let qDownloader = Qualified downloader dom s3meta = S3AssetMeta @@ -82,7 +85,7 @@ propLogDownload dom owner downloader meta pathTxt = QC.ioProperty $ do v3AssetAuditLogMetadata = Just meta } (_, logs) <- withStructuredJSONLogger $ \logger -> - runWithLogger logger (logDownload (Just qDownloader) s3meta pathTxt :: LoggerT IO ()) + runWithLogger logger (logDownload (Just qDownloader) mIp s3meta pathTxt :: LoggerT IO ()) pure $ case logs of (obj : _) -> let expected = @@ -94,6 +97,7 @@ propLogDownload dom owner downloader meta pathTxt = QC.ioProperty $ do "downloader.type" .= uploaderType downloader, "downloader.id" .= uploaderId downloader, "downloader.domain" .= domainText dom, + "downloader.backend.ip" .= maybe "N/A" (decodeUtf8 . toByteString') mIp, "conversation.id" .= T.pack (show (qUnqualified meta.convId)), "conversation.domain" .= domainText (qDomain meta.convId), "file.name" .= meta.filename, @@ -148,12 +152,12 @@ propLogSignedURLCreation qDownloader mMeta = QC.ioProperty $ do in obj QC.=== expected _ -> counterexample "No logs emitted" False -propLogDownloadRemoteAsset :: Domain -> UserId -> Domain -> QC.Property -propLogDownloadRemoteAsset domLocal uid domRemote = QC.ioProperty $ do +propLogDownloadRemoteAsset :: Domain -> UserId -> Domain -> Maybe IpAddr -> QC.Property +propLogDownloadRemoteAsset domLocal uid domRemote mIp = QC.ioProperty $ do let luid = toLocalUnsafe domLocal uid rmt = toRemoteUnsafe domRemote () (_, logs) <- withStructuredJSONLogger $ \logger -> - runWithLogger logger (logDownloadRemoteAsset luid rmt :: LoggerT IO ()) + runWithLogger logger (logDownloadRemoteAsset luid rmt mIp :: LoggerT IO ()) pure $ case logs of (obj : _) -> let expected = @@ -165,7 +169,8 @@ propLogDownloadRemoteAsset domLocal uid domRemote = QC.ioProperty $ do "downloader.type" .= ("user" :: T.Text), "downloader.id" .= T.pack (show uid), "downloader.domain" .= domainText domLocal, - "remote.domain" .= domainText domRemote + "remote.domain" .= domainText domRemote, + "remote.backend.ip" .= maybe "N/A" (decodeUtf8 . toByteString') mIp ] in obj QC.=== expected _ -> counterexample "No logs emitted" False diff --git a/services/cargohold/test/unit/Test/CargoHold/API/LogJSON.hs b/services/cargohold/test/unit/Test/CargoHold/API/LogJSON.hs index 3553ad5f6e..ff3991f58d 100644 --- a/services/cargohold/test/unit/Test/CargoHold/API/LogJSON.hs +++ b/services/cargohold/test/unit/Test/CargoHold/API/LogJSON.hs @@ -17,8 +17,6 @@ module Test.CargoHold.API.LogJSON ( withStructuredJSONLogger, - lookupText, - lookupBool, ) where @@ -26,8 +24,6 @@ import qualified Control.Concurrent as CC import Control.Concurrent.Chan (Chan, newChan, readChan, writeChan) import Data.Aeson (Value) import qualified Data.Aeson as A -import qualified Data.Aeson.Key as Key -import qualified Data.Aeson.KeyMap as KM import qualified Data.ByteString.Char8 as BS8 import qualified Data.ByteString.Lazy as LBS import qualified GHC.IO.Handle as IOH @@ -95,15 +91,3 @@ withStructuredJSONLogger action = bracket acquire release use Just v -> drain (v : acc) logs <- drain [] pure (r, logs) - -lookupText :: Text -> Value -> Maybe Text -lookupText k (A.Object o) = case KM.lookup (Key.fromText k) o of - Just (A.String t) -> Just t - _ -> Nothing -lookupText _ _ = Nothing - -lookupBool :: Text -> Value -> Maybe Bool -lookupBool k (A.Object o) = case KM.lookup (Key.fromText k) o of - Just (A.Bool b) -> Just b - _ -> Nothing -lookupBool _ _ = Nothing diff --git a/services/federator/default.nix b/services/federator/default.nix index 80604052aa..e1beff920c 100644 --- a/services/federator/default.nix +++ b/services/federator/default.nix @@ -40,6 +40,7 @@ , metrics-core , metrics-wai , mtl +, network , optparse-applicative , pem , polysemy @@ -108,6 +109,7 @@ mkDerivation { metrics-core metrics-wai mtl + network pem polysemy polysemy-wire-zoo diff --git a/services/federator/federator.cabal b/services/federator/federator.cabal index d81b38e50f..597cfc7e11 100644 --- a/services/federator/federator.cabal +++ b/services/federator/federator.cabal @@ -134,6 +134,7 @@ library , metrics-core , metrics-wai , mtl + , network , pem , polysemy , polysemy-wire-zoo diff --git a/services/federator/src/Federator/ExternalServer.hs b/services/federator/src/Federator/ExternalServer.hs index f4e3537ce5..51a5911272 100644 --- a/services/federator/src/Federator/ExternalServer.hs +++ b/services/federator/src/Federator/ExternalServer.hs @@ -27,6 +27,7 @@ where import Data.Bifunctor import Data.ByteString qualified as BS import Data.ByteString.Builder +import Data.ByteString.Char8 qualified as BS8 import Data.ByteString.Lazy qualified as LBS import Data.Domain import Data.Sequence qualified as Seq @@ -60,6 +61,7 @@ import Servant.Server.Generic (AsServerT) import System.Logger.Message qualified as Log import Wire.API.Federation.Component import Wire.API.Federation.Domain +import Wire.API.Federation.Endpoint (originIpHeaderName) import Wire.API.Routes.FederationDomainConfig import Wire.API.VersionInfo @@ -155,7 +157,17 @@ callInward component (RPC rpc) originDomain (CertHeader cert) wreq cont = do let path = LBS.toStrict (toLazyByteString (HTTP.encodePathSegments ["federation", rpc])) body <- embed $ Wai.lazyRequestBody wreq - let headers = filter ((== versionHeader) . fst) (Wai.requestHeaders wreq) + -- Build headers to forward internally: keep only API version and add Wire-Origin-IP if present + let reqHeaders = Wai.requestHeaders wreq + headersVersion = filter ((== versionHeader) . fst) reqHeaders + mFirstIP = + lookup "X-Forwarded-For" reqHeaders >>= \xff -> do + let fstHdr = BS8.strip $ BS8.takeWhile (/= ',') xff + guard (not (BS8.null fstHdr)) $> fstHdr + mXReal = lookup "X-Real-IP" reqHeaders + mOriginIp = (originIpHeaderName,) <$> (mFirstIP <|> mXReal) + -- FUTUREWORK: did we miss passing the request ID header? + headers = maybeToList mOriginIp <> headersVersion resp <- serviceCall component path headers body validatedDomain Log.debug $ Log.msg ("Inward Request response" :: ByteString) diff --git a/services/federator/src/Federator/Remote.hs b/services/federator/src/Federator/Remote.hs index 6e72c694f5..cbf49fe156 100644 --- a/services/federator/src/Federator/Remote.hs +++ b/services/federator/src/Federator/Remote.hs @@ -32,8 +32,12 @@ import Control.Monad.Codensity import Data.Binary.Builder import Data.ByteString.Lazy qualified as LBS import Data.Domain +import Data.IORef qualified as IORef import Data.Id +import Data.Sequence qualified as Seq +import Data.Text qualified as T import Data.Text.Encoding (decodeUtf8) +import Data.Text.Encoding qualified as TEnc import Federator.Discovery import Federator.Error import HTTP2.Client.Manager (Http2Manager) @@ -41,6 +45,7 @@ import HTTP2.Client.Manager qualified as H2Manager import Imports import Network.HTTP.Types qualified as HTTP import Network.HTTP2.Client qualified as HTTP2 +import Network.Socket qualified as Sock import Polysemy import Polysemy.Error import Polysemy.Input @@ -48,6 +53,7 @@ import Servant.Client.Core import System.Logger qualified as Log import Wire.API.Federation.Client import Wire.API.Federation.Component +import Wire.API.Federation.Endpoint (remoteIpHeaderName) import Wire.API.Federation.Error import Wire.Network.DNS.SRV @@ -106,10 +112,20 @@ interpretRemote = interpret $ \case req' = HTTP2.requestBuilder HTTP.methodPost path headers' body mgr <- input + ipRef <- embed @(Codensity IO) . liftIO $ IORef.newIORef Nothing resp <- mapError (RemoteError target pathT) . (fromEither @FederatorClientHTTP2Error =<<) . embed $ Codensity $ \k -> E.catches - (H2Manager.withHTTP2RequestOnSingleUseConn mgr (True, hostname, fromIntegral port) req' (consumeStreamingResponseWith $ k . Right)) + ( H2Manager.withHTTP2RequestOnSingleUseConnWithHook + mgr + (True, hostname, fromIntegral port) + req' + ( \peerAddr -> do + (mhost, _) <- Sock.getNameInfo [Sock.NI_NUMERICHOST] True False peerAddr + IORef.writeIORef ipRef mhost + ) + (consumeStreamingResponseWith (k . Right)) + ) [ E.Handler $ k . Left, E.Handler $ k . Left . FederatorClientTLSException, E.Handler $ k . Left . FederatorClientHTTP2Exception, @@ -124,4 +140,6 @@ interpretRemote = interpret $ \case pathT (responseStatusCode resp) (toLazyByteString bdy) - pure resp + mIpTxt <- embed @(Codensity IO) . liftIO $ IORef.readIORef ipRef + let mIpBS = fmap (TEnc.encodeUtf8 . T.pack) mIpTxt + pure $ maybe resp (\ipbs -> resp {responseHeaders = responseHeaders resp Seq.|> (remoteIpHeaderName, ipbs)}) mIpBS diff --git a/services/federator/test/unit/Test/Federator/ExternalServer.hs b/services/federator/test/unit/Test/Federator/ExternalServer.hs index 93af2e8ba9..2dd62bb7ba 100644 --- a/services/federator/test/unit/Test/Federator/ExternalServer.hs +++ b/services/federator/test/unit/Test/Federator/ExternalServer.hs @@ -127,6 +127,7 @@ requestBrigSuccess = { Wai.requestHeaders = ("Invalid-Header", "foo") : ("X-Wire-API-Version", "v0") + : ("X-Forwarded-For", ", ") : Wai.requestHeaders request0 } Right cert <- decodeCertificate <$> BS.readFile "test/resources/unit/localhost.example.com.pem" @@ -152,7 +153,14 @@ requestBrigSuccess = $ callInward Brig (RPC "get-user-by-handle") aValidDomain (CertHeader cert) request (saveResponse resRef) Just res <- readIORef resRef - let expectedCall = Call Brig "/federation/get-user-by-handle" [("X-Wire-API-Version", "v0")] "\"foo\"" aValidDomain + let expectedCall = + Call + { cComponent = Brig, + cPath = "/federation/get-user-by-handle", + cHeaders = [("Wire-Origin-IP", ""), ("X-Wire-API-Version", "v0")], + cBody = "\"foo\"", + cDomain = aValidDomain + } assertEqual "one call to brig should be made" [expectedCall] actualCalls Wai.responseStatus res @?= HTTP.status200 body <- Wai.lazyResponseBody res diff --git a/services/nginz/integration-test/conf/nginz/common_response.conf b/services/nginz/integration-test/conf/nginz/common_response.conf index 6cbeb98be3..200dc202bb 100644 --- a/services/nginz/integration-test/conf/nginz/common_response.conf +++ b/services/nginz/integration-test/conf/nginz/common_response.conf @@ -15,6 +15,9 @@ proxy_set_header Z-Conversation $zauth_conversation; proxy_set_header Z-Timestamp $zauth_timestamp; proxy_set_header Request-Id $request_id; + # Forward client IP information to upstreams + proxy_set_header X-Forwarded-For $xff_with_client; + proxy_set_header X-Real-IP $client_ip; # NOTE: This should only be used on endpoints where credentials are needed more_set_headers 'Access-Control-Allow-Credentials: true'; diff --git a/services/nginz/integration-test/conf/nginz/nginx.conf b/services/nginz/integration-test/conf/nginz/nginx.conf index 02b39c47e1..9ecf9ab4f8 100644 --- a/services/nginz/integration-test/conf/nginz/nginx.conf +++ b/services/nginz/integration-test/conf/nginz/nginx.conf @@ -84,6 +84,25 @@ http { gzip_min_length 1024; gzip_types 'text/plain text/css application/json text/xml'; + # 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; + + # Normalize client IP to IPv4 when possible for local tests + # - Collapse IPv6 loopback (::1) to 127.0.0.1 + # - Strip IPv4-mapped IPv6 prefix (::ffff:) + map $remote_addr $client_ip { + ~^::ffff:(?.+)$ $ipv4; + ~^::1$ 127.0.0.1; + default $remote_addr; + } + + # Build X-Forwarded-For by appending the normalized client IP + map $http_x_forwarded_for $xff_with_client { + default "$http_x_forwarded_for, $client_ip"; + "" "$client_ip"; + } + # # Proxied Upstream Services # @@ -149,6 +168,10 @@ http { set $sanitized_request $request; zauth off; + # Forward client IP information to upstream federator + proxy_set_header X-Forwarded-For $xff_with_client; + proxy_set_header X-Real-IP $client_ip; + proxy_set_header "X-SSL-Certificate" $ssl_client_escaped_cert; proxy_pass http://federator_external; From 5d7cd114d1392ee7d34d27dabb421dc5598afb4f Mon Sep 17 00:00:00 2001 From: Leif Battermann Date: Wed, 1 Oct 2025 09:30:11 +0000 Subject: [PATCH 2/5] use custom header to forward IP address --- charts/nginz/templates/conf/_nginx.conf.tpl | 10 +++++++--- .../federator/src/Federator/ExternalServer.hs | 18 ++++++++++++------ .../conf/nginz/common_response.conf | 5 +++-- .../integration-test/conf/nginz/nginx.conf | 6 ++++-- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/charts/nginz/templates/conf/_nginx.conf.tpl b/charts/nginz/templates/conf/_nginx.conf.tpl index 9538ca0352..a119fe9d3f 100644 --- a/charts/nginz/templates/conf/_nginx.conf.tpl +++ b/charts/nginz/templates/conf/_nginx.conf.tpl @@ -196,7 +196,7 @@ http { {{- $validUpstreams := include "valid_upstreams" . | fromJson }} {{- range $name, $_ := $validUpstreams }} - upstream {{ $name }}{{ if hasKey $.Values.nginx_conf.upstream_namespace $name }}.{{ get $.Values.nginx_conf.upstream_namespace $name }}{{end}} { + upstream {{ $name }}{{ if hasKey $.Values.nginx_conf.upstream_namespace $name }}.{{ get $.Values.nginx_conf.upstream_namespace $name }}{{end}} { zone {{ $name }} 64k; # needed for dynamic DNS updates least_conn; keepalive 32; @@ -323,8 +323,12 @@ http { proxy_http_version 1.1; # Forward client IP information to upstreams - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - proxy_set_header X-Real-IP $remote_addr; + # NOTE: Do NOT override X-Forwarded-For here. + # - We intentionally leave X-Forwarded-For as received from upstreams + # to preserve existing semantics (e.g., brig expects a single IP). + # - For components that need the full chain, we add X-Wire-Forwarded-For. + proxy_set_header X-Wire-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Real-IP $remote_addr; {{- if ($location.disable_request_buffering) }} proxy_request_buffering off; diff --git a/services/federator/src/Federator/ExternalServer.hs b/services/federator/src/Federator/ExternalServer.hs index 51a5911272..5fb9b06a7e 100644 --- a/services/federator/src/Federator/ExternalServer.hs +++ b/services/federator/src/Federator/ExternalServer.hs @@ -28,8 +28,10 @@ import Data.Bifunctor import Data.ByteString qualified as BS import Data.ByteString.Builder import Data.ByteString.Char8 qualified as BS8 +import Data.ByteString.Conversion (fromByteString) import Data.ByteString.Lazy qualified as LBS import Data.Domain +import Data.Misc import Data.Sequence qualified as Seq import Data.Text qualified as Text import Data.Text.Encoding qualified as Text @@ -46,6 +48,7 @@ import Federator.Service import Federator.Validation import Imports import Network.HTTP.Client (Manager) +import Network.HTTP.Types (RequestHeaders) import Network.HTTP.Types qualified as HTTP import Network.Wai qualified as Wai import Polysemy @@ -160,12 +163,7 @@ callInward component (RPC rpc) originDomain (CertHeader cert) wreq cont = do -- Build headers to forward internally: keep only API version and add Wire-Origin-IP if present let reqHeaders = Wai.requestHeaders wreq headersVersion = filter ((== versionHeader) . fst) reqHeaders - mFirstIP = - lookup "X-Forwarded-For" reqHeaders >>= \xff -> do - let fstHdr = BS8.strip $ BS8.takeWhile (/= ',') xff - guard (not (BS8.null fstHdr)) $> fstHdr - mXReal = lookup "X-Real-IP" reqHeaders - mOriginIp = (originIpHeaderName,) <$> (mFirstIP <|> mXReal) + mOriginIp = (originIpHeaderName,) <$> tryGetOriginIp reqHeaders -- FUTUREWORK: did we miss passing the request ID header? headers = maybeToList mOriginIp <> headersVersion resp <- serviceCall component path headers body validatedDomain @@ -180,6 +178,14 @@ callInward component (RPC rpc) originDomain (CertHeader cert) wreq cont = do (\(name, _) -> name == "Content-Type") (responseHeaders resp) } + where + tryGetOriginIp :: RequestHeaders -> Maybe ByteString + tryGetOriginIp headers = + let isIpAddr = isJust . fromByteString @IpAddr + firstFrom hdr = + lookup hdr headers >>= \val -> do + find isIpAddr $ BS8.strip <$> BS8.split ',' val + in firstFrom "X-Wire-Forwarded-For" <|> firstFrom "X-Forwarded-For" <|> firstFrom "X-Real-IP" serveInward :: Env -> Int -> IORef [IO ()] -> IO () serveInward env port cleanupsRef = diff --git a/services/nginz/integration-test/conf/nginz/common_response.conf b/services/nginz/integration-test/conf/nginz/common_response.conf index 200dc202bb..9f0c4fad11 100644 --- a/services/nginz/integration-test/conf/nginz/common_response.conf +++ b/services/nginz/integration-test/conf/nginz/common_response.conf @@ -16,8 +16,9 @@ proxy_set_header Z-Timestamp $zauth_timestamp; proxy_set_header Request-Id $request_id; # Forward client IP information to upstreams - proxy_set_header X-Forwarded-For $xff_with_client; - proxy_set_header X-Real-IP $client_ip; + # NOTE: Leave X-Forwarded-For untouched; add full chain for tests + proxy_set_header X-Wire-Forwarded-For $xff_with_client; + proxy_set_header X-Real-IP $client_ip; # NOTE: This should only be used on endpoints where credentials are needed more_set_headers 'Access-Control-Allow-Credentials: true'; diff --git a/services/nginz/integration-test/conf/nginz/nginx.conf b/services/nginz/integration-test/conf/nginz/nginx.conf index 9ecf9ab4f8..8d279ebc14 100644 --- a/services/nginz/integration-test/conf/nginz/nginx.conf +++ b/services/nginz/integration-test/conf/nginz/nginx.conf @@ -169,8 +169,10 @@ http { zauth off; # Forward client IP information to upstream federator - proxy_set_header X-Forwarded-For $xff_with_client; - proxy_set_header X-Real-IP $client_ip; + # NOTE: Do NOT override X-Forwarded-For in tests to mirror production behavior. + # Provide full chain via X-Wire-Forwarded-For and a single client IP via X-Real-IP. + proxy_set_header X-Wire-Forwarded-For $xff_with_client; + proxy_set_header X-Real-IP $client_ip; proxy_set_header "X-SSL-Certificate" $ssl_client_escaped_cert; proxy_pass http://federator_external; From 3f12e7edcd70d4ce87125c4736d27757f366990c Mon Sep 17 00:00:00 2001 From: Leif Battermann Date: Wed, 1 Oct 2025 12:11:40 +0000 Subject: [PATCH 3/5] validate IP address while extracting from header --- .../federator/src/Federator/ExternalServer.hs | 26 +++++++++++++++--- .../unit/Test/Federator/ExternalServer.hs | 27 +++++++++++++++++-- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/services/federator/src/Federator/ExternalServer.hs b/services/federator/src/Federator/ExternalServer.hs index 5fb9b06a7e..33258b5ed9 100644 --- a/services/federator/src/Federator/ExternalServer.hs +++ b/services/federator/src/Federator/ExternalServer.hs @@ -21,6 +21,7 @@ module Federator.ExternalServer RPC (..), CertHeader (..), server, + extractIp, ) where @@ -181,12 +182,29 @@ callInward component (RPC rpc) originDomain (CertHeader cert) wreq cont = do where tryGetOriginIp :: RequestHeaders -> Maybe ByteString tryGetOriginIp headers = - let isIpAddr = isJust . fromByteString @IpAddr - firstFrom hdr = - lookup hdr headers >>= \val -> do - find isIpAddr $ BS8.strip <$> BS8.split ',' val + let firstFrom hdr = + lookup hdr headers >>= \val -> listToMaybe $ mapMaybe extractIp (BS8.split ',' val) in firstFrom "X-Wire-Forwarded-For" <|> firstFrom "X-Forwarded-For" <|> firstFrom "X-Real-IP" +-- | Extract the IPv4/IPv6 portion from a host[:port] or [host]:port. +extractIp :: ByteString -> Maybe ByteString +extractIp str = + verify stripWs <|> verify stripPort <|> verify stripBrackets + where + stripBrackets :: ByteString + stripBrackets = BS8.dropWhile (== '[') . BS8.dropWhileEnd (== ']') $ stripPort + + stripPort :: ByteString + stripPort = BS8.dropWhileEnd (== ':') . BS8.dropWhileEnd isDigit $ stripWs + + stripWs :: ByteString + stripWs = BS8.strip str + + verify :: ByteString -> Maybe ByteString + verify bs = case fromByteString @IpAddr bs of + Just _ -> Just bs + Nothing -> Nothing + serveInward :: Env -> Int -> IORef [IO ()] -> IO () serveInward env port cleanupsRef = serveServant @(ToServantApi API) env port cleanupsRef $ toServant $ server env._httpManager env._internalPort diff --git a/services/federator/test/unit/Test/Federator/ExternalServer.hs b/services/federator/test/unit/Test/Federator/ExternalServer.hs index 2dd62bb7ba..b7a427505a 100644 --- a/services/federator/test/unit/Test/Federator/ExternalServer.hs +++ b/services/federator/test/unit/Test/Federator/ExternalServer.hs @@ -71,6 +71,7 @@ tests = requestInvalidCertificate, requestNoDomain, testInvalidPaths, + testExtractIp, testMethod ] @@ -127,7 +128,7 @@ requestBrigSuccess = { Wai.requestHeaders = ("Invalid-Header", "foo") : ("X-Wire-API-Version", "v0") - : ("X-Forwarded-For", ", ") + : ("X-Forwarded-For", "unknown, [2001:db8::1]:443, 127.0.0.43") : Wai.requestHeaders request0 } Right cert <- decodeCertificate <$> BS.readFile "test/resources/unit/localhost.example.com.pem" @@ -157,7 +158,7 @@ requestBrigSuccess = Call { cComponent = Brig, cPath = "/federation/get-user-by-handle", - cHeaders = [("Wire-Origin-IP", ""), ("X-Wire-API-Version", "v0")], + cHeaders = [("Wire-Origin-IP", "2001:db8::1"), ("X-Wire-API-Version", "v0")], cBody = "\"foo\"", cDomain = aValidDomain } @@ -166,6 +167,28 @@ requestBrigSuccess = body <- Wai.lazyResponseBody res body @?= "\"bar\"" +testExtractIp :: TestTree +testExtractIp = + testGroup + "extractIp" + [ testCase "trims whitespace and parses IPv4" $ do + extractIp " 127.0.0.1 " @?= Just "127.0.0.1", + testCase "parses IPv4 with port" $ do + extractIp "192.0.2.10:8443" @?= Just "192.0.2.10", + testCase "parses bracketed IPv6" $ do + extractIp "[2001:db8::1]" @?= Just "2001:db8::1", + testCase "parses bracketed IPv6 with port" $ do + extractIp "[2001:db8::2]:443" @?= Just "2001:db8::2", + testCase "parses bare IPv6" $ do + extractIp "2001:db8::3" @?= Just "2001:db8::3", + testCase "parses IPv4-mapped IPv6" $ do + extractIp "[::ffff:192.168.0.1]:123" @?= Just "::ffff:192.168.0.1", + testCase "rejects unknown" $ do + extractIp "unknown" @?= Nothing, + testCase "rejects invalid" $ do + extractIp "not-an-ip" @?= Nothing + ] + requestBrigFailure :: TestTree requestBrigFailure = testCase "should preserve the status code returned by the service" $ do From db54528f13c23fd5fff613bf1c9794ff460e90d6 Mon Sep 17 00:00:00 2001 From: Leif Battermann Date: Wed, 1 Oct 2025 13:27:13 +0000 Subject: [PATCH 4/5] clean up --- services/cargohold/src/CargoHold/Federation.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/cargohold/src/CargoHold/Federation.hs b/services/cargohold/src/CargoHold/Federation.hs index ec9ce8a555..ea618f085f 100644 --- a/services/cargohold/src/CargoHold/Federation.hs +++ b/services/cargohold/src/CargoHold/Federation.hs @@ -125,5 +125,5 @@ executeFederatedStreaming remote c onRemoteIp = do (runFederatorClientToCodensity @'Cargohold env c) ( either (throw . federationErrorToWai . FederationCallFailure) - (\(mIp, src) -> (runAppT appEnv $ (runExceptT (onRemoteIp mIp))) *> unSourceT src k) + (\(mIp, src) -> runAppT appEnv (runExceptT (onRemoteIp mIp)) *> unSourceT src k) ) From b1483ea1d9213cd67b8bbc6c9dbd3b6c7630fffb Mon Sep 17 00:00:00 2001 From: Leif Battermann Date: Thu, 2 Oct 2025 10:44:29 +0000 Subject: [PATCH 5/5] only rely on X-Forwared-For and leave nginz config alone --- charts/nginz/templates/conf/_nginx.conf.tpl | 10 +--------- services/federator/src/Federator/ExternalServer.hs | 5 +---- .../integration-test/conf/nginz/common_response.conf | 3 +-- services/nginz/integration-test/conf/nginz/nginx.conf | 5 +---- 4 files changed, 4 insertions(+), 19 deletions(-) diff --git a/charts/nginz/templates/conf/_nginx.conf.tpl b/charts/nginz/templates/conf/_nginx.conf.tpl index a119fe9d3f..b3dce8f465 100644 --- a/charts/nginz/templates/conf/_nginx.conf.tpl +++ b/charts/nginz/templates/conf/_nginx.conf.tpl @@ -196,7 +196,7 @@ http { {{- $validUpstreams := include "valid_upstreams" . | fromJson }} {{- range $name, $_ := $validUpstreams }} - upstream {{ $name }}{{ if hasKey $.Values.nginx_conf.upstream_namespace $name }}.{{ get $.Values.nginx_conf.upstream_namespace $name }}{{end}} { + upstream {{ $name }}{{ if hasKey $.Values.nginx_conf.upstream_namespace $name }}.{{ get $.Values.nginx_conf.upstream_namespace $name }}{{end}} { zone {{ $name }} 64k; # needed for dynamic DNS updates least_conn; keepalive 32; @@ -322,14 +322,6 @@ http { proxy_pass http://{{ $name }}{{ if hasKey $.Values.nginx_conf.upstream_namespace $name }}.{{ get $.Values.nginx_conf.upstream_namespace $name }}{{end}}; proxy_http_version 1.1; - # Forward client IP information to upstreams - # NOTE: Do NOT override X-Forwarded-For here. - # - We intentionally leave X-Forwarded-For as received from upstreams - # to preserve existing semantics (e.g., brig expects a single IP). - # - For components that need the full chain, we add X-Wire-Forwarded-For. - proxy_set_header X-Wire-Forwarded-For $proxy_add_x_forwarded_for; - proxy_set_header X-Real-IP $remote_addr; - {{- if ($location.disable_request_buffering) }} proxy_request_buffering off; {{ end -}} diff --git a/services/federator/src/Federator/ExternalServer.hs b/services/federator/src/Federator/ExternalServer.hs index 33258b5ed9..2a9c8a1b82 100644 --- a/services/federator/src/Federator/ExternalServer.hs +++ b/services/federator/src/Federator/ExternalServer.hs @@ -181,10 +181,7 @@ callInward component (RPC rpc) originDomain (CertHeader cert) wreq cont = do } where tryGetOriginIp :: RequestHeaders -> Maybe ByteString - tryGetOriginIp headers = - let firstFrom hdr = - lookup hdr headers >>= \val -> listToMaybe $ mapMaybe extractIp (BS8.split ',' val) - in firstFrom "X-Wire-Forwarded-For" <|> firstFrom "X-Forwarded-For" <|> firstFrom "X-Real-IP" + tryGetOriginIp headers = lookup "X-Forwarded-For" headers >>= listToMaybe . mapMaybe extractIp . BS8.split ',' -- | Extract the IPv4/IPv6 portion from a host[:port] or [host]:port. extractIp :: ByteString -> Maybe ByteString diff --git a/services/nginz/integration-test/conf/nginz/common_response.conf b/services/nginz/integration-test/conf/nginz/common_response.conf index 9f0c4fad11..dc16b645eb 100644 --- a/services/nginz/integration-test/conf/nginz/common_response.conf +++ b/services/nginz/integration-test/conf/nginz/common_response.conf @@ -17,8 +17,7 @@ proxy_set_header Request-Id $request_id; # Forward client IP information to upstreams # NOTE: Leave X-Forwarded-For untouched; add full chain for tests - proxy_set_header X-Wire-Forwarded-For $xff_with_client; - proxy_set_header X-Real-IP $client_ip; + proxy_set_header X-Forwarded-For $xff_with_client; # NOTE: This should only be used on endpoints where credentials are needed more_set_headers 'Access-Control-Allow-Credentials: true'; diff --git a/services/nginz/integration-test/conf/nginz/nginx.conf b/services/nginz/integration-test/conf/nginz/nginx.conf index 8d279ebc14..3a9d033601 100644 --- a/services/nginz/integration-test/conf/nginz/nginx.conf +++ b/services/nginz/integration-test/conf/nginz/nginx.conf @@ -169,10 +169,7 @@ http { zauth off; # Forward client IP information to upstream federator - # NOTE: Do NOT override X-Forwarded-For in tests to mirror production behavior. - # Provide full chain via X-Wire-Forwarded-For and a single client IP via X-Real-IP. - proxy_set_header X-Wire-Forwarded-For $xff_with_client; - proxy_set_header X-Real-IP $client_ip; + proxy_set_header X-Forwarded-For $xff_with_client; proxy_set_header "X-SSL-Certificate" $ssl_client_escaped_cert; proxy_pass http://federator_external;