From 586e543679e827ca2569950db239a4f743b24e04 Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Wed, 24 Jan 2024 16:26:00 +0100 Subject: [PATCH 1/7] [fix] reuse manager --- libs/ssl-util/src/Ssl/Util.hs | 5 +++-- services/brig/src/Brig/App.hs | 15 +++++++++------ services/brig/src/Brig/Provider/RPC.hs | 4 ++-- services/galley/src/Galley/App.hs | 4 +++- services/galley/src/Galley/Env.hs | 8 ++++---- services/galley/src/Galley/External.hs | 4 ++-- .../Galley/External/LegalHoldService/Internal.hs | 4 ++-- 7 files changed, 25 insertions(+), 19 deletions(-) diff --git a/libs/ssl-util/src/Ssl/Util.hs b/libs/ssl-util/src/Ssl/Util.hs index a7375a4084..1598c100bf 100644 --- a/libs/ssl-util/src/Ssl/Util.hs +++ b/libs/ssl-util/src/Ssl/Util.hs @@ -182,17 +182,18 @@ verifyRsaFingerprint d = verifyFingerprint $ \pk -> -- | this is used as a 'OpenSSL.Session.vpCallback' in 'Brig.App.initExtGetManager' -- and 'Galley.Env.initExtEnv' -extEnvCallback :: [Fingerprint Rsa] -> X509StoreCtx -> IO Bool +extEnvCallback :: IORef [Fingerprint Rsa] -> X509StoreCtx -> IO Bool extEnvCallback fingerprints store = do Just sha <- getDigestByName "SHA256" cert <- getStoreCtxCert store pk <- getPublicKey cert + fprs <- readIORef fingerprints case toPublicKey @RSAPubKey pk of Nothing -> pure False Just k -> do fp <- rsaFingerprint sha k -- find at least one matching fingerprint to continue - if not (any (constEqBytes fp . fingerprintBytes) fingerprints) + if not (any (constEqBytes fp . fingerprintBytes) fprs) then pure False else do -- Check if the certificate is self-signed. diff --git a/services/brig/src/Brig/App.hs b/services/brig/src/Brig/App.hs index 439aae35de..9a10bb66f5 100644 --- a/services/brig/src/Brig/App.hs +++ b/services/brig/src/Brig/App.hs @@ -172,7 +172,7 @@ data Env = Env _templateBranding :: TemplateBranding, _httpManager :: Manager, _http2Manager :: Http2Manager, - _extGetManager :: [Fingerprint Rsa] -> IO Manager, + _extGetManager :: (Manager, IORef [Fingerprint Rsa]), _settings :: Settings, _nexmoCreds :: Nexmo.Credentials, _twilioCreds :: Twilio.Credentials, @@ -246,6 +246,9 @@ newEnv o = do pure Nothing kpLock <- newMVar () rabbitChan <- traverse (Q.mkRabbitMqChannelMVar lgr) o.rabbitmq + fprVar <- newIORef [] + extMgr <- initExtGetManager fprVar + pure $! Env { _cargohold = mkEndpoint $ Opt.cargohold o, @@ -267,7 +270,7 @@ newEnv o = do _templateBranding = branding, _httpManager = mgr, _http2Manager = h2Mgr, - _extGetManager = initExtGetManager, + _extGetManager = (extMgr, fprVar), _settings = sett, _nexmoCreds = nxm, _twilioCreds = twl, @@ -359,8 +362,8 @@ initHttp2Manager = do -- faster. So, we reuse the context. -- TODO: somewhat duplicates Galley.App.initExtEnv -initExtGetManager :: [Fingerprint Rsa] -> IO Manager -initExtGetManager fingerprints = do +initExtGetManager :: IORef [Fingerprint Rsa] -> IO Manager +initExtGetManager fprVar = do ctx <- SSL.context SSL.contextAddOption ctx SSL_OP_NO_SSLv2 SSL.contextAddOption ctx SSL_OP_NO_SSLv3 @@ -369,8 +372,8 @@ initExtGetManager fingerprints = do ctx SSL.VerifyPeer { vpFailIfNoPeerCert = True, - vpClientOnce = False, - vpCallback = Just \_b -> extEnvCallback fingerprints + vpClientOnce = True, + vpCallback = Just \_b -> extEnvCallback fprVar } SSL.contextSetDefaultVerifyPaths ctx diff --git a/services/brig/src/Brig/Provider/RPC.hs b/services/brig/src/Brig/Provider/RPC.hs index 7244148216..e145b86514 100644 --- a/services/brig/src/Brig/Provider/RPC.hs +++ b/services/brig/src/Brig/Provider/RPC.hs @@ -71,8 +71,8 @@ data ServiceError createBot :: ServiceConn -> NewBotRequest -> ExceptT ServiceError (AppT r) NewBotResponse createBot scon new = do let fprs = toList (sconFingerprints scon) - manF <- view extGetManager - man <- liftIO $ manF fprs + (man, fprVar) <- view extGetManager + atomicModifyIORef' fprVar \oldFprs -> (nub (fprs <> oldFprs), ()) extHandleAll onExc $ do let req = reqBuilder Http.defaultRequest rs <- lift $ diff --git a/services/galley/src/Galley/App.hs b/services/galley/src/Galley/App.hs index 1680941a3e..8004879993 100644 --- a/services/galley/src/Galley/App.hs +++ b/services/galley/src/Galley/App.hs @@ -158,9 +158,11 @@ createEnv m o l = do mgr <- initHttpManager o h2mgr <- initHttp2Manager codeURIcfg <- validateOptions o + fprVar <- newIORef [] + extEnv <- initExtEnv fprVar Env (RequestId "N/A") m o l mgr h2mgr (o ^. O.federator) (o ^. O.brig) cass <$> Q.new 16000 - <*> pure initExtEnv + <*> pure (extEnv, fprVar) <*> maybe (pure Nothing) (fmap Just . Aws.mkEnv l mgr) (o ^. journal) <*> loadAllMLSKeys (fold (o ^. settings . mlsPrivateKeyPaths)) <*> traverse (mkRabbitMqChannelMVar l) (o ^. rabbitmq) diff --git a/services/galley/src/Galley/Env.hs b/services/galley/src/Galley/Env.hs index 4a9687c3e3..89f7d5b03a 100644 --- a/services/galley/src/Galley/Env.hs +++ b/services/galley/src/Galley/Env.hs @@ -54,11 +54,11 @@ data Env = Env _applog :: Logger, _manager :: Manager, _http2Manager :: Http2Manager, - _federator :: Maybe Endpoint, -- FUTUREWORK: should we use a better type here? E.g. to avoid fresh connections all the time? + _federator :: Maybe Endpoint, -- FUTUREWORK: should we use a better type ----- LegalHold.testLHClaimKeys01 FAIL (34.01 s) -----here? E.g. to avoid fresh connections all the time? _brig :: Endpoint, -- FUTUREWORK: see _federator _cstate :: ClientState, _deleteQueue :: Q.Queue DeleteItem, - _extGetManager :: [Fingerprint Rsa] -> IO Manager, + _extGetManager :: (Manager, IORef [Fingerprint Rsa]), _aEnv :: Maybe Aws.Env, _mlsKeys :: SignaturePurpose -> MLSKeys, _rabbitmqChannel :: Maybe (MVar Q.Channel), @@ -68,7 +68,7 @@ data Env = Env makeLenses ''Env -- TODO: somewhat duplicates Brig.App.initExtGetManager -initExtEnv :: [Fingerprint Rsa] -> IO Manager +initExtEnv :: IORef [Fingerprint Rsa] -> IO Manager initExtEnv fingerprints = do ctx <- Ssl.context Ssl.contextAddOption ctx SSL_OP_NO_SSLv2 @@ -79,7 +79,7 @@ initExtEnv fingerprints = do ctx Ssl.VerifyPeer { vpFailIfNoPeerCert = True, - vpClientOnce = False, + vpClientOnce = True, vpCallback = Just \_b -> extEnvCallback fingerprints } diff --git a/services/galley/src/Galley/External.hs b/services/galley/src/Galley/External.hs index fd775f44e3..f56ff3c538 100644 --- a/services/galley/src/Galley/External.hs +++ b/services/galley/src/Galley/External.hs @@ -151,8 +151,8 @@ urlPort (HttpsUrl u) = do sendMessage :: [Fingerprint Rsa] -> (Request -> Request) -> App () sendMessage fprs reqBuilder = do - mkMgr <- view extGetManager - man <- liftIO $ mkMgr fprs + (man, fprVar) <- view extGetManager + modifyIORef' fprVar (nub . (<> fprs)) let req = reqBuilder defaultRequest liftIO $ withConnection req man $ \_conn -> Http.withResponse req man (const $ pure ()) diff --git a/services/galley/src/Galley/External/LegalHoldService/Internal.hs b/services/galley/src/Galley/External/LegalHoldService/Internal.hs index 2106138763..877e355f16 100644 --- a/services/galley/src/Galley/External/LegalHoldService/Internal.hs +++ b/services/galley/src/Galley/External/LegalHoldService/Internal.hs @@ -77,6 +77,6 @@ makeVerifiedRequest :: (Http.Request -> Http.Request) -> App (Http.Response LC8.ByteString) makeVerifiedRequest fpr url reqBuilder = do - mkMgr <- view extGetManager - mgr <- liftIO $ mkMgr [fpr] + (mgr, fprVar) <- view extGetManager + modifyIORef' fprVar (nub . ([fpr] <>)) makeVerifiedRequestWithManager mgr url reqBuilder From 94e864c45ca12b6e2a03bb49097317f7f0419054 Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Wed, 24 Jan 2024 17:13:30 +0100 Subject: [PATCH 2/7] [feat] bring back no reuse of the manager for legalhold --- services/galley/src/Galley/Cassandra/LegalHold.hs | 3 +++ .../galley/src/Galley/Effects/LegalHoldStore.hs | 8 +++++++- .../galley/src/Galley/External/LegalHoldService.hs | 2 +- .../Galley/External/LegalHoldService/Internal.hs | 14 ++++++++++++++ 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/services/galley/src/Galley/Cassandra/LegalHold.hs b/services/galley/src/Galley/Cassandra/LegalHold.hs index ff09339630..db37db2657 100644 --- a/services/galley/src/Galley/Cassandra/LegalHold.hs +++ b/services/galley/src/Galley/Cassandra/LegalHold.hs @@ -73,6 +73,9 @@ interpretLegalHoldStoreToCassandra lh = interpret $ \case SetTeamLegalholdWhitelisted tid -> embedClient $ setTeamLegalholdWhitelisted tid UnsetTeamLegalholdWhitelisted tid -> embedClient $ unsetTeamLegalholdWhitelisted tid IsTeamLegalholdWhitelisted tid -> embedClient $ isTeamLegalholdWhitelisted lh tid + -- FUTUREWORK: should this action be part of a separate effect? + MakeVerifiedRequestFreshManager fpr url r -> + embedApp $ makeVerifiedRequestFreshManager fpr url r MakeVerifiedRequest fpr url r -> embedApp $ makeVerifiedRequest fpr url r ValidateServiceKey sk -> embed @IO $ validateServiceKey sk diff --git a/services/galley/src/Galley/Effects/LegalHoldStore.hs b/services/galley/src/Galley/Effects/LegalHoldStore.hs index 56d71864c5..e91dea42f4 100644 --- a/services/galley/src/Galley/Effects/LegalHoldStore.hs +++ b/services/galley/src/Galley/Effects/LegalHoldStore.hs @@ -36,6 +36,7 @@ module Galley.Effects.LegalHoldStore -- * Intra actions makeVerifiedRequest, + makeVerifiedRequestFreshManager, ) where @@ -61,7 +62,12 @@ data LegalHoldStore m a where SetTeamLegalholdWhitelisted :: TeamId -> LegalHoldStore m () UnsetTeamLegalholdWhitelisted :: TeamId -> LegalHoldStore m () IsTeamLegalholdWhitelisted :: TeamId -> LegalHoldStore m Bool - -- -- intra actions + -- intra actions + MakeVerifiedRequestFreshManager :: + Fingerprint Rsa -> + HttpsUrl -> + (Http.Request -> Http.Request) -> + LegalHoldStore m (Http.Response LC8.ByteString) MakeVerifiedRequest :: Fingerprint Rsa -> HttpsUrl -> diff --git a/services/galley/src/Galley/External/LegalHoldService.hs b/services/galley/src/Galley/External/LegalHoldService.hs index c5555bccc1..cca80ae880 100644 --- a/services/galley/src/Galley/External/LegalHoldService.hs +++ b/services/galley/src/Galley/External/LegalHoldService.hs @@ -60,7 +60,7 @@ checkLegalHoldServiceStatus :: HttpsUrl -> Sem r () checkLegalHoldServiceStatus fpr url = do - resp <- makeVerifiedRequest fpr url reqBuilder + resp <- makeVerifiedRequestFreshManager fpr url reqBuilder if Bilge.statusCode resp < 400 then pure () else do diff --git a/services/galley/src/Galley/External/LegalHoldService/Internal.hs b/services/galley/src/Galley/External/LegalHoldService/Internal.hs index 877e355f16..1e7db84947 100644 --- a/services/galley/src/Galley/External/LegalHoldService/Internal.hs +++ b/services/galley/src/Galley/External/LegalHoldService/Internal.hs @@ -17,6 +17,7 @@ module Galley.External.LegalHoldService.Internal ( makeVerifiedRequest, + makeVerifiedRequestFreshManager, ) where @@ -80,3 +81,16 @@ makeVerifiedRequest fpr url reqBuilder = do (mgr, fprVar) <- view extGetManager modifyIORef' fprVar (nub . ([fpr] <>)) makeVerifiedRequestWithManager mgr url reqBuilder + +-- | NOTE: Use this function wisely - this creates a new manager _every_ time it is called. +-- We should really _only_ use it in `checkLegalHoldServiceStatus` for the time being because +-- this is where we check for signatures, etc. If we reuse the manager, we are likely to reuse +-- an existing connection which will _not_ cause the new public key to be verified. +makeVerifiedRequestFreshManager :: + Fingerprint Rsa -> + HttpsUrl -> + (Http.Request -> Http.Request) -> + App (Http.Response LC8.ByteString) +makeVerifiedRequestFreshManager fpr url reqBuilder = do + mgr <- liftIO . initExtEnv =<< newIORef [fpr] + makeVerifiedRequestWithManager mgr url reqBuilder From a3305d8a9fcfda133cba6b0f7f4185506f4e8a28 Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Thu, 25 Jan 2024 13:55:40 +0100 Subject: [PATCH 3/7] [chore] at some tracing to find out what the issue with legalhold is --- libs/ssl-util/src/Ssl/Util.hs | 7 ++++++- services/galley/src/Galley/Env.hs | 2 ++ .../src/Galley/External/LegalHoldService/Internal.hs | 2 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/libs/ssl-util/src/Ssl/Util.hs b/libs/ssl-util/src/Ssl/Util.hs index 1598c100bf..79f5328678 100644 --- a/libs/ssl-util/src/Ssl/Util.hs +++ b/libs/ssl-util/src/Ssl/Util.hs @@ -38,6 +38,7 @@ import Data.ByteString.Builder import Data.Byteable (constEqBytes) import Data.Misc (Fingerprint (fingerprintBytes), Rsa) import Data.Time.Clock (getCurrentTime) +import Debug.Trace (traceM) import Imports import OpenSSL.BN (integerToMPI) import OpenSSL.EVP.Digest (Digest, digestLBS, getDigestByName) @@ -188,14 +189,18 @@ extEnvCallback fingerprints store = do cert <- getStoreCtxCert store pk <- getPublicKey cert fprs <- readIORef fingerprints + traceM (show fprs) case toPublicKey @RSAPubKey pk of Nothing -> pure False Just k -> do fp <- rsaFingerprint sha k -- find at least one matching fingerprint to continue if not (any (constEqBytes fp . fingerprintBytes) fprs) - then pure False + then do + traceM "fingerprint not contained in fprs" + pure False else do + traceM "fingerprint is contained in fprs" -- Check if the certificate is self-signed. self <- verifyX509 cert pk if (self /= VerifySuccess) diff --git a/services/galley/src/Galley/Env.hs b/services/galley/src/Galley/Env.hs index 89f7d5b03a..1397926cb1 100644 --- a/services/galley/src/Galley/Env.hs +++ b/services/galley/src/Galley/Env.hs @@ -26,6 +26,7 @@ import Data.Id import Data.Metrics.Middleware import Data.Misc (Fingerprint (..), HttpsUrl, Rsa) import Data.Range +import Debug.Trace import Galley.Aws qualified as Aws import Galley.Options import Galley.Options qualified as O @@ -70,6 +71,7 @@ makeLenses ''Env -- TODO: somewhat duplicates Brig.App.initExtGetManager initExtEnv :: IORef [Fingerprint Rsa] -> IO Manager initExtEnv fingerprints = do + traceM "initExtEnv" ctx <- Ssl.context Ssl.contextAddOption ctx SSL_OP_NO_SSLv2 Ssl.contextAddOption ctx SSL_OP_NO_SSLv3 diff --git a/services/galley/src/Galley/External/LegalHoldService/Internal.hs b/services/galley/src/Galley/External/LegalHoldService/Internal.hs index 1e7db84947..1529770542 100644 --- a/services/galley/src/Galley/External/LegalHoldService/Internal.hs +++ b/services/galley/src/Galley/External/LegalHoldService/Internal.hs @@ -29,6 +29,7 @@ import Control.Retry import Data.ByteString qualified as BS import Data.ByteString.Lazy.Char8 qualified as LC8 import Data.Misc +import Debug.Trace import Galley.API.Error import Galley.Env import Galley.Monad @@ -92,5 +93,6 @@ makeVerifiedRequestFreshManager :: (Http.Request -> Http.Request) -> App (Http.Response LC8.ByteString) makeVerifiedRequestFreshManager fpr url reqBuilder = do + traceM "makeVerifiedRequestFreshManager" mgr <- liftIO . initExtEnv =<< newIORef [fpr] makeVerifiedRequestWithManager mgr url reqBuilder From 2496b625da797f98093b7507cb5085f7c340f328 Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Thu, 25 Jan 2024 16:22:00 +0100 Subject: [PATCH 4/7] [fix] fresh manager in makeLegalHoldServiceRequest --- services/brig/src/Brig/App.hs | 3 +++ services/galley/src/Galley/External/LegalHoldService.hs | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/services/brig/src/Brig/App.hs b/services/brig/src/Brig/App.hs index 9a10bb66f5..30e8eb93a3 100644 --- a/services/brig/src/Brig/App.hs +++ b/services/brig/src/Brig/App.hs @@ -124,6 +124,7 @@ import Data.Text.IO qualified as Text import Data.Time.Clock import Data.Yaml (FromJSON) import Database.Bloodhound qualified as ES +import Debug.Trace (traceM) import HTTP2.Client.Manager (Http2Manager, http2ManagerWithSSLCtx) import Imports import Network.AMQP qualified as Q @@ -247,6 +248,7 @@ newEnv o = do kpLock <- newMVar () rabbitChan <- traverse (Q.mkRabbitMqChannelMVar lgr) o.rabbitmq fprVar <- newIORef [] + traceM "initExtGetManager brig" extMgr <- initExtGetManager fprVar pure $! @@ -364,6 +366,7 @@ initHttp2Manager = do -- TODO: somewhat duplicates Galley.App.initExtEnv initExtGetManager :: IORef [Fingerprint Rsa] -> IO Manager initExtGetManager fprVar = do + traceM "in init extGetManager in brig" ctx <- SSL.context SSL.contextAddOption ctx SSL_OP_NO_SSLv2 SSL.contextAddOption ctx SSL_OP_NO_SSLv3 diff --git a/services/galley/src/Galley/External/LegalHoldService.hs b/services/galley/src/Galley/External/LegalHoldService.hs index cca80ae880..0c4579a1c7 100644 --- a/services/galley/src/Galley/External/LegalHoldService.hs +++ b/services/galley/src/Galley/External/LegalHoldService.hs @@ -143,7 +143,7 @@ removeLegalHold tid uid = do -- helpers -- | Lookup legal hold service settings for a team and make a request to the service. Pins --- the TSL fingerprint via 'makeVerifiedRequest' and passes the token so the service can +-- the TSL fingerprint via 'makeVerifiedRequestFreshManager' and passes the token so the service can -- authenticate the request. makeLegalHoldServiceRequest :: ( Member (ErrorS 'LegalHoldServiceNotRegistered) r, @@ -162,7 +162,7 @@ makeLegalHoldServiceRequest tid reqBuilder = do legalHoldServiceFingerprint = fpr, legalHoldServiceToken = serviceToken } = lhSettings - makeVerifiedRequest fpr baseUrl $ mkReqBuilder serviceToken + makeVerifiedRequestFreshManager fpr baseUrl $ mkReqBuilder serviceToken where mkReqBuilder token = reqBuilder From 6cf3cfca50c289e8b3c34524b1648e5d047ee64f Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Thu, 25 Jan 2024 18:23:10 +0100 Subject: [PATCH 5/7] [fix] try out whether getting a fresh manager for each bot solves the failure --- services/brig/src/Brig/App.hs | 3 ++- services/brig/src/Brig/Provider/RPC.hs | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/services/brig/src/Brig/App.hs b/services/brig/src/Brig/App.hs index 30e8eb93a3..6c25065da3 100644 --- a/services/brig/src/Brig/App.hs +++ b/services/brig/src/Brig/App.hs @@ -46,6 +46,7 @@ module Brig.App httpManager, http2Manager, extGetManager, + initExtGetManager, nexmoCreds, twilioCreds, settings, @@ -71,7 +72,7 @@ module Brig.App -- * Crutches that should be removed once Brig has been completely - -- * transitioned to Polysemy + -- transitioned to Polysemy wrapClient, wrapClientE, wrapClientM, diff --git a/services/brig/src/Brig/Provider/RPC.hs b/services/brig/src/Brig/Provider/RPC.hs index e145b86514..69f8c1b960 100644 --- a/services/brig/src/Brig/Provider/RPC.hs +++ b/services/brig/src/Brig/Provider/RPC.hs @@ -35,7 +35,7 @@ import Brig.App import Brig.Provider.DB (ServiceConn (..)) import Brig.RPC import Control.Error -import Control.Lens (set, view, (^.)) +import Control.Lens (set, (^.)) import Control.Monad.Catch import Control.Retry (recovering) import Data.Aeson @@ -71,8 +71,10 @@ data ServiceError createBot :: ServiceConn -> NewBotRequest -> ExceptT ServiceError (AppT r) NewBotResponse createBot scon new = do let fprs = toList (sconFingerprints scon) - (man, fprVar) <- view extGetManager - atomicModifyIORef' fprVar \oldFprs -> (nub (fprs <> oldFprs), ()) + -- (man, fprVar) <- view extGetManager + -- atomicModifyIORef' fprVar \oldFprs -> (nub (fprs <> oldFprs), ()) + fprVar <- newIORef fprs + man <- liftIO $ initExtGetManager fprVar extHandleAll onExc $ do let req = reqBuilder Http.defaultRequest rs <- lift $ From 629a7ad8a965e47f274951d1443ddb3c53dd0b9d Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Thu, 25 Jan 2024 22:32:33 +0100 Subject: [PATCH 6/7] [chore] a couple of cleanups - remove traces - some minor style corrections - revert some changes that are probably incorrect/ not needed --- libs/ssl-util/src/Ssl/Util.hs | 7 +------ services/brig/src/Brig/App.hs | 3 --- services/brig/src/Brig/Provider/RPC.hs | 7 +++---- services/galley/src/Galley/Env.hs | 4 +--- services/galley/src/Galley/External/LegalHoldService.hs | 4 ++-- .../src/Galley/External/LegalHoldService/Internal.hs | 4 +--- 6 files changed, 8 insertions(+), 21 deletions(-) diff --git a/libs/ssl-util/src/Ssl/Util.hs b/libs/ssl-util/src/Ssl/Util.hs index 79f5328678..1598c100bf 100644 --- a/libs/ssl-util/src/Ssl/Util.hs +++ b/libs/ssl-util/src/Ssl/Util.hs @@ -38,7 +38,6 @@ import Data.ByteString.Builder import Data.Byteable (constEqBytes) import Data.Misc (Fingerprint (fingerprintBytes), Rsa) import Data.Time.Clock (getCurrentTime) -import Debug.Trace (traceM) import Imports import OpenSSL.BN (integerToMPI) import OpenSSL.EVP.Digest (Digest, digestLBS, getDigestByName) @@ -189,18 +188,14 @@ extEnvCallback fingerprints store = do cert <- getStoreCtxCert store pk <- getPublicKey cert fprs <- readIORef fingerprints - traceM (show fprs) case toPublicKey @RSAPubKey pk of Nothing -> pure False Just k -> do fp <- rsaFingerprint sha k -- find at least one matching fingerprint to continue if not (any (constEqBytes fp . fingerprintBytes) fprs) - then do - traceM "fingerprint not contained in fprs" - pure False + then pure False else do - traceM "fingerprint is contained in fprs" -- Check if the certificate is self-signed. self <- verifyX509 cert pk if (self /= VerifySuccess) diff --git a/services/brig/src/Brig/App.hs b/services/brig/src/Brig/App.hs index 6c25065da3..59a00fa3ba 100644 --- a/services/brig/src/Brig/App.hs +++ b/services/brig/src/Brig/App.hs @@ -125,7 +125,6 @@ import Data.Text.IO qualified as Text import Data.Time.Clock import Data.Yaml (FromJSON) import Database.Bloodhound qualified as ES -import Debug.Trace (traceM) import HTTP2.Client.Manager (Http2Manager, http2ManagerWithSSLCtx) import Imports import Network.AMQP qualified as Q @@ -249,7 +248,6 @@ newEnv o = do kpLock <- newMVar () rabbitChan <- traverse (Q.mkRabbitMqChannelMVar lgr) o.rabbitmq fprVar <- newIORef [] - traceM "initExtGetManager brig" extMgr <- initExtGetManager fprVar pure $! @@ -367,7 +365,6 @@ initHttp2Manager = do -- TODO: somewhat duplicates Galley.App.initExtEnv initExtGetManager :: IORef [Fingerprint Rsa] -> IO Manager initExtGetManager fprVar = do - traceM "in init extGetManager in brig" ctx <- SSL.context SSL.contextAddOption ctx SSL_OP_NO_SSLv2 SSL.contextAddOption ctx SSL_OP_NO_SSLv3 diff --git a/services/brig/src/Brig/Provider/RPC.hs b/services/brig/src/Brig/Provider/RPC.hs index 69f8c1b960..ae42498e4f 100644 --- a/services/brig/src/Brig/Provider/RPC.hs +++ b/services/brig/src/Brig/Provider/RPC.hs @@ -71,10 +71,9 @@ data ServiceError createBot :: ServiceConn -> NewBotRequest -> ExceptT ServiceError (AppT r) NewBotResponse createBot scon new = do let fprs = toList (sconFingerprints scon) - -- (man, fprVar) <- view extGetManager - -- atomicModifyIORef' fprVar \oldFprs -> (nub (fprs <> oldFprs), ()) - fprVar <- newIORef fprs - man <- liftIO $ initExtGetManager fprVar + -- fresh http manager + man <- liftIO do + initExtGetManager =<< newIORef fprs extHandleAll onExc $ do let req = reqBuilder Http.defaultRequest rs <- lift $ diff --git a/services/galley/src/Galley/Env.hs b/services/galley/src/Galley/Env.hs index 1397926cb1..b4e43adf16 100644 --- a/services/galley/src/Galley/Env.hs +++ b/services/galley/src/Galley/Env.hs @@ -26,7 +26,6 @@ import Data.Id import Data.Metrics.Middleware import Data.Misc (Fingerprint (..), HttpsUrl, Rsa) import Data.Range -import Debug.Trace import Galley.Aws qualified as Aws import Galley.Options import Galley.Options qualified as O @@ -71,7 +70,6 @@ makeLenses ''Env -- TODO: somewhat duplicates Brig.App.initExtGetManager initExtEnv :: IORef [Fingerprint Rsa] -> IO Manager initExtEnv fingerprints = do - traceM "initExtEnv" ctx <- Ssl.context Ssl.contextAddOption ctx SSL_OP_NO_SSLv2 Ssl.contextAddOption ctx SSL_OP_NO_SSLv3 @@ -81,7 +79,7 @@ initExtEnv fingerprints = do ctx Ssl.VerifyPeer { vpFailIfNoPeerCert = True, - vpClientOnce = True, + vpClientOnce = False, vpCallback = Just \_b -> extEnvCallback fingerprints } diff --git a/services/galley/src/Galley/External/LegalHoldService.hs b/services/galley/src/Galley/External/LegalHoldService.hs index 0c4579a1c7..cca80ae880 100644 --- a/services/galley/src/Galley/External/LegalHoldService.hs +++ b/services/galley/src/Galley/External/LegalHoldService.hs @@ -143,7 +143,7 @@ removeLegalHold tid uid = do -- helpers -- | Lookup legal hold service settings for a team and make a request to the service. Pins --- the TSL fingerprint via 'makeVerifiedRequestFreshManager' and passes the token so the service can +-- the TSL fingerprint via 'makeVerifiedRequest' and passes the token so the service can -- authenticate the request. makeLegalHoldServiceRequest :: ( Member (ErrorS 'LegalHoldServiceNotRegistered) r, @@ -162,7 +162,7 @@ makeLegalHoldServiceRequest tid reqBuilder = do legalHoldServiceFingerprint = fpr, legalHoldServiceToken = serviceToken } = lhSettings - makeVerifiedRequestFreshManager fpr baseUrl $ mkReqBuilder serviceToken + makeVerifiedRequest fpr baseUrl $ mkReqBuilder serviceToken where mkReqBuilder token = reqBuilder diff --git a/services/galley/src/Galley/External/LegalHoldService/Internal.hs b/services/galley/src/Galley/External/LegalHoldService/Internal.hs index 1529770542..d34acd22b5 100644 --- a/services/galley/src/Galley/External/LegalHoldService/Internal.hs +++ b/services/galley/src/Galley/External/LegalHoldService/Internal.hs @@ -29,7 +29,6 @@ import Control.Retry import Data.ByteString qualified as BS import Data.ByteString.Lazy.Char8 qualified as LC8 import Data.Misc -import Debug.Trace import Galley.API.Error import Galley.Env import Galley.Monad @@ -80,7 +79,7 @@ makeVerifiedRequest :: App (Http.Response LC8.ByteString) makeVerifiedRequest fpr url reqBuilder = do (mgr, fprVar) <- view extGetManager - modifyIORef' fprVar (nub . ([fpr] <>)) + modifyIORef' fprVar (nub . (fpr :)) makeVerifiedRequestWithManager mgr url reqBuilder -- | NOTE: Use this function wisely - this creates a new manager _every_ time it is called. @@ -93,6 +92,5 @@ makeVerifiedRequestFreshManager :: (Http.Request -> Http.Request) -> App (Http.Response LC8.ByteString) makeVerifiedRequestFreshManager fpr url reqBuilder = do - traceM "makeVerifiedRequestFreshManager" mgr <- liftIO . initExtEnv =<< newIORef [fpr] makeVerifiedRequestWithManager mgr url reqBuilder From b43d94ae775251e313f2938c9c8b5f943cc1c0fc Mon Sep 17 00:00:00 2001 From: Magnus Viernickel Date: Thu, 25 Jan 2024 22:36:08 +0100 Subject: [PATCH 7/7] [chore] add changelog entry --- changelog.d/5-internal/reuse-manager | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog.d/5-internal/reuse-manager diff --git a/changelog.d/5-internal/reuse-manager b/changelog.d/5-internal/reuse-manager new file mode 100644 index 0000000000..7ef933fd20 --- /dev/null +++ b/changelog.d/5-internal/reuse-manager @@ -0,0 +1,4 @@ +- reuse the http manager wherever possible +- don't reuse the http manager in legalhold scenarios +- don't concurrently modify the ssl context in such ways that + it can create race conditions