fix(marmot): revert snapshot-based stage/merge; capture pre-commit key in CommitResult
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
This commit is contained in:
Claude
+20
-27
@@ -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
|
||||
}
|
||||
|
||||
|
||||
+14
-154
@@ -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.
|
||||
*
|
||||
|
||||
+14
-93
@@ -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<Extension>,
|
||||
): 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
|
||||
|
||||
-112
@@ -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<String, ByteArray>,
|
||||
val pendingProposals: List<PendingProposal>,
|
||||
val sentKeys: Map<Int, KeyNonceGeneration>,
|
||||
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
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
+57
-37
@@ -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())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user