External commits: add additional checks (#2852)

wireapp · Nov 21, 2022 · df49506 · df49506
1 parent f9a0482
commit df49506
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 23 deletions.
diff --git a/changelog.d/5-internal/pr-2852 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
@@ -656,6 +656,7 @@ processExternalCommit ::
        ErrorS 'MLSClientSenderUserMismatch,
        ErrorS 'MLSKeyPackageRefNotFound,
        ErrorS 'MLSStaleMessage,
+       ErrorS 'MLSMissingSenderClient,
        ExternalAccess,
        FederatorAccess,
        GundeckAccess,
@@ -668,14 +669,15 @@ processExternalCommit ::
      ]
     r =>
   Qualified UserId ->
+  Maybe ClientId ->
   Local Data.Conversation ->
   ClientMap ->
   Epoch ->
   GroupId ->
   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
@@ -385,7 +385,7 @@ testAddUserWithBundleIncompleteWelcome = do
     bundle <- createBundle commit
     err <-
       responseJsonError
-        =<< postCommitBundle (ciUser (mpSender commit)) bundle
+        =<< postCommitBundle (mpSender commit) bundle
           <!! const 400 === statusCode
     liftIO $ Wai.label err @?= "mls-welcome-mismatch"
 
@@ -987,7 +987,7 @@ testExternalCommitNotMember = do
         <$> 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
@@ -130,15 +130,16 @@ postCommitBundle ::
     MonadHttp m,
     HasGalley m
   ) =>
-  UserId ->
+  ClientIdentity ->
   ByteString ->
   m ResponseLBS
 postCommitBundle sender bundle = do
   galley <- viewGalley
   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
         <!! const 201 === statusCode
   consumeMessage mp
   traverse_ consumeWelcome (mpWelcome mp)