fix(marmot): outer-encrypt commits with pre-commit exporter secret
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
This commit is contained in:
+41
-10
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user