From b039543807357893d4251b58b08e4214f5cc412b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Apr 2026 00:34:07 +0000 Subject: [PATCH] fix(marmot): revert snapshot-based stage/merge; capture pre-commit key in CommitResult MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The snapshot/restore machinery added in the previous commit (ca988b7) to implement MDK-style `stageCommit` + `mergeStagedCommit` introduced a tree-rebuild path (RatchetTree.decodeTls followed by a fresh SecretTree construction) that, in production, caused a recursive StackOverflowError in `SecretTree.getNodeSecret` during encryption after a second add-member cycle. The unit test flow did not exercise it and couldn't reproduce. Keep the underlying fix — outer-encrypt outbound kind:445 commits with the PRE-commit (epoch-N) exporter secret — but deliver it with far less machinery: - `CommitResult.preCommitExporterSecret` now carries the `MLS-Exporter("marmot", "group-event", 32)` value captured inside `MlsGroup.commit()` BEFORE any state mutation. It's the key existing members still at epoch N hold, so the outer kind:445 wrap with it is decryptable to them (RFC 9420 §12.4 + MDK parity). - `MlsGroup.commit()` captures this exporter once at the top of the function, threads it into the returned `CommitResult`. - `MarmotManager.addMember / removeMember / updateGroupMetadata` read `commitResult.preCommitExporterSecret` and pass it to `MarmotOutboundProcessor.buildCommitEvent(..., exporterKey=...)`. The eager `group.addMember` / etc. continue to advance the local epoch immediately — the same behavior as before the previous commit, which was proven safe under production load. Removed: - `MlsGroup.stageCommit` / `mergeStagedCommit` / `stageAddMember` / `stageRemoveMember` and their snapshot/restore helpers. - `MlsGroupSnapshot` / `StagedCommit` types. - `MlsGroupManager.stageAddMember` / `stageRemoveMember` / `stageRotateSigningKey` / `stageUpdateGroupExtensions` / `mergeStagedCommit`. - `tree` back to `val` in `MlsGroup`. Kept from the previous commit: - `MarmotOutboundProcessor.buildCommitEvent(... exporterKey: ByteArray?)` with default falling back to `groupManager.exporterSecret(...)`. - `MlsGroupManager.processCommit` retains epoch secrets AFTER successful apply (was a secondary bug polluting the retention window with duplicates of the current key when a commit failed). - `pushRetainedEpoch` helper that accepts a pre-captured RetainedEpochSecrets, used throughout instead of the old `retainEpochSecrets(nostrGroupId, group)`. Test updated: - `testCreatorCanAddTwoMembersAndAllDecryptFollowingMessage` now uses the eager `addMember` path and asserts that the member still at the old epoch can outer-decrypt with the pre-commit key shipped back via `CommitResult`. Also decrypts Alice's self-echo and sends multiple follow-up messages — this is the scenario that triggered the production stack overflow before the revert. - `testAddMemberExposesPreCommitExporterSecret` (renamed) confirms `CommitResult.preCommitExporterSecret` equals the exporter the group had immediately before the call, and that the post-commit exporter differs from it (proves we actually rotated). https://claude.ai/code/session_014zfdNeeKAfU1zGGyFw4bUL --- .../amethyst/commons/marmot/MarmotManager.kt | 47 +++-- .../quartz/marmot/mls/group/MlsGroup.kt | 168 ++---------------- .../marmot/mls/group/MlsGroupManager.kt | 107 ++--------- .../quartz/marmot/mls/group/StagedCommit.kt | 112 ------------ .../quartz/marmot/mls/messages/Commit.kt | 12 ++ .../quartz/marmot/MarmotPipelineTest.kt | 94 ++++++---- 6 files changed, 117 insertions(+), 423 deletions(-) delete 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 635ae97ea..1e62a6efc 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,39 +178,34 @@ class MarmotManager( "KeyPackage credential identity does not match memberPubKey" } - // 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) + // Per RFC 9420 §12.4 (and MDK), the outbound kind:445 MUST be + // outer-encrypted with the pre-commit (epoch-N) exporter secret so + // that other existing members still at epoch N can decrypt and + // process the commit. CommitResult.preCommitExporterSecret carries + // that key; the local group state has already advanced to N+1 by + // the time addMember returns, so we can't read it from the group + // any more. + val commitResult = groupManager.addMember(nostrGroupId, keyPackageBytes) val commitEvent = outboundProcessor.buildCommitEvent( nostrGroupId = nostrGroupId, - commitBytes = staged.framedCommitBytes, - exporterKey = staged.preCommitExporterSecret, + commitBytes = commitResult.framedCommitBytes, + exporterKey = commitResult.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. + // epoch we've already merged. inboundProcessor.markEventProcessed(commitEvent.signedEvent.id) - val welcomeBytes = - staged.welcomeBytes - ?: throw IllegalStateException( - "stageAddMember did not produce a Welcome for $memberPubKey", - ) val welcomeDelivery = - welcomeSender.wrapWelcomeBytes( - welcomeBytes = welcomeBytes, + welcomeSender.wrapWelcome( + commitResult = commitResult, 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) } @@ -286,15 +281,14 @@ class MarmotManager( nostrGroupId: HexKey, targetLeafIndex: Int, ): OutboundGroupEvent { - val staged = groupManager.stageRemoveMember(nostrGroupId, targetLeafIndex) + val commitResult = groupManager.removeMember(nostrGroupId, targetLeafIndex) val commitEvent = outboundProcessor.buildCommitEvent( nostrGroupId = nostrGroupId, - commitBytes = staged.framedCommitBytes, - exporterKey = staged.preCommitExporterSecret, + commitBytes = commitResult.framedCommitBytes, + exporterKey = commitResult.preCommitExporterSecret, ) inboundProcessor.markEventProcessed(commitEvent.signedEvent.id) - groupManager.mergeStagedCommit(nostrGroupId, staged) return commitEvent } @@ -306,16 +300,15 @@ class MarmotManager( nostrGroupId: HexKey, metadata: MarmotGroupData, ): OutboundGroupEvent { - val staged = - groupManager.stageUpdateGroupExtensions(nostrGroupId, listOf(metadata.toExtension())) + val commitResult = + groupManager.updateGroupExtensions(nostrGroupId, listOf(metadata.toExtension())) val commitEvent = outboundProcessor.buildCommitEvent( nostrGroupId = nostrGroupId, - commitBytes = staged.framedCommitBytes, - exporterKey = staged.preCommitExporterSecret, + commitBytes = commitResult.framedCommitBytes, + exporterKey = commitResult.preCommitExporterSecret, ) inboundProcessor.markEventProcessed(commitEvent.signedEvent.id) - groupManager.mergeStagedCommit(nostrGroupId, staged) return commitEvent } 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 9bd7f44a5..a8d99382e 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 var tree: RatchetTree, + private val tree: RatchetTree, private var myLeafIndex: Int, private var epochSecrets: EpochSecrets, private var secretTree: SecretTree, @@ -199,125 +199,6 @@ 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. */ @@ -522,6 +403,13 @@ class MlsGroup private constructor( // (i.e. no remaining member appears in the post-commit admin list). enforceNoAdminDepletion(proposals) + // Capture the pre-commit exporter secret BEFORE any mutation. + // Publishers of the outbound kind:445 MUST outer-encrypt with this + // key (epoch N) so that other existing members at epoch N can decrypt + // and process the commit. See CommitResult.preCommitExporterSecret. + val preCommitExporterSecret = + exporterSecret("marmot", "group-event".encodeToByteArray(), 32) + val proposalOrRefs = proposals.map { ProposalOrRef.Inline(it.proposal) } // Check if we need an UpdatePath (required unless only SelfRemove) @@ -682,6 +570,7 @@ class MlsGroup private constructor( welcomeBytes = welcomeBytes, groupInfoBytes = null, framedCommitBytes = framedCommitBytes, + preCommitExporterSecret = preCommitExporterSecret, ) } @@ -2190,54 +2079,25 @@ 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. + * Creates and applies a Commit with an Add proposal. The resulting + * [CommitResult.preCommitExporterSecret] is the key the outer kind:445 + * MUST be encrypted with (RFC 9420 §12.4 + MDK parity). */ 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. + * Creates and applies a Commit with a Remove proposal. See [addMember] + * for the pre-commit exporter key contract on the returned [CommitResult]. */ 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 bea9ca8ac..7fddab22b 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 @@ -359,18 +359,16 @@ class MlsGroupManager( null } - // --- Member Management (atomic stage+merge; retained for tests/offline use) --- + // --- Member Management --- /** - * Add a member and create a Commit, advancing the local epoch immediately. + * Add a member and create a Commit. * - * **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. + * The returned [CommitResult.preCommitExporterSecret] is the key the + * outbound kind:445 MUST be outer-encrypted with (RFC 9420 §12.4 + MDK + * parity). The local group state advances to the new epoch before this + * function returns; publishers get one shot at the correct pre-commit + * key via the [CommitResult] payload. */ suspend fun addMember( nostrGroupId: HexKey, @@ -386,8 +384,8 @@ class MlsGroupManager( } /** - * Remove a member and create a Commit. See [addMember] for the production - * caveat — use [stageRemoveMember] + [mergeStagedCommit] on the wire. + * Remove a member and create a Commit. See [addMember] for the + * pre-commit exporter key contract on the returned [CommitResult]. */ suspend fun removeMember( nostrGroupId: HexKey, @@ -403,9 +401,10 @@ class MlsGroupManager( } /** - * Rotate the signing key within a group and commit. See [addMember] for - * the production caveat — use [stageRotateSigningKey] + [mergeStagedCommit] - * on the wire. + * Rotate the signing key within a group and commit. + * + * Per MIP-00, members SHOULD self-update within 24 hours of joining. + * See [addMember] for the pre-commit exporter key contract. */ suspend fun rotateSigningKey(nostrGroupId: HexKey): CommitResult = mutex.withLock { @@ -420,10 +419,7 @@ class MlsGroupManager( /** * Update group extensions (e.g., MIP-01 metadata) via a GroupContextExtensions proposal. - * Creates the proposal, commits it, and persists the new state. - * - * See [addMember] for the production caveat — use [stageUpdateGroupExtensions] - * + [mergeStagedCommit] on the wire. + * See [addMember] for the pre-commit exporter key contract. */ suspend fun updateGroupExtensions( nostrGroupId: HexKey, @@ -444,81 +440,6 @@ class MlsGroupManager( 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 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 deleted file mode 100644 index f8aa53f0e..000000000 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/StagedCommit.kt +++ /dev/null @@ -1,112 +0,0 @@ -/* - * 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/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Commit.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Commit.kt index a0b4d5d9d..624dd8fff 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Commit.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Commit.kt @@ -107,6 +107,18 @@ data class CommitResult( * Wire format: MlsMessage(version=mls10, wireFormat=mls_public_message, payload=PublicMessage(...)). */ val framedCommitBytes: ByteArray = commitBytes, + /** + * `MLS-Exporter("marmot", "group-event", 32)` evaluated at the **pre-commit** + * epoch (N) — the key the group had when this commit was computed, before + * local state advanced to N+1. Publishers of the kind:445 MUST ChaCha20-wrap + * the commit with this key (RFC 9420 §12.4 and MDK parity). Using the + * post-commit (N+1) key makes the commit unreadable to existing members + * still at epoch N — the exact scenario that caused Eden to stall at + * epoch 1 and never decrypt any of David's subsequent messages. + * + * Empty by default for the test-only entry points that don't need it. + */ + val preCommitExporterSecret: ByteArray = ByteArray(0), ) { override fun equals(other: Any?): Boolean { if (this === other) return true 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 cdddf5da1..1aae80232 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt @@ -429,14 +429,13 @@ 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. + fun testAddMemberExposesPreCommitExporterSecret() { + // Regression: before the fix, MarmotManager.addMember outer-encrypted + // the kind:445 commit with the POST-commit (epoch N+1) exporter key, + // which meant existing members at epoch N couldn't decrypt it. The + // fix captures the pre-commit key inside MlsGroup.commit() and ships + // it back via CommitResult.preCommitExporterSecret so MarmotManager + // can pass it to the outbound builder. runBlocking { val manager = createGroupManager() manager.createGroup(groupId, "alice".encodeToByteArray()) @@ -448,22 +447,24 @@ class MarmotPipelineTest { manager .getGroup(groupId)!! .createKeyPackage("bob".encodeToByteArray(), ByteArray(0)) - val staged = manager.stageAddMember(groupId, bobBundle.keyPackage.toTlsBytes()) + val result = manager.addMember(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. + // Local state advanced to N+1. + assertEquals(preEpoch + 1, manager.getGroup(groupId)!!.epoch) + // But the returned pre-commit exporter secret matches what the + // group held BEFORE advancing — the key existing members still at + // epoch N are holding. kotlin.test.assertContentEquals( preEpochExporter, - staged.preCommitExporterSecret, + result.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) + // The post-commit exporter (what groupManager.exporterSecret now + // returns) MUST differ — otherwise we haven't actually rotated. + kotlin.test.assertFalse( + manager.exporterSecret(groupId).contentEquals(preEpochExporter), + "post-commit exporter key must differ from the pre-commit one", + ) + assertNotNull(result.welcomeBytes) } } @@ -509,33 +510,35 @@ class MarmotPipelineTest { val outbound = MarmotOutboundProcessor(aliceMgr) - // --- Step 1: Alice adds Bob via stage/merge --- - val stagedBob = aliceMgr.stageAddMember(groupId, bobBundle.keyPackage.toTlsBytes()) + // --- Step 1: Alice adds Bob. CommitResult carries the pre-commit + // exporter secret so buildCommitEvent can outer-encrypt with + // the epoch-N (pre-Bob) key that Bob-less Alice held. --- + val addBobResult = aliceMgr.addMember(groupId, bobBundle.keyPackage.toTlsBytes()) val addBobCommitEvent = outbound.buildCommitEvent( nostrGroupId = groupId, - commitBytes = stagedBob.framedCommitBytes, - exporterKey = stagedBob.preCommitExporterSecret, + commitBytes = addBobResult.framedCommitBytes, + exporterKey = addBobResult.preCommitExporterSecret, ) // Bob joins via Welcome (emulated: we hand him the Welcome bytes). - bobMgr.processWelcome(stagedBob.welcomeBytes!!, bobBundle) + bobMgr.processWelcome(addBobResult.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()) + // --- Step 2: Alice adds Carol. addCarolResult.preCommitExporterSecret + // is the epoch-N (2-member) key that Bob still holds. --- + val addCarolResult = aliceMgr.addMember(groupId, carolBundle.keyPackage.toTlsBytes()) val addCarolCommitEvent = outbound.buildCommitEvent( nostrGroupId = groupId, - commitBytes = stagedCarol.framedCommitBytes, - exporterKey = stagedCarol.preCommitExporterSecret, + commitBytes = addCarolResult.framedCommitBytes, + exporterKey = addCarolResult.preCommitExporterSecret, ) - // Carol joins via Welcome at epoch 2. - carolMgr.processWelcome(stagedCarol.welcomeBytes!!, carolBundle) + // Carol joins via Welcome at the new epoch. + carolMgr.processWelcome(addCarolResult.welcomeBytes!!, carolBundle) // *** The key assertion ***: Bob (still at epoch 1) receives the // add-Carol kind:445 and must be able to: @@ -545,10 +548,10 @@ class MarmotPipelineTest { 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.", + addCarolResult.preCommitExporterSecret, + "Bob's current exporter (pre-add-Carol) 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 = @@ -575,7 +578,6 @@ class MarmotPipelineTest { confirmationTag = publicMessage.confirmationTag!!, ) - aliceMgr.mergeStagedCommit(groupId, stagedCarol) val epochAfterAddCarol = aliceMgr.getGroup(groupId)!!.epoch assertEquals(epochAfterAddBob + 1, epochAfterAddCarol) assertEquals(epochAfterAddCarol, bobMgr.getGroup(groupId)!!.epoch) @@ -599,6 +601,24 @@ class MarmotPipelineTest { val carolMlsBytes = GroupEventEncryption.decrypt(appEvent.signedEvent.content, carolKey) val carolDecrypted = carolMgr.decrypt(groupId, carolMlsBytes) assertEquals(msg, carolDecrypted.content.decodeToString()) + + // --- Alice also receives her own echo (as the app does via the + // relay round-trip); this exercises the secretTree-based decrypt + // path with myLeafIndex != sender or when sentKeys misses. + val aliceMlsBytes = GroupEventEncryption.decrypt(appEvent.signedEvent.content, aliceKey) + val aliceDecrypted = aliceMgr.decrypt(groupId, aliceMlsBytes) + assertEquals(msg, aliceDecrypted.content.decodeToString()) + + // Send a second and third message to stress the secretTree ratchet + // and ensure getNodeSecret stays terminating across generations. + for (i in 1..3) { + val m = "msg-$i" + val ev = outbound.buildGroupEventFromBytes(groupId, m.encodeToByteArray()) + val keyNow = aliceMgr.exporterSecret(groupId) + val bytes = GroupEventEncryption.decrypt(ev.signedEvent.content, keyNow) + val dec = bobMgr.decrypt(groupId, bytes) + assertEquals(m, dec.content.decodeToString()) + } } }