fix(mls): decrypt retained-epoch messages correctly (Test 12 offline catchup)

The `tryDecryptWithRetainedEpoch` fallback in `MlsGroupManager.decrypt` exists precisely to cover the interop harness's Test 12 scenario — application messages encrypted under epoch N arriving at a receiver that has already processed a commit advancing to N+1. The primary path throws "Message epoch X doesn't match current epoch Y", then the manager iterates the retained epoch window and tries each. Two bugs kept the fallback from ever succeeding: 1. sender-data ciphertext sample length. The primary decrypt path uses `MlsCryptoProvider.HASH_OUTPUT_LENGTH` (32 bytes, = KDF.Nh for HKDF-SHA256, per RFC 9420 §6.3.2); the retained path was using `AEAD_KEY_LENGTH` (16). The comment on the primary path — "Using AEAD.Nk here made sender-data decryption fail against every spec-compliant sender" — was the same fix just never propagated to this branch. sender-data AEAD always failed, the try/catch swallowed it, and the fallback returned null. 2. content plaintext was returned raw. AEAD output is a PrivateMessageContent struct (§6.3.1): `applicationData<V> || signature<V> || padding`. The primary decrypt path parses this with `pmcReader.readOpaqueVarInt()` to extract applicationData; the retained path returned the whole struct. Callers saw a length-prefixed blob with signature + zero padding glued on — obvious in a raw diff, invisible in a pass/fail test that only checked "decrypted is not null". Fix both in `tryDecryptWithRetainedEpoch`. Existing quartz↔quartz tests still pass because they don't exercise out-of-order decrypt — this branch was effectively dead code before. Add testDecryptRetainedEpoch_ApplicationMessageAfterCommit as a regression: Bob encrypts at epoch N, Alice advances to N+1 via add-member, Bob's epoch-N message then arrives at Alice — must round- trip through the retained-epoch fallback and match the original plaintext exactly.
2026-04-23 00:30:57 +00:00
parent d05742a8b6
commit c88923a3d1
2 changed files with 101 additions and 3 deletions
@@ -660,8 +660,18 @@ class MlsGroupManager(
            if (privMsg.epoch != retained.epoch) return null

            // Derive sender data key/nonce using ciphertext sample (RFC 9420 §6.3.1)
+            // RFC 9420 §6.3.2: ciphertext_sample is the first KDF.Nh bytes
+            // (32 for HKDF-SHA256), not AEAD.Nk (16). Using AEAD.Nk here made
+            // sender-data decryption silently fail for every retained-epoch
+            // message and turned the fallback path into a no-op — the
+            // symptom was interop Test 12 (offline catch-up): kind:9
+            // messages encrypted under epoch N arriving after a 1→N+1
+            // commit got rejected with "Message epoch X doesn't match
+            // current epoch Y" instead of being pulled through this path.
+            // Same fix already applied to MlsGroup.decrypt (line ~878);
+            // this branch was missed when that one was patched.
            val ciphertextSample =
-                privMsg.ciphertext.copyOfRange(0, minOf(privMsg.ciphertext.size, MlsCryptoProvider.AEAD_KEY_LENGTH))
+                privMsg.ciphertext.copyOfRange(0, minOf(privMsg.ciphertext.size, MlsCryptoProvider.HASH_OUTPUT_LENGTH))
            val senderDataKey =
                MlsCryptoProvider.expandWithLabel(
                    retained.senderDataSecret,
@@ -710,13 +720,23 @@ class MlsGroupManager(
            contentAad.putUint8(privMsg.contentType.value)
            contentAad.putOpaqueVarInt(privMsg.authenticatedData)

-            val plaintext =
+            val pmcBytes =
                MlsCryptoProvider.aeadDecrypt(kng.key, guardedNonce, contentAad.toByteArray(), privMsg.ciphertext)

+            // AEAD plaintext is a PrivateMessageContent struct (RFC 9420
+            // §6.3.1): `applicationData<V> || signature<V> || padding`. The
+            // main decrypt path parses this and returns the inner
+            // applicationData; the retained-epoch branch was returning the
+            // raw struct, which made callers see a length-prefixed blob
+            // with signature + zero-padding glued onto the end. Extract the
+            // applicationData the same way.
+            val pmcReader = TlsReader(pmcBytes)
+            val applicationData = pmcReader.readOpaqueVarInt()
+
            DecryptedMessage(
                senderLeafIndex = senderLeafIndex,
                contentType = privMsg.contentType,
-                content = plaintext,
+                content = applicationData,
                epoch = privMsg.epoch,
            )
        } catch (_: Exception) {
@@ -158,6 +158,84 @@ class MarmotPipelineTest {
        }
    }

+    /**
+     * Regression: the interop harness's Test 12 (offline catch-up) drove
+     * kind:9 application messages encrypted under epoch N into an Amethyst
+     * receiver that had already processed a 1→N+1 commit (e.g. add-member).
+     * Those epoch-N messages ought to decrypt through the retained-epoch
+     * fallback in [MlsGroupManager.decrypt], but the fallback was silently
+     * returning null because `tryDecryptWithRetainedEpoch` derived the
+     * sender-data sample using `AEAD_KEY_LENGTH` (16) instead of
+     * `HASH_OUTPUT_LENGTH` (32) — the same bug that was fixed on the
+     * main decrypt path earlier but never propagated to the retained
+     * branch. This test reproduces the flow and asserts the retained
+     * path actually decrypts.
+     */
+    @Test
+    fun testDecryptRetainedEpoch_ApplicationMessageAfterCommit() {
+        runBlocking {
+            val aliceMgr = createGroupManager()
+            val bobMgr = createGroupManager()
+
+            // 32-byte identities so they fit the MarmotGroupData 32-byte
+            // admin-pubkey slots.
+            val aliceId = ByteArray(32) { 0xA1.toByte() }
+            val bobId = ByteArray(32) { 0xB2.toByte() }
+            val carolId = ByteArray(32) { 0xC3.toByte() }
+
+            aliceMgr.createGroup(groupId, aliceId)
+            // Install the MarmotGroupData extension so Welcome messages carry
+            // the NostrGroupData (MlsGroupManager.processWelcome requires it).
+            aliceMgr.updateGroupExtensions(
+                nostrGroupId = groupId,
+                extensions =
+                    listOf(
+                        com.vitorpamplona.quartz.marmot.mip01Groups
+                            .MarmotGroupData(
+                                nostrGroupId = groupId,
+                                adminPubkeys = listOf(aliceId.toHexKey()),
+                            ).toExtension(),
+                    ),
+            )
+
+            // Bob joins at epoch 2 (epoch 0 = create, epoch 1 = GCE above,
+            // epoch 2 = add Bob).
+            val aliceGroup = aliceMgr.getGroup(groupId)!!
+            val bobBundle = aliceGroup.createKeyPackage(bobId, ByteArray(0))
+            val addBob = aliceMgr.addMember(groupId, bobBundle.keyPackage.toTlsBytes())
+            bobMgr.processWelcome(addBob.welcomeBytes!!, bobBundle)
+            val bobJoinEpoch = aliceMgr.getGroup(groupId)!!.epoch
+            assertEquals(bobJoinEpoch, bobMgr.getGroup(groupId)!!.epoch, "bob must be at alice's epoch after Welcome")
+
+            // Bob encrypts a message at his current epoch — this would be
+            // held by a lossy relay and only reach Alice after she's
+            // already processed the next commit.
+            val bobStaleMsg = bobMgr.encrypt(groupId, "bob's offline epoch-N message".encodeToByteArray())
+
+            // Alice adds Carol, advancing to epoch N+1. Bob's earlier
+            // message is now out-of-order relative to Alice's tree state.
+            val carolBundle = aliceGroup.createKeyPackage(carolId, ByteArray(0))
+            aliceMgr.addMember(groupId, carolBundle.keyPackage.toTlsBytes())
+            assertEquals(
+                bobJoinEpoch + 1,
+                aliceMgr.getGroup(groupId)!!.epoch,
+                "alice must have advanced one epoch past bob's encrypt time",
+            )
+
+            // Bob's stale message now arrives at Alice. The primary decrypt
+            // path throws "Message epoch X doesn't match current epoch X+1";
+            // the retained-epoch fallback inside MlsGroupManager.decrypt
+            // must pick it up.
+            val decrypted = aliceMgr.decrypt(groupId, bobStaleMsg)
+            assertEquals(
+                "bob's offline epoch-N message",
+                decrypted.content.decodeToString(),
+                "Alice must decrypt Bob's pre-commit message via retained-epoch fallback",
+            )
+            assertEquals(bobJoinEpoch, decrypted.epoch, "message was encrypted at bob's join epoch")
+        }
+    }
+
    @Test
    fun testInboundRejectsNonMemberGroup() {
        runBlocking {