From f3ebf29f023a9496591a0ca099dac47fa4df15ef Mon Sep 17 00:00:00 2001 From: Stefan Matting Date: Fri, 18 Nov 2022 16:43:17 +0100 Subject: [PATCH 1/4] Add missing checks + simplify --- services/galley/src/Galley/API/MLS/Message.hs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/services/galley/src/Galley/API/MLS/Message.hs b/services/galley/src/Galley/API/MLS/Message.hs index 6e2a04aaf2c..0278bf5ce81 100644 --- a/services/galley/src/Galley/API/MLS/Message.hs +++ b/services/galley/src/Galley/API/MLS/Message.hs @@ -688,13 +688,24 @@ processExternalCommit qusr lconv cm epoch groupId action updatePath = withCommit throw . mlsProtocolError $ "The external commit must not have add proposals" - cid <- case kpIdentity (rmValue newKeyPackage) of - Left e -> throw (mlsProtocolError $ "Failed to parse the client identity: " <> e) - Right v -> pure v newRef <- kpRef' newKeyPackage & note (mlsProtocolError "An invalid key package in the update path") + -- validate and update mapping in brig + mCid <- + nkpresClientIdentity + <$$> validateAndAddKeyPackageRef + NewKeyPackage + { nkpConversation = Data.convId <$> qUntagged lconv, + nkpKeyPackage = KeyPackageData (rmRaw newKeyPackage) + } + cid <- mCid & note (mlsProtocolError "Tried to add invalid KeyPackage") + + unless (cidQualifiedUser cid == qusr) $ + throw . mlsProtocolError $ + "The external commit attempts to add another user" + -- check if there is a key package ref in the remove proposal remRef <- if Map.null (paRemove action) @@ -707,21 +718,8 @@ processExternalCommit qusr lconv cm epoch groupId action updatePath = withCommit $ "The external commit attempts to remove a client from a user other than themselves" pure (Just r) - -- first perform checks and map the key package if valid - addKeyPackageRef - newRef - (cidQualifiedUser cid) - (ciClient cid) - (Data.convId <$> qUntagged lconv) - -- now it is safe to update the mapping without further checks - -- FUTUREWORK: This call is redundent and reduces to the previous - -- call of addKeyPackageRef when remRef is Nothing! Should be - -- limited to cases where remRef is not Nothing. updateKeyPackageMapping lconv qusr (ciClient cid) remRef newRef - -- FUTUREWORK: Resubmit backend-provided proposals when processing an - -- external commit. - -- increment epoch number setConversationEpoch (Data.convId (tUnqualified lconv)) (succ epoch) -- fetch local conversation with new epoch From 4d31e1a5740cf6bad2819dba1f29210bed69d00a Mon Sep 17 00:00:00 2001 From: Stefan Matting Date: Mon, 21 Nov 2022 11:00:18 +0100 Subject: [PATCH 2/4] External commits: require zclient and match kp --- services/galley/src/Galley/API/MLS/Message.hs | 15 +++++++++++++-- services/galley/test/integration/API/MLS.hs | 4 ++-- services/galley/test/integration/API/MLS/Util.hs | 7 ++++--- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/services/galley/src/Galley/API/MLS/Message.hs b/services/galley/src/Galley/API/MLS/Message.hs index 0278bf5ce81..d8496d92f62 100644 --- a/services/galley/src/Galley/API/MLS/Message.hs +++ b/services/galley/src/Galley/API/MLS/Message.hs @@ -656,6 +656,7 @@ processExternalCommit :: ErrorS 'MLSClientSenderUserMismatch, ErrorS 'MLSKeyPackageRefNotFound, ErrorS 'MLSStaleMessage, + ErrorS 'MLSMissingSenderClient, ExternalAccess, FederatorAccess, GundeckAccess, @@ -668,6 +669,7 @@ processExternalCommit :: ] r => Qualified UserId -> + Maybe ClientId -> Local Data.Conversation -> ClientMap -> Epoch -> @@ -675,7 +677,7 @@ processExternalCommit :: ProposalAction -> Maybe UpdatePath -> Sem r () -processExternalCommit qusr lconv cm epoch groupId action updatePath = withCommitLock groupId epoch $ do +processExternalCommit qusr mSenderClient lconv cm epoch groupId action updatePath = withCommitLock groupId epoch $ do newKeyPackage <- upLeaf <$> note @@ -706,6 +708,15 @@ processExternalCommit qusr lconv cm epoch groupId action updatePath = withCommit throw . mlsProtocolError $ "The external commit attempts to add another user" + senderClient <- noteS @'MLSMissingSenderClient mSenderClient + + unless (ciClient cid == senderClient) $ + throw . mlsProtocolError $ + "The external commit attempts to add another client of the user, it must only add itself" + + -- for_ senderClient $ \senderClient' -> + -- pure () + -- check if there is a key package ref in the remove proposal remRef <- if Map.null (paRemove action) @@ -780,7 +791,7 @@ processCommitWithAction :: processCommitWithAction qusr senderClient con lconv cm epoch groupId action sender commit = case sender of MemberSender ref -> processInternalCommit qusr senderClient con lconv cm epoch groupId action ref commit - NewMemberSender -> processExternalCommit qusr lconv cm epoch groupId action (cPath commit) $> [] + NewMemberSender -> processExternalCommit qusr senderClient lconv cm epoch groupId action (cPath commit) $> [] _ -> throw (mlsProtocolError "Unexpected sender") processInternalCommit :: diff --git a/services/galley/test/integration/API/MLS.hs b/services/galley/test/integration/API/MLS.hs index 5de91f2a385..63c947d64e5 100644 --- a/services/galley/test/integration/API/MLS.hs +++ b/services/galley/test/integration/API/MLS.hs @@ -385,7 +385,7 @@ testAddUserWithBundleIncompleteWelcome = do bundle <- createBundle commit err <- responseJsonError - =<< postCommitBundle (ciUser (mpSender commit)) bundle + =<< postCommitBundle (mpSender commit) bundle getGroupInfo (ciUser alice1) qcnv mp <- createExternalCommit bob1 (Just pgs) qcnv bundle <- createBundle mp - postCommitBundle (ciUser (mpSender mp)) bundle + postCommitBundle (mpSender mp) bundle !!! const 404 === statusCode testExternalCommitSameClient :: TestM () diff --git a/services/galley/test/integration/API/MLS/Util.hs b/services/galley/test/integration/API/MLS/Util.hs index 5e2c332550d..29e935b3668 100644 --- a/services/galley/test/integration/API/MLS/Util.hs +++ b/services/galley/test/integration/API/MLS/Util.hs @@ -130,7 +130,7 @@ postCommitBundle :: MonadHttp m, HasGalley m ) => - UserId -> + ClientIdentity -> ByteString -> m ResponseLBS postCommitBundle sender bundle = do @@ -138,7 +138,8 @@ postCommitBundle sender bundle = do post ( galley . paths ["mls", "commit-bundles"] - . zUser sender + . zUser (ciUser sender) + . zClient (ciClient sender) . zConn "conn" . content "application/x-protobuf" . bytes bundle @@ -888,7 +889,7 @@ sendAndConsumeCommitBundle mp = do events <- fmap mmssEvents . responseJsonError - =<< postCommitBundle (ciUser (mpSender mp)) bundle + =<< postCommitBundle (mpSender mp) bundle Date: Mon, 21 Nov 2022 11:03:27 +0100 Subject: [PATCH 3/4] remove commented-out code --- services/galley/src/Galley/API/MLS/Message.hs | 3 --- 1 file changed, 3 deletions(-) diff --git a/services/galley/src/Galley/API/MLS/Message.hs b/services/galley/src/Galley/API/MLS/Message.hs index d8496d92f62..078029a2bbb 100644 --- a/services/galley/src/Galley/API/MLS/Message.hs +++ b/services/galley/src/Galley/API/MLS/Message.hs @@ -714,9 +714,6 @@ processExternalCommit qusr mSenderClient lconv cm epoch groupId action updatePat throw . mlsProtocolError $ "The external commit attempts to add another client of the user, it must only add itself" - -- for_ senderClient $ \senderClient' -> - -- pure () - -- check if there is a key package ref in the remove proposal remRef <- if Map.null (paRemove action) From cec481ee4a44628276acd0983c7717b780f356ac Mon Sep 17 00:00:00 2001 From: Stefan Matting Date: Mon, 21 Nov 2022 11:04:20 +0100 Subject: [PATCH 4/4] Add changelog entry --- changelog.d/5-internal/pr-2852 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5-internal/pr-2852 diff --git a/changelog.d/5-internal/pr-2852 b/changelog.d/5-internal/pr-2852 new file mode 100644 index 00000000000..eff5f1bc2b9 --- /dev/null +++ b/changelog.d/5-internal/pr-2852 @@ -0,0 +1 @@ +External commits: add additional checks