fix(marmot): populate UpdatePath on empty commits + carry pending signing key
Two bugs that surfaced together once FramedContentTBS signing started being checked at decrypt time: 1. Empty-proposal commits (pure forward-secrecy / self-update) were skipping the UpdatePath entirely, so the sender's commit_secret fell back to an all-zero vector while a spec-compliant receiver would derive a real commit_secret from the path — diverging the epoch key schedule on the very next message. Per RFC 9420 §12.4.1 the path value MUST be populated when the proposal list is empty. 2. When an Update proposal rotates our own signing key, commit() rebuilds the committer leaf for the UpdatePath and signs it with signingPrivateKey (the PRE-rotation key) before the post-commit promotion step swaps signingPrivateKey for pendingSigningKey. The receiver then stores our leaf with the pre-rotation signature_key, while we start minting FramedContentTBS signatures with the new key — every subsequent decrypt fails signature verification. Use pendingSigningKey when present to seal the UpdatePath leaf. Un-Ignore the two empty-commit / multi-epoch tests that documented this divergence (one in MlsGroupLifecycleTest, one in MlsGroupEdgeCaseTest). Both pass now, as does the existing signing- key rotation cross-member round-trip test. https://claude.ai/code/session_01HfHdd5S5rvxUW2ihEpLGJr
This commit is contained in:
Claude
@@ -413,8 +413,15 @@ class MlsGroup private constructor(
|
||||
|
||||
val proposalOrRefs = proposals.map { ProposalOrRef.Inline(it.proposal) }
|
||||
|
||||
// Check if we need an UpdatePath (required unless only SelfRemove)
|
||||
val needsPath = proposals.any { it.proposal !is Proposal.SelfRemove }
|
||||
// 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-
|
||||
// secrecy / self-update commit) or if it contains any Update or
|
||||
// Remove proposal. A commit whose only non-SelfRemove proposals are
|
||||
// Adds MAY omit the path — we still include one for extra forward
|
||||
// secrecy, which is spec-compliant.
|
||||
val needsPath =
|
||||
proposals.isEmpty() ||
|
||||
proposals.any { it.proposal !is Proposal.SelfRemove }
|
||||
|
||||
// Apply proposals to tree FIRST (RFC 9420 Section 12.4.2)
|
||||
// Order: Updates/Removes first, then Adds (so blank slots are freed before reuse)
|
||||
@@ -468,16 +475,23 @@ class MlsGroup private constructor(
|
||||
UpdatePathNode(pathKey.publicKey, encryptedSecrets)
|
||||
}
|
||||
|
||||
// If a signing-key rotation is pending (from
|
||||
// proposeSigningKeyRotation), the UpdatePath's leaf must be
|
||||
// sealed with the NEW signing identity — otherwise the
|
||||
// receiver's copy of our leaf keeps the pre-rotation
|
||||
// signature_key and every post-commit FramedContentTBS
|
||||
// signature we mint fails to verify.
|
||||
val effectiveSigningKey = pendingSigningKey ?: signingPrivateKey
|
||||
val newEncKp = X25519.generateKeyPair()
|
||||
val newLeafNode =
|
||||
buildLeafNode(
|
||||
encryptionKey = newEncKp.publicKey,
|
||||
signatureKey = Ed25519.publicFromPrivate(signingPrivateKey),
|
||||
signatureKey = Ed25519.publicFromPrivate(effectiveSigningKey),
|
||||
identity =
|
||||
(tree.getLeaf(myLeafIndex)?.credential as? Credential.Basic)?.identity
|
||||
?: ByteArray(0),
|
||||
source = LeafNodeSource.COMMIT,
|
||||
signingKey = signingPrivateKey,
|
||||
signingKey = effectiveSigningKey,
|
||||
groupId = groupId,
|
||||
leafIndex = myLeafIndex,
|
||||
)
|
||||
|
||||
-7
@@ -22,7 +22,6 @@ package com.vitorpamplona.quartz.marmot.mls
|
||||
|
||||
import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup
|
||||
import com.vitorpamplona.quartz.marmot.mls.messages.KeyPackageBundle
|
||||
import kotlin.test.Ignore
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertContentEquals
|
||||
import kotlin.test.assertEquals
|
||||
@@ -225,12 +224,6 @@ class MlsGroupEdgeCaseTest {
|
||||
// 6. Multiple epochs of encrypt/decrypt
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
// BUG: same empty-commit (no proposals, pure UpdatePath) divergence as
|
||||
// MlsGroupLifecycleTest.testEmptyCommit_AdvancesEpoch. The
|
||||
// SecretTree.getNodeSecret non-full-tree fix doesn't address this — after
|
||||
// 5 empty commits Alice and Bob's ratchet secrets diverge and AEAD
|
||||
// decryption fails with "Tag mismatch". Tracked separately.
|
||||
@Ignore
|
||||
@Test
|
||||
fun testMultipleEpochTransitions_EncryptDecryptStillWorks() {
|
||||
val alice = MlsGroup.create("alice".encodeToByteArray())
|
||||
|
||||
-7
@@ -22,7 +22,6 @@ package com.vitorpamplona.quartz.marmot.mls
|
||||
|
||||
import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup
|
||||
import com.vitorpamplona.quartz.marmot.mls.messages.KeyPackageBundle
|
||||
import kotlin.test.Ignore
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertContentEquals
|
||||
import kotlin.test.assertEquals
|
||||
@@ -466,12 +465,6 @@ class MlsGroupLifecycleTest {
|
||||
// 12. Empty commit (no proposals, just UpdatePath for forward secrecy)
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
// BUG: empty-commit (no proposals, pure UpdatePath) diverges Alice's and
|
||||
// Bob's exporter secrets after processCommit. Unrelated to the SecretTree
|
||||
// non-full-tree fix — the other @Ignored "processCommit diverges" tests in
|
||||
// this file and MlsGroupEdgeCaseTest now pass, but this one still fails.
|
||||
// Tracked separately.
|
||||
@Ignore
|
||||
@Test
|
||||
fun testEmptyCommit_AdvancesEpoch() {
|
||||
val alice = MlsGroup.create("alice".encodeToByteArray())
|
||||
|
||||
Reference in New Issue
Block a user