From 5705a2b84075d901ef66eb913fb31ab53653c2b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Apr 2026 23:38:31 +0000 Subject: [PATCH] fix(marmot): outer-encrypt commits with pre-commit exporter secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per RFC 9420 §12.4 and the MDK reference implementation, a kind:445 Commit MUST be outer-encrypted with the epoch-N exporter secret — the key existing members still hold — so that they can decrypt, process the commit, and advance to N+1. Amethyst was instead encrypting with the epoch-(N+1) key because MlsGroupManager.addMember / removeMember / updateGroupExtensions applied the commit locally *before* handing the bytes to MarmotOutboundProcessor.buildCommitEvent, which read the current (post-commit) exporter via groupManager.exporterSecret. Observed in a 3-party Amethyst-to-Amethyst test (David creates group, adds Eden then Fred, sends "Hi"): Eden joined at epoch 1 via Welcome, then "succeeded" in outer-decrypting the add-Eden kind:445 with her own post-commit key but failed to apply ("Duplicate encryption key: leaf 1") because she was already in the tree from the Welcome. Eden never advanced past epoch 1, so Fred's subsequent join and David's application message were undecryptable. Fred, joining directly at epoch 2, saw the Hi message fine — matching the symptom where the last-added member is the only one who can read new messages. Fix splits commit creation from commit application on the outbound path, mirroring OpenMLS / MDK's `create_commit` + `merge_pending_commit` pair: - MlsGroup gains stageCommit() / mergeStagedCommit(). stageCommit captures a full snapshot, runs the existing commit logic (which mutates in place), captures the post-commit snapshot, then restores the pre-commit snapshot so the caller observes epoch N until merge. The returned StagedCommit carries the pre-commit exporter secret and the framed commit bytes. stageAddMember / stageRemoveMember are thin wrappers that propose + stage. - MlsGroupManager exposes stageAddMember, stageRemoveMember, stageRotateSigningKey, stageUpdateGroupExtensions, and mergeStagedCommit. The eager addMember/removeMember/commit/etc. entry points are retained (documented as test-only) so the MLS unit tests in MlsGroupTest, MlsGroupLifecycleTest, etc. keep working unchanged. - MarmotOutboundProcessor.buildCommitEvent takes an optional exporterKey override; callers pass StagedCommit.preCommitExporterSecret. - MarmotManager.addMember / removeMember / updateGroupMetadata now stage → build kind:445 → markEventProcessed → mergeStagedCommit. - MlsGroupManager.processCommit was also retaining epoch secrets *before* calling group.processCommit, so a failed commit (such as the add-me relay echo that the new member can't apply) polluted the retained window with a duplicate of the current epoch key. Now retain only on success. Regression tests: - testStagedAddMemberUsesPreCommitExporter: asserts stage does not advance the epoch and that preCommitExporterSecret equals the pre-stage exporter output. - testCreatorCanAddTwoMembersAndAllDecryptFollowingMessage: end-to-end David/Eden/Fred reproduction — creator adds two members in sequence and publishes an application message; both members must decrypt it. Fails without the fix, passes with it. https://claude.ai/code/session_014zfdNeeKAfU1zGGyFw4bUL --- .../amethyst/commons/marmot/MarmotManager.kt | 51 ++++- .../quartz/marmot/MarmotOutboundProcessor.kt | 19 +- .../quartz/marmot/mls/group/MlsGroup.kt | 153 ++++++++++++++- .../marmot/mls/group/MlsGroupManager.kt | 148 ++++++++++++--- .../quartz/marmot/mls/group/StagedCommit.kt | 112 +++++++++++ .../quartz/marmot/MarmotPipelineTest.kt | 175 ++++++++++++++++++ 6 files changed, 613 insertions(+), 45 deletions(-) create mode 100644 quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/StagedCommit.kt diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt index b5b8c94de..635ae97ea 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt @@ -178,21 +178,39 @@ class MarmotManager( "KeyPackage credential identity does not match memberPubKey" } - val commitResult = groupManager.addMember(nostrGroupId, keyPackageBytes) - val commitEvent = outboundProcessor.buildCommitEvent(nostrGroupId, commitResult.framedCommitBytes) - // We've already applied this commit locally; prevent the relay-echoed - // copy from being re-applied by the inbound pipeline. + // Stage-then-merge (MDK / RFC 9420 §12.4): build the kind:445 with the + // PRE-commit epoch's exporter secret so that other existing members + // can outer-decrypt and process it. Only advance our own epoch once + // the framed commit has been handed to the outbound path. + val staged = groupManager.stageAddMember(nostrGroupId, keyPackageBytes) + val commitEvent = + outboundProcessor.buildCommitEvent( + nostrGroupId = nostrGroupId, + commitBytes = staged.framedCommitBytes, + exporterKey = staged.preCommitExporterSecret, + ) + // The published kind:445 will echo back from the relay — without this + // dedup our own inbound pipeline would try to re-apply a commit whose + // epoch we've already merged below. inboundProcessor.markEventProcessed(commitEvent.signedEvent.id) + val welcomeBytes = + staged.welcomeBytes + ?: throw IllegalStateException( + "stageAddMember did not produce a Welcome for $memberPubKey", + ) val welcomeDelivery = - welcomeSender.wrapWelcome( - commitResult = commitResult, + welcomeSender.wrapWelcomeBytes( + welcomeBytes = welcomeBytes, recipientPubKey = memberPubKey, keyPackageEventId = keyPackageEventId, relays = relays, nostrGroupId = nostrGroupId, ) + // Advance local epoch now that the commit + welcome have been built. + groupManager.mergeStagedCommit(nostrGroupId, staged) + return Pair(commitEvent, welcomeDelivery) } @@ -268,9 +286,15 @@ class MarmotManager( nostrGroupId: HexKey, targetLeafIndex: Int, ): OutboundGroupEvent { - val commitResult = groupManager.removeMember(nostrGroupId, targetLeafIndex) - val commitEvent = outboundProcessor.buildCommitEvent(nostrGroupId, commitResult.framedCommitBytes) + val staged = groupManager.stageRemoveMember(nostrGroupId, targetLeafIndex) + val commitEvent = + outboundProcessor.buildCommitEvent( + nostrGroupId = nostrGroupId, + commitBytes = staged.framedCommitBytes, + exporterKey = staged.preCommitExporterSecret, + ) inboundProcessor.markEventProcessed(commitEvent.signedEvent.id) + groupManager.mergeStagedCommit(nostrGroupId, staged) return commitEvent } @@ -282,9 +306,16 @@ class MarmotManager( nostrGroupId: HexKey, metadata: MarmotGroupData, ): OutboundGroupEvent { - val commitResult = groupManager.updateGroupExtensions(nostrGroupId, listOf(metadata.toExtension())) - val commitEvent = outboundProcessor.buildCommitEvent(nostrGroupId, commitResult.framedCommitBytes) + val staged = + groupManager.stageUpdateGroupExtensions(nostrGroupId, listOf(metadata.toExtension())) + val commitEvent = + outboundProcessor.buildCommitEvent( + nostrGroupId = nostrGroupId, + commitBytes = staged.framedCommitBytes, + exporterKey = staged.preCommitExporterSecret, + ) inboundProcessor.markEventProcessed(commitEvent.signedEvent.id) + groupManager.mergeStagedCommit(nostrGroupId, staged) return commitEvent } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotOutboundProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotOutboundProcessor.kt index df3cecc28..3f0e276cb 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotOutboundProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotOutboundProcessor.kt @@ -121,20 +121,29 @@ class MarmotOutboundProcessor( /** * Build a GroupEvent carrying a Commit for publishing. * - * Used after MlsGroupManager.commit() or addMember()/removeMember(). - * The commit bytes are already MLS-formatted. + * Used after MlsGroupManager.stageAddMember()/stageRemoveMember()/etc. + * The commit bytes are already MLS-formatted (PublicMessage envelope). * * @param nostrGroupId the Nostr group ID - * @param commitBytes the raw MLS commit bytes from CommitResult + * @param commitBytes the framed MLS commit bytes from [com.vitorpamplona.quartz.marmot.mls.group.StagedCommit.framedCommitBytes] + * @param exporterKey optional explicit outer-encryption key. Callers + * publishing a Commit MUST pass the **pre-commit** exporter secret + * (from [com.vitorpamplona.quartz.marmot.mls.group.StagedCommit.preCommitExporterSecret]) + * so that other existing members at epoch N can decrypt and process + * the commit. If null, falls back to the current epoch's exporter + * secret — which is only correct when the commit has *not* been + * applied locally yet (i.e. this call is made before + * [com.vitorpamplona.quartz.marmot.mls.group.MlsGroup.mergeStagedCommit]). * @return the signed GroupEvent ready for relay publishing */ suspend fun buildCommitEvent( nostrGroupId: HexKey, commitBytes: ByteArray, + exporterKey: ByteArray? = null, ): OutboundGroupEvent { // Outer ChaCha20-Poly1305 encryption of the MLS commit - val exporterKey = groupManager.exporterSecret(nostrGroupId) - val encryptedContent = GroupEventEncryption.encrypt(commitBytes, exporterKey) + val outerKey = exporterKey ?: groupManager.exporterSecret(nostrGroupId) + val encryptedContent = GroupEventEncryption.encrypt(commitBytes, outerKey) // Build the GroupEvent template val template = diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt index 74abe20fc..9bd7f44a5 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt @@ -98,7 +98,7 @@ import com.vitorpamplona.quartz.utils.mac.MacInstance */ class MlsGroup private constructor( private var groupContext: GroupContext, - private val tree: RatchetTree, + private var tree: RatchetTree, private var myLeafIndex: Int, private var epochSecrets: EpochSecrets, private var secretTree: SecretTree, @@ -199,6 +199,125 @@ class MlsGroup private constructor( var reInitPending: Proposal.ReInit? = null private set + // --- Stage/Merge Infrastructure (RFC 9420 §12.4 / MDK parity) --- + + /** + * Capture a complete snapshot of every mutable field so this group can be + * restored atomically. Used by [stageCommit] to rewind after computing a + * Commit so the outer code can encrypt the kind:445 with the pre-commit + * exporter secret before advancing locally. + */ + private fun captureSnapshot(): MlsGroupSnapshot { + val treeWriter = TlsWriter() + tree.encodeTls(treeWriter) + return MlsGroupSnapshot( + groupContext = groupContext, + treeBytes = treeWriter.toByteArray(), + myLeafIndex = myLeafIndex, + epochSecrets = epochSecrets, + secretTree = secretTree, + initSecret = initSecret, + signingPrivateKey = signingPrivateKey, + encryptionPrivateKey = encryptionPrivateKey, + interimTranscriptHash = interimTranscriptHash, + pskStore = pskStore.toMap(), + pendingProposals = pendingProposals.toList(), + sentKeys = sentKeys.toMap(), + pendingSigningKey = pendingSigningKey, + pendingEncryptionKey = pendingEncryptionKey, + reInitPending = reInitPending, + ) + } + + /** + * Overwrite every mutable field of this group from a prior snapshot. + * See [captureSnapshot] for the use case. + */ + private fun restoreSnapshot(s: MlsGroupSnapshot) { + groupContext = s.groupContext + tree = RatchetTree.decodeTls(TlsReader(s.treeBytes)) + myLeafIndex = s.myLeafIndex + epochSecrets = s.epochSecrets + // secretTree is keyed on encryption_secret + leafCount; rebuilding from + // the snapshot's encryption_secret avoids relying on SecretTree being + // a pure data class, which it currently is, but keeps us robust. + secretTree = SecretTree(s.epochSecrets.encryptionSecret, tree.leafCount) + initSecret = s.initSecret + signingPrivateKey = s.signingPrivateKey + encryptionPrivateKey = s.encryptionPrivateKey + interimTranscriptHash = s.interimTranscriptHash + pskStore.clear() + pskStore.putAll(s.pskStore) + pendingProposals.clear() + pendingProposals.addAll(s.pendingProposals) + sentKeys.clear() + sentKeys.putAll(s.sentKeys) + pendingSigningKey = s.pendingSigningKey + pendingEncryptionKey = s.pendingEncryptionKey + reInitPending = s.reInitPending + } + + /** + * Build a Commit from the currently pending proposals **without** advancing + * the local epoch. The caller can then outer-encrypt the kind:445 using + * [StagedCommit.preCommitExporterSecret] — which is still the current + * epoch's key — and call [mergeStagedCommit] to advance locally only after + * the kind:445 has been handed to the transport. + * + * This mirrors OpenMLS / MDK's `create_commit` + `merge_pending_commit` + * pair and is the only correct way to encrypt a Commit: existing members + * still at epoch N need the pre-commit exporter secret to decrypt the + * outer layer, while the new member joining via Welcome does not need to + * process this Commit at all. + * + * On success, the group state is unchanged. + * On failure, the group state is unchanged. + */ + fun stageCommit(): StagedCommit { + val preCommitSnapshot = captureSnapshot() + val preCommitExporter = exporterSecret("marmot", "group-event".encodeToByteArray(), 32) + val preCommitEpoch = groupContext.epoch + + val committed: CommitResult = + try { + commit() + } catch (e: Exception) { + // commit() may have mutated fields before throwing — always roll back. + restoreSnapshot(preCommitSnapshot) + throw e + } + + val postCommitSnapshot = captureSnapshot() + val postCommitEpoch = groupContext.epoch + + // Rewind to pre-commit state so the outer code sees epoch N until it + // explicitly calls mergeStagedCommit. + restoreSnapshot(preCommitSnapshot) + + return StagedCommit( + preCommitExporterSecret = preCommitExporter, + preCommitEpoch = preCommitEpoch, + postCommitEpoch = postCommitEpoch, + commitBytes = committed.commitBytes, + framedCommitBytes = committed.framedCommitBytes, + welcomeBytes = committed.welcomeBytes, + groupInfoBytes = committed.groupInfoBytes, + postState = postCommitSnapshot, + ) + } + + /** + * Apply a previously [stageCommit]ed transition, advancing this group from + * epoch N to epoch N+1. Call once the Commit has been successfully handed + * to the transport. + */ + fun mergeStagedCommit(staged: StagedCommit) { + check(groupContext.epoch == staged.preCommitEpoch) { + "mergeStagedCommit: group at epoch ${groupContext.epoch} but staged commit is for epoch ${staged.preCommitEpoch}" + } + restoreSnapshot(staged.postState) + } + /** * Register a pre-shared key for use in PSK proposals. */ @@ -2072,21 +2191,53 @@ class MlsGroup private constructor( /** * Add a member to the group by their KeyPackage. * Creates and applies a Commit with an Add proposal. + * + * This advances the local epoch eagerly; the returned [CommitResult] is + * encrypted on the wire with the **post-commit** exporter key, which is + * only correct for tests / offline MLS scenarios. Production callers MUST + * use [stageAddMember] + [mergeStagedCommit] so the kind:445 can be + * wrapped with the pre-commit exporter key, matching the MDK reference + * and allowing other existing members to actually decrypt and process the + * commit. */ fun addMember(keyPackageBytes: ByteArray): CommitResult { proposeAdd(keyPackageBytes) return commit() } + /** + * Stage an Add-member Commit without advancing the local epoch. + * Returned [StagedCommit] carries the pre-commit exporter secret that the + * outer kind:445 layer MUST be encrypted with (RFC 9420 §12.4 + MDK). + * Call [mergeStagedCommit] after the commit has been handed to the + * transport to advance locally. + */ + fun stageAddMember(keyPackageBytes: ByteArray): StagedCommit { + proposeAdd(keyPackageBytes) + return stageCommit() + } + /** * Remove a member from the group. * Creates and applies a Commit with a Remove proposal. + * + * See [addMember] for why production callers should prefer + * [stageRemoveMember] instead. */ fun removeMember(targetLeafIndex: Int): CommitResult { proposeRemove(targetLeafIndex) return commit() } + /** + * Stage a Remove-member Commit without advancing the local epoch. + * See [stageAddMember] for the rationale. + */ + fun stageRemoveMember(targetLeafIndex: Int): StagedCommit { + proposeRemove(targetLeafIndex) + return stageCommit() + } + /** * Remove self from the group. * diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt index bdfec1984..bea9ca8ac 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt @@ -267,11 +267,9 @@ class MlsGroupManager( suspend fun commit(nostrGroupId: HexKey): CommitResult = mutex.withLock { val group = requireGroup(nostrGroupId) - - // Retain current epoch secrets before transition - retainEpochSecrets(nostrGroupId, group) - + val retainedBefore = group.retainedSecrets() val result = group.commit() + pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) result } @@ -292,10 +290,16 @@ class MlsGroupManager( ) = mutex.withLock { val group = requireGroup(nostrGroupId) - // Retain current epoch secrets before transition - retainEpochSecrets(nostrGroupId, group) + // Capture the outgoing epoch's secrets BEFORE advancing, but only + // commit them to the retention window once processCommit succeeds — + // otherwise a failed commit (e.g. "Duplicate encryption key" on an + // add-me relay echo) would pollute the window with a duplicate of + // the current epoch key, wasting the finite retention slots. + val retainedBefore = group.retainedSecrets() group.processCommit(commitBytes, senderLeafIndex, confirmationTag) + + pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) } @@ -355,10 +359,18 @@ class MlsGroupManager( null } - // --- Member Management --- + // --- Member Management (atomic stage+merge; retained for tests/offline use) --- /** - * Add a member and create a Commit. + * Add a member and create a Commit, advancing the local epoch immediately. + * + * **Production callers MUST use [stageAddMember] + [mergeStagedCommit] + * instead.** The commit bytes returned here are wrapped on the wire with + * the *post-commit* exporter key, which is wrong — other existing members + * at epoch N cannot decrypt it, so the commit is unprocessable by anyone + * except the new member (who doesn't need it because their Welcome already + * carries the post-commit state). This entry point is retained only for + * unit tests and scenarios where outer framing isn't used. */ suspend fun addMember( nostrGroupId: HexKey, @@ -366,14 +378,16 @@ class MlsGroupManager( ): CommitResult = mutex.withLock { val group = requireGroup(nostrGroupId) - retainEpochSecrets(nostrGroupId, group) + val retainedBefore = group.retainedSecrets() val result = group.addMember(keyPackageBytes) + pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) result } /** - * Remove a member and create a Commit. + * Remove a member and create a Commit. See [addMember] for the production + * caveat — use [stageRemoveMember] + [mergeStagedCommit] on the wire. */ suspend fun removeMember( nostrGroupId: HexKey, @@ -381,27 +395,25 @@ class MlsGroupManager( ): CommitResult = mutex.withLock { val group = requireGroup(nostrGroupId) - retainEpochSecrets(nostrGroupId, group) + val retainedBefore = group.retainedSecrets() val result = group.removeMember(targetLeafIndex) + pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) result } /** - * Rotate the signing key within a group and commit. - * - * Per MIP-00, members SHOULD self-update within 24 hours of joining. - * This creates an Update proposal with a fresh signing key and commits it. - * - * @param nostrGroupId hex-encoded Nostr group ID - * @return the [CommitResult] to publish + * Rotate the signing key within a group and commit. See [addMember] for + * the production caveat — use [stageRotateSigningKey] + [mergeStagedCommit] + * on the wire. */ suspend fun rotateSigningKey(nostrGroupId: HexKey): CommitResult = mutex.withLock { val group = requireGroup(nostrGroupId) - retainEpochSecrets(nostrGroupId, group) + val retainedBefore = group.retainedSecrets() group.proposeSigningKeyRotation() val result = group.commit() + pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) result } @@ -410,12 +422,8 @@ class MlsGroupManager( * Update group extensions (e.g., MIP-01 metadata) via a GroupContextExtensions proposal. * Creates the proposal, commits it, and persists the new state. * - * Authorization (MIP-01): extension updates require admin privileges once - * the group has at least one admin. During bootstrap — before any - * `admin_pubkeys` are configured — any member may seed the initial - * extension set (e.g. the creator installing the first - * [com.vitorpamplona.quartz.marmot.mip01Groups.MarmotGroupData] with - * themselves as admin). + * See [addMember] for the production caveat — use [stageUpdateGroupExtensions] + * + [mergeStagedCommit] on the wire. */ suspend fun updateGroupExtensions( nostrGroupId: HexKey, @@ -428,13 +436,89 @@ class MlsGroupManager( check(!adminsConfigured || group.isLocalAdmin()) { "MIP-01: only admins may update group extensions" } - retainEpochSecrets(nostrGroupId, group) + val retainedBefore = group.retainedSecrets() group.proposeGroupContextExtensions(extensions) val result = group.commit() + pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) result } + // --- Staged Member Management (production path; MDK parity) --- + + /** + * Stage an Add-member Commit without advancing the local epoch. + * The returned [StagedCommit.preCommitExporterSecret] is the epoch-N key + * that the outer kind:445 ChaCha20 layer MUST use so that existing + * members at epoch N can decrypt + process it. Call [mergeStagedCommit] + * after the kind:445 has been handed to the transport. + */ + suspend fun stageAddMember( + nostrGroupId: HexKey, + keyPackageBytes: ByteArray, + ): StagedCommit = + mutex.withLock { + requireGroup(nostrGroupId).stageAddMember(keyPackageBytes) + } + + /** + * Stage a Remove-member Commit. See [stageAddMember]. + */ + suspend fun stageRemoveMember( + nostrGroupId: HexKey, + targetLeafIndex: Int, + ): StagedCommit = + mutex.withLock { + requireGroup(nostrGroupId).stageRemoveMember(targetLeafIndex) + } + + /** + * Stage a signing-key rotation Commit. See [stageAddMember]. + */ + suspend fun stageRotateSigningKey(nostrGroupId: HexKey): StagedCommit = + mutex.withLock { + val group = requireGroup(nostrGroupId) + group.proposeSigningKeyRotation() + group.stageCommit() + } + + /** + * Stage a GroupContextExtensions Commit (MIP-01 metadata). See + * [stageAddMember]. + */ + suspend fun stageUpdateGroupExtensions( + nostrGroupId: HexKey, + extensions: List, + ): StagedCommit = + mutex.withLock { + val group = requireGroup(nostrGroupId) + val currentMarmot = group.currentMarmotData() + val adminsConfigured = currentMarmot != null && currentMarmot.adminPubkeys.isNotEmpty() + check(!adminsConfigured || group.isLocalAdmin()) { + "MIP-01: only admins may update group extensions" + } + group.proposeGroupContextExtensions(extensions) + group.stageCommit() + } + + /** + * Merge a previously [stageAddMember]/[stageRemoveMember]/etc. commit, + * advancing the local epoch to N+1. Retains the pre-commit epoch's + * secrets for late-message decryption, then persists. + */ + suspend fun mergeStagedCommit( + nostrGroupId: HexKey, + staged: StagedCommit, + ) = mutex.withLock { + val group = requireGroup(nostrGroupId) + // Retain the outgoing epoch's secrets BEFORE advancing (they live in + // the pre-commit snapshot, which is this group's current state). + val retainedBefore = group.retainedSecrets() + group.mergeStagedCommit(staged) + pushRetainedEpoch(nostrGroupId, retainedBefore) + persistGroup(nostrGroupId) + } + /** * Leave a group (self-remove). * Returns the SelfRemove proposal bytes to publish, then removes @@ -564,12 +648,18 @@ class MlsGroupManager( } } - private fun retainEpochSecrets( + /** + * Push a previously-captured [RetainedEpochSecrets] into the bounded + * retention window. Call after the epoch advance has been applied + * successfully so that failed commits don't pollute the window with + * duplicate current-epoch keys. + */ + private fun pushRetainedEpoch( nostrGroupId: HexKey, - group: MlsGroup, + retainedBefore: RetainedEpochSecrets, ) { val retained = retainedEpochs.getOrPut(nostrGroupId) { mutableListOf() } - retained.add(group.retainedSecrets()) + retained.add(retainedBefore) // Trim to retention window (keep only the most recent N-1 epochs) while (retained.size > EPOCH_RETENTION_WINDOW) { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/StagedCommit.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/StagedCommit.kt new file mode 100644 index 000000000..f8aa53f0e --- /dev/null +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/StagedCommit.kt @@ -0,0 +1,112 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.marmot.mls.group + +import com.vitorpamplona.quartz.marmot.mls.messages.GroupContext +import com.vitorpamplona.quartz.marmot.mls.messages.Proposal +import com.vitorpamplona.quartz.marmot.mls.schedule.EpochSecrets +import com.vitorpamplona.quartz.marmot.mls.schedule.KeyNonceGeneration +import com.vitorpamplona.quartz.marmot.mls.schedule.SecretTree + +/** + * Snapshot of every mutable field of [MlsGroup] at a single point in time. + * + * Used to capture state both before a commit (for rollback) and after a commit + * (for later merging via [MlsGroup.mergeStagedCommit]). + * + * Contains secret key material — treat as sensitive. Do not serialize or log. + */ +internal data class MlsGroupSnapshot( + val groupContext: GroupContext, + /** TLS-encoded [com.vitorpamplona.quartz.marmot.mls.tree.RatchetTree] bytes. */ + val treeBytes: ByteArray, + val myLeafIndex: Int, + val epochSecrets: EpochSecrets, + val secretTree: SecretTree, + val initSecret: ByteArray, + val signingPrivateKey: ByteArray, + val encryptionPrivateKey: ByteArray, + val interimTranscriptHash: ByteArray, + val pskStore: Map, + val pendingProposals: List, + val sentKeys: Map, + val pendingSigningKey: ByteArray?, + val pendingEncryptionKey: ByteArray?, + val reInitPending: Proposal.ReInit?, +) + +/** + * Result of staging (but not yet merging) a Commit. + * + * Mirrors the MDK / OpenMLS `add_members` → `merge_pending_commit` flow + * (RFC 9420 §12.4): first the Commit is *computed* from the current group + * state (which remains at epoch N) so that the outer kind:445 can be + * ChaCha20-encrypted with the **pre-commit (epoch N)** exporter secret — + * the key that other existing members still hold. Only after the Commit has + * been handed to the transport does the local group actually advance to + * epoch N+1 via [MlsGroup.mergeStagedCommit]. + * + * [preCommitExporterSecret] is the output of + * `MLS-Exporter("marmot", "group-event", 32)` at the pre-commit epoch — + * exactly the key existing members need to outer-decrypt and process the + * Commit on inbound. + * + * [commitBytes] is the raw `Commit` TLS struct. [framedCommitBytes] is the + * full `MlsMessage(PublicMessage(Commit))` envelope ready for + * ChaCha20-Poly1305 wrapping; publishers MUST use the framed bytes so that + * receivers can recover the sender's leaf index and confirmation tag. + */ +@ConsistentCopyVisibility +data class StagedCommit internal constructor( + /** Pre-commit (epoch N) MLS-Exporter output for outer kind:445 encryption. */ + val preCommitExporterSecret: ByteArray, + /** Pre-commit epoch (N). */ + val preCommitEpoch: Long, + /** Post-commit epoch (N+1). */ + val postCommitEpoch: Long, + /** Raw Commit (RFC 9420 §12.4) bytes, for tests / processCommit. */ + val commitBytes: ByteArray, + /** MlsMessage(PublicMessage(Commit)) envelope for on-the-wire distribution. */ + val framedCommitBytes: ByteArray, + /** Welcome message bytes for any members added by this Commit, else null. */ + val welcomeBytes: ByteArray?, + /** GroupInfo bytes for external joiners, else null. */ + val groupInfoBytes: ByteArray?, + /** Full post-commit snapshot applied by [MlsGroup.mergeStagedCommit]. */ + internal val postState: MlsGroupSnapshot, +) { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is StagedCommit) return false + return commitBytes.contentEquals(other.commitBytes) && + framedCommitBytes.contentEquals(other.framedCommitBytes) && + preCommitEpoch == other.preCommitEpoch && + postCommitEpoch == other.postCommitEpoch + } + + override fun hashCode(): Int { + var result = commitBytes.contentHashCode() + result = 31 * result + framedCommitBytes.contentHashCode() + result = 31 * result + preCommitEpoch.hashCode() + result = 31 * result + postCommitEpoch.hashCode() + return result + } +} diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt index bd574472f..cdddf5da1 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt @@ -25,6 +25,7 @@ import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEvent import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEventEncryption import com.vitorpamplona.quartz.marmot.mls.group.MlsGroupManager import com.vitorpamplona.quartz.marmot.mls.group.MlsGroupStateStore +import com.vitorpamplona.quartz.nip01Core.core.toHexKey import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal import kotlinx.coroutines.runBlocking @@ -427,6 +428,180 @@ class MarmotPipelineTest { } } + @Test + fun testStagedAddMemberUsesPreCommitExporter() { + // Regression: before the stage/merge split, MarmotManager.addMember + // was outer-encrypting the kind:445 commit with the POST-commit + // epoch key. Existing members still at epoch N couldn't decrypt it, + // and the newly-added member (joining via Welcome at epoch N+1) + // *could* decrypt it but then failed to apply ("Duplicate encryption + // key: leaf X"). The fix exposes the pre-commit exporter secret via + // StagedCommit so callers can encrypt correctly. + runBlocking { + val manager = createGroupManager() + manager.createGroup(groupId, "alice".encodeToByteArray()) + + val preEpochExporter = manager.exporterSecret(groupId) + val preEpoch = manager.getGroup(groupId)!!.epoch + + val bobBundle = + manager + .getGroup(groupId)!! + .createKeyPackage("bob".encodeToByteArray(), ByteArray(0)) + val staged = manager.stageAddMember(groupId, bobBundle.keyPackage.toTlsBytes()) + + // Stage must NOT have advanced the local epoch yet. + assertEquals(preEpoch, manager.getGroup(groupId)!!.epoch) + // preCommitExporterSecret must equal the pre-stage exporter key. + kotlin.test.assertContentEquals( + preEpochExporter, + staged.preCommitExporterSecret, + ) + // The staged post-commit epoch is N+1. + assertEquals(preEpoch + 1, staged.postCommitEpoch) + assertNotNull(staged.welcomeBytes) + + // Merge advances to N+1. + manager.mergeStagedCommit(groupId, staged) + assertEquals(preEpoch + 1, manager.getGroup(groupId)!!.epoch) + } + } + + @Test + fun testCreatorCanAddTwoMembersAndAllDecryptFollowingMessage() { + // End-to-end regression for the David/Eden/Fred scenario: a group + // creator (Alice) adds two members (Bob then Carol) in sequence and + // then publishes an application message. Both members MUST be able to + // decrypt the message. Before the stage/merge fix, the add-Carol + // commit was outer-encrypted with the post-commit (epoch 2) key, + // leaving Bob stuck at epoch 1 and unable to decrypt any of Alice's + // subsequent application messages. + runBlocking { + val aliceMgr = createGroupManager() + val bobMgr = createGroupManager() + val carolMgr = createGroupManager() + + // Use 32-byte identities so they fit the MarmotGroupData + // admin_pubkeys 32-byte slots. + val aliceIdBytes = ByteArray(32) { 0xA1.toByte() } + val bobIdBytes = ByteArray(32) { 0xB2.toByte() } + val carolIdBytes = ByteArray(32) { 0xC3.toByte() } + aliceMgr.createGroup(groupId, aliceIdBytes) + // Install the Marmot group data extension so the Welcome carries + // the NostrGroupData (MlsGroupManager.processWelcome requires it). + aliceMgr.updateGroupExtensions( + nostrGroupId = groupId, + extensions = + listOf( + com.vitorpamplona.quartz.marmot.mip01Groups + .MarmotGroupData( + nostrGroupId = groupId, + adminPubkeys = listOf(aliceIdBytes.toHexKey()), + ).toExtension(), + ), + ) + val aliceGroup = aliceMgr.getGroup(groupId)!! + + // KeyPackages are just MLS artifacts carrying identity + init key; + // createKeyPackage on any group instance produces a usable bundle. + val bobBundle = aliceGroup.createKeyPackage(bobIdBytes, ByteArray(0)) + val carolBundle = aliceGroup.createKeyPackage(carolIdBytes, ByteArray(0)) + + val outbound = MarmotOutboundProcessor(aliceMgr) + + // --- Step 1: Alice adds Bob via stage/merge --- + val stagedBob = aliceMgr.stageAddMember(groupId, bobBundle.keyPackage.toTlsBytes()) + val addBobCommitEvent = + outbound.buildCommitEvent( + nostrGroupId = groupId, + commitBytes = stagedBob.framedCommitBytes, + exporterKey = stagedBob.preCommitExporterSecret, + ) + + // Bob joins via Welcome (emulated: we hand him the Welcome bytes). + bobMgr.processWelcome(stagedBob.welcomeBytes!!, bobBundle) + + aliceMgr.mergeStagedCommit(groupId, stagedBob) + val epochAfterAddBob = aliceMgr.getGroup(groupId)!!.epoch + assertEquals(epochAfterAddBob, bobMgr.getGroup(groupId)!!.epoch) + + // --- Step 2: Alice adds Carol via stage/merge --- + val stagedCarol = aliceMgr.stageAddMember(groupId, carolBundle.keyPackage.toTlsBytes()) + val addCarolCommitEvent = + outbound.buildCommitEvent( + nostrGroupId = groupId, + commitBytes = stagedCarol.framedCommitBytes, + exporterKey = stagedCarol.preCommitExporterSecret, + ) + + // Carol joins via Welcome at epoch 2. + carolMgr.processWelcome(stagedCarol.welcomeBytes!!, carolBundle) + + // *** The key assertion ***: Bob (still at epoch 1) receives the + // add-Carol kind:445 and must be able to: + // (a) outer-decrypt using his current (epoch 1) exporter key, + // (b) parse the MlsMessage → PublicMessage → COMMIT, + // (c) apply the commit and advance to epoch 2. + val bobExporterAtE1 = bobMgr.exporterSecret(groupId) + kotlin.test.assertContentEquals( + bobExporterAtE1, + stagedCarol.preCommitExporterSecret, + "Bob's epoch-1 exporter must match the pre-commit key used to " + + "outer-encrypt the add-Carol commit; otherwise Bob cannot " + + "decrypt and will be stuck on the old epoch.", + ) + + val decryptedMlsBytes = + GroupEventEncryption.decrypt( + addCarolCommitEvent.signedEvent.content, + bobExporterAtE1, + ) + val mlsMessage = + com.vitorpamplona.quartz.marmot.mls.framing.MlsMessage + .decodeTls( + com.vitorpamplona.quartz.marmot.mls.codec + .TlsReader(decryptedMlsBytes), + ) + val publicMessage = + com.vitorpamplona.quartz.marmot.mls.framing.PublicMessage + .decodeTls( + com.vitorpamplona.quartz.marmot.mls.codec + .TlsReader(mlsMessage.payload), + ) + bobMgr.processCommit( + nostrGroupId = groupId, + commitBytes = publicMessage.content, + senderLeafIndex = publicMessage.sender.leafIndex, + confirmationTag = publicMessage.confirmationTag!!, + ) + + aliceMgr.mergeStagedCommit(groupId, stagedCarol) + val epochAfterAddCarol = aliceMgr.getGroup(groupId)!!.epoch + assertEquals(epochAfterAddBob + 1, epochAfterAddCarol) + assertEquals(epochAfterAddCarol, bobMgr.getGroup(groupId)!!.epoch) + assertEquals(epochAfterAddCarol, carolMgr.getGroup(groupId)!!.epoch) + + // --- Step 3: Alice sends an application message. Both members must decrypt. --- + val msg = "Hi from Alice" + val appEvent = + outbound.buildGroupEventFromBytes(groupId, msg.encodeToByteArray()) + + val bobKey = bobMgr.exporterSecret(groupId) + val carolKey = carolMgr.exporterSecret(groupId) + val aliceKey = aliceMgr.exporterSecret(groupId) + kotlin.test.assertContentEquals(aliceKey, bobKey, "Bob must share Alice's epoch-2 key") + kotlin.test.assertContentEquals(aliceKey, carolKey, "Carol must share Alice's epoch-2 key") + + val bobMlsBytes = GroupEventEncryption.decrypt(appEvent.signedEvent.content, bobKey) + val bobDecrypted = bobMgr.decrypt(groupId, bobMlsBytes) + assertEquals(msg, bobDecrypted.content.decodeToString()) + + val carolMlsBytes = GroupEventEncryption.decrypt(appEvent.signedEvent.content, carolKey) + val carolDecrypted = carolMgr.decrypt(groupId, carolMlsBytes) + assertEquals(msg, carolDecrypted.content.decodeToString()) + } + } + @Test fun testFullRoundtripEncryptDecrypt() { runBlocking {