laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-05 19:28:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(marmot): every member applies the commit that evicts a leaver, and it survives a restart

Browse Source 
Two defects, both found by `leaver-removal-secrecy` and both isolated with a
failing test first. Between them a departing member stayed in the tree —
holding the group's keys and reading everything sent after they left — while
the group believed the departure had been processed.

A peer's proposal must be REFERENCED, not inlined
--------------------------------------------------
`commit()` inlined every staged proposal, including ones another member
authored. An inline proposal carries no sender, so a receiver attributes it to
the committer. For a `SelfRemove` that is not cosmetic: the proposal means
"remove my leaf", so inlining someone else's says "remove the COMMITTER's
leaf". Now only our own proposals go inline; a peer's goes in by
`ProposalOrRef.Reference`, which resolves against the receiver's own pool where
their copy of the same standalone proposal already sits with the original
proposer's leaf index. `PendingProposal.authenticatedContentBytes` documented
this contract all along; the code did not implement it.

A path-less commit must contribute a ZERO commit secret
--------------------------------------------------------
`commit()` derives path secrets unconditionally — it needs them to build the
UpdatePath when there is one — and then keyed the commit secret on whether
those secrets existed rather than on whether the path was actually SENT. A
SelfRemove-only commit omits the path (RFC 9420 §12.4.1), so every receiver
used the zero vector while the committer used a derived one: different epoch
secrets, and every witness rejected the commit with a confirmation-tag
mismatch and fell an epoch behind. The same branch also overwrote
`pathPrivateKeys` with keys that were never published, discarding the ones that
could still decrypt commits addressed to our ancestors.

The pool is an obligation, so it is durable
--------------------------------------------
`MlsGroupState` gains `pendingProposals` (STATE_VERSION 4; older blobs decode
with an empty pool), and staging a peer's standalone proposal now persists the
group the way an epoch change does — it only mutated memory before. Losing the
pool to a restart does not lose a message, it loses the obligation: nobody is
left holding the proposal that evicts the leaver.

That also makes `stageCommit`'s explicit hand-off of the pool redundant, so it
goes back to the shared `stage` helper and `adoptPendingProposals` is removed.

