WPB-29949: heal ghost MLS member-client rows from the ratchet tree

changelog.d/3-bug-fixes/WPB-29949

@@ -0,0 +1 @@
+Galley can now self-heal lost MLS member-client rows ("ghost clients").

charts/wire-server/templates/galley/configmap.yaml

@@ -116,6 +116,9 @@ data:
       {{- if .settings.checkGroupInfo }}
       checkGroupInfo: {{ .settings.checkGroupInfo }}
       {{- end }}
+      {{- if .settings.mlsGroupInfoRepair }}
+      mlsGroupInfoRepair: {{ .settings.mlsGroupInfoRepair }}
+      {{- end }}
       meetings:
         {{- with .settings.meetings }}
         {{- if .validityPeriod }}

charts/wire-server/values.yaml

@@ -154,6 +154,7 @@ galley:
         maxRateLimitedKeys: 100000 # Estimated memory usage: 4 MB
 
       checkGroupInfo: false
+      mlsGroupInfoRepair: false
 
       meetings:
         validityPeriod: "48h"

docs/src/developer/reference/config-options.md

@@ -1335,6 +1335,27 @@ Enabling consistency checks makes Galley parse the ratchet tree
 extension in the group info of every commit and compare it to the
 group state after applying the commit.
 
+#### MLS group info repair
+
+To enable repairing MLS member-client rows from the ratchet tree stored
+in the group info on every commit:
+```
+# galley.yaml
+config:
+  settings:
+    mlsGroupInfoRepair: true
+```
+
+Enabling the repair makes Galley reconcile the
+`mls_group_member_client`/`mls_history_client` rows with the ratchet
+tree of the previously processed commit bundle before validating a new
+bundle, restoring rows lost to e.g. the external-commit re-join race
+(WPB-29648). Off by default; enabling it is the rollout step, and
+disabling it is the rollback. Note: history-client rows are only
+present for Postgres-resident conversations, so healing of
+`mls_history_client` rows applies to those; conversations still served
+from Cassandra only get their `mls_group_member_client` rows healed.
+
 #### Disabling API versions
 
 It is possible to disable one ore more API versions. When an API version is disabled it won’t be advertised on the `GET /api-version` endpoint, neither in the `supported`, nor in the `development` section. Requests made to any endpoint of a disabled API version will result in the same error response as a request made to an API version that does not exist.

hack/helm_vars/wire-federation-v0/values.yaml.gotmpl

@@ -168,6 +168,7 @@ galley:
       maxTeamSize: 32
       maxFanoutSize: 18
       maxConvSize: 16
+      mlsGroupInfoRepair: true
       conversationCodeURI: https://kube-staging-nginz-https.zinfra.io/conversation-join/
       # See helmfile for the real value
       federationDomain: federation-test-helper.wire-federation-v0.svc.cluster.local

hack/helm_vars/wire-server/values.yaml.gotmpl

@@ -323,6 +323,7 @@ galley:
       maxTeamSize: 32
       maxFanoutSize: 18
       maxConvSize: 16
+      mlsGroupInfoRepair: true
       conversationCodeURI: https://kube-staging-nginz-https.zinfra.io/conversation-join/
       # See helmfile for the real value
       federationDomain: integration.example.com

integration/test/API/GalleyInternal.hs

@@ -135,6 +135,40 @@ getMLSMemberClients user conv = do
       ("/i/group/" <> BS.unpack (B64U.encodeUnpadded gidBytes) <> "/member-clients")
   submit "GET" req
 
+-- | DELETE /i/group/:gid/member-clients/:domain/:uid/:client — damage
+-- injector / manual repair tool. 'owner' selects the backend the request is
+-- sent to; 'victim' identifies whose client row is deleted.
+deleteMLSMemberClient ::
+  (HasCallStack, MakesValue owner, MakesValue victim, MakesValue client) =>
+  owner ->
+  ConvId ->
+  victim ->
+  client ->
+  App Response
+deleteMLSMemberClient owner conv victim client = do
+  gid <- case conv.groupId of
+    Just g -> pure g
+    Nothing -> assertFailure "conversation has no MLS group id"
+  gidBytes <- either (assertFailure . ("invalid MLS group id " <>) . (gid <>)) pure (B64.decode (cs gid))
+  vid <- objId victim
+  vdomain <- objDomain victim
+  cidStr <- asString client
+  req <-
+    baseRequest
+      owner
+      Galley
+      Unversioned
+      ( "/i/group/"
+          <> BS.unpack (B64U.encodeUnpadded gidBytes)
+          <> "/member-clients/"
+          <> vdomain
+          <> "/"
+          <> vid
+          <> "/"
+          <> cidStr
+      )
+  submit "DELETE" req
+
 -- | https://staging-nginz-https.zinfra.io/api-internal/swagger-ui/galley/#/galley/get_i_teams__tid__features_legalhold
 legalholdIsEnabled :: (HasCallStack, MakesValue tid, MakesValue uid) => tid -> uid -> App Response
 legalholdIsEnabled tid uid = do

