fix(quartz/mls): correct MLS commit framing on send and receive

Two bugs in MlsGroup were blocking 12 of 209 jvmTest cases (all RFC 9420 interop vectors already passed; only our higher-level lifecycle code diverged from the spec). Bug A — write/read asymmetry on member commits. processCommit requires signature + confirmation_tag as separate parameters, but CommitResult only exposed framedCommitBytes (the full PublicMessage envelope). Tests called processCommit(commitBytes, ..., ByteArray(0)) and immediately hit "FramedContentTBS signature missing on commit from leaf N". Add MlsGroup.processFramedCommit(framedCommitBytes) that unpacks the MlsMessage(PublicMessage(Commit)) envelope, verifies membership_tag for member senders (RFC 9420 §6.2), and delegates to processCommit. Mirrors the unwrap MarmotInboundProcessor already does in production. Updates the 9 affected tests to use the new entry point. Bug B — externalJoin used the wrong HPKE info bytes. The joiner encrypted UpdatePath path-secrets against groupContext.toTlsBytes() (pre-mutation), but receivers HPKE-Open against the post-mutation context (epoch+1, new tree_hash) per RFC 9420 §7.6. Result: AEADBadTagException on every external join. Refactor externalJoin to follow the same staging pattern as commit(): stage public keys, applyUpdatePath, patch parent_hash, replace placeholder leaf — then compute pathEncContextBytes from the post- mutation tree, then HPKE-encrypt path-secrets with that context. To make the framed pipeline available to external commits as well, introduce ExternalJoinResult exposing both commitBytes and framedCommitBytes (PublicMessage with sender = new_member_commit, no membership_tag). processFramedCommit resolves NEW_MEMBER_COMMIT senders to the receiver-side leaf index using the same first-blank-or-append algorithm as RatchetTree.addLeaf, so wire decoding doesn't need to carry the joiner's chosen index in the empty Sender field. After: 209/209 MLS tests pass.
2026-04-22 15:20:29 +00:00
parent 03f5f18894
commit f9c6a4711f
6 changed files with 183 additions and 59 deletions
@@ -39,6 +39,7 @@ import com.vitorpamplona.quartz.marmot.mls.framing.encodeSender
 import com.vitorpamplona.quartz.marmot.mls.messages.Commit
 import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult
 import com.vitorpamplona.quartz.marmot.mls.messages.EncryptedGroupSecrets
+import com.vitorpamplona.quartz.marmot.mls.messages.ExternalJoinResult
 import com.vitorpamplona.quartz.marmot.mls.messages.GroupContext
 import com.vitorpamplona.quartz.marmot.mls.messages.GroupInfo
 import com.vitorpamplona.quartz.marmot.mls.messages.GroupSecrets
