From c74bbb85109ebe6d296087f9f966b7e983ccad8d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Apr 2026 23:56:47 +0000 Subject: [PATCH 1/9] test(marmot-interop): unwrap newer whitenoise-rs `groups show` shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whitenoise-rs ≥ v0.2.x nests the group object one level deeper — `{"result": {"group": {...}}}` instead of `{"result": {...}}`. The two metadata-name pollers in tests 07 and 10 still asked for `.result.name` and silently saw the empty string, so even when wn-side `groups show` returned the correct new name the polling loops timed out: 07 metadata rename fail B saw name="" not "Interop-02-renamed" 10 concurrent commits fail diverged: A sees "race-from-amethyst", B sees "" Both queries now also peel a `.group` wrapper when present, leaving the older bare-`result` shape working too. Re-running the headless harness flips 07 + 10 from fail → pass; the remaining failures (09, 14, 15) are unrelated MLS-encryption / Remove-commit issues that need their own fix. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- cli/tests/marmot/tests-extras.sh | 5 ++++- cli/tests/marmot/tests-manage.sh | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/cli/tests/marmot/tests-extras.sh b/cli/tests/marmot/tests-extras.sh index d61e9a733..fa602e656 100644 --- a/cli/tests/marmot/tests-extras.sh +++ b/cli/tests/marmot/tests-extras.sh @@ -89,7 +89,10 @@ test_10_concurrent_commits() { sleep 10 local b_name - b_name=$(wn_b --json groups show "$mls_gid" 2>/dev/null | jq -r '(.result // .) | .name // empty') + # whitenoise-rs ≥ v0.2.x wraps the group payload one level deeper as + # `{"result": {"group": {…name…}}}`; the older shape was a bare group + # object under `.result`. Accept both. + b_name=$(wn_b --json groups show "$mls_gid" 2>/dev/null | jq -r '(.result // .) | (.group // .) | .name // empty') local a_name a_name=$(amy_field '.name' marmot group show "$gid" 2>/dev/null || echo "") diff --git a/cli/tests/marmot/tests-manage.sh b/cli/tests/marmot/tests-manage.sh index 7923811ab..03763d420 100644 --- a/cli/tests/marmot/tests-manage.sh +++ b/cli/tests/marmot/tests-manage.sh @@ -74,7 +74,11 @@ test_07_metadata_rename() { local deadline=$(( $(date +%s) + 120 )) seen="" while [[ $(date +%s) -lt $deadline ]]; do - seen=$(wn_b --json groups show "$mls_gid" 2>/dev/null | jq -r '(.result // .) | .name // empty') + # whitenoise-rs ≥ v0.2.x wraps the group payload one level deeper as + # `{"result": {"group": {…name…}}}`; older builds returned the bare + # group object under `.result`. Probe both shapes so the test survives + # either schema. + seen=$(wn_b --json groups show "$mls_gid" 2>/dev/null | jq -r '(.result // .) | (.group // .) | .name // empty') [[ "$seen" == "Interop-02-renamed" ]] && break sleep 3 done From 359b6069f1960e28d5b82b8f1767eed12595c9cc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 01:32:08 +0000 Subject: [PATCH 2/9] fix(marmot): handle being removed mid-commit without OOM (test 14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When wn admin-removes A in interop test 14, A processed the proposal locally — `tree.removeLeaf(A.leafIndex)` blanked her leaf and shrank `tree.leafCount` past `myLeafIndex` — and then immediately walked into BinaryTree.directPath(myLeafIndex, tree.leafCount) at MlsGroup.processCommitInner. With `myLeafIndex >= leafCount`, the left-balanced parent walk had no valid stopping point: each recursion step doubled the candidate parent index, integer-overflowed past 2^31, and either returned garbage or kept appending to the result list until the JVM OOM'd. The OOM bubbled up as a `runBlocking` failure that rolled back the whole commit — so A also stayed locally convinced she was still a member, and the test timed out waiting for `not_member`. Three layered fixes so the failure mode can't recur: * `BinaryTree.parent`/`directPath`/`copath` now `require` an in-range input. The root and any node ≥ nodeCount used to silently loop or return garbage; now they throw `IllegalArgumentException` immediately. This is defense-in-depth — a future caller passing an invalid index gets a stack trace at the boundary instead of an OOM five frames deep. * `MlsGroup.processCommitInner` short-circuits the path-decrypt + epoch advancement when the proposals in the commit just removed *us*. We preserve the proposal-side tree mutations so the caller (and the outer state machine) can observe that we're out, clear pending proposals + sent keys, and return. Without this short-circuit the function would derive a bogus all-zero `commit_secret`, fail `confirmation_tag` verification, throw, and roll the snapshot back — leaving us still convinced we were a live member. * `MlsGroupManager.isMember` and a new `MlsGroup.isLocalMember()` helper now return false once our leaf is null or past `leafCount`. The post-Remove group entry still lives in `groups` so callers can inspect the final tree, but every cli command (`group show`, `message send`, etc.) sees `not_member` and returns the right error. Also add a comprehensive `BinaryTreeTest` covering non-power-of-2 trees (3, 5, …, 32 leaves) and the boundary cases (root has no parent, leafIndex ≥ leafCount must throw). The pre-existing tests only exercised the 4-leaf example from RFC 9420 Appendix C; nothing hit the `parentInRange` branch, which is exactly where the OOM lived. Test 14's bash polling needed a small companion fix: it captured amy's stderr through `2>&1` and fed the result to `jq`, but the captured stream is interleaved with quartz `Log.d(…)` debug lines, so the JSON parse always failed. Switch to a `grep` for the `"error":"not_member"` literal — that signature only appears in the JSON payload and survives the debug noise. Also tee the captured output into `$LOG_FILE` so post-mortem logs include each polling iteration's `[cli] ingest …` traces. Marmot interop score: 11/16 → 14/16 (tests 9, 15 are unrelated MLS issues — see follow-up). https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- cli/tests/marmot/tests-extras.sh | 16 ++- .../quartz/marmot/mls/group/MlsGroup.kt | 32 +++++ .../marmot/mls/group/MlsGroupManager.kt | 13 +- .../quartz/marmot/mls/tree/BinaryTree.kt | 64 ++++++---- .../quartz/marmot/mls/BinaryTreeTest.kt | 118 ++++++++++++++++++ 5 files changed, 218 insertions(+), 25 deletions(-) diff --git a/cli/tests/marmot/tests-extras.sh b/cli/tests/marmot/tests-extras.sh index fa602e656..dd4e03e3d 100644 --- a/cli/tests/marmot/tests-extras.sh +++ b/cli/tests/marmot/tests-extras.sh @@ -240,12 +240,24 @@ test_14_wn_removes_a() { # Amy should observe that she's no longer a member. `group show` returns # a not_member error as soon as the Remove commit is applied locally. + # + # The amy CLI contract puts error JSON on stderr (README: "stdout is + # JSON … stderr is for humans"), but stderr is interleaved with debug + # logs from `Log.d(…)` calls in quartz, so feeding the captured stream + # straight into `jq` fails to parse. Capture stderr alongside stdout + # via `2>&1` and grep for the `"error":"not_member"` literal — that + # signature is unambiguous (the debug log lines never produce JSON). + # Also tee the per-iteration capture into $LOG_FILE so post-mortem + # logs include each polling sync's `[cli] ingest …` trace, mirroring + # what amy_json normally writes when stderr isn't being captured. local deadline=$(( $(date +%s) + 120 )) removed=0 while [[ $(date +%s) -lt $deadline ]]; do local show rc - show=$(amy_json marmot group show "$a_gid" 2>&1) + show=$("$AMY_BIN" --data-dir "$A_DIR" --secret-backend plaintext \ + marmot group show "$a_gid" 2>&1) rc=$? - if [[ $rc -ne 0 ]] && printf '%s' "$show" | jq -e '.error == "not_member"' >/dev/null 2>&1; then + printf '%s\n' "$show" >>"$LOG_FILE" + if [[ $rc -ne 0 ]] && printf '%s' "$show" | grep -qE '"error":[[:space:]]*"not_member"'; then removed=1; break fi sleep 3 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 1174e9910..56f522d6a 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 @@ -133,6 +133,18 @@ class MlsGroup private constructor( val leafIndex: Int get() = myLeafIndex val extensions: List get() = groupContext.extensions + /** + * True while our own leaf is still live in the tree. + * + * After `processCommit` applies a Remove proposal that targets us, the + * group is intentionally kept in the manager so callers can inspect the + * final tree state — but our leaf is null and `leafCount` may have + * shrunk past `myLeafIndex`. Either condition means we can't propose, + * commit, encrypt, or decrypt any further; surface that here so callers + * get a clean answer instead of poking at private tree internals. + */ + fun isLocalMember(): Boolean = myLeafIndex < tree.leafCount && tree.getLeaf(myLeafIndex) != null + // --- Marmot admin helpers (MIP-01 / MIP-03) --- /** Raw BasicCredential identity bytes of the member at the given leaf, or null. */ @@ -1272,6 +1284,26 @@ class MlsGroup private constructor( newLeavesInCommit.add(applyProposalAdd(add)) } + // If the proposals just removed *us*, there is no path-decrypt to do + // and no confirmation_tag to verify against our (now bogus) commit + // secret — we have no valid leaf and our copy of `commit_secret` + // would be all-zeros, so the keyschedule would diverge from every + // remaining member's. Without this short-circuit, the path-decrypt + // block calls `BinaryTree.directPath(myLeafIndex, tree.leafCount)` + // with an out-of-range index and OOMs the JVM (interop test 14). + // + // The proposals have already mutated the tree; preserve those + // mutations on return so the outer state machine knows we are out + // of the group. `pendingProposals` are cleared because they were + // tied to the previous epoch. + val removedSelf = + myLeafIndex >= tree.leafCount || tree.getLeaf(myLeafIndex) == null + if (removedSelf) { + pendingProposals.clear() + sentKeys.clear() + return + } + // Process UpdatePath var commitSecret = ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) if (commit.updatePath != null) { 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 500d43a3d..7c6d5cb09 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 @@ -158,9 +158,18 @@ class MlsGroupManager( fun activeGroupIds(): Set = groups.keys.toSet() /** - * Check if we are a member of the given group. + * Check if we are a (live) member of the given group. + * + * After processing a commit that removes us, the group entry stays in + * [groups] so callers can still inspect the post-Remove tree, but our + * own leaf has been set to null and the leaf count may have shrunk + * past `myLeafIndex`. Either condition means we cannot encrypt any + * more messages, propose anything, or decrypt future epochs — i.e. + * we are functionally not a member, and surface as such here so + * callers (`amy marmot group show`, the inbound processor) get a + * clean `not_member` answer instead of a misleading `true`. */ - fun isMember(nostrGroupId: HexKey): Boolean = groups.containsKey(nostrGroupId) + fun isMember(nostrGroupId: HexKey): Boolean = groups[nostrGroupId]?.isLocalMember() ?: false // --- Group Creation --- diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt index 2bfa316fd..a95ce22fe 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt @@ -88,34 +88,40 @@ object BinaryTree { /** * Parent of a node in a tree with [nodeCount] total nodes. * Uses the left-balanced tree parent formula per RFC 9420 Appendix C. + * + * The root and any index ≥ [nodeCount] have no defined parent — the + * left-balanced formula keeps walking up through virtual parents that + * never come back into range, integer-overflows after ~31 doublings, + * and either returns garbage or OOMs the call site that's storing the + * result. Both modes are signs of a tree-accounting bug upstream + * (e.g., calling directPath with a leafIndex past the post-Remove + * leafCount); fail loudly here so callers can't silently corrupt + * the rest of the commit-processing pipeline. */ fun parent( nodeIndex: Int, nodeCount: Int, ): Int { - // For left-balanced trees with non-power-of-2 leaves, - // the parent might be beyond nodeCount. In that case, - // the node's parent is the next valid ancestor. - val k = level(nodeIndex) - val b = (nodeIndex shr (k + 1)) and 1 - val p = - if (b == 0) { - nodeIndex + (1 shl k) - } else { - nodeIndex - (1 shl k) - } - - return if (p < nodeCount) { - p - } else { - // Parent is out of range. This node is at the rightmost - // edge of a non-full tree. Walk up through virtual parents - // until we find one within range. - parentInRange(p, nodeCount) + require(nodeCount > 0) { "nodeCount must be positive, got $nodeCount" } + require(nodeIndex in 0 until nodeCount) { + "nodeIndex $nodeIndex out of range [0, $nodeCount)" } + // The structural root of a left-balanced tree with `leafCount` + // leaves sits at (1 << ceil(log2(leafCount))) - 1, derivable from + // nodeCount via leafCount = (nodeCount + 1) / 2. + val leafCount = (nodeCount + 1) / 2 + require(nodeIndex != root(leafCount)) { + "nodeIndex $nodeIndex is the root of a $leafCount-leaf tree and has no parent" + } + return parentRec(nodeIndex, nodeCount) } - private fun parentInRange( + /** + * Inner walker that assumes [nodeIndex] has at least one valid parent + * within [nodeCount]. Public callers go through [parent] which performs + * the bounds check above. + */ + private fun parentRec( nodeIndex: Int, nodeCount: Int, ): Int { @@ -127,7 +133,7 @@ object BinaryTree { } else { nodeIndex - (1 shl k) } - return if (p < nodeCount) p else parentInRange(p, nodeCount) + return if (p < nodeCount) p else parentRec(p, nodeCount) } /** Root node index for a tree with [leafCount] leaves */ @@ -141,11 +147,20 @@ object BinaryTree { /** * Direct path from a leaf to the root (excluding the leaf itself). * These are the parent nodes along the path from leaf to root. + * + * [leafIndex] must be in `[0, leafCount)`. Calling with `leafIndex >= + * leafCount` is an upstream bug — most often a stale local index that + * was valid before the tree shrank under a Remove proposal — and used + * to OOM here because `parent()` would loop on the out-of-range start. */ fun directPath( leafIndex: Int, leafCount: Int, ): List { + require(leafCount > 0) { "leafCount must be positive, got $leafCount" } + require(leafIndex in 0 until leafCount) { + "leafIndex $leafIndex out of range [0, $leafCount)" + } val nodeIdx = leafToNode(leafIndex) val n = nodeCount(leafCount) val rootIdx = root(leafCount) @@ -162,11 +177,18 @@ object BinaryTree { /** * Copath of a leaf: the siblings of each node on the direct path. * The copath determines which nodes need to receive encrypted path secrets. + * + * Same range contract as [directPath]: `leafIndex` must be in + * `[0, leafCount)`. */ fun copath( leafIndex: Int, leafCount: Int, ): List { + require(leafCount > 0) { "leafCount must be positive, got $leafCount" } + require(leafIndex in 0 until leafCount) { + "leafIndex $leafIndex out of range [0, $leafCount)" + } val nodeIdx = leafToNode(leafIndex) val n = nodeCount(leafCount) val rootIdx = root(leafCount) diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt index f7b517312..95923a6c3 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.quartz.marmot.mls import com.vitorpamplona.quartz.marmot.mls.tree.BinaryTree import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFails import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -160,4 +161,121 @@ class BinaryTreeTest { // Subtree of leaf 0 is just [0] assertEquals(listOf(0), BinaryTree.subtreeLeaves(0, 4)) } + + // ----- Non-power-of-2 tree shapes ---------------------------------------- + // + // RFC 9420 Appendix C only worked example is the 4-leaf tree, but every + // group with ≠ a power-of-2 members exercises the parentInRange branch in + // BinaryTree.parent. Those branches were entirely uncovered before, and a + // bug there (infinite recursion / infinite loop in directPath) is what + // OOM'd amy when wn removed her in marmot-interop test 14. + + @Test + fun testRoot_nonPowerOfTwo() { + // root = (1 << ceil(log2(n))) - 1 + assertEquals(3, BinaryTree.root(3)) // ceil(log2(3))=2, 2^2-1 = 3 + assertEquals(7, BinaryTree.root(5)) // ceil(log2(5))=3, 2^3-1 = 7 + assertEquals(7, BinaryTree.root(6)) + assertEquals(7, BinaryTree.root(7)) + assertEquals(15, BinaryTree.root(9)) + } + + @Test + fun testDirectPath3Leaves() { + // 3 leaves, n=5, root=3: + // 3 + // / \ + // 1 4 + // / \ \ + // 0 2 (leaf 2 sits at node 4; node 5 doesn't exist) + // Node 4 is a leaf node here because the tree is not full — its + // "parent slot" 5 would be ≥ nodeCount and is collapsed away by + // parentInRange. + assertEquals(listOf(1, 3), BinaryTree.directPath(0, 3)) + assertEquals(listOf(1, 3), BinaryTree.directPath(1, 3)) + assertEquals(listOf(3), BinaryTree.directPath(2, 3)) + } + + @Test + fun testDirectPath5Leaves() { + // 5 leaves, n=9, root=7: + // 7 + // / \ + // 3 8 ← leaf 4 collapsed up + // / \ + // 1 5 + // / \ / \ + // 0 2 4 6 + assertEquals(listOf(1, 3, 7), BinaryTree.directPath(0, 5)) + assertEquals(listOf(1, 3, 7), BinaryTree.directPath(1, 5)) + assertEquals(listOf(5, 3, 7), BinaryTree.directPath(2, 5)) + assertEquals(listOf(5, 3, 7), BinaryTree.directPath(3, 5)) + assertEquals(listOf(7), BinaryTree.directPath(4, 5)) + } + + @Test + fun testDirectPathTerminatesForAllLeafCountsUpTo32() { + // Property: every valid leaf in any tree size 1..32 has a directPath + // that ends at root(leafCount) and has length == log2-ish. We don't + // assert exact lengths — we just want to catch any future regression + // where some (leafCount, leafIndex) triggers the parent() infinite + // loop. Each call is wrapped in a generous timeout via the kotlin + // test runner (default test timeout is fine: a non-terminating call + // would OOM long before any test deadline). + for (leafCount in 1..32) { + val rootIdx = BinaryTree.root(leafCount) + for (leafIndex in 0 until leafCount) { + val dp = BinaryTree.directPath(leafIndex, leafCount) + if (leafCount == 1) { + assertTrue(dp.isEmpty(), "leafCount=1 has no path") + } else { + assertEquals(rootIdx, dp.last(), "directPath($leafIndex, $leafCount) should end at root") + // copath must align with the directPath one-for-one + assertEquals(dp.size, BinaryTree.copath(leafIndex, leafCount).size) + } + } + } + } + + // ----- Out-of-range inputs ----------------------------------------------- + // + // Marmot-interop test 14 fails with a Java OOM because amy calls + // `BinaryTree.directPath(myLeafIndex, tree.leafCount)` with `myLeafIndex + // == tree.leafCount` (her own leaf was just removed and the tree shrank + // past it). `parent()` then walks node indices ≥ nodeCount forever and + // the result list explodes. + // + // These tests pin down that boundary so the failure becomes a clean + // IllegalArgumentException instead of an OOM, AND so the MLS code path + // that hits it has a deterministic regression. + + @Test + fun testDirectPathRejectsOutOfRangeLeafIndex() { + // leafIndex == leafCount — the exact shape the post-Remove path hits. + assertFails { BinaryTree.directPath(2, 2) } + // leafIndex > leafCount. + assertFails { BinaryTree.directPath(5, 3) } + } + + @Test + fun testDirectPathRejectsNegativeLeafIndex() { + assertFails { BinaryTree.directPath(-1, 4) } + } + + @Test + fun testCopathRejectsOutOfRangeLeafIndex() { + assertFails { BinaryTree.copath(2, 2) } + assertFails { BinaryTree.copath(-1, 4) } + } + + @Test + fun testParentRejectsRootOrAbove() { + // The root has no parent. Asking anyway is the symptom of a tree + // accounting bug — surface it loudly instead of looping. + val n = BinaryTree.nodeCount(4) + assertFails { BinaryTree.parent(BinaryTree.root(4), n) } + // Index strictly above nodeCount also has no defined parent. + assertFails { BinaryTree.parent(n, n) } + assertFails { BinaryTree.parent(n + 5, n) } + } } From 9d912ad82a37d2552f74f3968857612ec8251b79 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 02:22:04 +0000 Subject: [PATCH 3/9] fix(marmot): receive standalone SelfRemove proposals (test 15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs that conspired to break interop test 15 once the test 14 OOM was fixed: 1. **No receive path for standalone PublicMessage proposals.** wn/openmls publishes a non-admin's `SelfRemove` as a kind:445 carrying a `PublicMessage(content_type=PROPOSAL)` envelope and waits for an admin to fold it into the next commit. Quartz's `MarmotInboundProcessor` answered every such event with `Error("Standalone proposals not yet supported")` and dropped it. The admin's subsequent commit then failed with `Commit references unknown proposal (ref not found in pending proposals)` because nobody had staged the SelfRemove. Add `MlsGroup.receivePublicMessageProposal(pubMsg)` that: - rejects mismatched epoch / group_id / sender, - reconstructs the FramedContentTBS exactly as the proposer did and verifies the leaf signature, - verifies the membership_tag against the current epoch's membership_key (same threat model as inbound PublicMessage commits), - decodes the inner Proposal (only `SelfRemove` is accepted today — other types come bundled in commits' `proposals` lists), - stages the proposal in `pendingProposals` so a later commit can resolve its `ProposalRef`. `processPublicMessage` now routes `ContentType.PROPOSAL` through that helper and returns a new `GroupEventResult.ProposalStaged` variant (also surfaced as `MarmotIngestResult.ProposalStaged`), replacing the old hard-error path. 2. **Wrong `ProposalRef` hash input.** RFC 9420 §5.2 specifies that a `ProposalRef` hashes the **encoded `AuthenticatedContent`** that delivered the proposal, not the bare `Proposal` struct. Quartz was hashing `proposal.toTlsBytes()` only — fine for our local-only flows where commits inline rather than reference our own pending proposals, but fatal once we needed to match wn's reference to an inbound proposal. Extend `PendingProposal` with an optional `authenticatedContentBytes` field. The standalone-proposal receive path captures the full `wire_format || FramedContent || FramedContentAuthData` envelope at stage time. The reference-resolution code in `processCommitInner` prefers those bytes when present and falls back to the bare-proposal hash for locally-proposed entries (which never get referenced today). Marmot interop score: 14/16 → 15/16 (test 9 — amy's kind:7 reaction triggers a `SecretReuseError` on B's wn — is unrelated to this path and remains for follow-up). https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../amethyst/commons/marmot/MarmotIngest.kt | 15 ++ .../amethyst/commons/marmot/MarmotManager.kt | 4 + .../quartz/marmot/MarmotInboundProcessor.kt | 31 +++- .../quartz/marmot/mls/group/MlsGroup.kt | 138 +++++++++++++++++- 4 files changed, 185 insertions(+), 3 deletions(-) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotIngest.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotIngest.kt index 7f5517e5c..20c6492f0 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotIngest.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotIngest.kt @@ -61,6 +61,17 @@ sealed class MarmotIngestResult { val inner: GroupEventResult.CommitProcessed, ) : MarmotIngestResult() + /** + * A kind:445 carried a standalone Proposal (currently only `SelfRemove`) + * which was staged in the group's pending pool. The group epoch did + * not advance — a later Commit referencing this proposal will pick + * it up. + */ + data class ProposalStaged( + val groupId: HexKey, + val senderLeafIndex: Int, + ) : MarmotIngestResult() + /** A kind:445 whose outer layer we couldn't decrypt (pre-join epoch, etc). Debug-only. */ data class UndecryptableOuter( val groupId: HexKey, @@ -138,6 +149,10 @@ private suspend fun MarmotManager.ingestGroupEvent(ge: GroupEvent): MarmotIngest MarmotIngestResult.Commit(result) } + is GroupEventResult.ProposalStaged -> { + MarmotIngestResult.ProposalStaged(result.groupId, result.senderLeafIndex) + } + is GroupEventResult.Duplicate, is GroupEventResult.CommitPending, -> { 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 5aa55bb1d..c7003f277 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 @@ -113,6 +113,10 @@ class MarmotManager( subscriptionManager.updateGroupSince(result.groupId, groupEvent.createdAt) } + is GroupEventResult.ProposalStaged -> { + subscriptionManager.updateGroupSince(result.groupId, groupEvent.createdAt) + } + is GroupEventResult.CommitPending, is GroupEventResult.Duplicate, is GroupEventResult.UndecryptableOuterLayer, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt index 77b745796..2030217fb 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt @@ -94,6 +94,17 @@ sealed class GroupEventResult { val retainedEpochCount: Int, ) : GroupEventResult() + /** + * A standalone Proposal (currently only `SelfRemove`) was decoded, + * verified, and staged in the group's pending-proposals pool. The + * group epoch did not advance — the proposal is dormant until a + * subsequent Commit references it by hash. + */ + data class ProposalStaged( + val groupId: HexKey, + val senderLeafIndex: Int, + ) : GroupEventResult() + /** * The event could not be processed. */ @@ -474,7 +485,25 @@ class MarmotInboundProcessor( } ContentType.PROPOSAL -> { - GroupEventResult.Error(groupId, "Standalone proposals not yet supported") + // wn/openmls publishes SelfRemove as a standalone PublicMessage + // proposal — admins fold it into their next commit. Stage it + // locally so a subsequent commit's `ProposalRef` can resolve; + // 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") + try { + group.receivePublicMessageProposal(pubMsg) + GroupEventResult.ProposalStaged(groupId, pubMsg.sender.leafIndex) + } catch (e: Exception) { + GroupEventResult.Error( + groupId, + "Failed to stage standalone proposal: ${e.message}", + e, + ) + } } ContentType.APPLICATION -> { 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 56f522d6a..fd4535797 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 @@ -1260,8 +1260,18 @@ class MlsGroup private constructor( val refHash = proposalOrRef.proposalRef val resolved = pendingProposals.find { pending -> - val proposalBytes = pending.proposal.toTlsBytes() - val hash = MlsCryptoProvider.refHash("MLS 1.0 Proposal Reference", proposalBytes) + // RFC 9420 §5.2: a ProposalRef hashes the encoded + // AuthenticatedContent that delivered the proposal, + // not just the bare Proposal struct. For inbound + // proposals (e.g. C's standalone SelfRemove) we + // captured those bytes at receive time. For local + // proposals (which we always inline in our own + // commits, never reference) we fall back to the + // bare-proposal hash — keeps existing flows + // working without needing an AC reconstruction + // round-trip on the send side. + val refValue = pending.authenticatedContentBytes ?: pending.proposal.toTlsBytes() + val hash = MlsCryptoProvider.refHash("MLS 1.0 Proposal Reference", refValue) hash.contentEquals(refHash) } requireNotNull(resolved) { @@ -3083,11 +3093,135 @@ class MlsGroup private constructor( ) return MlsMessage.fromPublicMessage(publicMessage).toTlsBytes() to preCommitExporterSecret } + + /** + * Receive a standalone-proposal PublicMessage and stage it locally so a + * subsequent commit that references it (via `ProposalRef`) can resolve. + * + * Sent today by `wn`/openmls when a non-admin member self-removes — the + * member can't commit themselves out (admins reject `RequiredPathNotFound`), + * so they publish a `SelfRemove` proposal as a `PublicMessage` and wait + * for an admin to fold it into the next commit. Without this receiver + * side every other member silently drops the proposal, and the admin's + * subsequent commit then fails with "Commit references unknown proposal + * (ref not found in pending proposals)" — which is exactly the failure + * mode of marmot-interop test 15. + * + * Validation mirrors what every member-sender PublicMessage commit goes + * through: epoch + group_id match the current epoch, FramedContentTBS + * signature verifies against the sender's leaf signing key, and the + * membership_tag verifies against the current epoch's membership key. + * Anything missing or mismatched is a hard reject — the same threat + * model as PublicMessage commit reception. + */ + fun receivePublicMessageProposal(pubMsg: PublicMessage) { + require(pubMsg.contentType == ContentType.PROPOSAL) { + "Expected PublicMessage with content_type == PROPOSAL, got ${pubMsg.contentType}" + } + require(pubMsg.epoch == groupContext.epoch) { + "Proposal epoch ${pubMsg.epoch} doesn't match current epoch ${groupContext.epoch}" + } + require(pubMsg.groupId.contentEquals(groupContext.groupId)) { + "Proposal group_id doesn't match current group" + } + require(pubMsg.sender.senderType == SenderType.MEMBER) { + "Standalone proposals from non-members are not accepted" + } + val senderLeafIndex = pubMsg.sender.leafIndex + require(senderLeafIndex in 0 until tree.leafCount) { + "Sender leaf index $senderLeafIndex out of range" + } + val senderLeaf = + requireNotNull(tree.getLeaf(senderLeafIndex)) { + "Sender leaf is blank at index $senderLeafIndex" + } + + // Reconstruct FramedContentTBS exactly as the sender did in + // `buildSelfRemoveProposalMessage` so the signature and the + // membership_tag both verify against bit-identical bytes. + val tbsWriter = TlsWriter() + tbsWriter.putUint16(MlsMessage.MLS_VERSION_10) + tbsWriter.putUint16(WireFormat.PUBLIC_MESSAGE.value) + tbsWriter.putOpaqueVarInt(pubMsg.groupId) + tbsWriter.putUint64(pubMsg.epoch) + encodeSender(tbsWriter, pubMsg.sender) + tbsWriter.putOpaqueVarInt(pubMsg.authenticatedData) + tbsWriter.putUint8(ContentType.PROPOSAL.value) + tbsWriter.putBytes(pubMsg.content) // proposal struct, no length prefix + tbsWriter.putBytes(groupContext.toTlsBytes()) + val tbs = tbsWriter.toByteArray() + + require(MlsCryptoProvider.verifyWithLabel(senderLeaf.signatureKey, "FramedContentTBS", tbs, pubMsg.signature)) { + "Invalid FramedContentTBS signature on PublicMessage proposal from leaf $senderLeafIndex" + } + + val membershipTag = + requireNotNull(pubMsg.membershipTag) { + "PublicMessage proposal from leaf $senderLeafIndex is missing membership_tag" + } + val tbmWriter = TlsWriter() + tbmWriter.putBytes(tbs) + tbmWriter.putOpaqueVarInt(pubMsg.signature) + require(verifyMembershipTag(tbmWriter.toByteArray(), membershipTag)) { + "Invalid membership_tag on PublicMessage proposal from leaf $senderLeafIndex" + } + + val proposal = Proposal.decodeTls(TlsReader(pubMsg.content)) + // SelfRemove is the only proposal type wn currently emits as a + // standalone PublicMessage. Other types come bundled inside a + // commit's `proposals` list. Defending against e.g. a standalone + // Add/Remove here would also work — they'd land in pendingProposals + // and be picked up by the next commit that references them — but we + // refuse anything other than SelfRemove for now to keep the receive + // side minimal until there's an interop reason to widen it. + require(proposal is Proposal.SelfRemove) { + "Only standalone SelfRemove proposals are accepted; got ${proposal::class.simpleName}" + } + + // Capture the AuthenticatedContent bytes (RFC 9420 §6.1) so a + // subsequent commit's `ProposalRef` lookup can hash them per §5.2: + // + // AuthenticatedContent = wire_format || FramedContent || FramedContentAuthData + // + // FramedContentAuthData for a PROPOSAL is just `signature` (no + // confirmation_tag). The TBS prefix (version + GroupContext) is + // NOT part of the AuthenticatedContent — those go into the + // signature input only. + val acWriter = TlsWriter() + acWriter.putUint16(WireFormat.PUBLIC_MESSAGE.value) + // FramedContent + acWriter.putOpaqueVarInt(pubMsg.groupId) + acWriter.putUint64(pubMsg.epoch) + encodeSender(acWriter, pubMsg.sender) + acWriter.putOpaqueVarInt(pubMsg.authenticatedData) + acWriter.putUint8(ContentType.PROPOSAL.value) + acWriter.putBytes(pubMsg.content) + // FramedContentAuthData (PROPOSAL: signature only) + acWriter.putOpaqueVarInt(pubMsg.signature) + val authenticatedContentBytes = acWriter.toByteArray() + + pendingProposals.add( + PendingProposal( + proposal = proposal, + senderLeafIndex = senderLeafIndex, + authenticatedContentBytes = authenticatedContentBytes, + ), + ) + } } data class PendingProposal( val proposal: Proposal, val senderLeafIndex: Int, + /** + * Encoded `AuthenticatedContent` bytes for the message that delivered + * this proposal — the input to `MakeProposalRef` per RFC 9420 §5.2. + * Null when the proposal was created locally and never went through + * the MLS framing layer; in that case the lookup falls back to the + * bare `proposal.toTlsBytes()` since local commits inline rather than + * reference our own pending proposals. + */ + val authenticatedContentBytes: ByteArray? = null, ) data class DecryptedMessage( From fc99bed55a41ef9a0f4dbbfe6f99600ba46ef185 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 02:53:38 +0000 Subject: [PATCH 4/9] fix(cli): default amy launcher to C.UTF-8 so emoji argv survives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `marmot message react "$gid" "$id" "🍕"` was producing a kind:7 whose inner content was four `U+FFFD` replacement characters instead of the emoji. Whitenoise's `message_aggregator::emoji_utils` rejected it with `Invalid reaction content: ����`, and the openmls receiver retried the event ten times before giving up — by which point the secret tree had already consumed that generation, so the retry surfaced as a confusing `Ciphertext generation out of bounds 1 / SecretReuseError`. That last error was a downstream symptom; the root cause is argv decoding. Why it happens: JEP 400 (Java 18+) pins `file.encoding` to UTF-8 but deliberately leaves `sun.jnu.encoding` — the encoding the JVM uses to decode argv, filenames, and native interop strings — tied to the OS locale, and `sun.jnu.encoding` is read **before** the JVM parses any `-D` flag, so it can't be overridden via `applicationDefaultJvmArgs`. Under POSIX/C locales (no `LANG`/`LC_ALL` set, common on CI runners and stripped-down containers) `sun.jnu.encoding` ends up at `ANSI_X3.4-1968` — i.e. ASCII — and every byte > 0x7F in argv lands as `U+FFFD` before our code ever sees it. The four UTF-8 bytes of 🍕 (F0 9F 8D 95) become four replacement chars, get signed into the kind:7 content, and the test fails on receipt. The only place we can set the encoding from is the launching shell. Patch the gradle-generated start scripts (POSIX `amy`, Windows `amy.bat`) right after `:cli:startScripts`: * POSIX: if neither `LANG` nor `LC_ALL` is set, `export LANG=C.UTF-8` before invoking Java. Existing locale settings are respected. * Windows: `chcp 65001` to switch the console to UTF-8 before Java runs. Tested manually: with `LANG=` (sandbox default) `amy` previously sent `argv[0]=????` for an emoji argument; with the patch it sends `🍕` and `sun.jnu.encoding=UTF-8`, and whitenoise accepts the kind:7 reaction. The interop test 09 polling also needed a fix unrelated to the encoding bug: wn aggregates kind:7 reactions onto the anchor message under `.reactions.by_emoji.` instead of surfacing them as standalone messages whose `.content` is the emoji, so the previous `wait_for_message B "$mls_gid" "🍕"` would have failed even with a correctly-decoded reaction. The new poll inspects each message's `.reactions.by_emoji` keys for the emoji literal. Marmot interop score: 15/16 → **16/16** 🎉 https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- cli/build.gradle.kts | 78 ++++++++++++++++++++++++++++++++ cli/tests/marmot/tests-extras.sh | 22 ++++++++- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/cli/build.gradle.kts b/cli/build.gradle.kts index 55871b14b..93295b50d 100644 --- a/cli/build.gradle.kts +++ b/cli/build.gradle.kts @@ -35,6 +35,84 @@ application { applicationName = "amy" } +// Inject `LANG=C.UTF-8` (and the matching Windows code page) into the +// launcher scripts when `LANG`/`LC_ALL` aren't already set. Without this, +// running amy under a POSIX/C locale leaves `sun.jnu.encoding` as +// `ANSI_X3.4-1968` (i.e. ASCII) — JEP 400 pins `file.encoding` to UTF-8 +// but deliberately leaves `sun.jnu.encoding` tied to the OS locale, and +// it's read by the JVM *before* any `-D` flag has a chance to apply. So +// `-Dsun.jnu.encoding=UTF-8` does nothing; the encoding has to be set in +// the environment that launches Java. +// +// The symptom this fixes: `marmot message react "$gid" "$id" "🍕"` — +// the shell hands the four UTF-8 bytes (F0 9F 8D 95) to the JVM, the +// JVM decodes each one as ASCII (every byte > 0x7F → U+FFFD), and amy +// then signs a kind:7 whose `content` is four replacement characters. +// Whitenoise rejects it with "Invalid reaction content". +val patchAmyLauncherCharset by tasks.registering { + val appName = application.applicationName + val startScriptsTask = tasks.named("startScripts") + dependsOn(startScriptsTask) + // Patch the scripts at their source location so installDist (and any + // downstream packaging like jpackage) copies the patched copies. + doLast { + val unixScript = layout.buildDirectory.file("scripts/$appName").get().asFile + if (unixScript.exists()) { + val text = unixScript.readText() + val marker = "# Default LANG to UTF-8 so the JVM picks UTF-8 for sun.jnu.encoding" + if (!text.contains(marker)) { + // Build the snippet with explicit literal `$` characters to avoid + // regex-replacement-string escapes when we splice it in. + val injected = buildString { + append("\n") + append(marker).append("\n") + append("# (POSIX/C locales force ANSI_X3.4-1968, which mangles non-ASCII argv).\n") + append("if [ -z \"") + append("$").append("{LANG-}").append("$").append("{LC_ALL-}") + append("\" ]; then\n") + append(" export LANG=C.UTF-8\n") + append("fi\n") + } + // Insert right after the shebang. Use indexOf+substring instead + // of regex replaceFirst so the `$` chars in `injected` aren't + // mistaken for backreferences. + val nl = text.indexOf('\n') + val patched = + if (nl >= 0 && text.startsWith("#!")) { + text.substring(0, nl + 1) + injected + text.substring(nl + 1) + } else { + injected.trimStart('\n') + text + } + unixScript.writeText(patched) + } + } + val batScript = layout.buildDirectory.file("scripts/$appName.bat").get().asFile + if (batScript.exists()) { + val text = batScript.readText() + val marker = "rem Pin code page to UTF-8 so sun.jnu.encoding picks UTF-8" + if (!text.contains(marker)) { + val injected = + "$marker\r\n" + + "chcp 65001 > NUL 2>&1\r\n" + val anchor = "@if \"%DEBUG%\"==\"\" @echo off" + val idx = text.indexOf(anchor) + if (idx >= 0) { + val afterAnchor = text.indexOf('\n', idx) + if (afterAnchor >= 0) { + val patched = + text.substring(0, afterAnchor + 1) + injected + text.substring(afterAnchor + 1) + batScript.writeText(patched) + } + } + } + } + } +} + +tasks.named("installDist") { + dependsOn(patchAmyLauncherCharset) +} + // --------------------------------------------------------------------------- // Native distribution (jlink + jpackage) // diff --git a/cli/tests/marmot/tests-extras.sh b/cli/tests/marmot/tests-extras.sh index dd4e03e3d..aa0959670 100644 --- a/cli/tests/marmot/tests-extras.sh +++ b/cli/tests/marmot/tests-extras.sh @@ -55,8 +55,26 @@ test_09_reply_react_unreact() { record_result "$id" fail "amy marmot message react failed"; return fi - # Round-trip: B should surface amy's kind:7 reaction in its messages stream. - if wait_for_message B "$mls_gid" "🍕" 90; then + # Round-trip: B should surface amy's kind:7 reaction. wn aggregates + # reactions onto the anchor message (`.reactions.by_emoji[]`), + # not as a standalone entry whose `.content` is the emoji — so polling + # `messages list` for an entry whose content equals "🍕" would never + # match, even when the reaction was successfully decrypted. Look for + # the emoji under any message's `reactions.by_emoji` keys instead. + local deadline=$(( $(date +%s) + 90 )) saw=0 + while [[ $(date +%s) -lt $deadline ]]; do + local payload + payload=$(wn_b_json messages list "$mls_gid" --limit 50 2>/dev/null || true) + if [[ -n "$payload" ]] && \ + printf '%s' "$payload" \ + | jq -e '(.result // .) | .[]? | (.reactions.by_emoji // {}) | keys[]?' \ + 2>/dev/null \ + | grep -qF '"🍕"'; then + saw=1; break + fi + sleep 3 + done + if [[ "$saw" -eq 1 ]]; then record_result "$id" pass else record_result "$id" fail "B didn't receive amy's reaction" From d6a61d5ac530e73c12d20c027b962d79e83fe991 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 03:56:45 +0000 Subject: [PATCH 5/9] =?UTF-8?q?fix(marmot):=20inbound=20MIP-03=20admin=20g?= =?UTF-8?q?ate,=20RFC=209420=20=C2=A75.3=20PSK=20secret,=20ProposalRef=20A?= =?UTF-8?q?C=20bytes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three audit gaps in the Marmot MLS implementation, fixed together because they share the same call sites: 1. **MIP-03 inbound authorization gate.** `enforceAuthorizedProposalSet` only fired for *outbound* commits (it implicitly checked the local member). A peer could send us a non-admin GroupContextExtensions rename, a non-admin Remove, or an admin-emptying GCE and we would silently apply it. `enforceAuthorizedProposalSet` now takes an explicit `committerLeafIndex` (defaults to `myLeafIndex` for the local case) and uses `isLeafAdmin(committerLeafIndex)` instead of `isLocalAdmin()`. `processCommitInner` resolves the proposal list against our pending pool, then runs both `enforceAuthorizedProposalSet` and `enforceNoAdminDepletion` against the resolved set before applying. External commits skip the check (sender has no leaf yet, and §12.4.3.2 already restricts the proposal list). 2. **RFC 9420 §5.3 psk_secret derivation.** The previous `computePskSecret` HKDF-Extracted bare PSK values with the running `pskSecret` as salt and ignored `psktype` / `psk_nonce` / index / count entirely — incompatible with any spec-conformant peer. Per §5.3 each step is now: psk_extracted_i = HKDF.Extract(0, psk_i) psk_input_i = ExpandWithLabel(psk_extracted_i, "derived psk", PSKLabel(id_i, i, n), Nh) psk_secret_i = HKDF.Extract(psk_secret_{i-1}, psk_input_i) `buildPskLabel` encodes the full `PreSharedKeyID || index || count` struct. Resumption PSKs (`psktype == 2`) reject loudly because `Proposal.Psk` lacks `(usage, psk_group_id, psk_epoch)` — silently encoding a broken PSKLabel would diverge from peers without warning. 3. **ProposalRef hash for locally-published proposals.** RFC 9420 §5.2 hashes the encoded AuthenticatedContent, not the bare Proposal. `buildSelfRemoveProposalMessage` now stages the published proposal in `pendingProposals` together with the AC bytes (wire_format ‖ FramedContent ‖ FramedContentAuthData), so a subsequent inbound commit that folds it in by ProposalRef can resolve the hash. Bare- proposal fallback in `processCommitInner` retained for legacy local entries that pre-date the capture. Tests: 7 new cases in `MarmotMipBehaviorTest` covering inbound non-admin rejection (Add / GCE rename), admin-depletion rejection, AC bytes capture, PSK empty/single/ordering/resumption-rejected paths. Full quartz JVM test suite passes; marmot-interop-headless 16/16. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../quartz/marmot/mls/group/MlsGroup.kt | 256 +++++++++++++--- .../quartz/marmot/MarmotMipBehaviorTest.kt | 281 ++++++++++++++++++ 2 files changed, 493 insertions(+), 44 deletions(-) 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 fd4535797..994e006f6 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 @@ -145,6 +145,14 @@ class MlsGroup private constructor( */ fun isLocalMember(): Boolean = myLeafIndex < tree.leafCount && tree.getLeaf(myLeafIndex) != null + /** + * Read-only snapshot of the staged-proposal pool. Exposed at module + * scope so tests can inspect what `proposeAdd` / `proposeRemove` / + * `buildSelfRemoveProposalMessage` etc. actually stage (notably the + * `authenticatedContentBytes` we capture for ProposalRef matching). + */ + internal fun pendingProposalsSnapshot(): List = pendingProposals.toList() + // --- Marmot admin helpers (MIP-01 / MIP-03) --- /** Raw BasicCredential identity bytes of the member at the given leaf, or null. */ @@ -1236,27 +1244,20 @@ class MlsGroup private constructor( } } - // Apply proposals (resolve references from pending pool). - // Matches the committer's order: apply non-Add proposals first, then Adds, - // so leaves freed by Remove are available for Add reuse (RFC 9420 §12.4.2). - // Also track the post-Add leaf indices so the UpdatePath resolution filter - // can exclude them (mirrors the encryption-side exclusion). - val resolvedProposals = mutableListOf() - val inlineAdds = mutableListOf() - val referenceAddSenders = mutableListOf>() + // Resolve proposal references against our pending pool BEFORE + // applying anything, so MIP-03 authorization can run on a static + // snapshot of (proposal, original-sender-leaf) pairs and so the + // depletion guard can simulate the post-commit tree shape from the + // pre-commit state. + val resolvedPending = mutableListOf() for (proposalOrRef in commit.proposals) { when (proposalOrRef) { is ProposalOrRef.Inline -> { - if (proposalOrRef.proposal is Proposal.Add) { - inlineAdds.add(proposalOrRef.proposal) - } else { - applyProposal(proposalOrRef.proposal, senderLeafIndex) - } - resolvedProposals.add(proposalOrRef.proposal) + // Inline proposals are authored by the committer. + resolvedPending.add(PendingProposal(proposalOrRef.proposal, senderLeafIndex)) } is ProposalOrRef.Reference -> { - // Resolve proposal by reference hash from pending proposals (RFC 9420 §12.4.2) val refHash = proposalOrRef.proposalRef val resolved = pendingProposals.find { pending -> @@ -1264,12 +1265,11 @@ class MlsGroup private constructor( // AuthenticatedContent that delivered the proposal, // not just the bare Proposal struct. For inbound // proposals (e.g. C's standalone SelfRemove) we - // captured those bytes at receive time. For local - // proposals (which we always inline in our own - // commits, never reference) we fall back to the - // bare-proposal hash — keeps existing flows - // working without needing an AC reconstruction - // round-trip on the send side. + // captured those bytes at receive time. Locally- + // proposed entries that have been published as a + // standalone PublicMessage also carry their AC + // bytes; the bare-proposal fallback below covers + // legacy local entries that pre-date that capture. val refValue = pending.authenticatedContentBytes ?: pending.proposal.toTlsBytes() val hash = MlsCryptoProvider.refHash("MLS 1.0 Proposal Reference", refValue) hash.contentEquals(refHash) @@ -1277,15 +1277,50 @@ class MlsGroup private constructor( requireNotNull(resolved) { "Commit references unknown proposal (ref not found in pending proposals)" } - if (resolved.proposal is Proposal.Add) { - referenceAddSenders.add(resolved.proposal to resolved.senderLeafIndex) - } else { - applyProposal(resolved.proposal, resolved.senderLeafIndex) - } - resolvedProposals.add(resolved.proposal) + resolvedPending.add(resolved) } } } + + // MIP-03 authorization & admin-depletion gates on inbound commits + // (mirror what `commit()` enforces locally — without these a peer + // could send us a non-admin GCE rename, a non-admin Remove, or a + // commit that empties `admin_pubkeys` and we'd silently apply it). + // External commits get a pass: the sender doesn't have a leaf yet, + // so the admin lookup is moot, and an external joiner can't include + // arbitrary proposals — only Add/Remove/PSK/ExternalInit per + // RFC 9420 §12.4.3.2. + if (!isExternalCommit) { + enforceAuthorizedProposalSet(resolvedPending, committerLeafIndex = senderLeafIndex) + enforceNoAdminDepletion(resolvedPending) + } + + // Apply the resolved proposals. Matches the committer's order: apply + // non-Add proposals first, then Adds, so leaves freed by Remove are + // available for Add reuse (RFC 9420 §12.4.2). Also track the + // post-Add leaf indices so the UpdatePath resolution filter can + // exclude them (mirrors the encryption-side exclusion). + // + // `resolvedPending[i].senderLeafIndex` is already the correct + // author for both inline (committer) and reference (original + // proposer) entries, since we stamped inline entries with + // `senderLeafIndex` when building the snapshot above. + val resolvedProposals = mutableListOf() + val inlineAdds = mutableListOf() + val referenceAddSenders = mutableListOf>() + for ((idx, pending) in resolvedPending.withIndex()) { + val isInline = commit.proposals[idx] is ProposalOrRef.Inline + if (pending.proposal is Proposal.Add) { + if (isInline) { + inlineAdds.add(pending.proposal) + } else { + referenceAddSenders.add(pending.proposal to pending.senderLeafIndex) + } + } else { + applyProposal(pending.proposal, pending.senderLeafIndex) + } + resolvedProposals.add(pending.proposal) + } val newLeavesInCommit = mutableSetOf() for (add in inlineAdds) { newLeavesInCommit.add(applyProposalAdd(add)) @@ -1602,26 +1637,117 @@ class MlsGroup private constructor( /** * Compute the PSK secret from PSK proposals (RFC 9420 Section 8.4). * - * psk_secret is derived by chaining Extract calls over all PSK values. - * If no PSK proposals, returns zeros (default PSK secret). + * Derive `psk_secret` per RFC 9420 §5.3. + * + * For a list of `n` proposed PSKs, the `i`-th step is: + * + * ``` + * psk_extracted_i = HKDF.Extract(salt = 0, ikm = psk_i) + * psk_input_i = ExpandWithLabel(psk_extracted_i, "derived psk", + * PSKLabel(psk_id_i, i, n), Nh) + * psk_secret_i = HKDF.Extract(salt = psk_secret_{i-1}, ikm = psk_input_i) + * ``` + * + * The `PSKLabel` carries the full `PreSharedKeyID` (psktype, type- + * specific fields, psk_nonce) plus the `index, count` pair — without + * those, every member that resolves the PSK list in a different order + * (or with a different total count) would derive a different + * `psk_secret` and the post-commit confirmation_tag would silently + * mismatch. + * + * The previous implementation HKDF-Extracted the bare PSK value with + * the running pskSecret as salt and ignored psktype / psk_nonce + * entirely — non-conformant with §5.3 and incompatible with any peer + * that follows the spec. Returns zeros when no PSK proposals are + * present (the `default_psk_secret` per §8.1). */ - private fun computePskSecret(proposals: List): ByteArray { + internal fun computePskSecret(proposals: List): ByteArray { val pskProposals = proposals.filterIsInstance() if (pskProposals.isEmpty()) { return ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) } - // Chain: psk_secret = Extract(Extract(...Extract(0, psk_1), psk_2)..., psk_n) - var pskSecret = ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) - for (pskProposal in pskProposals) { + val zero = ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) + val count = pskProposals.size + var pskSecret = zero + for ((index, p) in pskProposals.withIndex()) { val pskValue = - pskStore[pskProposal.pskId.toHexKey()] - ?: throw IllegalStateException("PSK not found in store: ${pskProposal.pskId.toHexKey()}") - pskSecret = MlsCryptoProvider.hkdfExtract(pskSecret, pskValue) + pskStore[p.pskId.toHexKey()] + ?: throw IllegalStateException("PSK not found in store: ${p.pskId.toHexKey()}") + val pskExtracted = MlsCryptoProvider.hkdfExtract(zero, pskValue) + val pskLabel = buildPskLabel(p, index, count) + val pskInput = + MlsCryptoProvider.expandWithLabel( + secret = pskExtracted, + label = "derived psk", + context = pskLabel, + length = MlsCryptoProvider.HASH_OUTPUT_LENGTH, + ) + pskSecret = MlsCryptoProvider.hkdfExtract(pskSecret, pskInput) } return pskSecret } + /** + * Encode the `PSKLabel` struct used as the `context` argument to + * ExpandWithLabel during `psk_secret` derivation (RFC 9420 §5.3): + * + * ``` + * struct { + * PreSharedKeyID id; + * uint16 index; + * uint16 count; + * } PSKLabel; + * + * struct { + * PSKType psktype; + * select (PreSharedKeyID.psktype) { + * case external: opaque psk_id; + * case resumption: ResumptionPSKUsage usage; + * opaque psk_group_id; + * uint64 psk_epoch; + * }; + * opaque psk_nonce; + * } PreSharedKeyID; + * ``` + * + * Resumption PSK (`psktype == 2`) carries `usage / psk_group_id / + * psk_epoch` fields that aren't representable on `Proposal.Psk` today — + * the on-wire schema there is just `(pskType, pskId, pskNonce)`. Reject + * loudly until the proposal type is widened, rather than silently + * encoding a broken PSKLabel that would diverge from peers. + */ + private fun buildPskLabel( + psk: Proposal.Psk, + index: Int, + count: Int, + ): ByteArray { + val w = TlsWriter() + // PreSharedKeyID + w.putUint8(psk.pskType) + when (psk.pskType) { + PSK_TYPE_EXTERNAL -> { + w.putOpaqueVarInt(psk.pskId) + } + + PSK_TYPE_RESUMPTION -> { + throw IllegalStateException( + "Resumption PSKs are not supported yet — Proposal.Psk lacks " + + "(usage, psk_group_id, psk_epoch) per RFC 9420 §5.3.", + ) + } + + else -> { + throw IllegalStateException("Unknown PSKType ${psk.pskType}") + } + } + w.putOpaqueVarInt(psk.pskNonce) + // PSKLabel tail + w.putUint16(index) + w.putUint16(count) + return w.toByteArray() + } + /** * Build the ConfirmedTranscriptHashInput (RFC 9420 Section 8.2). * @@ -1931,36 +2057,46 @@ class MlsGroup private constructor( } /** - * MIP-03 authorization gate for local commits. + * MIP-03 authorization gate. * * Once the group has at least one admin configured in `admin_pubkeys`, * non-admin senders may only issue: * - a single self-Update proposal, or - * - one-or-more SelfRemove proposals authored by this member + * - one-or-more SelfRemove proposals authored by the committer. * * Admins may commit any proposal type. Before any admin is configured * (group bootstrap) the check is relaxed, mirroring the bootstrap policy * in [MlsGroupManager.updateGroupExtensions]. + * + * [committerLeafIndex] is the leaf that signed the commit — `myLeafIndex` + * for our own outbound commits, `pubMsg.sender.leafIndex` for inbound + * commits. The "self-only" rule is checked against the committer; when + * the committer is an admin the rule is skipped entirely so admin-folded + * inbound proposals (e.g. another member's `SelfRemove` referenced by + * an admin's GCE commit) are accepted. */ - private fun enforceAuthorizedProposalSet(proposals: List) { + internal fun enforceAuthorizedProposalSet( + proposals: List, + committerLeafIndex: Int = myLeafIndex, + ) { if (proposals.isEmpty()) return val marmot = currentMarmotData() val adminsConfigured = marmot != null && marmot.adminPubkeys.isNotEmpty() - if (!adminsConfigured || isLocalAdmin()) return + if (!adminsConfigured || isLeafAdmin(committerLeafIndex)) return val allSelfRemove = - proposals.all { it.proposal is Proposal.SelfRemove && it.senderLeafIndex == myLeafIndex } + proposals.all { it.proposal is Proposal.SelfRemove && it.senderLeafIndex == committerLeafIndex } if (allSelfRemove) return val singleSelfUpdate = proposals.size == 1 && proposals[0].proposal is Proposal.Update && - proposals[0].senderLeafIndex == myLeafIndex + proposals[0].senderLeafIndex == committerLeafIndex if (singleSelfUpdate) return throw IllegalStateException( "MIP-03: non-admin members may only commit a single self-Update or SelfRemove-only " + - "proposals; got ${proposals.map { it.proposal::class.simpleName }}", + "proposals; got ${proposals.map { it.proposal::class.simpleName }} from leaf $committerLeafIndex", ) } @@ -1973,7 +2109,7 @@ class MlsGroup private constructor( * once the group has a configured admin set — it does not kick in during * bootstrap before any admin is named. */ - private fun enforceNoAdminDepletion(proposals: List) { + internal fun enforceNoAdminDepletion(proposals: List) { val currentAdmins = currentMarmotData()?.adminPubkeys?.toSet().orEmpty() if (currentAdmins.isEmpty()) return // Bootstrap: no admins yet, nothing to deplete. @@ -2179,6 +2315,10 @@ class MlsGroup private constructor( // unreadable to OpenMLS/MDK/whitenoise.) private const val RATCHET_TREE_EXTENSION_TYPE = 0x0002 + // RFC 9420 §5.3 PSKType registry. + private const val PSK_TYPE_EXTERNAL = 1 + private const val PSK_TYPE_RESUMPTION = 2 + /** * Build the FramedContentTBS bytes for a member-sender commit * (RFC 9420 §6.1). The signature over this value is the @@ -3091,6 +3231,34 @@ class MlsGroup private constructor( confirmationTag = null, membershipTag = membershipTag, ) + + // Stage the proposal in our own pending pool with the encoded + // AuthenticatedContent (RFC 9420 §6.1: wire_format ‖ FramedContent ‖ + // FramedContentAuthData) so a subsequent inbound commit that folds + // this SelfRemove in by ProposalRef can resolve the hash per §5.2. + // Today's `leaveGroup` caller drops the group state immediately + // after this returns and never sees that commit, but a future + // caller that keeps the group around (to confirm the removal, + // log the closing epoch, etc.) needs the entry here. The AC + // bytes are bit-identical to what a peer reconstructs in + // [receivePublicMessageProposal]. + val acWriter = TlsWriter() + acWriter.putUint16(WireFormat.PUBLIC_MESSAGE.value) + acWriter.putOpaqueVarInt(ctx.groupId) + acWriter.putUint64(ctx.epoch) + encodeSender(acWriter, Sender(SenderType.MEMBER, myLeafIndex)) + acWriter.putOpaqueVarInt(ByteArray(0)) // authenticated_data + acWriter.putUint8(ContentType.PROPOSAL.value) + acWriter.putBytes(proposalBytes) + acWriter.putOpaqueVarInt(signature) // FramedContentAuthData (PROPOSAL: signature only) + pendingProposals.add( + PendingProposal( + proposal = proposal, + senderLeafIndex = myLeafIndex, + authenticatedContentBytes = acWriter.toByteArray(), + ), + ) + return MlsMessage.fromPublicMessage(publicMessage).toTlsBytes() to preCommitExporterSecret } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index 174de21f7..546b72597 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -367,6 +367,162 @@ class MarmotMipBehaviorTest { assertNotNull(relaysTag, "MIP-02: rumor MUST carry a relays tag") } + // ---------------------------------------------------------------------- + // MIP-03 inbound authorization gates + // ---------------------------------------------------------------------- + // + // The local-commit path has always run the authorization-set + admin- + // depletion guards (see `commit_adminDepletionGuardRejectsEmptyingAdminList` + // above). The inbound counterpart was missing — `processCommitInner` + // didn't call them, so a peer could send a commit our local code would + // never have produced and we'd accept it. These tests exercise the + // refactored guard functions directly with a `committerLeafIndex` + // parameter, which is the shape `processCommitInner` calls them in. + + @Test + fun enforceAuthorizedProposalSet_rejectsNonAdminCommitterRemove() = + runBlocking { + // Group with Alice as the only configured admin. + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + manager.updateGroupExtensions( + groupId, + listOf(MarmotGroupData(nostrGroupId = groupId, adminPubkeys = listOf(aliceId)).toExtension()), + ) + // Add Bob (non-admin) so leaf index 1 is occupied. + val bobBundle = createStandaloneKeyPackage(bobId) + manager.addMember(groupId, bobBundle.keyPackage.toTlsBytes()) + + val alice = manager.getGroup(groupId)!! + // Bob (leaf 1) is NOT an admin. Pretend he committed a Remove of + // himself authored by himself — `enforceAuthorizedProposalSet` + // must reject because Remove is admin-only. + val proposals = + listOf( + com.vitorpamplona.quartz.marmot.mls.group + .PendingProposal( + proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .Remove(removedLeafIndex = 0), + senderLeafIndex = 1, + ), + ) + val ex = + assertFailsWith { + alice.enforceAuthorizedProposalSet(proposals, committerLeafIndex = 1) + } + assertTrue( + ex.message!!.contains("non-admin members may only commit"), + "expected MIP-03 violation message, got: ${ex.message}", + ) + } + + @Test + fun enforceAuthorizedProposalSet_acceptsAdminCommitterFoldingAnotherMembersSelfRemove() = + runBlocking { + // Mirrors marmot-interop test 15: Bob (admin) commits a + // SelfRemove proposal authored by Carol. Inline-as-fold flows + // tag the proposal with the committer's leaf index, so Bob's + // Remove-style fold of Carol's leaf still authenticates. + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + manager.updateGroupExtensions( + groupId, + listOf(MarmotGroupData(nostrGroupId = groupId, adminPubkeys = listOf(aliceId)).toExtension()), + ) + + val alice = manager.getGroup(groupId)!! + // Alice (leaf 0) is admin. The committer-is-admin shortcut fires + // before the per-proposal author check, so even a heterogeneous + // proposal list passes. + val proposals = + listOf( + com.vitorpamplona.quartz.marmot.mls.group + .PendingProposal( + proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .SelfRemove(), + senderLeafIndex = 99, + ), + ) + // Should not throw. + alice.enforceAuthorizedProposalSet(proposals, committerLeafIndex = 0) + } + + @Test + fun enforceNoAdminDepletion_rejectsCommitThatEmptiesAdminList() = + runBlocking { + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + manager.updateGroupExtensions( + groupId, + listOf(MarmotGroupData(nostrGroupId = groupId, adminPubkeys = listOf(aliceId)).toExtension()), + ) + + val alice = manager.getGroup(groupId)!! + val proposals = + listOf( + com.vitorpamplona.quartz.marmot.mls.group + .PendingProposal( + proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .GroupContextExtensions( + extensions = + listOf( + MarmotGroupData( + nostrGroupId = groupId, + adminPubkeys = emptyList(), + ).toExtension(), + ), + ), + senderLeafIndex = 0, + ), + ) + assertFailsWith { + alice.enforceNoAdminDepletion(proposals) + } + } + + // ---------------------------------------------------------------------- + // RFC 9420 §5.2 ProposalRef hashing — local standalone proposals + // ---------------------------------------------------------------------- + + @Test + fun buildSelfRemoveProposalMessage_stagesPendingWithAuthenticatedContentBytes() = + runBlocking { + // Setup: Alice creates a group where she is NOT admin (a + // throwaway bobId is the sole configured admin). Without that, + // `buildSelfRemoveProposalMessage` rejects per MIP-01. + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + manager.updateGroupExtensions( + groupId, + listOf(MarmotGroupData(nostrGroupId = groupId, adminPubkeys = listOf(bobId)).toExtension()), + ) + + val alice = manager.getGroup(groupId)!! + assertEquals(0, alice.pendingProposalsSnapshot().size) + + val (_, _) = alice.buildSelfRemoveProposalMessage() + + val staged = alice.pendingProposalsSnapshot() + assertEquals(1, staged.size, "buildSelfRemoveProposalMessage must also stage to pending pool") + val entry = staged.single() + assertIs(entry.proposal) + assertEquals(alice.leafIndex, entry.senderLeafIndex) + // The captured AC bytes are what RFC 9420 §5.2's MakeProposalRef + // hashes — must be present so a peer's commit referencing this + // proposal by hash resolves against our pool. + assertNotNull( + entry.authenticatedContentBytes, + "RFC 9420 §5.2: standalone-published proposals must carry the encoded AuthenticatedContent", + ) + assertTrue(entry.authenticatedContentBytes.isNotEmpty()) + } + // ---------------------------------------------------------------------- // MIP-03 group event h-tag shape // ---------------------------------------------------------------------- @@ -395,6 +551,131 @@ class MarmotMipBehaviorTest { ) } + // ---------------------------------------------------------------------- + // RFC 9420 §5.3 psk_secret derivation + // ---------------------------------------------------------------------- + + /** + * Empty PSK list MUST collapse to the all-zero `default_psk_secret` + * (RFC 9420 §8.1) — every epoch where no PSKs are proposed feeds zeros + * into the joiner_secret extract step. + */ + @Test + fun computePskSecret_emptyListReturnsAllZeros() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val out = alice.computePskSecret(emptyList()) + assertEquals(32, out.size, "psk_secret length must be Nh = 32 for SHA-256") + assertTrue(out.all { it == 0.toByte() }, "default_psk_secret is all zeros") + } + + /** + * Single external PSK case — verify the derived `psk_secret` matches the + * RFC 9420 §5.3 reference computation: + * + * ``` + * psk_extracted_0 = HKDF.Extract(salt = 0, ikm = psk_0) + * psk_input_0 = ExpandWithLabel(psk_extracted_0, "derived psk", + * PSKLabel(id_0, 0, 1), 32) + * psk_secret_0 = HKDF.Extract(salt = 0, ikm = psk_input_0) + * ``` + * + * The previous implementation HKDF-Extracted the bare PSK value with the + * running pskSecret as salt and never built a PSKLabel — its output + * would not match this expected value. + */ + @Test + fun computePskSecret_singleExternalPsk_matchesSpecDerivation() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val pskId = ByteArray(16) { (it + 1).toByte() } + val pskNonce = ByteArray(16) { (0x80 or it).toByte() } + val pskValue = ByteArray(32) { (0xA0 or (it and 0x0F)).toByte() } + alice.registerPsk(pskId, pskValue) + + val proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .Psk(pskType = 1, pskId = pskId, pskNonce = pskNonce) + + val actual = alice.computePskSecret(listOf(proposal)) + + // Reference computation per §5.3 (PSKType=1, no usage/group/epoch). + val zero = ByteArray(32) + val crypto = com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider + val pskExtracted = crypto.hkdfExtract(salt = zero, ikm = pskValue) + + val labelWriter = + com.vitorpamplona.quartz.marmot.mls.codec + .TlsWriter() + labelWriter.putUint8(1) // PSKType external + labelWriter.putOpaqueVarInt(pskId) + labelWriter.putOpaqueVarInt(pskNonce) + labelWriter.putUint16(0) // index + labelWriter.putUint16(1) // count + + val pskInput = + crypto.expandWithLabel( + secret = pskExtracted, + label = "derived psk", + context = labelWriter.toByteArray(), + length = 32, + ) + val expected = crypto.hkdfExtract(salt = zero, ikm = pskInput) + + assertContentEquals(expected, actual, "psk_secret must match RFC 9420 §5.3 derivation") + } + + /** + * Resumption PSK (psktype = 2) carries usage/psk_group_id/psk_epoch + * fields that aren't representable on `Proposal.Psk` today. Encoding a + * PSKLabel without them would silently diverge from spec-conformant + * peers — we reject loudly until the proposal type is widened. + */ + @Test + fun computePskSecret_resumptionPskRejectsUntilProposalWidened() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val pskId = ByteArray(16) { it.toByte() } + alice.registerPsk(pskId, ByteArray(32)) + + val proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .Psk(pskType = 2, pskId = pskId, pskNonce = ByteArray(16)) + + assertFailsWith { + alice.computePskSecret(listOf(proposal)) + } + } + + /** + * The `(index, count)` tail of PSKLabel ensures peers that resolve the + * SAME PSK in different list positions derive DIFFERENT psk_secret — + * the previous implementation ignored ordering entirely. + */ + @Test + fun computePskSecret_orderingChangesOutput() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val idA = ByteArray(16) { 0x11 } + val idB = ByteArray(16) { 0x22 } + alice.registerPsk(idA, ByteArray(32) { 0x33 }) + alice.registerPsk(idB, ByteArray(32) { 0x44 }) + + val pskA = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .Psk(pskType = 1, pskId = idA, pskNonce = ByteArray(8)) + val pskB = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .Psk(pskType = 1, pskId = idB, pskNonce = ByteArray(8)) + + val ab = alice.computePskSecret(listOf(pskA, pskB)) + val ba = alice.computePskSecret(listOf(pskB, pskA)) + assertTrue( + !ab.contentEquals(ba), + "PSKLabel index/count means [A,B] and [B,A] must derive distinct psk_secret", + ) + } + @Test fun processGroupEvent_acceptsInnerEventWithMatchingPubkey() = runBlocking { From a42499435b077d49a902a5db83f9acca4df0af93 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 04:22:42 +0000 Subject: [PATCH 6/9] =?UTF-8?q?fix(marmot):=20enforce=20required=5Fcapabil?= =?UTF-8?q?ities=20on=20Add=20and=20Welcome=20(RFC=209420=20=C2=A77.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a group installs a `required_capabilities` extension (every Marmot group does, listing MarmotGroupData=0xF2EE, SelfRemove=0x000A, Basic credential), every member's leaf MUST advertise those types in its own [Capabilities]. We were validating none of that: - `applyProposalAdd` checked version + ciphersuite but never matched the new KP's capabilities against the group's required_capabilities. A non-conformant member silently joined; the next commit that touched their leaf got rejected by spec-conformant peers, splitting the group. - `processWelcome` similarly never checked the joiner's own KP against the group's required set, nor did it sweep existing members' leaves. A misconfigured GroupInfo signer could invite us into an incoherent group whose first commit we'd silently reject forever. Adds two helpers in `MlsGroup.Companion`: - `findRequiredCapabilities(extensions)`: decodes the §7.2 struct from a GroupContext extension list, or returns null if absent. - `requireCapabilitiesMeetRequirements(caps, req, who)`: throws with a specific (extensions=, proposals=, credentials=) diff naming the missing types — turns silent interop breaks into one debuggable line. Wires the gate in two places: - `applyProposalAdd` rejects the Add proposal if the new leaf falls short of the group's required set. - `processWelcome` rejects the join if either (a) our own KP doesn't meet the group's requirements or (b) any existing member's leaf doesn't — the latter catches a malformed GroupInfo at join time. 5 new tests: round-trip decoding, rejection on missing extension / proposal, acceptance when caps are a superset, and an end-to-end `addMember` rejection of a hand-crafted KP with SelfRemove stripped (re-signed so the rejection is from the capability gate, not the signature check). Quartz test suite green; marmot interop 16/16. Closes audit gaps #4, #5, #10. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../quartz/marmot/mls/group/MlsGroup.kt | 91 +++++++++++ .../quartz/marmot/MarmotMipBehaviorTest.kt | 149 ++++++++++++++++++ 2 files changed, 240 insertions(+) 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 994e006f6..eee74727c 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 @@ -2053,6 +2053,16 @@ class MlsGroup private constructor( "KeyPackage does not support ciphersuite 0x0001" } + // RFC 9420 §12.4.2: an Add proposal MUST be rejected if the new + // member's leaf capabilities don't advertise every type listed in + // the group's `required_capabilities` extension. Without this gate + // a non-conformant member silently joins, and any subsequent commit + // that touches their leaf is rejected by peers that DO enforce the + // requirement — splitting the group on the next epoch. + findRequiredCapabilities(groupContext.extensions)?.let { req -> + requireCapabilitiesMeetRequirements(caps, req, "Add KeyPackage leaf") + } + return tree.addLeaf(leafNode) } @@ -2545,6 +2555,67 @@ class MlsGroup private constructor( ) } + /** + * Parsed view of the RFC 9420 §7.2 `required_capabilities` extension. + * + * The on-wire struct is three `uint16` vectors — + * `extensions / proposals / credentials` — that name the types every + * member of the group MUST advertise in their leaf [Capabilities]. + */ + internal data class RequiredCapabilities( + val extensions: List, + val proposals: List, + val credentials: List, + ) + + /** + * Decode the `required_capabilities` extension from the GroupContext + * extension list, or `null` if the group hasn't installed one (some + * peers may omit it; treat missing as "no restriction"). + */ + internal fun findRequiredCapabilities(extensions: List): RequiredCapabilities? { + val ext = extensions.find { it.extensionType == REQUIRED_CAPABILITIES_EXTENSION_TYPE } ?: return null + val r = TlsReader(ext.extensionData) + + fun readUint16List(data: ByteArray): List { + val rr = TlsReader(data) + val list = mutableListOf() + while (rr.hasRemaining) list.add(rr.readUint16()) + return list + } + val exts = readUint16List(r.readOpaqueVarInt()) + val props = readUint16List(r.readOpaqueVarInt()) + val creds = readUint16List(r.readOpaqueVarInt()) + return RequiredCapabilities(exts, props, creds) + } + + /** + * RFC 9420 §7.2 + §12.4.2: every member's leaf [Capabilities] MUST + * advertise every type listed in the group's `required_capabilities` + * extension. Adding a member whose KeyPackage doesn't meet the + * requirement leaves the group in a non-conformant state where + * peers that DO enforce the requirement will reject any commit that + * touches that leaf. + * + * Throws [IllegalStateException] with a precise diff so debugging + * an interop break against another implementation is one log line. + */ + internal fun requireCapabilitiesMeetRequirements( + caps: Capabilities, + req: RequiredCapabilities, + who: String, + ) { + val missingExts = req.extensions.filter { it !in caps.extensions } + val missingProps = req.proposals.filter { it !in caps.proposals } + val missingCreds = req.credentials.filter { it !in caps.credentials } + if (missingExts.isNotEmpty() || missingProps.isNotEmpty() || missingCreds.isNotEmpty()) { + throw IllegalStateException( + "$who capabilities don't meet required_capabilities: " + + "missing extensions=$missingExts proposals=$missingProps credentials=$missingCreds", + ) + } + } + /** * Default MLS leaf Capabilities that advertise support for Marmot's * required extensions and proposals so new members can join a group @@ -2734,6 +2805,26 @@ class MlsGroup private constructor( "GroupInfo tree_hash does not match ratchet_tree extension" } + // RFC 9420 §7.2 + §12.4.3.1: a joiner MUST refuse to join a + // group whose `required_capabilities` lists types the joiner's + // own KeyPackage doesn't advertise — peers that DO enforce the + // requirement would reject every commit the joiner produces + // anyway. Catching this at join time turns a silent "your + // commits get dropped forever" into an actionable error. + findRequiredCapabilities(groupContext.extensions)?.let { req -> + val myLeaf = tree.getLeaf(myLeafIndex) + requireNotNull(myLeaf) { "Joiner's leaf is blank after tree reconstruction" } + requireCapabilitiesMeetRequirements(myLeaf.capabilities, req, "Joiner KeyPackage") + // Mirror the same check across every existing member — + // if a peer's leaf doesn't meet the group's stated + // requirements, the GroupInfo signer mis-installed the + // requirement set and the group is incoherent. + for (i in 0 until tree.leafCount) { + val leaf = tree.getLeaf(i) ?: continue + requireCapabilitiesMeetRequirements(leaf.capabilities, req, "Member leaf $i") + } + } + // Derive epoch secrets directly from memberSecret (RFC 9420 Section 8.3) // For Welcome, epoch_secret = ExpandWithLabel(member_secret, "epoch", GroupContext, Nh) val epochSecret = diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index 546b72597..18bf4e76f 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -551,6 +551,155 @@ class MarmotMipBehaviorTest { ) } + // ---------------------------------------------------------------------- + // RFC 9420 §7.2 / §12.4.2 required_capabilities enforcement + // ---------------------------------------------------------------------- + + /** + * Round-trip: the `required_capabilities` extension Marmot installs on + * every fresh group decodes back to its declared (extensions, proposals, + * credentials) triple. + */ + @Test + fun findRequiredCapabilities_decodesMarmotExtensionInstalledByCreate() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val req = + MlsGroup.findRequiredCapabilities(alice.extensions) + ?: error("required_capabilities must be present after create()") + assertEquals(listOf(0xF2EE), req.extensions, "MarmotGroupData (0xF2EE) must be required") + assertEquals(listOf(0x000A), req.proposals, "SelfRemove (0x000A) must be required") + assertEquals(listOf(0x0001), req.credentials, "Basic credential must be required") + } + + /** + * `requireCapabilitiesMeetRequirements` must throw when ANY required + * type is missing — extension OR proposal OR credential — and the + * error must name the missing types so an interop debugger can + * diagnose without grepping. + */ + @Test + fun requireCapabilitiesMeetRequirements_rejectsMissingExtension() { + val req = + MlsGroup.Companion.RequiredCapabilities( + extensions = listOf(0xF2EE), + proposals = listOf(0x000A), + credentials = listOf(0x0001), + ) + // Missing 0xF2EE. + val caps = + com.vitorpamplona.quartz.marmot.mls.tree.Capabilities( + extensions = emptyList(), + proposals = listOf(0x000A), + credentials = listOf(0x0001), + ) + val ex = + assertFailsWith { + MlsGroup.requireCapabilitiesMeetRequirements(caps, req, "test") + } + assertTrue( + ex.message!!.contains("extensions=[62190]") || ex.message!!.contains("0xF2EE"), + "error must name the missing extension type: ${ex.message}", + ) + } + + @Test + fun requireCapabilitiesMeetRequirements_rejectsMissingProposal() { + val req = + MlsGroup.Companion.RequiredCapabilities( + extensions = emptyList(), + proposals = listOf(0x000A), + credentials = emptyList(), + ) + val caps = + com.vitorpamplona.quartz.marmot.mls.tree.Capabilities( + extensions = emptyList(), + proposals = emptyList(), + credentials = listOf(0x0001), + ) + assertFailsWith { + MlsGroup.requireCapabilitiesMeetRequirements(caps, req, "test") + } + } + + @Test + fun requireCapabilitiesMeetRequirements_passesWhenCapsAreSuperset() { + val req = + MlsGroup.Companion.RequiredCapabilities( + extensions = listOf(0xF2EE), + proposals = listOf(0x000A), + credentials = listOf(0x0001), + ) + val caps = + com.vitorpamplona.quartz.marmot.mls.tree.Capabilities( + extensions = listOf(0xF2EE, 0x1234), + proposals = listOf(0x000A, 0x000B), + credentials = listOf(0x0001, 0x0002), + ) + // Should not throw. + MlsGroup.requireCapabilitiesMeetRequirements(caps, req, "test") + } + + /** + * End-to-end gate: `addMember` MUST reject a KeyPackage whose leaf + * doesn't advertise the group's `required_capabilities`. We tamper the + * KP's leaf capabilities to drop SelfRemove, then re-encode and re-sign + * to keep the KP signature valid (RFC 9420 §10.1) so the rejection is + * coming from the capability gate and not the signature check. + */ + @Test + fun addMember_rejectsKeyPackageMissingRequiredProposal() = + runBlocking { + // Setup: standard Marmot group (required_capabilities lists + // SelfRemove + MarmotGroupData + Basic). + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + + // Bob's KP, but with SelfRemove stripped from his leaf + // capabilities. He's still announcing himself as a Marmot peer + // — just lying about supporting SelfRemove. + val tampered = createKeyPackageWithoutSelfRemove(bobId) + + assertFailsWith { + manager.addMember(groupId, tampered.toTlsBytes()) + } + } + + /** + * Build a KeyPackage whose leaf [Capabilities] does NOT list 0x000A + * (SelfRemove), then re-sign so the KP's outer signature still + * validates. Useful for testing the §7.2 gate in isolation. + */ + private fun createKeyPackageWithoutSelfRemove(identity: String): com.vitorpamplona.quartz.marmot.mls.messages.MlsKeyPackage { + val tempGroup = MlsGroup.create(identity.hexToByteArray()) + val bundle = tempGroup.createKeyPackage(identity.hexToByteArray(), ByteArray(0)) + val original = bundle.keyPackage + val originalLeaf = original.leafNode + + // Strip SelfRemove (0x000A) from the leaf's advertised proposals. + val tamperedCaps = + originalLeaf.capabilities.copy( + proposals = originalLeaf.capabilities.proposals.filter { it != 0x000A }, + ) + // Re-build the leaf node and re-sign its TBS so the leaf signature + // still verifies (otherwise we'd hit the LeafNode signature check + // before the capability gate fires). + val tamperedLeaf = + originalLeaf.copy(capabilities = tamperedCaps).let { lf -> + val tbs = lf.encodeTbs() + val sig = + com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider + .signWithLabel(bundle.signaturePrivateKey, "LeafNodeTBS", tbs) + lf.copy(signature = sig) + } + // Re-sign the KeyPackage TBS over the new leaf. + val unsigned = original.copy(leafNode = tamperedLeaf, signature = ByteArray(0)) + return unsigned.copy( + signature = + com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider + .signWithLabel(bundle.signaturePrivateKey, "KeyPackageTBS", unsigned.encodeTbs()), + ) + } + // ---------------------------------------------------------------------- // RFC 9420 §5.3 psk_secret derivation // ---------------------------------------------------------------------- From 1f42afa61a7d621c2d539b99983582ffab7197be Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 04:37:57 +0000 Subject: [PATCH 7/9] fix(marmot): hard-fail UpdatePath silent decrypt + verify parent_hash on Welcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two RFC 9420 hardening gaps in the inbound path: **#3 — UpdatePath silent decrypt failures.** When `processCommit` was handed a commit whose UpdatePath either (a) had no node at our common ancestor with the sender or (b) had no ciphertext encrypted to a node in our copath resolution, the path-decrypt block silently fell through and left `commit_secret = 0`. The downstream confirmation_tag check caught it and rolled back, but the rollback surfaced as a generic "ConfirmationTagMismatch" instead of pointing at the real cause — a tree-shape mismatch between our view and the sender's. Now hard-fail with a precise error naming the indices involved. **#6 — parent_hash chain verification on Welcome.** `processWelcome` was checking the GroupInfo signature and the GroupContext.tree_hash, but neither gates a forged parent_hash inside a COMMIT-source leaf — the tree_hash check only proves the ratchet_tree extension matches the bytes the signer signed, not that the stored parent_hash values are consistent with the tree shape. A peer that DOES validate parent_hash (per RFC 9420 §7.9) would reject every commit produced from such a tree, splitting the group on the next epoch. Adds `verifyTreeParentHashesForJoin(tree)` as a static-tree counterpart to `verifyParentHash` (which only handles the post-UpdatePath dynamic case). For each COMMIT-source leaf, recompute the parent_hash chain top-down on the leaf's filtered direct path and compare to the stored value. Returns null on success or a human- readable mismatch reason. Wired into `processWelcome` right after the tree_hash check, before any capability or key-schedule work. Also moves `encodeParentHashInput` into the companion object so the static and instance variants share one TLS encoder, and adds an `exportTreeBytes()` test accessor. 2 new tests: trivial single-member tree accepts; a tampered COMMIT-source parent_hash on a 3-member tree is rejected with a message naming the offending leaf. Quartz marmot tests green; marmot-interop-headless 16/16. (Two unrelated NostrClient network tests fail under the full :quartz:jvmTest run — pre-existing flake, no MLS code touched.) Closes audit gaps #3 and #6. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../quartz/marmot/mls/group/MlsGroup.kt | 252 ++++++++++++++---- .../quartz/marmot/MarmotMipBehaviorTest.kt | 77 ++++++ 2 files changed, 280 insertions(+), 49 deletions(-) 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 eee74727c..8b7a7754f 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 @@ -153,6 +153,18 @@ class MlsGroup private constructor( */ internal fun pendingProposalsSnapshot(): List = pendingProposals.toList() + /** + * Encode the current ratchet tree the same way it's serialized into + * the GroupInfo's `ratchet_tree` extension on a Welcome — a freshly- + * decoded copy is what a joiner sees, so this is the right input for + * exercising [verifyTreeParentHashesForJoin] in tests. + */ + internal fun exportTreeBytes(): ByteArray { + val w = TlsWriter() + tree.encodeTls(w) + return w.toByteArray() + } + // --- Marmot admin helpers (MIP-01 / MIP-03) --- /** Raw BasicCredential identity bytes of the member at the given leaf, or null. */ @@ -1441,50 +1453,65 @@ class MlsGroup private constructor( val commonAncestorNode = if (commonAncestorUnfilteredIdx >= 0) unfilteredDirectPath[commonAncestorUnfilteredIdx] else -1 val commonAncestorFilteredIdx = filteredDp.indexOf(commonAncestorNode) - if (commonAncestorFilteredIdx >= 0 && commonAncestorFilteredIdx < updatePath.nodes.size) { - val pathNode = updatePath.nodes[commonAncestorFilteredIdx] - val copathNodeIdx = filteredCp[commonAncestorFilteredIdx] - val resolution = - tree.resolution(copathNodeIdx).filterNot { resNode -> - BinaryTree.isLeaf(resNode) && - BinaryTree.nodeToLeaf(resNode) in newLeavesInCommit - } - // Find which encrypted secret corresponds to our position - val myNodeIdx = BinaryTree.leafToNode(myLeafIndex) - val myResIdx = resolution.indexOf(myNodeIdx) - - if (myResIdx >= 0 && myResIdx < pathNode.encryptedPathSecret.size) { - val ct = pathNode.encryptedPathSecret[myResIdx] - val pathSecret = - MlsCryptoProvider.decryptWithLabel( - encryptionPrivateKey, - "UpdatePathNode", - pathDecContextBytes, - ct.kemOutput, - ct.ciphertext, - ) - - // Derive remaining path secrets from common ancestor up to root, - // then one more step to reach the `commit_secret` (RFC 9420 §9.2: - // commit_secret = DeriveSecret(root_path_secret, "path")). Openmls - // advances one step past the root; quartz was stopping at the root - // and diverging — that's what caused `ConfirmationTagMismatch` on - // every cross-impl commit. - // - // Step count is measured against the UNFILTERED direct - // path — the path_secret chain advances one KDF step per - // level regardless of whether that level emits a - // UpdatePath node, so filtering changes which nodes carry - // ciphertext but not the number of KDF steps. - val stepsToRoot = unfilteredDirectPath.size - commonAncestorUnfilteredIdx - 1 - var currentSecret = pathSecret - repeat(stepsToRoot) { - currentSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") - } - commitSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") - } + // RFC 9420 §7.6: every existing member that survives the commit + // MUST be able to decrypt the path_secret at the common ancestor + // — otherwise the sender's tree shape doesn't match ours and + // we'd silently advance the epoch with `commit_secret = 0`, + // diverging from the sender's key schedule. The confirmation_tag + // check below catches that downstream and rolls back, but the + // mismatch surfaces as a generic "ConfirmationTagMismatch" + // instead of pointing at the real cause. Hard-fail here with a + // specific error so an interop break is immediately diagnosable. + check(commonAncestorFilteredIdx in 0 until updatePath.nodes.size) { + "UpdatePath has no node at the common ancestor of leaves $senderLeafIndex / $myLeafIndex " + + "(filtered_dp_idx=$commonAncestorFilteredIdx, update_path_nodes=${updatePath.nodes.size})" } + val pathNode = updatePath.nodes[commonAncestorFilteredIdx] + val copathNodeIdx = filteredCp[commonAncestorFilteredIdx] + val resolution = + tree.resolution(copathNodeIdx).filterNot { resNode -> + BinaryTree.isLeaf(resNode) && + BinaryTree.nodeToLeaf(resNode) in newLeavesInCommit + } + + // Find which encrypted secret corresponds to our position + val myNodeIdx = BinaryTree.leafToNode(myLeafIndex) + val myResIdx = resolution.indexOf(myNodeIdx) + check(myResIdx in 0 until pathNode.encryptedPathSecret.size) { + "UpdatePath at common ancestor carries no ciphertext for us " + + "(my_leaf=$myLeafIndex, my_node=$myNodeIdx, resolution=$resolution, " + + "encrypted_path_secrets=${pathNode.encryptedPathSecret.size})" + } + + val ct = pathNode.encryptedPathSecret[myResIdx] + val pathSecret = + MlsCryptoProvider.decryptWithLabel( + encryptionPrivateKey, + "UpdatePathNode", + pathDecContextBytes, + ct.kemOutput, + ct.ciphertext, + ) + + // Derive remaining path secrets from common ancestor up to root, + // then one more step to reach the `commit_secret` (RFC 9420 §9.2: + // commit_secret = DeriveSecret(root_path_secret, "path")). Openmls + // advances one step past the root; quartz was stopping at the root + // and diverging — that's what caused `ConfirmationTagMismatch` on + // every cross-impl commit. + // + // Step count is measured against the UNFILTERED direct + // path — the path_secret chain advances one KDF step per + // level regardless of whether that level emits a + // UpdatePath node, so filtering changes which nodes carry + // ciphertext but not the number of KDF steps. + val stepsToRoot = unfilteredDirectPath.size - commonAncestorUnfilteredIdx - 1 + var currentSecret = pathSecret + repeat(stepsToRoot) { + currentSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") + } + commitSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") } // Update transcript hashes (RFC 9420 Section 8.2) @@ -1861,13 +1888,7 @@ class MlsGroup private constructor( encryptionKey: ByteArray, parentHash: ByteArray, originalSiblingTreeHash: ByteArray, - ): ByteArray { - val w = TlsWriter() - w.putOpaqueVarInt(encryptionKey) - w.putOpaqueVarInt(parentHash) - w.putOpaqueVarInt(originalSiblingTreeHash) - return w.toByteArray() - } + ): ByteArray = Companion.encodeParentHashInput(encryptionKey, parentHash, originalSiblingTreeHash) private fun capturePreUpdateSiblingHashes(senderLeafIndex: Int): Map { val directPath = BinaryTree.directPath(senderLeafIndex, tree.leafCount) @@ -2616,6 +2637,127 @@ class MlsGroup private constructor( } } + /** + * RFC 9420 §7.9.2 ParentHashInput TLS encoding: + * + * ``` + * struct { + * HPKEPublicKey encryption_key; + * opaque parent_hash; + * opaque original_sibling_tree_hash; + * } ParentHashInput; + * ``` + */ + internal fun encodeParentHashInput( + encryptionKey: ByteArray, + parentHash: ByteArray, + originalSiblingTreeHash: ByteArray, + ): ByteArray { + val w = TlsWriter() + w.putOpaqueVarInt(encryptionKey) + w.putOpaqueVarInt(parentHash) + w.putOpaqueVarInt(originalSiblingTreeHash) + return w.toByteArray() + } + + /** + * RFC 9420 §7.9 parent_hash chain verification for a STATIC tree — + * specifically, the ratchet_tree extension a joiner reconstructs + * from a Welcome's GroupInfo. Without this, a malicious or + * misconfigured GroupInfo signer could ship a tree whose stored + * parent_hash values are inconsistent with the actual tree shape; + * peers that DO validate would reject every commit produced from + * this tree, but the joiner wouldn't notice until the next epoch + * silently rolled back. + * + * For each leaf with `source == COMMIT` (the only source that + * carries a parent_hash payload), recompute the parent_hash chain + * top-down on the leaf's filtered direct path and verify the + * leaf's stored parent_hash matches what the chain produces. + * + * Returns `null` on success, or a human-readable failure reason. + * Skips KEY_PACKAGE and UPDATE leaves — those don't carry a + * meaningful parent_hash on the wire. + */ + internal fun verifyTreeParentHashesForJoin(tree: RatchetTree): String? { + if (tree.leafCount == 0) return null + val nodeCount = BinaryTree.nodeCount(tree.leafCount) + for (leafIdx in 0 until tree.leafCount) { + val leaf = tree.getLeaf(leafIdx) ?: continue + if (leaf.leafNodeSource != LeafNodeSource.COMMIT) continue + val expected = computeStaticLeafParentHash(tree, leafIdx, nodeCount) + val stored = leaf.parentHash ?: ByteArray(0) + if (!stored.contentEquals(expected)) { + return "leaf $leafIdx parent_hash mismatch (stored=${stored.size}B, expected=${expected.size}B)" + } + } + return null + } + + /** + * Top-down recomputation of the parent_hash that a COMMIT-source + * leaf at [leafIdx] should carry, given the current tree shape. + * Mirrors [computeSenderParentHashes] but uses + * [RatchetTree.treeHashNode] for sibling tree hashes (no + * pre-update / post-update distinction in static validation). + */ + private fun computeStaticLeafParentHash( + tree: RatchetTree, + leafIdx: Int, + nodeCount: Int, + ): ByteArray { + val (filteredDp, _) = tree.filteredDirectPath(leafIdx) + if (filteredDp.isEmpty()) return ByteArray(0) + + // Walk top-down from root, propagating the expected parent_hash. + val hashes = mutableMapOf() + hashes[filteredDp.last()] = ByteArray(0) + for (i in filteredDp.size - 2 downTo 0) { + val xIdx = filteredDp[i] + val parentIdx = filteredDp[i + 1] + val parentNode = tree.getNode(parentIdx) + if (parentNode !is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent) { + hashes[xIdx] = ByteArray(0) + continue + } + // x's sibling under parent — parent has children left/right; + // sibling is whichever isn't x's ancestor. + val left = BinaryTree.left(parentIdx) + val right = BinaryTree.right(parentIdx) + val siblingIdx = if (xIdx == left) right else left + val siblingTreeHash = tree.treeHashNode(siblingIdx) + hashes[xIdx] = + MlsCryptoProvider.hash( + encodeParentHashInput( + encryptionKey = parentNode.parentNode.encryptionKey, + parentHash = hashes[parentIdx] ?: ByteArray(0), + originalSiblingTreeHash = siblingTreeHash, + ), + ) + } + + // The leaf's expected parent_hash is the chain value AT the + // immediate parent (filteredDp[0]) — same convention as the + // committer-side computation in [computeSenderParentHashes]. + val immediateParentIdx = filteredDp.first() + val immediateParent = tree.getNode(immediateParentIdx) + if (immediateParent !is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent) { + return ByteArray(0) + } + // Sibling of the leaf's node at the immediate parent. + val leafNodeIdx = BinaryTree.leafToNode(leafIdx) + val left = BinaryTree.left(immediateParentIdx) + val right = BinaryTree.right(immediateParentIdx) + val leafSiblingIdx = if (leafNodeIdx == left) right else left + return MlsCryptoProvider.hash( + encodeParentHashInput( + encryptionKey = immediateParent.parentNode.encryptionKey, + parentHash = hashes[immediateParentIdx] ?: ByteArray(0), + originalSiblingTreeHash = tree.treeHashNode(leafSiblingIdx), + ), + ) + } + /** * Default MLS leaf Capabilities that advertise support for Marmot's * required extensions and proposals so new members can join a group @@ -2805,6 +2947,18 @@ class MlsGroup private constructor( "GroupInfo tree_hash does not match ratchet_tree extension" } + // RFC 9420 §7.9 parent_hash integrity. The tree_hash check + // above only proves the wire bytes match what the GroupInfo + // signer signed — it does NOT validate that the stored + // parent_hash values are consistent with the tree shape. + // Without this, every COMMIT-source leaf could carry a + // forged parent_hash and the group would accept it; every + // commit produced from this tree would then be rejected by + // peers that DO validate, splitting on the next epoch. + verifyTreeParentHashesForJoin(tree)?.let { reason -> + throw IllegalStateException("GroupInfo ratchet_tree fails parent_hash validation: $reason") + } + // RFC 9420 §7.2 + §12.4.3.1: a joiner MUST refuse to join a // group whose `required_capabilities` lists types the joiner's // own KeyPackage doesn't advertise — peers that DO enforce the diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index 18bf4e76f..f21557b38 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -551,6 +551,83 @@ class MarmotMipBehaviorTest { ) } + // ---------------------------------------------------------------------- + // RFC 9420 §7.9 parent_hash chain validation on Welcome + // ---------------------------------------------------------------------- + + /** + * A freshly-created single-member tree has no COMMIT-source leaves + * (the seed leaf is KEY_PACKAGE source) and no parents — the + * static-tree validator must accept it as trivially valid. + */ + @Test + fun verifyTreeParentHashesForJoin_acceptsSingleMemberTree() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val tree = + com.vitorpamplona.quartz.marmot.mls.tree.RatchetTree + .decodeTls( + com.vitorpamplona.quartz.marmot.mls.codec + .TlsReader(alice.exportTreeBytes()), + ) + assertNull(MlsGroup.verifyTreeParentHashesForJoin(tree)) + } + + /** + * Tampering a COMMIT-source leaf's parent_hash produces a clear + * rejection from the validator. This is the exact attack a + * misconfigured GroupInfo signer (or a malicious one) could mount — + * the tree_hash check would still pass, but parent_hash chain + * validation must catch it. + */ + @Test + fun verifyTreeParentHashesForJoin_rejectsTamperedLeafParentHash() = + runBlocking { + // Build a 3-member group so we have at least one + // COMMIT-source leaf with a real parent_hash chain. + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + manager.addMember(groupId, createStandaloneKeyPackage(bobId).keyPackage.toTlsBytes()) + // Trigger a self-update on Alice so her leaf becomes + // COMMIT-source with a non-empty parent_hash chain. (Using + // updateGroupExtensions as a stand-in to force a commit + // that touches the path.) + val seed = MarmotGroupData(nostrGroupId = groupId, adminPubkeys = listOf(aliceId)) + manager.updateGroupExtensions(groupId, listOf(seed.toExtension())) + + val alice = manager.getGroup(groupId)!! + val originalTree = + com.vitorpamplona.quartz.marmot.mls.tree.RatchetTree + .decodeTls( + com.vitorpamplona.quartz.marmot.mls.codec + .TlsReader(alice.exportTreeBytes()), + ) + + // Sanity: the original tree validates. + assertNull(MlsGroup.verifyTreeParentHashesForJoin(originalTree)) + + // Find the first COMMIT-source leaf and tamper its parent_hash. + var tamperedLeafIdx = -1 + for (i in 0 until originalTree.leafCount) { + val leaf = originalTree.getLeaf(i) ?: continue + if (leaf.leafNodeSource == + com.vitorpamplona.quartz.marmot.mls.tree.LeafNodeSource.COMMIT + ) { + val tampered = leaf.copy(parentHash = ByteArray(32) { 0x99.toByte() }) + originalTree.setLeaf(i, tampered) + tamperedLeafIdx = i + break + } + } + assertTrue(tamperedLeafIdx >= 0, "test setup must produce a COMMIT-source leaf") + + val reason = MlsGroup.verifyTreeParentHashesForJoin(originalTree) + assertNotNull(reason) + assertTrue( + reason.contains("leaf $tamperedLeafIdx parent_hash mismatch"), + "rejection message must name the tampered leaf: $reason", + ) + } + // ---------------------------------------------------------------------- // RFC 9420 §7.2 / §12.4.2 required_capabilities enforcement // ---------------------------------------------------------------------- From 8c38394385b5b74cd0218bc60a647b6659d353d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 04:48:40 +0000 Subject: [PATCH 8/9] fix(marmot): bound forward-ratchet steps and tighten PrivateMessage AAD cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two DoS surfaces in the inbound path: **#8 — unbounded forward-ratchet on PrivateMessage decrypt.** A malicious sender (or any peer who can put bytes in the wire frame) controls the `generation` field of an inbound PrivateMessage. The SecretTree was happy to fast-forward the sender's application or handshake ratchet by however many steps that field implied — every step costing one HKDF-Expand. A single packet with `generation = 2^31` would have pinned a CPU at SHA-256 for minutes. The skipped-key cache (`MAX_SKIPPED_KEYS = 1000`) bounds memory but NOT compute: it just stops *caching* past the cap, the ratchet keeps walking. Adds `MAX_RATCHET_STEPS_PER_CALL = 4096` and rejects any decrypt that asks for a larger jump from the sender's current head, applied at both `applicationKeyNonceForGeneration` and `handshakeKeyNonceForGeneration`. 4096 leaves room for legitimate catch-up (mobile waking from a long sleep) while bounding the worst-case per-packet cost to ~4096 SHA-256 invocations. **#11 — oversized PrivateMessage AAD allocation.** The underlying [TlsReader] already caps every opaque read at 1 MiB, so the ciphertext field is bounded — but `authenticated_data` and `encrypted_sender_data` were also allowed up to 1 MiB even though both fields legitimately carry a few hundred bytes at most. A pre-verification frame would force ~3 MiB of allocation per inbound packet. Tightens both fields to 64 KiB at PrivateMessage decode (below TlsReader's global cap) so the per-frame floor is predictable. 2 new tests: `secretTree_rejectsRatchetJumpsBeyondCap` exercises the boundary (4096 OK, larger throws with a "jump too large" message) and `privateMessage_rejectsOversizedAuthenticatedData` hand-builds a frame with a 65 KiB AAD field and confirms rejection. Marmot test suite green; interop 16/16. Closes audit gaps #8 and #11. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../quartz/marmot/mls/framing/MlsMessage.kt | 54 +++++++++++++--- .../quartz/marmot/mls/schedule/SecretTree.kt | 31 ++++++++++ .../quartz/marmot/MarmotMipBehaviorTest.kt | 61 +++++++++++++++++++ 3 files changed, 138 insertions(+), 8 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/framing/MlsMessage.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/framing/MlsMessage.kt index db6b9e638..f01a6f465 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/framing/MlsMessage.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/framing/MlsMessage.kt @@ -196,11 +196,17 @@ data class PublicMessage( override fun hashCode(): Int = groupId.contentHashCode() companion object { + /** Same DoS cap that PrivateMessage applies to its AAD field. */ + const val MAX_AUTHENTICATED_DATA_BYTES = 1 shl 16 // 64 KiB + fun decodeTls(reader: TlsReader): PublicMessage { val groupId = reader.readOpaqueVarInt() val epoch = reader.readUint64() val sender = decodeSender(reader) val authenticatedData = reader.readOpaqueVarInt() + require(authenticatedData.size <= MAX_AUTHENTICATED_DATA_BYTES) { + "PublicMessage authenticated_data too large: ${authenticatedData.size} > $MAX_AUTHENTICATED_DATA_BYTES" + } val contentType = ContentType.fromValue(reader.readUint8()) // RFC 9420 §6 FramedContent.content body varies by content_type. @@ -289,15 +295,47 @@ data class PrivateMessage( override fun hashCode(): Int = groupId.contentHashCode() companion object { - fun decodeTls(reader: TlsReader): PrivateMessage = - PrivateMessage( - groupId = reader.readOpaqueVarInt(), - epoch = reader.readUint64(), - contentType = ContentType.fromValue(reader.readUint8()), - authenticatedData = reader.readOpaqueVarInt(), - encryptedSenderData = reader.readOpaqueVarInt(), - ciphertext = reader.readOpaqueVarInt(), + /** + * Per-field DoS cap for `authenticated_data` and + * `encrypted_sender_data`. The underlying [TlsReader] already + * refuses any opaque field larger than 1 MiB (its global + * `MAX_OPAQUE_SIZE`), but those fields shouldn't carry anything + * close to that — `encrypted_sender_data` is always exactly the + * sender-data AEAD ciphertext (a couple hundred bytes), and + * Marmot doesn't put anything in `authenticated_data` today. + * Tightening to 64 KiB makes the per-frame memory floor + * predictable and turns "we'd allocate up to 3 MiB on every + * inbound packet that escapes verification" into "we'll + * allocate at most ~1 MiB even before AEAD." + */ + const val MAX_AAD_BYTES = 1 shl 16 // 64 KiB + + fun decodeTls(reader: TlsReader): PrivateMessage { + val groupId = reader.readOpaqueVarInt() + val epoch = reader.readUint64() + val contentType = ContentType.fromValue(reader.readUint8()) + val authenticatedData = reader.readOpaqueVarInt() + require(authenticatedData.size <= MAX_AAD_BYTES) { + "PrivateMessage authenticated_data too large: ${authenticatedData.size} > $MAX_AAD_BYTES" + } + val encryptedSenderData = reader.readOpaqueVarInt() + require(encryptedSenderData.size <= MAX_AAD_BYTES) { + "PrivateMessage encrypted_sender_data too large: ${encryptedSenderData.size} > $MAX_AAD_BYTES" + } + // `ciphertext` size is bounded by [TlsReader.MAX_OPAQUE_SIZE] + // (1 MiB) — which dominates any peer-side practical limit + // (Marmot kind:445 events ride a Nostr relay's per-event + // ceiling, typically 64 KiB). No tighter cap needed here. + val ciphertext = reader.readOpaqueVarInt() + return PrivateMessage( + groupId = groupId, + epoch = epoch, + contentType = contentType, + authenticatedData = authenticatedData, + encryptedSenderData = encryptedSenderData, + ciphertext = ciphertext, ) + } } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt index 596b80262..ccb8dfeab 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt @@ -71,6 +71,23 @@ class SecretTree( /** Maximum consumed generation entries to track per sender before pruning. */ const val MAX_CONSUMED_GENERATIONS_PER_SENDER = 1000 + + /** + * Hard cap on how many ratchet steps a single decrypt request may + * fast-forward through. Without this an attacker who can deliver a + * single PrivateMessage with a forged `generation` field can force + * the receiver into ~`generation` HKDF-Expand steps — trivially + * many seconds of CPU per packet. The skipped-key cache itself + * stops at [MAX_SKIPPED_KEYS] entries, but the ratchet keeps + * advancing past that point, so the cache cap doesn't bound + * compute cost. + * + * 4096 leaves room for a realistic application/handshake gap + * (e.g. mobile catching up after a long sleep) while making the + * attack worst-case ~4096 SHA-256 invocations — bounded enough + * that the receiver stays responsive. + */ + const val MAX_RATCHET_STEPS_PER_CALL = 4096 } /** @@ -149,6 +166,15 @@ class SecretTree( require(generation >= state.applicationGeneration) { "Generation $generation already consumed (current: ${state.applicationGeneration})" } + // DoS guard: a malicious sender can put any uint32 in the + // PrivateMessage generation field, and without this cap a single + // packet would force unbounded HKDF-Expand work. See + // MAX_RATCHET_STEPS_PER_CALL. + require(generation - state.applicationGeneration <= MAX_RATCHET_STEPS_PER_CALL) { + "Application generation jump too large: " + + "${generation - state.applicationGeneration} steps (cap $MAX_RATCHET_STEPS_PER_CALL); " + + "sender $leafIndex from ${state.applicationGeneration} to $generation" + } // Replay detection: reject if this (sender, generation) was already used val senderConsumed = consumedGenerations.getOrPut(leafIndex) { mutableSetOf() } @@ -219,6 +245,11 @@ class SecretTree( require(generation >= state.handshakeGeneration) { "Handshake generation $generation already consumed (current: ${state.handshakeGeneration})" } + require(generation - state.handshakeGeneration <= MAX_RATCHET_STEPS_PER_CALL) { + "Handshake generation jump too large: " + + "${generation - state.handshakeGeneration} steps (cap $MAX_RATCHET_STEPS_PER_CALL); " + + "sender $leafIndex from ${state.handshakeGeneration} to $generation" + } val senderConsumed = consumedHandshakeGenerations.getOrPut(leafIndex) { mutableSetOf() } require(generation !in senderConsumed) { diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index f21557b38..6c90c280c 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -551,6 +551,67 @@ class MarmotMipBehaviorTest { ) } + // ---------------------------------------------------------------------- + // DoS guards: forward-ratchet cap + PrivateMessage size caps + // ---------------------------------------------------------------------- + + /** + * A receiver that's been silent at generation 0 for one peer must + * NOT be forced to do unbounded HKDF-Expand work just because the + * peer (or an attacker) sets a multi-billion `generation` field on + * an inbound PrivateMessage. The SecretTree caps the per-call + * ratchet jump at 4096 — anything past that throws before any HKDF + * runs. + */ + @Test + fun secretTree_rejectsRatchetJumpsBeyondCap() { + val st = + com.vitorpamplona.quartz.marmot.mls.schedule.SecretTree( + encryptionSecret = ByteArray(32), + leafCount = 1, + ) + // 4096 is allowed (boundary case — fast-forwards 4096 steps). + st.applicationKeyNonceForGeneration(0, 4096) + // 4097 above the previous head is rejected. + val ex = + assertFailsWith { + st.applicationKeyNonceForGeneration(0, 4096 + 4097 + 1) + } + assertTrue(ex.message!!.contains("jump too large"), "must explain the cap: ${ex.message}") + } + + /** + * Marmot tightens `authenticated_data` to 64 KiB even though the + * underlying TlsReader cap is 1 MiB — those fields are never + * legitimately that large, and tightening turns a 1 MiB pre- + * verification allocation into 64 KiB. Hand-build a frame with a + * 65 KiB authenticated_data and verify rejection. + */ + @Test + fun privateMessage_rejectsOversizedAuthenticatedData() { + val w = + com.vitorpamplona.quartz.marmot.mls.codec + .TlsWriter() + w.putOpaqueVarInt(ByteArray(32)) // group_id + w.putUint64(0L) // epoch + w.putUint8(1) // content_type = APPLICATION + w.putOpaqueVarInt(ByteArray((1 shl 16) + 1)) // authenticated_data: 64 KiB + 1 + w.putOpaqueVarInt(ByteArray(0)) // encrypted_sender_data + w.putOpaqueVarInt(ByteArray(64)) // ciphertext (small) + val ex = + assertFailsWith { + com.vitorpamplona.quartz.marmot.mls.framing.PrivateMessage + .decodeTls( + com.vitorpamplona.quartz.marmot.mls.codec + .TlsReader(w.toByteArray()), + ) + } + assertTrue( + ex.message!!.contains("authenticated_data too large"), + "must name the field: ${ex.message}", + ) + } + // ---------------------------------------------------------------------- // RFC 9420 §7.9 parent_hash chain validation on Welcome // ---------------------------------------------------------------------- From 57c3265ca08bbab735f47add31c1b193c5761693 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 05:00:24 +0000 Subject: [PATCH 9/9] =?UTF-8?q?feat(marmot):=20receive=20standalone=20Priv?= =?UTF-8?q?ateMessage=20proposals=20(RFC=209420=20=C2=A76.3.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PROPOSAL branch of `MlsGroup.decrypt` previously threw `IllegalStateException("Standalone PrivateMessage proposals not yet supported")`. That worked in the openmls-as-Whitenoise topology because MDK emits SelfRemove as a PublicMessage — but any peer using the AlwaysCiphertext wire-format policy (RFC 9420 §6.3.2) wraps standalone proposals in PrivateMessage, and we'd hard-fail on first contact. Implements the PROPOSAL branch symmetrically with the existing COMMIT branch: 1. Decode the Proposal struct from PrivateMessageContent (no length prefix), read signature, drain zero-padding. 2. Restrict to SelfRemove (mirrors `receivePublicMessageProposal`'s policy — the only standalone proposal type that needs to ride outside a commit; widening is one-line if interop demands it). 3. Verify the FramedContentTBS signature with `wire_format = PRIVATE_MESSAGE` against the sender's leaf signature_key. 4. Stage the proposal in `pendingProposals` together with the encoded AuthenticatedContent (wire_format ‖ FramedContent ‖ signature) so a subsequent inbound commit folding it in by ProposalRef can resolve via the §5.2 hash. PrivateMessage AC bytes carry no membership_tag — auth = signature only. Adds a symmetric `encryptProposalAsPrivateMessage` helper (internal, mirrors `encrypt` for application data) so tests can exercise the inbound path with a real wire frame produced by the same code path peers use. 2 new tests: round-trip Bob→Alice SelfRemove via Welcome flow with independent MlsGroup instances, verifies the proposal lands in Alice's pending pool with AC bytes captured; and rejection of a non-SelfRemove standalone PrivateMessage proposal (PSK in the test). Marmot test suite green; marmot-interop-headless 16/16. Closes audit gap #1. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../quartz/marmot/mls/group/MlsGroup.kt | 180 +++++++++++++++++- .../quartz/marmot/MarmotMipBehaviorTest.kt | 86 +++++++++ 2 files changed, 265 insertions(+), 1 deletion(-) 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 8b7a7754f..40bff183f 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 @@ -873,6 +873,101 @@ class MlsGroup private constructor( return MlsMessage.fromPrivateMessage(msg).toTlsBytes() } + /** + * Encrypt a standalone Proposal as a PrivateMessage (RFC 9420 §6.3.2, + * content_type = PROPOSAL). Mirrors [encrypt] but writes a Proposal + * struct + signature in the PrivateMessageContent instead of + * application_data. The receiver-side counterpart lives in [decrypt] + * under `ContentType.PROPOSAL`. + * + * Used today only for tests — production callers publish SelfRemove + * via [buildSelfRemoveProposalMessage] (PublicMessage) — but exposing + * the encode path keeps the wire format symmetric so a future caller + * that wants a fully-encrypted proposal envelope doesn't have to + * re-derive it. + */ + internal fun encryptProposalAsPrivateMessage(proposal: Proposal): ByteArray { + if (sentKeys.size > MAX_SENT_KEYS) { + val sortedKeys = sentKeys.keys.sorted() + val toRemove = sortedKeys.take(sentKeys.size - MAX_SENT_KEYS) + for (key in toRemove) sentKeys.remove(key) + } + + // PROPOSALs ride the handshake ratchet (RFC 9420 §6.3.2), not + // the application ratchet — same as PrivateMessage commits. + val kng = secretTree.nextHandshakeKeyNonce(myLeafIndex) + + val reuseGuard = MlsCryptoProvider.randomBytes(REUSE_GUARD_LENGTH) + val guardedNonce = kng.nonce.copyOf() + for (i in 0 until REUSE_GUARD_LENGTH) { + guardedNonce[i] = (guardedNonce[i].toInt() xor reuseGuard[i].toInt()).toByte() + } + + val proposalWriter = TlsWriter() + proposal.encodeTls(proposalWriter) + val proposalBytes = proposalWriter.toByteArray() + + // FramedContentTBS for a member-sender PROPOSAL over PRIVATE_MESSAGE. + val tbsWriter = TlsWriter() + tbsWriter.putUint16(MlsMessage.MLS_VERSION_10) + tbsWriter.putUint16(WireFormat.PRIVATE_MESSAGE.value) + tbsWriter.putOpaqueVarInt(groupId) + tbsWriter.putUint64(epoch) + encodeSender(tbsWriter, Sender(SenderType.MEMBER, myLeafIndex)) + tbsWriter.putOpaqueVarInt(ByteArray(0)) // authenticated_data + tbsWriter.putUint8(ContentType.PROPOSAL.value) + tbsWriter.putBytes(proposalBytes) + tbsWriter.putBytes(groupContext.toTlsBytes()) + val signature = MlsCryptoProvider.signWithLabel(signingPrivateKey, "FramedContentTBS", tbsWriter.toByteArray()) + + // PrivateMessageContent for PROPOSAL: proposal struct (no length + // prefix) || signature || padding. + val pmcWriter = TlsWriter() + pmcWriter.putBytes(proposalBytes) + pmcWriter.putOpaqueVarInt(signature) + val pmcPlaintext = pmcWriter.toByteArray() + + val contentAad = buildPrivateContentAAD(groupId, epoch, ContentType.PROPOSAL, ByteArray(0)) + val ciphertext = MlsCryptoProvider.aeadEncrypt(kng.key, guardedNonce, contentAad, pmcPlaintext) + + val senderDataWriter = TlsWriter() + senderDataWriter.putUint32(myLeafIndex.toLong()) + senderDataWriter.putUint32(kng.generation.toLong()) + senderDataWriter.putBytes(reuseGuard) + val senderDataPlain = senderDataWriter.toByteArray() + + val ciphertextSample = ciphertext.copyOfRange(0, minOf(ciphertext.size, MlsCryptoProvider.HASH_OUTPUT_LENGTH)) + val senderDataKey = + MlsCryptoProvider.expandWithLabel( + epochSecrets.senderDataSecret, + "key", + ciphertextSample, + MlsCryptoProvider.AEAD_KEY_LENGTH, + ) + val senderDataNonce = + MlsCryptoProvider.expandWithLabel( + epochSecrets.senderDataSecret, + "nonce", + ciphertextSample, + MlsCryptoProvider.AEAD_NONCE_LENGTH, + ) + val senderDataAad = buildSenderDataAAD(groupId, epoch, ContentType.PROPOSAL) + val encryptedSenderData = + MlsCryptoProvider.aeadEncrypt(senderDataKey, senderDataNonce, senderDataAad, senderDataPlain) + + return MlsMessage + .fromPrivateMessage( + PrivateMessage( + groupId = groupId, + epoch = epoch, + contentType = ContentType.PROPOSAL, + authenticatedData = ByteArray(0), + encryptedSenderData = encryptedSenderData, + ciphertext = ciphertext, + ), + ).toTlsBytes() + } + /** * Decrypt an application message from a PrivateMessage. * Returns null if decryption fails (e.g., corrupted message, wrong epoch). @@ -1049,7 +1144,90 @@ class MlsGroup private constructor( } ContentType.PROPOSAL -> { - throw IllegalStateException("Standalone PrivateMessage proposals not yet supported") + // PrivateMessageContent for a Proposal: the Proposal struct + // (no length prefix) followed by signature, then zero + // padding. There's no membership_tag — PrivateMessage is + // already AEAD-protected by sender membership in the + // secret tree, so RFC 9420 §6.2's HMAC isn't applied. + val proposal = Proposal.decodeTls(pmcReader) + val proposalWriter = TlsWriter() + proposal.encodeTls(proposalWriter) + val proposalBytes = proposalWriter.toByteArray() + val signature = pmcReader.readOpaqueVarInt() + while (pmcReader.hasRemaining) { + require(pmcReader.readBytes(1)[0] == 0.toByte()) { + "PrivateMessageContent padding must be zero" + } + } + + // Mirror the same defensive policy as + // [receivePublicMessageProposal]: only SelfRemove proposals + // legitimately arrive standalone today (openmls/mdk emit + // it as a separate PROPOSAL message because non-admins + // can't self-remove via a commit). Reject anything else + // here so a peer can't pre-stage e.g. an Add we never asked + // to fold in. Widen if/when interop demands it. + require(proposal is Proposal.SelfRemove) { + "Only SelfRemove is accepted as a standalone PrivateMessage proposal " + + "(got ${proposal::class.simpleName}); other proposal types must be folded into a commit" + } + + // Verify the FramedContent signature with wire_format = + // PRIVATE_MESSAGE (not PUBLIC_MESSAGE) so the TBS bytes + // match what the sender signed. + val tbsWriter = TlsWriter() + tbsWriter.putUint16(MlsMessage.MLS_VERSION_10) + tbsWriter.putUint16(WireFormat.PRIVATE_MESSAGE.value) + tbsWriter.putOpaqueVarInt(privMsg.groupId) + tbsWriter.putUint64(privMsg.epoch) + encodeSender(tbsWriter, Sender(SenderType.MEMBER, senderLeafIndex)) + tbsWriter.putOpaqueVarInt(privMsg.authenticatedData) + tbsWriter.putUint8(ContentType.PROPOSAL.value) + tbsWriter.putBytes(proposalBytes) + tbsWriter.putBytes(groupContext.toTlsBytes()) // member sender appends context + val tbs = tbsWriter.toByteArray() + + val senderLeaf = + requireNotNull(tree.getLeaf(senderLeafIndex)) { + "Sender leaf is blank at index $senderLeafIndex" + } + require( + MlsCryptoProvider.verifyWithLabel( + senderLeaf.signatureKey, + "FramedContentTBS", + tbs, + signature, + ), + ) { "FramedContentTBS signature verification failed on PrivateMessage proposal" } + + // RFC 9420 §5.2: ProposalRef hashes the encoded + // AuthenticatedContent. For PrivateMessage proposals the + // AC carries no membership_tag — auth = signature only. + // Stash these bytes so a future commit referencing this + // proposal by hash resolves against our pool. + val acWriter = TlsWriter() + acWriter.putUint16(WireFormat.PRIVATE_MESSAGE.value) + acWriter.putOpaqueVarInt(privMsg.groupId) + acWriter.putUint64(privMsg.epoch) + encodeSender(acWriter, Sender(SenderType.MEMBER, senderLeafIndex)) + acWriter.putOpaqueVarInt(privMsg.authenticatedData) + acWriter.putUint8(ContentType.PROPOSAL.value) + acWriter.putBytes(proposalBytes) + acWriter.putOpaqueVarInt(signature) + pendingProposals.add( + PendingProposal( + proposal = proposal, + senderLeafIndex = senderLeafIndex, + authenticatedContentBytes = acWriter.toByteArray(), + ), + ) + + return DecryptedMessage( + senderLeafIndex = senderLeafIndex, + contentType = privMsg.contentType, + content = proposalBytes, + epoch = privMsg.epoch, + ) } } } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index 6c90c280c..2dcb186e0 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -551,6 +551,92 @@ class MarmotMipBehaviorTest { ) } + // ---------------------------------------------------------------------- + // RFC 9420 §6.3.2 standalone PrivateMessage proposal RX + // ---------------------------------------------------------------------- + + /** + * Round-trip: Bob (member 1) encrypts a SelfRemove as a + * PrivateMessage PROPOSAL, Alice (admin, member 0) decrypts it. + * Alice's pending pool picks it up with the AC bytes captured for + * RFC 9420 §5.2 ProposalRef matching, so a follow-on commit + * referencing the proposal by hash resolves. + */ + @Test + fun decrypt_acceptsStandaloneSelfRemoveAsPrivateMessage() = + runBlocking { + val (alice, bob) = build2MemberGroupWithBobJoined() + + val proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .SelfRemove() + val before = alice.pendingProposalsSnapshot().size + val wireBytes = bob.encryptProposalAsPrivateMessage(proposal) + val result = alice.decrypt(wireBytes) + + assertEquals( + com.vitorpamplona.quartz.marmot.mls.framing.ContentType.PROPOSAL, + result.contentType, + ) + assertEquals(bob.leafIndex, result.senderLeafIndex) + val after = alice.pendingProposalsSnapshot() + assertEquals(before + 1, after.size, "decrypt must stage the proposal in pending pool") + val staged = after.last() + assertIs(staged.proposal) + assertEquals(bob.leafIndex, staged.senderLeafIndex) + assertNotNull( + staged.authenticatedContentBytes, + "PrivateMessage proposal must capture AC bytes for RFC 9420 §5.2 ProposalRef matching", + ) + assertTrue(staged.authenticatedContentBytes.isNotEmpty()) + } + + /** + * Standalone PrivateMessage proposals are restricted to SelfRemove + * (mirrors `receivePublicMessageProposal`'s policy). A peer that + * tries to pre-stage e.g. a PSK proposal must be rejected, even + * though the AEAD layer accepted the frame. + */ + @Test + fun decrypt_rejectsNonSelfRemovePrivateMessageProposal() = + runBlocking { + val (alice, bob) = build2MemberGroupWithBobJoined() + + val proposal = + com.vitorpamplona.quartz.marmot.mls.messages + .Proposal + .Psk(pskType = 1, pskId = ByteArray(16) { 0xAB.toByte() }, pskNonce = ByteArray(16)) + val wireBytes = bob.encryptProposalAsPrivateMessage(proposal) + + val ex = + assertFailsWith { + alice.decrypt(wireBytes) + } + assertTrue( + ex.message!!.contains("Only SelfRemove"), + "rejection must explain the policy: ${ex.message}", + ) + } + + /** + * Build a two-member group via the real Welcome flow so Alice and + * Bob have INDEPENDENT [MlsGroup] instances (independent secret-tree + * consumed-generation sets) — required for any test that + * encrypts on one side and decrypts on the other. + */ + private suspend fun build2MemberGroupWithBobJoined(): Pair { + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + val bobBundle = createStandaloneKeyPackage(bobId) + val commitResult = manager.addMember(groupId, bobBundle.keyPackage.toTlsBytes()) + val alice = manager.getGroup(groupId)!! + val welcomeBytes = + requireNotNull(commitResult.welcomeBytes) { "addMember must produce a Welcome" } + val bob = MlsGroup.processWelcome(welcomeBytes, bobBundle) + return alice to bob + } + // ---------------------------------------------------------------------- // DoS guards: forward-ratchet cap + PrivateMessage size caps // ----------------------------------------------------------------------