integration/test/Test/MLS.hs

@@ -1068,6 +1068,104 @@ testExternalCommitDuplicateClient = do
     resp.status `shouldMatchInt` 400
     resp.json %. "label" `shouldMatch` "mls-protocol-error"
 
+-- | A ghost client (member-client row lost) is healed from the stored ratchet
+-- tree on the next commit bundle; remove/re-add then works again
+-- (no 400 mls-invalid-leaf-node-index).
+testGhostMemberRowRepair :: (HasCallStack) => App ()
+testGhostMemberRowRepair = do
+  [alice, bob] <- createAndConnectUsers [OwnDomain, OwnDomain]
+  [alice1, alice2, bob1] <- traverse (createMLSClient def) [alice, alice, bob]
+  -- Bob needs a KP for the add commit and another for the final re-add.
+  -- Alice2's KP must NOT exist yet: the first commit of a conversation
+  -- validates the sender's user against brig (all MLS-capable clients must be
+  -- covered by Add proposals), so an unused KP for alice2 here would make
+  -- galley reject the commit with 409 mls-client-mismatch.
+  traverse_ (uploadNewKeyPackage def) [bob1, bob1]
+  convId <- createNewGroup def alice1
+  void $ createAddCommit alice1 convId [bob] >>= sendAndConsumeCommitBundle
+
+  -- Inject the damage: delete bob1's member-client row.
+  void $ I.deleteMLSMemberClient alice convId bob bob1.client >>= assertSuccess
+
+  -- Sanity: bob1 no longer receives fan-out.
+  withWebSockets [alice1, bob1] $ \[_, wsBob] -> do
+    mp <- createApplicationMessage convId alice1 "before repair"
+    bindResponse (postMLSMessage mp.sender mp.message) $ \resp ->
+      resp.status `shouldMatchInt` 201
+    expectFailure (const $ pure ())
+      $ awaitMatch
+        (\m -> nPayload m %. "type" `isEqual` "conversation.mls-message-add")
+        wsBob
+
+  -- alice2 needs a registered KP to join via external commit.
+  void $ uploadNewKeyPackage def alice2
+  -- Any commit bundle triggers the repair from the stored ratchet tree; the
+  -- external commit must come from a non-member (galley rejects external commits
+  -- from existing members without a self-remove proposal), so a fresh client of
+  -- alice joins here.
+  void $ createExternalCommit convId alice2 Nothing >>= sendAndConsumeCommitBundle
+
+  -- bob1's row is back with its TRUE leaf index.
+  gs <- getClientGroupState bob1
+  groupData <- assertJust "no MLS group state for bob1" (Map.lookup convId gs.groups)
+  members <- readGroupState groupData
+  idx <-
+    assertJust "bob1 missing from group state"
+      $ listToMaybe [idx | (RegularClient c, idx) <- members, c == bob1]
+  rows <- I.getMLSMemberClients alice convId >>= getJSON 200 >>= (%. "member_clients") >>= asList
+  idxs <- for rows $ \r -> do
+    ci <- r %. "client"
+    uid <- ci %. "user_id" & asString
+    client <- ci %. "client_id" & asString
+    idx' <- r %. "leaf_index" & asIntegral @Word32
+    pure (uid, client, idx')
+  [idx' | (u, c, idx') <- idxs, u == bob1.user, c == bob1.client] `shouldMatch` [idx]
+
+  -- bob1 receives messages again.
+  withWebSockets [alice1, bob1] $ \[_, wsBob] -> do
+    mp <- createApplicationMessage convId alice1 "after repair"
+    bindResponse (postMLSMessage mp.sender mp.message) $ \resp ->
+      resp.status `shouldMatchInt` 201
+    n <- awaitMatch (\n -> nPayload n %. "type" `isEqual` "conversation.mls-message-add") wsBob
+    nPayload n %. "data" `shouldMatch` T.decodeUtf8 (Base64.encode mp.message)
+
+  -- Remove/re-add bob without the 400 mls-invalid-leaf-node-index.
+  mp <- createRemoveCommit alice1 convId [bob1]
+  void $ sendAndConsumeCommitBundle mp
+  -- bob1 is in membersToBeRemoved for the remove commit, so the harness never
+  -- lets it consume that commit and its local group state goes stale; reset
+  -- the group state (keeping the keystore with bob1's registered signature
+  -- key, or the re-add KP upload fails with 400 'Unrecognised signature key')
+  -- or the re-add welcome trips the existing-client assertion in
+  -- consumeWelcome.
+  gs0 <- getClientGroupState bob1
+  setClientGroupState bob1 def {keystore = gs0.keystore, credType = gs0.credType}
+  void $ uploadNewKeyPackage def bob1
+  void $ createAddCommit alice1 convId [bob] >>= sendAndConsumeCommitBundle
+
+-- | WPB-29648: with the repair disabled, removing a ghost client (whose
+-- member-client row was lost) is rejected with the pre-fix failure from the
+-- bug report: 400 mls-invalid-leaf-node-index.
+testGhostRemoveRejectedWithoutRepair :: (HasCallStack) => App ()
+testGhostRemoveRejectedWithoutRepair = do
+  withModifiedBackend
+    (def {galleyCfg = setField "settings.mlsGroupInfoRepair" (False :: Bool)})
+    $ \domain -> do
+      [alice, bob] <- createAndConnectUsers [domain, domain]
+      [alice1, bob1] <- traverse (createMLSClient def) [alice, bob]
+      traverse_ (uploadNewKeyPackage def) [bob1]
+      convId <- createNewGroup def alice1
+      void $ createAddCommit alice1 convId [bob] >>= sendAndConsumeCommitBundle
+
+      -- Inject the damage: delete bob1's member-client row.
+      void $ I.deleteMLSMemberClient alice convId bob bob1.client >>= assertSuccess
+
+      -- Remove-commit bundle for the ghost client is rejected with the exact
+      -- error from the bug report (no healing happened to fix the indexMap).
+      mp <- createRemoveCommit alice1 convId [bob1]
+      err <- postMLSCommitBundle mp.sender (mkBundle mp) >>= getJSON 400
+      err %. "label" `shouldMatch` "mls-invalid-leaf-node-index"
+
 testExternalCommitSameClient :: (HasCallStack) => App ()
 testExternalCommitSameClient = do
   [alice, bob] <- createAndConnectUsers [OwnDomain, OwnDomain]