@@ -1036,6 +1037,62 @@ class MlsGroup private constructor(

    // --- Commit Processing ---

+    /**
+     * Process a fully-framed `MlsMessage(PublicMessage(Commit))` produced by
+     * [CommitResult.framedCommitBytes]. Unpacks the wire envelope, verifies the
+     * RFC 9420 §6.2 membership_tag, and delegates to [processCommit] with the
+     * correct signature, confirmation_tag, and sender leaf index. This is the
+     * symmetric peer-side entry point for commits returned by [commit] /
+     * [addMember] / [removeMember] — tests and any caller that holds the
+     * framed bytes should prefer this over the low-level [processCommit].
+     *
+     * Mirrors the production unwrap done by `MarmotInboundProcessor` for
+     * inbound kind:445 PublicMessage commits.
+     */
+    fun processFramedCommit(framedCommitBytes: ByteArray) {
+        val mlsMessage = MlsMessage.decodeTls(TlsReader(framedCommitBytes))
+        require(mlsMessage.wireFormat == WireFormat.PUBLIC_MESSAGE) {
+            "processFramedCommit expects a PublicMessage envelope, got ${mlsMessage.wireFormat}"
+        }
+        val pubMsg = PublicMessage.decodeTls(TlsReader(mlsMessage.payload))
+        require(pubMsg.contentType == ContentType.COMMIT) {
+            "Framed commit must contain ContentType.COMMIT, got ${pubMsg.contentType}"
+        }
+        val tag =
+            requireNotNull(pubMsg.confirmationTag) {
+                "PublicMessage commit missing confirmation_tag"
+            }
+        // External commits don't carry a membership_tag (sender isn't yet a
+        // member). For member-sender commits, RFC 9420 §6.2 requires the tag.
+        if (pubMsg.sender.senderType == SenderType.MEMBER) {
+            require(verifyPublicMessageCommitMembershipTag(pubMsg)) {
+                "Invalid membership_tag on PublicMessage commit"
+            }
+        }
+        // RFC 9420 §6: `new_member_commit` senders carry no leaf_index field
+        // on the wire — the joiner's slot is determined by the receiver
+        // using the same algorithm the joiner used in [RatchetTree.addLeaf]:
+        // first blank leaf, else append past the current leaf_count.
+        val senderLeafIndex =
+            when (pubMsg.sender.senderType) {
+                SenderType.NEW_MEMBER_COMMIT -> {
+                    (0 until tree.leafCount).firstOrNull { tree.getLeaf(it) == null }
+                        ?: tree.leafCount
+                }
+
+                else -> {
+                    pubMsg.sender.leafIndex
+                }
+            }
+        processCommit(
+            commitBytes = pubMsg.content,
+            senderLeafIndex = senderLeafIndex,
+            confirmationTag = tag,
+            signature = pubMsg.signature,
+            wireFormat = WireFormat.PUBLIC_MESSAGE,
+        )
+    }
+
    /**
     * Process a received Commit message, advancing the epoch.
     *
@@ -2513,13 +2570,15 @@ class MlsGroup private constructor(
         * @param groupInfoBytes TLS-serialized GroupInfo
         * @param identity the joiner's identity
         * @param signingKey optional Ed25519 signing key (generated if null)
-         * @return the new MlsGroup and the commit bytes to send to the group
+         * @return the new MlsGroup along with the raw inner commit bytes and a
+         *   wire-ready PublicMessage envelope. Existing group members consume
+         *   the framed bytes via [MlsGroup.processFramedCommit].
         */
        fun externalJoin(
            groupInfoBytes: ByteArray,
            identity: ByteArray,
            signingKey: ByteArray? = null,
-        ): Pair<MlsGroup, ByteArray> {
+        ): ExternalJoinResult {
            val groupInfo = GroupInfo.decodeTls(TlsReader(groupInfoBytes))
            val groupContext = groupInfo.groupContext

@@ -2600,37 +2659,23 @@ class MlsGroup private constructor(
                    map
                }

-            // Build UpdatePath for our leaf
+            // Build UpdatePath for our leaf using the same staging pattern
+            // as regular commit() (RFC 9420 §7.6): stage public keys → apply →
+            // patch parent_hash → rebuild leaf → compute post-mutation context
+            // → HPKE-encrypt with that context. Encrypting against the
+            // PRE-mutation groupContext (the bug we're fixing here) makes
+            // existing members fail to open the path-secret AEAD because
+            // their decryption context bumps the epoch and folds in the new
+            // tree_hash.
            val leafSecret = MlsCryptoProvider.randomBytes(MlsCryptoProvider.HASH_OUTPUT_LENGTH)
            val pathSecrets = tree.derivePathSecrets(myLeafIndex, leafSecret)
            val copath = BinaryTree.copath(myLeafIndex, tree.leafCount)
-            val pathNodes =
-                pathSecrets.zip(copath).map { (pathKey, copathNode) ->
-                    val resolution = tree.resolution(copathNode)
-                    val encryptedSecrets =
-                        resolution.mapNotNull { resNode ->
-                            val node = tree.getNode(resNode) ?: return@mapNotNull null
-                            val recipientPub =
-                                when (node) {
-                                    is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Leaf -> {
-                                        node.leafNode.encryptionKey
-                                    }

-                                    is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent -> {
-                                        node.parentNode.encryptionKey
-                                    }
-                                }
-                            MlsCryptoProvider.encryptWithLabel(
-                                recipientPub,
-                                "UpdatePathNode",
-                                groupContext.toTlsBytes(),
-                                pathKey.pathSecret,
-                            )
-                        }
-                    UpdatePathNode(pathKey.publicKey, encryptedSecrets)
-                }
-
-            tree.applyUpdatePath(myLeafIndex, pathNodes)
+            // Stage path-keys WITHOUT encrypted secrets so subsequent
+            // parent_hash / tree_hash computations reflect the new keys.
+            val stagedPathNodes =
+                pathSecrets.map { pathKey -> UpdatePathNode(pathKey.publicKey, emptyList()) }
+            tree.applyUpdatePath(myLeafIndex, stagedPathNodes)

            // Compute parent_hash chain + the leaf's parent_hash (RFC 9420
            // §7.9.2) on the post-update tree, then patch parent nodes and
@@ -2662,6 +2707,42 @@ class MlsGroup private constructor(
                )
            tree.setLeaf(myLeafIndex, leafNode)

+            // Post-mutation path-encryption context: bump epoch, swap in the
+            // post-update tree_hash. Matches what existing members compute
+            // in processCommitInner when they HPKE-Open the path secret.
+            val pathEncContextBytes =
+                groupContext
+                    .copy(
+                        epoch = groupContext.epoch + 1,
+                        treeHash = tree.treeHash(),
+                    ).toTlsBytes()
+
+            val pathNodes =
+                stagedPathNodes.zip(copath).zip(pathSecrets) { (staged, copathNode), pathKey ->
+                    val resolution = tree.resolution(copathNode)
+                    val encryptedSecrets =
+                        resolution.mapNotNull { resNode ->
+                            val node = tree.getNode(resNode) ?: return@mapNotNull null
+                            val recipientPub =
+                                when (node) {
+                                    is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Leaf -> {
+                                        node.leafNode.encryptionKey
+                                    }
+
+                                    is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent -> {
+                                        node.parentNode.encryptionKey
+                                    }
+                                }
+                            MlsCryptoProvider.encryptWithLabel(
+                                recipientPub,
+                                "UpdatePathNode",
+                                pathEncContextBytes,
+                                pathKey.pathSecret,
+                            )
+                        }
+                    UpdatePathNode(staged.encryptionKey, encryptedSecrets)
+                }
+
            val updatePath = UpdatePath(leafNode, pathNodes)

            // Build Commit
@@ -2723,7 +2804,32 @@ class MlsGroup private constructor(
                    interimTranscriptHash = interimTranscriptHash,
                )

-            return Pair(group, commitBytes)
+            // Wrap the commit in a PublicMessage envelope so existing members
+            // can extract sender / signature / confirmation_tag via
+            // MlsGroup.processFramedCommit (the symmetric receive path).
+            // External commits (RFC 9420 §12.4.3) carry sender =
+            // new_member_commit and no membership_tag — the joiner is
+            // claiming a slot that doesn't yet exist in the group's
+            // membership_key MAC space.
+            val publicMessage =
+                PublicMessage(
+                    groupId = groupContext.groupId,
+                    epoch = groupContext.epoch,
+                    sender = Sender(SenderType.NEW_MEMBER_COMMIT, 0),
+                    authenticatedData = ByteArray(0),
+                    contentType = ContentType.COMMIT,
+                    content = commitBytes,
+                    signature = ByteArray(0),
+                    confirmationTag = confirmationTag,
+                    membershipTag = null,
+                )
+            val framedCommitBytes = MlsMessage.fromPublicMessage(publicMessage).toTlsBytes()
+
+            return ExternalJoinResult(
+                group = group,
+                commitBytes = commitBytes,
+                framedCommitBytes = framedCommitBytes,
+            )
        }

        /**
@@ -24,6 +24,7 @@ import com.vitorpamplona.quartz.marmot.mls.codec.TlsReader
 import com.vitorpamplona.quartz.marmot.mls.codec.TlsWriter
 import com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider
 import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult
+import com.vitorpamplona.quartz.marmot.mls.messages.ExternalJoinResult
 import com.vitorpamplona.quartz.marmot.mls.messages.KeyPackageBundle
 import com.vitorpamplona.quartz.marmot.mls.schedule.KeySchedule
 import com.vitorpamplona.quartz.marmot.mls.schedule.SecretTree
@@ -246,12 +247,12 @@ class MlsGroupManager(
        groupInfoBytes: ByteArray,
        identity: ByteArray,
        signingKey: ByteArray? = null,
-    ): Pair<MlsGroup, ByteArray> =
+    ): ExternalJoinResult =
        mutex.withLock {
-            val (group, commitBytes) = MlsGroup.externalJoin(groupInfoBytes, identity, signingKey)
-            groups[nostrGroupId] = group
+            val result = MlsGroup.externalJoin(groupInfoBytes, identity, signingKey)
+            groups[nostrGroupId] = result.group
            persistGroup(nostrGroupId)
-            Pair(group, commitBytes)
+            result
        }

    // --- Epoch Transitions ---
@@ -128,3 +128,27 @@ data class CommitResult(

    override fun hashCode(): Int = commitBytes.contentHashCode()
 }
+
+/**
+ * Result of [com.vitorpamplona.quartz.marmot.mls.group.MlsGroup.externalJoin].
+ *
+ * [commitBytes] is the raw TLS-encoded [Commit] struct. For on-the-wire
+ * distribution to existing group members, callers MUST publish [framedCommitBytes]
+ * — a `MlsMessage(PublicMessage(FramedContent(commit)))` envelope with sender
+ * `new_member_commit` (RFC 9420 §12.4.3) — so that receivers can parse the
+ * confirmation_tag and process the commit via
+ * [com.vitorpamplona.quartz.marmot.mls.group.MlsGroup.processFramedCommit].
+ */
+data class ExternalJoinResult(
+    val group: com.vitorpamplona.quartz.marmot.mls.group.MlsGroup,
+    val commitBytes: ByteArray,
+    val framedCommitBytes: ByteArray,
+) {
+    override fun equals(other: Any?): Boolean {
+        if (this === other) return true
+        if (other !is ExternalJoinResult) return false
+        return commitBytes.contentEquals(other.commitBytes)
+    }
+
+    override fun hashCode(): Int = commitBytes.contentHashCode()
+}
@@ -234,7 +234,7 @@ class MlsGroupEdgeCaseTest {
        // Advance through several epochs with empty commits
        for (i in 0 until 5) {
            val commitResult = alice.commit()
-            bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+            bob.processFramedCommit(commitResult.framedCommitBytes)
        }

        // epoch 0 (create) -> epoch 1 (add bob) -> 5 empty commits = epoch 6
@@ -271,7 +271,7 @@ class MlsGroupEdgeCaseTest {

        for (i in 0 until 3) {
            val commitResult = alice.commit()
-            bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+            bob.processFramedCommit(commitResult.framedCommitBytes)
            keys.add(alice.exporterSecret("marmot", "group-event".encodeToByteArray(), 32))
        }

@@ -170,11 +170,6 @@ class MlsGroupLifecycleTest {
    // 4. Three-member group: sequential additions
    // -----------------------------------------------------------------------

-    // BUG: processCommit does not derive the same epoch secrets as commit().
-    // After Bob.processCommit(Alice's commit), Bob's key schedule diverges
-    // because the commit_secret decryption from the UpdatePath does not
-    // correctly walk the ratchet tree to find the common ancestor's path secret.
-    // This causes AEAD decryption failures on cross-member messages.
    @Test
    fun testThreeMemberGroup_SequentialAdditions() {
        // Alice creates the group
@@ -190,7 +185,7 @@ class MlsGroupLifecycleTest {
        // Alice adds Carol (Bob processes Alice's commit)
        val carolBundle = createStandaloneKeyPackage("carol")
        val addCarolResult = alice.addMember(carolBundle.keyPackage.toTlsBytes())
-        bob.processCommit(addCarolResult.commitBytes, alice.leafIndex, ByteArray(0))
+        bob.processFramedCommit(addCarolResult.framedCommitBytes)
        val carol = MlsGroup.processWelcome(addCarolResult.welcomeBytes!!, carolBundle)

        assertEquals(2L, alice.epoch)
@@ -228,7 +223,7 @@ class MlsGroupLifecycleTest {
        val addCarolResult = bob.addMember(carolBundle.keyPackage.toTlsBytes())

        // Alice processes Bob's commit
-        alice.processCommit(addCarolResult.commitBytes, bob.leafIndex, ByteArray(0))
+        alice.processFramedCommit(addCarolResult.framedCommitBytes)

        assertEquals(2L, alice.epoch)
        assertEquals(2L, bob.epoch)
@@ -240,18 +235,18 @@ class MlsGroupLifecycleTest {
    // 6. External join via GroupInfo
    // -----------------------------------------------------------------------

-    // BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
    @Test
    fun testExternalJoin_ZaraJoinsViaGroupInfo() {
        val alice = MlsGroup.create("alice".encodeToByteArray())
        val groupInfoBytes = alice.groupInfo().toTlsBytes()

        // Zara joins externally
-        val (zara, commitBytes) = MlsGroup.externalJoin(groupInfoBytes, "zara".encodeToByteArray())
+        val externalJoin = MlsGroup.externalJoin(groupInfoBytes, "zara".encodeToByteArray())
+        val zara = externalJoin.group
        assertEquals(1L, zara.epoch)

        // Alice processes the external commit
-        alice.processCommit(commitBytes, zara.leafIndex, ByteArray(0))
+        alice.processFramedCommit(externalJoin.framedCommitBytes)
        assertEquals(1L, alice.epoch)
        assertEquals(2, alice.memberCount)

@@ -262,14 +257,14 @@ class MlsGroupLifecycleTest {
        assertContentEquals(msg, dec.content)
    }

-    // BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
    @Test
    fun testExternalJoin_ExporterSecretsAgree() {
        val alice = MlsGroup.create("alice".encodeToByteArray())
        val groupInfoBytes = alice.groupInfo().toTlsBytes()

-        val (zara, commitBytes) = MlsGroup.externalJoin(groupInfoBytes, "zara".encodeToByteArray())
-        alice.processCommit(commitBytes, zara.leafIndex, ByteArray(0))
+        val externalJoin = MlsGroup.externalJoin(groupInfoBytes, "zara".encodeToByteArray())
+        val zara = externalJoin.group
+        alice.processFramedCommit(externalJoin.framedCommitBytes)

        val aliceKey = alice.exporterSecret("marmot", "group-event".encodeToByteArray(), 32)
        val zaraKey = zara.exporterSecret("marmot", "group-event".encodeToByteArray(), 32)
@@ -307,7 +302,6 @@ class MlsGroupLifecycleTest {
    // 8. Signing key rotation (Update proposal)
    // -----------------------------------------------------------------------

-    // BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
    @Test
    fun testSigningKeyRotation_EpochAdvances() {
        val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -322,7 +316,7 @@ class MlsGroupLifecycleTest {
        val commitResult = alice.commit()

        // Bob processes Alice's rotation commit
-        bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+        bob.processFramedCommit(commitResult.framedCommitBytes)

        assertEquals(2L, alice.epoch)
        assertEquals(2L, bob.epoch)
@@ -336,7 +330,6 @@ class MlsGroupLifecycleTest {
        assertFalse(keyBefore.contentEquals(aliceKey), "Exporter secret should change after rotation")
    }

-    // BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
    @Test
    fun testEncryptDecryptAfterSigningKeyRotation() {
        val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -347,7 +340,7 @@ class MlsGroupLifecycleTest {
        // Alice rotates her signing key
        alice.proposeSigningKeyRotation()
        val commitResult = alice.commit()
-        bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+        bob.processFramedCommit(commitResult.framedCommitBytes)

        // Both directions should still work after rotation
        val msg1 = "After rotation from Alice".encodeToByteArray()
@@ -405,7 +398,6 @@ class MlsGroupLifecycleTest {
    // 10. PSK proposal: register and use in commit
    // -----------------------------------------------------------------------

-    // BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
    @Test
    fun testPskProposal_EpochAdvancesWithPsk() {
        val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -426,7 +418,7 @@ class MlsGroupLifecycleTest {
        val commitResult = alice.commit()

        // Bob processes the commit
-        bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+        bob.processFramedCommit(commitResult.framedCommitBytes)

        assertEquals(epochBefore + 1, alice.epoch)
        assertEquals(alice.epoch, bob.epoch)
@@ -457,7 +449,7 @@ class MlsGroupLifecycleTest {
        assertNotNull(alice.reInitPending, "ReInit should be pending after commit")

        // Bob processes and should also see reInit
-        bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+        bob.processFramedCommit(commitResult.framedCommitBytes)
        assertNotNull(bob.reInitPending, "Bob should also see ReInit pending")
    }

@@ -476,7 +468,7 @@ class MlsGroupLifecycleTest {

        // Alice commits with no proposals (purely for forward secrecy / UpdatePath)
        val commitResult = alice.commit()
-        bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
+        bob.processFramedCommit(commitResult.framedCommitBytes)

        assertEquals(epochBefore + 1, alice.epoch)
        assertEquals(alice.epoch, bob.epoch)
@@ -254,17 +254,18 @@ class MlsGroupTest {
        val groupInfoBytes = alice.groupInfo().toTlsBytes()

        // Zara joins via external commit (without a Welcome)
-        val (zara, commitBytes) =
+        val externalJoin =
            MlsGroup.externalJoin(
                groupInfoBytes,
                "zara".encodeToByteArray(),
            )
+        val zara = externalJoin.group

        // Zara is now in the group at epoch 1
        assertEquals(1L, zara.epoch)

        // Alice processes Zara's external commit
-        alice.processCommit(commitBytes, zara.leafIndex, ByteArray(0))
+        alice.processFramedCommit(externalJoin.framedCommitBytes)
        assertEquals(1L, alice.epoch)
        assertEquals(2, alice.memberCount)
    }