`leaver-removal-secrecy` now replays instead of asserting its own divergence.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kCuA6tc4JQzHPCDd39GHq
This commit is contained in:
Claude
2026-09-10 13:22:23 +00:00
parent 5f36618767
commit 5fc9aa9f58
6 changed files with 247 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotLeaveProposalTest.kt
+110 -8
View File
@@ -27,6 +27,7 @@ import kotlinx.coroutines.runBlocking
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertIs
import kotlin.test.assertNotNull
import kotlin.test.assertTrue
/**
@@ -44,14 +45,18 @@ class MarmotLeaveProposalTest {
private class Fixture {
val signer = NostrSignerInternal(KeyPair())
val manager =
MarmotManager(
signer,
SnapshotStateStore(),
SnapshotMessageStore(),
SnapshotBundleStore(),
publisher = ACCEPTING_RELAY,
)
val mlsStore = SnapshotStateStore()
val messageStore = SnapshotMessageStore()
val bundleStore = SnapshotBundleStore()
var manager = build()
private fun build() = MarmotManager(signer, mlsStore, messageStore, bundleStore, publisher = ACCEPTING_RELAY)
/** Drop the process and come back over the same durable stores. */
suspend fun restart() {
manager = build()
manager.restoreAll()
}
}
@Test
@@ -96,4 +101,101 @@ class MarmotLeaveProposalTest {
"the group's keys and keep reading everything sent after they left",
)
}
@Test
fun `every member applies the commit that evicts the leaver, not just the committer`() =
runBlocking {
// Three members, because the bug only shows with a WITNESS: alice
// commits carol's departure, and bob has to reach the same state
// from the commit alone.
val alice = Fixture()
val bob = Fixture()
val carol = Fixture()
alice.manager.createCurrentProfileGroup(
nostrGroupId = nostrGroupId,
relays = listOf("wss://relay.invalid"),
profile = GroupProfileV1("departures", ""),
)
val (commit, welcomes) =
alice.manager.addMembers(
nostrGroupId,
listOf(
bob.manager.generateKeyPackageEvent(relays = emptyList()),
carol.manager.generateKeyPackageEvent(relays = emptyList()),
),
emptyList(),
)
welcomes.forEach { delivery ->
when (delivery.recipientPubKey) {
bob.signer.pubKey -> bob.manager.ingest(delivery.giftWrapEvent)
carol.signer.pubKey -> carol.manager.ingest(delivery.giftWrapEvent)
}
}
alice.manager.ingest(commit.signedEvent)
assertEquals(3, alice.manager.memberCount(nostrGroupId))
// Carol departs. Her proposal reaches everyone, as it does on the
// wire — it is published as its own group event.
val proposal = carol.manager.leaveGroup(nostrGroupId)
alice.manager.ingest(proposal.signedEvent)
bob.manager.ingest(proposal.signedEvent)
// Alice, the admin, commits it.
val eviction = alice.manager.commitPendingProposals(nostrGroupId, emptyList())
assertNotNull(eviction, "the staged SelfRemove must produce a commit")
assertEquals(2, alice.manager.memberCount(nostrGroupId))
// Bob applies that commit. He must land exactly where alice is.
bob.manager.ingest(eviction.signedEvent)
assertEquals(
alice.manager.groupEpoch(nostrGroupId),
bob.manager.groupEpoch(nostrGroupId),
"a witness that stays an epoch behind cannot read anything the group sends next",
)
assertEquals(2, bob.manager.memberCount(nostrGroupId))
// And the right person left. An inline SelfRemove is attributed to
// whoever committed it, so getting this wrong evicts the COMMITTER.
val remaining =
bob.manager
.memberPubkeys(nostrGroupId)
.map { it.pubkey }
.toSet()
assertEquals(setOf(alice.signer.pubKey, bob.signer.pubKey), remaining)
}
@Test
fun `a staged SelfRemove survives a restart`() =
runBlocking {
val alice = Fixture()
val bob = Fixture()
alice.manager.createCurrentProfileGroup(
nostrGroupId = nostrGroupId,
relays = listOf("wss://relay.invalid"),
profile = GroupProfileV1("durable departures", ""),
)
val kp = bob.manager.generateKeyPackageEvent(relays = emptyList())
val (commit, welcome) = alice.manager.addMember(nostrGroupId, kp, emptyList())
bob.manager.ingest(welcome!!.giftWrapEvent)
alice.manager.ingest(commit.signedEvent)
// Bob departs and alice stages his proposal — then alice's process
// dies before anyone commits it.
alice.manager.ingest(bob.manager.leaveGroup(nostrGroupId).signedEvent)
assertTrue(alice.manager.groupManager.hasPendingProposals(nostrGroupId))
alice.restart()
// The obligation has to come back. Losing it is not losing a
// message — it leaves bob in the tree holding the group's keys,
// with nobody holding the proposal that evicts him.
assertTrue(
alice.manager.groupManager.hasPendingProposals(nostrGroupId),
"a staged SelfRemove must survive a restart, or the leaver never leaves",
)
assertNotNull(alice.manager.commitPendingProposals(nostrGroupId, emptyList()))
assertEquals(1, alice.manager.memberCount(nostrGroupId))
}
}
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/scenario/MarmotScenarioVectorTest.kt
+10 -23
View File
@@ -125,32 +125,19 @@ class MarmotScenarioVectorTest {
fun restartDeliveryFaults() = replay("restart-delivery-faults.v1.json")
/**
* `leaver-removal-secrecy` gets most of the way and stops at a narrower
* bug than the one it found.
* A leaver stops being able to read the group at the commit that evicts
* them — and every OTHER member applies that commit too.
*
* It surfaced that a departing member's `SelfRemove` was staged, committed,
* and then NOT applied — `MlsGroup.saveState()` does not carry the
* staged-proposal pool, so the staging clone committed an empty proposal
* list, advanced the epoch, and left the leaver in the tree with the keys.
* That is fixed (see `MarmotLeaveProposalTest`), and the committer now
* reaches the expected epoch and membership.
*
* What remains: a PEER that has the same proposal staged does not apply the
* commit carrying it inline — bob stays an epoch behind and cannot read
* what follows. Asserted rather than deleted so it stays visible; the day
* that is fixed this test fails and the vector moves up to [replay].
* This vector found two real defects. The staged-proposal pool did not
* travel with the group state, so the commit meant to evict the leaver
* carried an empty proposal list and left them in the tree with the keys.
* And a peer's proposal was inlined into that commit, which attributes it
* to the committer, while the committer derived a path-based commit secret
* for a commit that carries no path — so every witness rejected it and
* fell an epoch behind.
*/
@Test
fun aPeerWithTheSameProposalStagedStillMissesTheCommit() {
val thrown =
assertFailsWith<IllegalStateException> {
runBlocking { MarmotScenarioRunner(load("leaver-removal-secrecy.v1.json")).run() }
}
assertTrue(
thrown.message.orEmpty().contains("bob[default] epoch 2, expected 3"),
"the remaining divergence must still be the peer left behind: ${thrown.message}",
)
}
fun leaverRemovalSecrecy() = replay("leaver-removal-secrecy.v1.json")
/**
* `convergence-committer-selected` concludes with a `convergence_decision`
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt
+6 -4
View File
@@ -666,11 +666,13 @@ class MarmotInboundProcessor(
// without this every other member silently dropped the
// proposal and the admin's commit then failed with "Commit
// references unknown proposal" (marmot-interop test 15).
val group =
groupManager.getGroup(groupId)
?: return GroupEventResult.Error(groupId, "Group not found")
if (groupManager.getGroup(groupId) == null) {
return GroupEventResult.Error(groupId, "Group not found")
}
try {
group.receivePublicMessageProposal(pubMsg)
// Staged AND persisted: the pool is an obligation to
// commit, so it has to outlive this process.
groupManager.receiveStandaloneProposal(groupId, pubMsg)
GroupEventResult.ProposalStaged(groupId, pubMsg.sender.leafIndex)
} catch (e: Exception) {
GroupEventResult.Error(
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt
+46 -18
View File
@@ -187,21 +187,6 @@ class MlsGroup private constructor(
*/
fun hasPendingProposals(): Boolean = pendingProposals.isNotEmpty()
/**
* Replace this group's staged-proposal pool with [proposals].
*
* Exists for one reason: [saveState] does NOT serialize the pool, so a
* clone made for staging a commit starts empty, and `commit()` on it
* produces an EMPTY commit — the epoch advances and every proposal the
* commit was meant to apply is silently dropped. A departing member's
* `SelfRemove` is the case that bites: the group looks like it processed
* the departure, and the leaver is still in the tree holding the keys.
*/
internal fun adoptPendingProposals(proposals: List<PendingProposal>) {
pendingProposals.clear()
pendingProposals.addAll(proposals)
}
/**
* The GroupContext extension list as it stands. Test-only: callers
* that want the dictionary should use [appDataDictionary], which
@@ -337,6 +322,11 @@ class MlsGroup private constructor(
// key+nonce within this epoch (RFC 9420 §9).
senderRatchetStates = secretTree.exportSenderStates(),
pathPrivateKeys = pathPrivateKeys.toMap(),
// A staged proposal is an obligation, not a message: a departing
// member's SelfRemove sits here until someone commits it, and a
// restart that forgot it would leave the leaver in the tree with
// the group's keys and nobody holding the proposal to evict them.
pendingProposals = pendingProposals.toList(),
)
}
@@ -668,7 +658,27 @@ class MlsGroup private constructor(
// `ValidationError(InvalidMembershipTag)`.
val preCommitExtensions = groupContext.extensions
val proposalOrRefs = proposals.map { ProposalOrRef.Inline(it.proposal) }
// Inline only what WE authored. A proposal from another member has to
// go in by REFERENCE, because an inline proposal carries no sender: a
// receiving peer attributes it to the committer (see the
// `ProposalOrRef.Inline` branch of `processCommitInner`). For a
// `SelfRemove` that is not a cosmetic difference — the proposal means
// "remove my leaf", so inlining someone else's says "remove the
// committer's leaf", and every witness either evicts the wrong member
// or refuses the commit outright and falls an epoch behind.
//
// A reference resolves against the receiver's own pending pool, which
// is where their copy of the same standalone proposal already sits,
// carrying the ORIGINAL proposer's leaf index.
val proposalOrRefs =
proposals.map { pending ->
if (pending.senderLeafIndex == myLeafIndex) {
ProposalOrRef.Inline(pending.proposal)
} else {
val refValue = pending.authenticatedContentBytes ?: pending.proposal.toTlsBytes()
ProposalOrRef.Reference(MlsCryptoProvider.refHash("MLS 1.0 Proposal Reference", refValue))
}
}
// Check if we need an UpdatePath. RFC 9420 §12.4.1: the path value
// MUST be populated if the proposal list is empty (pure forward-
@@ -707,7 +717,13 @@ class MlsGroup private constructor(
// We just minted the keys for our whole direct path. Keep the private
// halves: the next committer will address us at one of these nodes,
// not at our leaf, as soon as our subtree is merged.
run {
//
// ONLY when this commit actually carries the path. A commit that omits
// the UpdatePath never publishes these public halves, so the tree keeps
// the old keys and a peer still encrypts to those — storing the fresh
// private halves here would overwrite the ones that can actually
// decrypt the next commit addressed to our ancestors.
if (needsPath && pathSecrets.isNotEmpty()) {
val fullPath = BinaryTree.directPath(myLeafIndex, tree.leafCount)
pathPrivateKeys.keys.retainAll(fullPath.toSet())
for ((i, nodeIdx) in fullPath.withIndex()) {
@@ -890,8 +906,19 @@ class MlsGroup private constructor(
// encryption-key seed rather than the key-schedule contribution. That
// one-step gap silently diverged the two sides' epoch_secret and made
// every cross-impl commit fail `ConfirmationTagMismatch`.
//
// Keyed on whether the commit CARRIES a path, not on whether we happened
// to derive path secrets. RFC 9420 §12.4.2: a commit with no
// `update_path` contributes a zero commit_secret, which is exactly what
// every receiver uses (see the `commit.updatePath != null` branch of
// `processCommitInner`). We derive `pathSecrets` unconditionally to
// build the path when it is needed; using them for the key schedule
// when the path was OMITTED gives the committer an epoch secret nobody
// else can reach, and every member rejects the commit with a
// confirmation-tag mismatch. A SelfRemove-only commit — a departing
// member's eviction — is precisely the case that omits the path.
val commitSecret =
if (pathSecrets.isNotEmpty()) {
if (updatePath != null && pathSecrets.isNotEmpty()) {
MlsCryptoProvider.deriveSecret(pathSecrets.last().pathSecret, "path")
} else {
ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH)
@@ -4133,6 +4160,7 @@ class MlsGroup private constructor(
encryptionPrivateKey = state.encryptionPrivateKey,
interimTranscriptHash = state.interimTranscriptHash,
pathPrivateKeys = state.pathPrivateKeys.toMutableMap(),
pendingProposals = state.pendingProposals.toMutableList(),
)
}
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt
+24 -13
View File
@@ -23,6 +23,7 @@ package com.vitorpamplona.quartz.marmot.mls.group
import com.vitorpamplona.quartz.marmot.mls.codec.TlsReader
import com.vitorpamplona.quartz.marmot.mls.codec.TlsWriter
import com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider
import com.vitorpamplona.quartz.marmot.mls.framing.PublicMessage
import com.vitorpamplona.quartz.marmot.mls.group.MlsGroupManager.Companion.EPOCH_RETENTION_WINDOW
import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult
import com.vitorpamplona.quartz.marmot.mls.messages.ExternalJoinResult
@@ -468,6 +469,24 @@ class MlsGroupManager(
}
}
/**
* Stage a peer's standalone proposal and PERSIST the group.
*
* Staging alone only mutates memory, and a staged proposal is an
* obligation rather than a message: a departing member's `SelfRemove` sits
* in the pool until someone commits it. Losing it to a restart leaves the
* leaver in the tree, still holding the group's keys, with nobody holding
* the proposal that would evict them — so this writes through the same way
* an epoch change does.
*/
suspend fun receiveStandaloneProposal(
nostrGroupId: HexKey,
pubMsg: PublicMessage,
) = mutex.withLock {
requireGroup(nostrGroupId).receivePublicMessageProposal(pubMsg)
persistGroup(nostrGroupId)
}
/** Whether [nostrGroupId] has a staged proposal waiting for a Commit. */
fun hasPendingProposals(nostrGroupId: HexKey): Boolean = groups[nostrGroupId]?.hasPendingProposals() == true
@@ -476,20 +495,12 @@ class MlsGroupManager(
*
* Unlike every other `stage*` entry point this one has nothing of its own
* to propose — the proposals are already in the LIVE group's pool, put
* there by ingesting a peer's standalone proposal. [MlsGroup.saveState]
* does not carry that pool, so the clone must be handed it explicitly;
* without that this commits an empty proposal list, advances the epoch,
* and drops the very proposal it was called to apply.
* there by ingesting a peer's standalone proposal. That pool travels with
* [MlsGroup.saveState], so the clone inherits it; when it did not, this
* committed an empty proposal list, advanced the epoch, and dropped the
* very proposal it was called to apply.
*/
suspend fun stageCommit(nostrGroupId: HexKey): StagedCommit =
mutex.withLock {
val live = requireGroup(nostrGroupId)
val priorState = live.saveState()
val clone = MlsGroup.restore(priorState)
clone.adoptPendingProposals(live.pendingProposalsSnapshot())
val result = clone.commit()
StagedCommit(result, priorState, clone.saveState())
}
suspend fun stageCommit(nostrGroupId: HexKey): StagedCommit = stage(nostrGroupId) { it.commit() }
/**
* Process a received Commit, advancing the epoch.
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupState.kt
+51 -1
View File
@@ -23,6 +23,7 @@ package com.vitorpamplona.quartz.marmot.mls.group
import com.vitorpamplona.quartz.marmot.mls.codec.TlsReader
import com.vitorpamplona.quartz.marmot.mls.codec.TlsWriter
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.SenderRatchetState
@@ -71,6 +72,16 @@ data class MlsGroupState(
* across a restart makes the same group undecryptable on relaunch.
*/
val pathPrivateKeys: Map<Int, ByteArray> = emptyMap(),
/**
* Proposals staged but not yet committed (STATE_VERSION 4+).
*
* Mostly this pool holds a departing member's standalone `SelfRemove`,
* waiting for an authorized member to commit it. Dropping it on restart
* does not lose a message — it loses the OBLIGATION: the leaver stays in
* the tree, still holding the group's keys, and nobody is left holding
* the proposal that would evict them.
*/
val pendingProposals: List<PendingProposal> = emptyList(),
) {
fun encodeTls(): ByteArray {
val writer = TlsWriter()
@@ -133,6 +144,17 @@ data class MlsGroupState(
writer.putOpaqueVarInt(key)
}
// Staged proposals (STATE_VERSION 4+). The AuthenticatedContent bytes
// travel with each entry because they, not the bare proposal, are what
// a later `ProposalRef` hashes (RFC 9420 §5.2) — a restored pool that
// lost them could no longer be matched by a commit that references it.
writer.putUint32(pendingProposals.size.toLong())
for (pending in pendingProposals) {
writer.putUint32(pending.senderLeafIndex.toLong())
writer.putOpaqueVarInt(pending.proposal.toTlsBytes())
writer.putOpaqueVarInt(pending.authenticatedContentBytes ?: ByteArray(0))
}
return writer.toByteArray()
}
@@ -156,8 +178,11 @@ data class MlsGroupState(
* v3: appends [pathPrivateKeys] so a restore can still decrypt an
* UpdatePath addressed at one of our ancestors. Older blobs decode
* with an empty map and refill on the next commit we process.
* v4: appends [pendingProposals] so a departing member's staged
* `SelfRemove` survives a restart instead of leaving them in the
* tree. Older blobs decode with an empty pool.
*/
private const val STATE_VERSION = 3
private const val STATE_VERSION = 4
fun decodeTls(data: ByteArray): MlsGroupState {
val reader = TlsReader(data)
@@ -234,6 +259,30 @@ data class MlsGroupState(
emptyMap()
}
// v4+: proposals staged and not yet committed. Absent for older
// blobs, which restore with an empty pool — the same behaviour
// every version before this one had.
val pendingProposals =
if (version >= 4 && reader.hasRemaining) {
val count = reader.readUint32().toInt()
buildList {
repeat(count) {
val senderLeafIndex = reader.readUint32().toInt()
val proposal = Proposal.decodeTls(TlsReader(reader.readOpaqueVarInt()))
val authenticatedContentBytes = reader.readOpaqueVarInt()
add(
PendingProposal(
proposal = proposal,
senderLeafIndex = senderLeafIndex,
authenticatedContentBytes = authenticatedContentBytes.takeIf { it.isNotEmpty() },
),
)
}
}
} else {
emptyList()
}
return MlsGroupState(
groupContext = groupContext,
treeBytes = treeBytes,
@@ -246,6 +295,7 @@ data class MlsGroupState(
encryptionSecret = encryptionSecret,
senderRatchetStates = senderRatchetStates,
pathPrivateKeys = pathPrivateKeys,
pendingProposals = pendingProposals,
)
}
}