libs/wire-api/src/Wire/API/Routes/Internal/Galley.hs

@@ -199,6 +199,22 @@ type InternalAPIBase =
                     '[JSON]
                     (Respond 200 "MLS member clients" MLSMemberClientList)
            )
+    -- Deletes a single member-client row; support/ops manual repair tool and
+    -- damage injector for integration tests.
+    :<|> Named
+           "delete-mls-member-client"
+           ( Summary "Delete an MLS conversation member client row"
+               :> "group"
+               :> Capture "gid" GroupId
+               :> "member-clients"
+               :> Capture "domain" Domain
+               :> Capture "uid" UserId
+               :> Capture "client" ClientId
+               :> MultiVerb1
+                    'DELETE
+                    '[JSON]
+                    (RespondEmpty 204 "Deleted")
+           )
     :<|> Named
            "guard-legalhold-policy-conflicts"
            ( "guard-legalhold-policy-conflicts"

libs/wire-subsystems/src/Wire/ConversationStore.hs

@@ -131,6 +131,10 @@ data ConversationStore m a where
   RemoveAllMLSClients :: GroupId -> ConversationStore m ()
   LookupMLSClients :: GroupId -> ConversationStore m (ClientMap LeafIndex)
   LookupHistoryClients :: GroupId -> ConversationStore m [(HistoryClientId, Int32, Bool)]
+  -- | Reconcile member-client rows of a group with the authoritative
+  -- ratchet-tree leaf map. Pending-removal rows are untouched. Returns
+  -- (inserted, updated, deleted).
+  ReconcileMLSClients :: GroupId -> IndexMap -> ConversationStore m (Int, Int, Int)
   LookupMLSClientLeafIndices :: GroupId -> ConversationStore m (ClientMap LeafIndex, IndexMap)
   -- SUB CONVERSATION OPERATIONS
   UpsertSubConversation :: ConvId -> SubConvId -> GroupId -> ConversationStore m SubConversation

libs/wire-subsystems/src/Wire/ConversationStore/Cassandra.hs

@@ -76,6 +76,7 @@ import Wire.ConversationStore qualified as ConvStore
 import Wire.ConversationStore.Cassandra.Instances ()
 import Wire.ConversationStore.Cassandra.Queries qualified as Cql
 import Wire.ConversationStore.Cassandra.Queries qualified as Queries
+import Wire.ConversationStore.MLS.Reconcile (MlsReconcileOps (..), mlsReconcileOps)
 import Wire.ConversationStore.MLS.Types
 import Wire.ConversationStore.Migration.Cleanup
 import Wire.ConversationStore.Postgres (interpretConversationStoreToPostgres)
@@ -774,6 +775,31 @@ lookupMLSClientRows groupId =
 lookupMLSClients :: GroupId -> Client (ClientMap LeafIndex)
 lookupMLSClients = fmap mkClientMap . lookupMLSClientRows
 
+-- | Reconcile member-client rows with the tree leaves. History clients live
+-- exclusively in Postgres in the dual-write era, so the diff covers member
+-- rows only here. Inserts and leaf updates reuse 'Cql.addMLSClient' (an
+-- upsert that clears @removal_pending@), which is safe because both target
+-- identities that reconcile proved are non-pending.
+reconcileMLSClients :: GroupId -> IndexMap -> Client (Int, Int, Int)
+reconcileMLSClients groupId leaves = do
+  memberRows <- lookupMLSClientRows groupId
+  let ops = mlsReconcileOps memberRows [] leaves
+  retry x5 . batch $ do
+    setType BatchLogged
+    setConsistency LocalQuorum
+    -- addMLSClient is an upsert that clears removal_pending; inserts and leaf
+    -- updates are the same write here, and both only target identities the
+    -- diff proved are non-pending.
+    for_ (ops.roMemberInsert <> ops.roMemberUpdateLeaf) $ \(dom, usr, c, idx) ->
+      addPrepQuery Cql.addMLSClient (groupId, dom, usr, c, idx)
+    for_ ops.roMemberDelete $ \(dom, usr, c) ->
+      addPrepQuery Cql.removeMLSClient (groupId, dom, usr, c)
+  pure
+    ( length ops.roMemberInsert,
+      length ops.roMemberUpdateLeaf,
+      length ops.roMemberDelete
+    )
+
 -----------------------------------------------------------------------------------------
 -- SUB CONVERSATION STORE
 
@@ -1035,6 +1061,9 @@ interpretConversationStoreToCassandra client = interpret $ \case
   UpsertMLSClient gid ci idx -> do
     logEffect "ConversationStore.UpsertMLSClient"
     embedClient client $ addMLSClients gid (cidQualifiedUser ci) (Set.singleton (ciClient ci, idx))
+  ReconcileMLSClients gid leaves -> do
+    logEffect "ConversationStore.ReconcileMLSClients"
+    embedClient client $ reconcileMLSClients gid leaves
   AddHistoryClient groupId hid idx -> do
     logEffect "ConversationStore.AddHistoryClient"
     interpretConversationStoreToPostgres $ ConvStore.addHistoryClient groupId hid idx
@@ -1451,6 +1480,13 @@ interpretConversationStoreToCassandraAndPostgres client = interpret $ \case
       isConvInPostgres cid >>= \case
         False -> embedClient client $ addMLSClients gid (cidQualifiedUser ci) (Set.singleton (ciClient ci, idx))
         True -> interpretConversationStoreToPostgres (ConvStore.upsertMLSClient gid ci idx)
+  ReconcileMLSClients gid leaves -> do
+    logEffect "ConversationStore.ReconcileMLSClients"
+    cid <- groupIdToConvId gid
+    withMigrationLockAndCleanup client LockShared (Left cid) $
+      isConvInPostgres cid >>= \case
+        False -> embedClient client $ reconcileMLSClients gid leaves
+        True -> interpretConversationStoreToPostgres (ConvStore.reconcileMLSClients gid leaves)
   AddHistoryClient groupId hid idx -> do
     logEffect "ConversationStore.AddHistoryClient"
     interpretConversationStoreToPostgres $ ConvStore.addHistoryClient groupId hid idx