diff --git a/changelog.d/5-internal/pr-2852 b/changelog.d/5-internal/pr-2852 new file mode 100644 index 0000000000..eff5f1bc2b --- /dev/null +++ b/changelog.d/5-internal/pr-2852 @@ -0,0 +1 @@ +External commits: add additional checks diff --git a/services/galley/src/Galley/API/MLS/Message.hs b/services/galley/src/Galley/API/MLS/Message.hs index 6e2a04aaf2..078029a2bb 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 @@ -688,13 +690,30 @@ 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" + + 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" + -- check if there is a key package ref in the remove proposal remRef <- if Map.null (paRemove action) @@ -707,21 +726,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 @@ -782,7 +788,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 5de91f2a38..63c947d64e 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 5e2c332550..29e935b366 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