diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt index 92a194a95..d26302f4f 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -2263,6 +2263,14 @@ class Account( /** * Leave a Marmot MLS group. * Publishes the SelfRemove proposal and removes local state. + * + * MIP-01/MIP-03: admins MUST first publish a GroupContextExtensions + * commit dropping themselves from `admin_pubkeys` before issuing a + * SelfRemove proposal. Without that, [MlsGroup.selfRemove] throws + * `IllegalStateException("Admin must self-demote via GroupContextExtensions + * before SelfRemove (MIP-01)")` and the leave aborts. Demote commit and + * SelfRemove proposal both go to the same group relays, demote first so + * peers apply it before they see the SelfRemove. */ suspend fun leaveMarmotGroup( nostrGroupId: HexKey, @@ -2271,6 +2279,27 @@ class Account( val manager = marmotManager ?: return if (!isWriteable()) return + val metadata = manager.groupMetadata(nostrGroupId) + if (metadata != null && metadata.adminPubkeys.contains(signer.pubKey)) { + val remaining = metadata.adminPubkeys.filter { it != signer.pubKey }.toMutableList() + // MIP-03 also rejects any GCE commit that leaves the group with zero + // admins. If we're the only one, promote an arbitrary non-self + // member to admin before stepping down. + if (remaining.isEmpty()) { + val heir = + manager + .memberPubkeys(nostrGroupId) + .map { it.pubkey } + .firstOrNull { it != signer.pubKey } + if (heir != null) remaining.add(heir) + } + if (remaining.isNotEmpty()) { + val demoted = metadata.copy(adminPubkeys = remaining) + val demoteCommit = manager.updateGroupMetadata(nostrGroupId, demoted) + client.publish(demoteCommit.signedEvent, groupRelays) + } + } + val outbound = manager.leaveGroup(nostrGroupId) client.publish(outbound.signedEvent, groupRelays) } diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt index 638376795..e2540ebbc 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt @@ -219,14 +219,26 @@ class Context( * - kind:445 group events per active group → feed into inbound processor * * Incrementally advances the `since` cursors in [state] so the next run - * only asks relays for newer events. + * only asks relays for newer events. Two wrinkles: + * + * 1. NIP-59 gift wraps are published with a random-past `created_at` + * (see [com.vitorpamplona.quartz.utils.TimeUtils.randomWithTwoDays]) + * so a newly-published wrap can trivially have `created_at` earlier + * than the last cursor we saw. To avoid silently dropping such wraps + * we always subtract a 2-day lookback window from the gift-wrap + * `since`, and dedup is handled inside [MarmotInboundProcessor]. + * 2. We only advance the on-disk cursor when events actually arrive. + * Snapping an empty sync up to "now" on the first invocation would + * make every later `since` query skip any past-dated wrap or 445. */ suspend fun syncIncoming(timeoutMs: Long = 8_000) { val inbox = inboxRelays().ifEmpty { anyRelays() } val gwSince = state.giftWrapSince + val gwFilterSince = + gwSince?.let { (it - GIFT_WRAP_LOOKBACK_SECS).coerceAtLeast(0L) } val gwFilter = - if (gwSince != null) { - MarmotFilters.giftWrapsForUserSince(identity.pubKeyHex, gwSince) + if (gwFilterSince != null) { + MarmotFilters.giftWrapsForUserSince(identity.pubKeyHex, gwFilterSince) } else { MarmotFilters.giftWrapsForUser(identity.pubKeyHex) } @@ -255,34 +267,89 @@ class Context( if (filterMap.isEmpty()) return val events = drain(filterMap, timeoutMs) - val now = System.currentTimeMillis() / 1000 var maxGwSeen = gwSince ?: 0L val maxGroupSeen = perGroupFilters.keys.associateWith { state.groupSince[it] ?: 0L }.toMutableMap() + var sawGiftWrap = false + val sawGroupEvent = mutableSetOf() for ((relay, event) in events) { // All the MLS/NIP-59 decryption + persistence lives in MarmotIngest — // we only care about bookkeeping (since-cursors, logging) here. val result = marmot.ingest(event) - System.err.println("[cli] ingest ${event.kind}/${event.id.take(8)} via $relay → ${result::class.simpleName}") + val detail = + when (result) { + is com.vitorpamplona.amethyst.commons.marmot.MarmotIngestResult.Failure -> " ${result.message}" + else -> "" + } + System.err.println("[cli] ingest ${event.kind}/${event.id.take(8)} via $relay → ${result::class.simpleName}$detail") when (event.kind) { GiftWrapEvent.KIND -> { + sawGiftWrap = true if (event.createdAt > maxGwSeen) maxGwSeen = event.createdAt } GroupEvent.KIND -> { val gid = (event as? GroupEvent)?.groupId() ?: continue + sawGroupEvent.add(gid) val prev = maxGroupSeen[gid] ?: 0L if (event.createdAt > prev) maxGroupSeen[gid] = event.createdAt } } } - state.giftWrapSince = if (maxGwSeen > 0) maxGwSeen else now - for ((gid, seen) in maxGroupSeen) { - state.groupSince[gid] = if (seen > 0) seen else now + if (sawGiftWrap && maxGwSeen > 0) { + state.giftWrapSince = maxGwSeen } + for (gid in sawGroupEvent) { + val seen = maxGroupSeen[gid] ?: continue + if (seen > 0) state.groupSince[gid] = seen + } + + // If any welcome we processed consumed a KeyPackage, MIP-00 requires + // us to immediately publish a replacement (a KP can only be used for + // ONE welcome; leaving the old one on relays lets a second sender + // invite us with a bundle we no longer have private keys for). The + // Amethyst UI handles this via its own rotation scheduler; the CLI + // has no scheduler, so we rotate inline right after sync. + if (marmot.needsKeyPackageRotation()) { + try { + val kpRelays = keyPackageRelays().ifEmpty { outboxRelays() }.ifEmpty { anyRelays() } + if (kpRelays.isNotEmpty()) { + val rotated = marmot.rotateConsumedKeyPackages(kpRelays.toList()) + for (event in rotated) { + publish(event, kpRelays) + System.err.println("[cli] rotated KeyPackage → ${event.id.take(8)} on ${kpRelays.size} relay(s)") + } + } + } catch (e: Exception) { + System.err.println("[cli] key-package rotation failed: ${e.message}") + } + } + } + + /** + * Resolve a group identifier given on the CLI to the nostr_group_id that + * amy's [MarmotManager] indexes on. + * + * amy internally keys everything off MIP-01's `nostr_group_id`. whitenoise + * (and every other mdk consumer) keys off the MLS `GroupContext.groupId` — + * a separate 32-byte random value stamped at group creation. Cross-client + * scripts therefore wind up juggling both ids, and it's very easy to pass + * the wrong one to amy. Rather than make every caller translate, we accept + * either format and resolve here: + * 1. If the input is an active nostr_group_id, use it unchanged. + * 2. Otherwise scan active groups for one whose MLS groupId matches. + * 3. Otherwise return the input unchanged (so the caller still gets a + * sensible `not_member` response rather than a silent mismatch). + */ + fun resolveGroupId(input: HexKey): HexKey { + if (marmot.isMember(input)) return input + val normalized = input.lowercase() + return marmot.activeGroupIds().firstOrNull { nostrId -> + marmot.mlsGroupIdHex(nostrId)?.lowercase() == normalized + } ?: input } fun marmotGroupRelays(nostrGroupId: HexKey): Set { @@ -303,6 +370,13 @@ class Context( } companion object { + /** + * Lookback applied to the gift-wrap `since` filter to compensate for + * NIP-59's randomised-past `created_at`. 2 days matches + * [com.vitorpamplona.quartz.utils.TimeUtils.randomWithTwoDays]. + */ + private const val GIFT_WRAP_LOOKBACK_SECS: Long = 2L * 24 * 60 * 60 + /** Build a Context but require an identity to already exist — most commands can't run without one. */ fun open(dataDir: DataDir): Context { val identity = diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/AwaitCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/AwaitCommands.kt index 35358b75f..a741a536b 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/AwaitCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/AwaitCommands.kt @@ -108,6 +108,7 @@ object AwaitCommands { Json.writeLine( mapOf( "group_id" to match, + "mls_group_id" to ctx.marmot.mlsGroupIdHex(match), "name" to (ctx.marmot.groupMetadata(match)?.name ?: ""), "epoch" to ctx.marmot.groupEpoch(match), ), @@ -127,7 +128,7 @@ object AwaitCommands { rest: Array, ): Int = pollCondition(dataDir, rest, "await member ", targetIdx = 1) { ctx, rawArgs -> - val gid = rawArgs[0] + val gid = ctx.resolveGroupId(rawArgs[0]) val target = ctx.requireUserHex(rawArgs[1]) if (!ctx.marmot.isMember(gid)) { null @@ -143,7 +144,7 @@ object AwaitCommands { rest: Array, ): Int = pollCondition(dataDir, rest, "await admin ", targetIdx = 1) { ctx, rawArgs -> - val gid = rawArgs[0] + val gid = ctx.resolveGroupId(rawArgs[0]) val target = ctx.requireUserHex(rawArgs[1]) if (!ctx.marmot.isMember(gid)) { null @@ -163,13 +164,13 @@ object AwaitCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "await rename --name ") - val gid = rest[0] val args = Args(rest.drop(1).toTypedArray()) val wantedName = args.requireFlag("name") val timeoutSecs = args.longFlag("timeout", 30) val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) val deadline = System.currentTimeMillis() + timeoutSecs * 1000 while (System.currentTimeMillis() < deadline) { ctx.syncIncoming(timeoutMs = 3_000) @@ -191,13 +192,13 @@ object AwaitCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "await epoch --min N") - val gid = rest[0] val args = Args(rest.drop(1).toTypedArray()) val min = args.longFlag("min", 1) val timeoutSecs = args.longFlag("timeout", 30) val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) val deadline = System.currentTimeMillis() + timeoutSecs * 1000 while (System.currentTimeMillis() < deadline) { ctx.syncIncoming(timeoutMs = 3_000) @@ -219,13 +220,13 @@ object AwaitCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "await message --match STRING") - val gid = rest[0] val args = Args(rest.drop(1).toTypedArray()) val needle = args.requireFlag("match") val timeoutSecs = args.longFlag("timeout", 30) val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) val deadline = System.currentTimeMillis() + timeoutSecs * 1000 while (System.currentTimeMillis() < deadline) { ctx.syncIncoming(timeoutMs = 3_000) diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupAddMemberCommand.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupAddMemberCommand.kt index be96d891b..89e1650a6 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupAddMemberCommand.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupAddMemberCommand.kt @@ -37,10 +37,10 @@ object GroupAddMemberCommand { rest: Array, ): Int { if (rest.size < 2) return Json.error("bad_args", "group add [ ...]") - val gid = rest[0] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupCreateCommand.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupCreateCommand.kt index 35c6d214f..47cd31377 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupCreateCommand.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupCreateCommand.kt @@ -40,9 +40,12 @@ object GroupCreateCommand { ctx.prepare() val gid = RandomInstance.bytes(32).toHexKey() - ctx.marmot.createGroup(gid) - - // Stamp initial metadata via the shared factory so UI + CLI stay byte-identical. + // Stamp initial metadata via the shared factory so UI + CLI stay + // byte-identical. Bake the MarmotGroupData extension into the + // epoch-0 GroupContext directly (see `MarmotManager.createGroup`) + // so later invitees receive a pre-populated group from the + // welcome and never have to chase an undecryptable bootstrap + // commit that predates their membership. val outboxUrls = ctx.outboxRelays().map { it.url } val metadata = MarmotGroupData.bootstrap( @@ -51,19 +54,14 @@ object GroupCreateCommand { outboxRelays = outboxUrls, name = name, ) - val commit = ctx.marmot.updateGroupMetadata(gid, metadata) - - // Group relays == what the metadata carries, which on first commit is our outbox. - val targets = ctx.outboxRelays() - val ack = ctx.publish(commit.signedEvent, targets) + ctx.marmot.createGroup(gid, initialMetadata = metadata) Json.writeLine( mapOf( "group_id" to gid, + "mls_group_id" to ctx.marmot.mlsGroupIdHex(gid), "name" to name, "epoch" to ctx.marmot.groupEpoch(gid), - "commit_event_id" to commit.signedEvent.id, - "published_to" to ack.filterValues { it }.keys.map { it.url }, ), ) return 0 diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMembershipCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMembershipCommands.kt index a67cc62e8..2e7d84c52 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMembershipCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMembershipCommands.kt @@ -30,10 +30,10 @@ object GroupMembershipCommands { rest: Array, ): Int { if (rest.size < 2) return Json.error("bad_args", "group remove ") - val gid = rest[0] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) val target = ctx.requireUserHex(rest[1]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) @@ -66,18 +66,46 @@ object GroupMembershipCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "group leave ") - val gid = rest[0] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) val targets = ctx.marmotGroupRelays(gid).ifEmpty { ctx.outboxRelays() } + + // MIP-01/MIP-03: members listed in `admin_pubkeys` MUST NOT issue + // a SelfRemove proposal before first publishing a GCE that drops + // themselves from the admin list (MlsGroup.selfRemove enforces + // this with `check(!isLocalAdmin())`), and that same GCE MUST NOT + // leave the group with zero admins (admin depletion). If we're + // the only admin, hand admin to another member first. + val demoteEventId: String? = + ctx.marmot.groupMetadata(gid)?.let { metadata -> + if (!metadata.adminPubkeys.contains(ctx.identity.pubKeyHex)) return@let null + + val newAdmins = metadata.adminPubkeys.filter { it != ctx.identity.pubKeyHex }.toMutableList() + if (newAdmins.isEmpty()) { + val heir = + ctx.marmot + .memberPubkeys(gid) + .map { it.pubkey } + .firstOrNull { it != ctx.identity.pubKeyHex } + ?: return@let null // solo group — skip demote, let MLS state cleanup handle it + newAdmins.add(heir) + } + val demoted = metadata.copy(adminPubkeys = newAdmins) + val demoteCommit = ctx.marmot.updateGroupMetadata(gid, demoted) + ctx.publish(demoteCommit.signedEvent, targets) + demoteCommit.signedEvent.id + } + val outbound = ctx.marmot.leaveGroup(gid) val ack = ctx.publish(outbound.signedEvent, targets) Json.writeLine( mapOf( "group_id" to gid, + "self_demote_event_id" to demoteEventId, "proposal_event_id" to outbound.signedEvent.id, "published_to" to ack.filterValues { it }.keys.map { it.url }, ), diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMetadataCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMetadataCommands.kt index 0fb3fa106..4afdb7f54 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMetadataCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupMetadataCommands.kt @@ -66,12 +66,13 @@ object GroupMetadataCommands { private suspend fun edit( dataDir: DataDir, - gid: HexKey, + rawGid: HexKey, mutate: suspend (Context, MarmotGroupData) -> MarmotGroupData, ): Int { val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rawGid) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) val outboxUrls = ctx.outboxRelays().map { it.url } diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupReadCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupReadCommands.kt index 9ac6b78f7..3337204cb 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupReadCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GroupReadCommands.kt @@ -56,10 +56,10 @@ object GroupReadCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "group show ") - val gid = rest[0] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) val meta = ctx.marmot.groupMetadata(gid) @@ -70,6 +70,7 @@ object GroupReadCommands { Json.writeLine( mapOf( "group_id" to gid, + "mls_group_id" to ctx.marmot.mlsGroupIdHex(gid), "name" to (meta?.name ?: ""), "description" to (meta?.description ?: ""), "epoch" to ctx.marmot.groupEpoch(gid), @@ -90,10 +91,10 @@ object GroupReadCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "group members ") - val gid = rest[0] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) val members = @@ -112,10 +113,10 @@ object GroupReadCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "group admins ") - val gid = rest[0] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) val m = ctx.marmot.groupMetadata(gid) diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt index f2f2a9fcd..5cc5bf9d1 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/MessageCommands.kt @@ -45,11 +45,11 @@ object MessageCommands { rest: Array, ): Int { if (rest.size < 2) return Json.error("bad_args", "message send ") - val gid = rest[0] val text = rest[1] val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) @@ -77,12 +77,12 @@ object MessageCommands { rest: Array, ): Int { if (rest.isEmpty()) return Json.error("bad_args", "message list ") - val gid = rest[0] val args = Args(rest.drop(1).toTypedArray()) val limit = args.intFlag("limit", Int.MAX_VALUE) val ctx = Context.open(dataDir) try { ctx.prepare() + val gid = ctx.resolveGroupId(rest[0]) ctx.syncIncoming() if (!ctx.marmot.isMember(gid)) return Json.error("not_member", gid) 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 2caec99ab..870c75b52 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 @@ -27,6 +27,7 @@ import com.vitorpamplona.quartz.marmot.mip02Welcome.WelcomeEvent import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEvent import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip59Giftwrap.seals.SealedRumorEvent import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent /** @@ -95,11 +96,23 @@ suspend fun MarmotManager.ingest(event: Event): MarmotIngestResult = private suspend fun MarmotManager.ingestGiftWrap(wrap: GiftWrapEvent): MarmotIngestResult = try { - val inner = wrap.unwrapOrNull(signer) ?: return MarmotIngestResult.Ignored - if (!MarmotInboundProcessor.isWelcomeEvent(inner) || inner !is WelcomeEvent) { + // NIP-59 wraps contain TWO encryption layers: + // kind:1059 gift wrap → kind:13 sealed rumor → the rumor itself. + // `GiftWrapEvent.unwrapOrNull` only peels the outer layer; when the + // result is a [SealedRumorEvent] we must unseal it to reach the + // kind:444 Welcome rumor. The old code checked `isWelcomeEvent` on + // the seal (kind:13) and always took the Ignored branch, which is + // why every inbound Welcome was silently dropped by the CLI and by + // any non-Amethyst consumer. + val rumor = + when (val inner = wrap.unwrapOrNull(signer) ?: return MarmotIngestResult.Ignored) { + is SealedRumorEvent -> inner.unsealOrNull(signer) ?: return MarmotIngestResult.Ignored + else -> inner + } + if (!MarmotInboundProcessor.isWelcomeEvent(rumor) || rumor !is WelcomeEvent) { return MarmotIngestResult.Ignored } - when (val result = processWelcome(inner, inner.nostrGroupId())) { + when (val result = processWelcome(rumor, rumor.nostrGroupId())) { is WelcomeResult.Joined -> { MarmotIngestResult.JoinedGroup( nostrGroupId = result.nostrGroupId, 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 cac1eed8e..d8a509ab8 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 @@ -262,11 +262,24 @@ class MarmotManager( /** * Create a new MLS group. + * + * When [initialMetadata] is non-null it is baked into epoch 0's + * GroupContext.extensions. Later joiners' welcomes therefore carry the + * group name / admin list / relays from the get-go and no separate + * "bootstrap commit" needs to be published. The bootstrap-commit path + * works in theory, but it produces a kind:445 encrypted with epoch 0's + * exporter secret that no post-membership peer (amethyst or wn) has, + * so each such peer wastes their commit-retry budget on an + * undecryptable event before processing the real state. */ - suspend fun createGroup(nostrGroupId: HexKey): HexKey { + suspend fun createGroup( + nostrGroupId: HexKey, + initialMetadata: MarmotGroupData? = null, + ): HexKey { Log.d("MarmotManager") { "createGroup($nostrGroupId): by ${signer.pubKey.take(8)}…" } val identity = signer.pubKey.hexToByteArray() - groupManager.createGroup(nostrGroupId, identity) + val extras = initialMetadata?.let { listOf(it.toExtension()) } ?: emptyList() + groupManager.createGroup(nostrGroupId, identity, initialExtensions = extras) subscriptionManager.subscribeGroup(nostrGroupId) Log.d("MarmotManager") { "createGroup($nostrGroupId): persisted and subscribed" } return nostrGroupId @@ -277,15 +290,18 @@ class MarmotManager( * Returns proposal bytes to publish (as a GroupEvent). */ suspend fun leaveGroup(nostrGroupId: HexKey): OutboundGroupEvent { - // Build the outbound event BEFORE deleting group state (needs exporter secret) - val group = - groupManager.getGroup(nostrGroupId) - ?: throw IllegalStateException("Not a member of group $nostrGroupId") - val proposalBytes = group.selfRemove() - val outboundEvent = outboundProcessor.buildCommitEvent(nostrGroupId, proposalBytes) + // leaveGroup() returns the framed standalone SelfRemove proposal + // (PublicMessage{Proposal}) plus the pre-commit exporter key for + // outer encryption. Runs BEFORE we tear down the subscriptions so + // the pre-commit exporter is still derivable. + val (framedBytes, exporterKey) = groupManager.leaveGroup(nostrGroupId) + val outboundEvent = + outboundProcessor.buildCommitEvent( + nostrGroupId = nostrGroupId, + commitBytes = framedBytes, + exporterKey = exporterKey, + ) - // Now clean up group state - groupManager.removeGroupState(nostrGroupId) subscriptionManager.unsubscribeGroup(nostrGroupId) try { messageStore?.delete(nostrGroupId) @@ -346,13 +362,24 @@ class MarmotManager( /** * Update group metadata (name, description, etc.) via a GroupContextExtensions proposal. * Creates a GCE proposal, commits it, and returns the commit event to publish. + * + * RFC 9420 §12.1.7: a GroupContextExtensions proposal REPLACES the entire + * extension list in GroupContext. Peers (notably mdk-core / whitenoise-rs) + * reject welcomes whose GroupContext is missing the required-capabilities + * extension, and strip metadata from groups whose context lacks the + * MarmotGroupData extension. We therefore preserve every other existing + * extension and overwrite only the slot we actually want to update. */ suspend fun updateGroupMetadata( nostrGroupId: HexKey, metadata: MarmotGroupData, ): OutboundGroupEvent { - val commitResult = - groupManager.updateGroupExtensions(nostrGroupId, listOf(metadata.toExtension())) + val group = + groupManager.getGroup(nostrGroupId) + ?: throw IllegalStateException("Not a member of group $nostrGroupId") + val preserved = group.extensions.filter { it.extensionType != MarmotGroupData.EXTENSION_ID_INT } + val merged = preserved + metadata.toExtension() + val commitResult = groupManager.updateGroupExtensions(nostrGroupId, merged) val commitEvent = outboundProcessor.buildCommitEvent( nostrGroupId = nostrGroupId, @@ -480,6 +507,18 @@ class MarmotManager( */ fun groupEpoch(nostrGroupId: HexKey): Long? = groupManager.getGroup(nostrGroupId)?.epoch + /** + * Hex-encoded MLS group id for the group keyed by [nostrGroupId], or null if + * that group is not locally known. + * + * Interop note: whitenoise-rs (and every mdk consumer) indexes groups by the + * MLS GroupContext's groupId, NOT the MIP-01 nostr_group_id that we use as + * the primary key internally. When a harness or external caller needs to + * cross-reference a group with another client (e.g. the interop harness + * calling `wn messages list `), it needs this translation. + */ + fun mlsGroupIdHex(nostrGroupId: HexKey): HexKey? = groupManager.getGroup(nostrGroupId)?.groupId?.toHexKey() + /** * Resolve the MLS leaf index for a member by Nostr pubkey, or null if that * pubkey isn't currently in the group. 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 1160561d7..412133729 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt @@ -511,16 +511,37 @@ class MarmotInboundProcessor( when (mlsMessage.wireFormat) { WireFormat.PRIVATE_MESSAGE -> { - // For private commits, MLS decrypt handles epoch advancement - val decrypted = groupManager.decrypt(groupId, mlsMessage.toTlsBytes()) - if (decrypted.contentType == ContentType.COMMIT) { - val group = groupManager.getGroup(groupId) - GroupEventResult.CommitProcessed(groupId, group?.epoch ?: 0) - } else { - GroupEventResult.Error( - groupId, - "Expected COMMIT but got ${decrypted.contentType}", - ) + // Sniff the PrivateMessage epoch without consuming any + // ratchet state. Past-epoch echoes and future-epoch + // arrivals must not advance the secret tree — otherwise + // the real handshake / application message gets rejected + // when it finally arrives. + val privPeek = PrivateMessage.decodeTls(TlsReader(mlsMessage.payload)) + val currentEpoch = groupManager.getGroup(groupId)?.epoch + when { + currentEpoch != null && privPeek.epoch < currentEpoch -> { + GroupEventResult.Duplicate(groupId) + } + + currentEpoch != null && privPeek.epoch > currentEpoch -> { + GroupEventResult.Error( + groupId, + "PrivateMessage epoch ${privPeek.epoch} is ahead of local epoch $currentEpoch; ignoring", + ) + } + + else -> { + val decrypted = groupManager.decrypt(groupId, mlsMessage.toTlsBytes()) + if (decrypted.contentType == ContentType.COMMIT) { + val group = groupManager.getGroup(groupId) + GroupEventResult.CommitProcessed(groupId, group?.epoch ?: 0) + } else { + GroupEventResult.Error( + groupId, + "Expected COMMIT but got ${decrypted.contentType}", + ) + } + } } } @@ -556,14 +577,29 @@ class MarmotInboundProcessor( } else -> { - groupManager.processCommit( - nostrGroupId = groupId, - commitBytes = pubMsg.content, - senderLeafIndex = pubMsg.sender.leafIndex, - confirmationTag = tag, - ) + // RFC 9420 §6.2 — reject PublicMessage commits + // whose membership_tag doesn't match what the + // current epoch's membership_key would produce. + // Without this an outsider with the outer + // exporter secret could inject arbitrary commit + // bytes and advance the group past them. val group = groupManager.getGroup(groupId) - GroupEventResult.CommitProcessed(groupId, group?.epoch ?: 0) + if (group != null && !group.verifyPublicMessageCommitMembershipTag(pubMsg)) { + GroupEventResult.Error( + groupId, + "Invalid membership_tag on PublicMessage commit", + ) + } else { + groupManager.processCommit( + nostrGroupId = groupId, + commitBytes = pubMsg.content, + senderLeafIndex = pubMsg.sender.leafIndex, + confirmationTag = tag, + signature = pubMsg.signature, + ) + val post = groupManager.getGroup(groupId) + GroupEventResult.CommitProcessed(groupId, post?.epoch ?: 0) + } } } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip01Groups/MarmotGroupData.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip01Groups/MarmotGroupData.kt index 142b8fd5d..78213a394 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip01Groups/MarmotGroupData.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip01Groups/MarmotGroupData.kt @@ -191,7 +191,22 @@ data class MarmotGroupData( fun toExtension(): Extension = Extension(EXTENSION_ID_INT, encodeTls()) companion object { - const val CURRENT_VERSION = 3 + /** + * Version we emit for freshly created groups and fresh GCE commits. + * + * Held at 2 (not 3) because the Rust mdk-core MLS engine used by + * whitenoise-rs (the only other shipping Marmot client today) is + * stricter than MIP-01's "ignore trailing bytes for forward + * compatibility" rule — it rejects v3 payloads with + * `ExtensionFormatError("Trailing bytes in NostrGroupDataExtension")`, + * which means our v3 welcomes/commits never get applied and every + * cross-client group flow breaks. We still PARSE v3 happily via + * [decodeTls] (any peer that sends us a v3 group will round-trip), + * but we don't create them until mdk-core catches up. + * + * Bump back to 3 once mdk publishes the forward-compat fix. + */ + const val CURRENT_VERSION = 2 /** Versions this implementation understands. v0 is reserved/invalid per MIP-01. */ val SUPPORTED_VERSIONS: Set = setOf(1, 2, 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 da84e71d6..a1cbdd8e2 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 @@ -97,6 +97,18 @@ import com.vitorpamplona.quartz.utils.mac.MacInstance * val key = group.exporterSecret("marmot", "group-event", 32) * ``` */ +private fun constantTimeEquals( + a: ByteArray, + b: ByteArray, +): Boolean { + if (a.size != b.size) return false + var result = 0 + for (i in a.indices) { + result = result or (a[i].toInt() xor b[i].toInt()) + } + return result == 0 +} + class MlsGroup private constructor( private var groupContext: GroupContext, private val tree: RatchetTree, @@ -411,6 +423,15 @@ class MlsGroup private constructor( val preCommitExporterSecret = exporterSecret("marmot", "group-event".encodeToByteArray(), 32) + // Snapshot the pre-proposal extensions. GroupContextExtensions proposals + // mutate `groupContext.extensions` the moment they're applied, but + // openmls signs the commit's FramedContentTBS over the UNMUTATED + // context (the one in `public_group` before `diff` applies proposals). + // Without this snapshot, a GCE commit on the quartz side uses the new + // extensions for TBS + membership_tag, and openmls rejects it with + // `ValidationError(InvalidMembershipTag)`. + val preCommitExtensions = groupContext.extensions + val proposalOrRefs = proposals.map { ProposalOrRef.Inline(it.proposal) } // Check if we need an UpdatePath. RFC 9420 §12.4.1: the path value @@ -445,55 +466,48 @@ class MlsGroup private constructor( val leafSecret = MlsCryptoProvider.randomBytes(MlsCryptoProvider.HASH_OUTPUT_LENGTH) val pathSecrets = tree.derivePathSecrets(myLeafIndex, leafSecret) - // Build UpdatePath with HPKE-encrypted path secrets for each copath node - val updatePath = + // RFC 9420 §12.4.1: newly-added leaves (from Add proposals in THIS commit) + // MUST be excluded from the copath resolution — they join via the Welcome + // at epoch N+1 and don't need the path secret. Keeping them in the list + // shifts every other resolution index by one, so strict receivers + // (openmls/mdk) pick the wrong ciphertext and fail with + // UpdatePathError(UnableToDecrypt). + val newLeafIndices = addedMembers.map { it.first }.toSet() + + // Build the UpdatePath in three stages so the HPKE info used for path- + // secret encryption matches what openmls/mdk compute: + // 1. derive path-secret keypairs and stage the committer's new leaf + // 2. apply the path locally, patch parent_hashes, swap in the leaf + // 3. compute the post-mutation tree_hash + bump epoch so + // `serialized_group_context` has the new tree_hash, the new epoch, + // and the old confirmed_transcript_hash — then HPKE-encrypt each + // copath resolution under that intermediate context + // Without this ordering the committer uses the PRE-commit context and + // every strict-validating member derives a different AEAD key, turning + // the decryption into `UpdatePathError(UnableToDecrypt)`. + val updatePath: UpdatePath? = if (needsPath && pathSecrets.isNotEmpty()) { val copath = BinaryTree.copath(myLeafIndex, tree.leafCount) - val pathNodes = - pathSecrets.zip(copath).map { (pathKey, copathNode) -> - val resolution = tree.resolution(copathNode) - val encryptedSecrets = - resolution.mapNotNull { resNode -> - val node = tree.getNode(resNode) ?: return@mapNotNull null - val recipientPub = - when (node) { - is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Leaf -> { - node.leafNode.encryptionKey - } - - is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent -> { - node.parentNode.encryptionKey - } - } - MlsCryptoProvider.encryptWithLabel( - recipientPub, - "UpdatePathNode", - groupContext.toTlsBytes(), - pathKey.pathSecret, - ) - } - UpdatePathNode(pathKey.publicKey, encryptedSecrets) - } // Capture sibling tree hashes BEFORE applying the UpdatePath — // parent_hash computation (RFC 9420 §7.9.2) uses the // ORIGINAL sibling-subtree tree hashes. val preUpdateSiblingHashes = capturePreUpdateSiblingHashes(myLeafIndex) - // Apply the UpdatePath to our own tree first so the parent - // nodes carry the new encryption keys. Their parent_hash - // fields are filled in next. - tree.applyUpdatePath(myLeafIndex, pathNodes) + // Stage path-keys into the tree so subsequent parent_hash / + // tree_hash computations reflect the new keys. We'll fill in + // the HPKE-encrypted secrets once we know the post-commit + // context bytes. + val stagedPathNodes = + pathSecrets.zip(copath).map { (pathKey, _) -> + UpdatePathNode(pathKey.publicKey, emptyList()) + } + tree.applyUpdatePath(myLeafIndex, stagedPathNodes) // Compute parent_hash for every direct-path parent node and - // for the committer's leaf (RFC 9420 §7.9.2). Without this - // chain, spec-strict peers (ts-mls, OpenMLS) reject the - // Welcome with "Unable to verify parent hash". + // for the committer's leaf (RFC 9420 §7.9.2). val (parentNodeHashes, leafParentHash) = computeSenderParentHashes(myLeafIndex, preUpdateSiblingHashes) - - // Patch each parent node with its computed parent_hash so - // subsequent treeHash / serialization uses the final values. val directPath = BinaryTree.directPath(myLeafIndex, tree.leafCount) for (nodeIdx in directPath) { val existing = tree.getNode(nodeIdx) @@ -528,38 +542,117 @@ class MlsGroup private constructor( leafIndex = myLeafIndex, parentHash = leafParentHash, ) - encryptionPrivateKey = newEncKp.privateKey tree.setLeaf(myLeafIndex, newLeafNode) + // Path-encryption context (RFC 9420 §7.6 / openmls `compute_path`): + // serialize the GroupContext AFTER applying the tree mutations + // and bumping the epoch, but BEFORE the confirmed_transcript_hash + // gets folded in. We don't commit this copy to the field yet — + // the transcript-hash update below needs the current value. + val pathEncContextBytes = + groupContext + .copy( + epoch = groupContext.epoch + 1, + treeHash = tree.treeHash(), + ).toTlsBytes() + + val pathNodes = + stagedPathNodes.zip(copath).zip(pathSecrets) { (staged, copathNode), pathKey -> + val resolution = + tree.resolution(copathNode).filterNot { resNode -> + BinaryTree.isLeaf(resNode) && + BinaryTree.nodeToLeaf(resNode) in newLeafIndices + } + val encryptedSecrets = + resolution.mapNotNull { resNode -> + val node = tree.getNode(resNode) ?: return@mapNotNull null + val recipientPub = + when (node) { + is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Leaf -> { + node.leafNode.encryptionKey + } + + is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent -> { + node.parentNode.encryptionKey + } + } + MlsCryptoProvider.encryptWithLabel( + recipientPub, + "UpdatePathNode", + pathEncContextBytes, + pathKey.pathSecret, + ) + } + UpdatePathNode(staged.encryptionKey, encryptedSecrets) + } + UpdatePath(newLeafNode, pathNodes) } else { null } val commit = Commit(proposalOrRefs, updatePath) + val commitBytes = commit.toTlsBytes() - // Advance epoch + // Advance epoch. + // RFC 9420 §9.2: commit_secret is the path_secret for the "virtual" node + // one step past the root — i.e. `DeriveSecret(path_secret_at_root, "path")`, + // NOT the path_secret at the root itself. Openmls/mdk derives exactly this + // value; quartz was using the root's own path_secret, which is the + // encryption-key seed rather than the key-schedule contribution. That + // one-step gap silently diverged the two sides' epoch_secret and made + // every cross-impl commit fail `ConfirmationTagMismatch`. val commitSecret = if (pathSecrets.isNotEmpty()) { - pathSecrets.last().pathSecret + MlsCryptoProvider.deriveSecret(pathSecrets.last().pathSecret, "path") } else { ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) } - // Update transcript hashes (RFC 9420 Section 8.2) - val confirmedTranscriptHashInput = buildConfirmedTranscriptHashInput(commit, myLeafIndex) - val confirmedInput = TlsWriter() - confirmedInput.putBytes(interimTranscriptHash) - confirmedInput.putBytes(confirmedTranscriptHashInput) - val newConfirmedTranscriptHash = MlsCryptoProvider.hash(confirmedInput.toByteArray()) - val newTreeHash = tree.treeHash() val oldEpoch = groupContext.epoch val preCommitGroupId = groupContext.groupId val committerLeafIndex = myLeafIndex val newEpoch = oldEpoch + 1 + // Capture pre-commit values needed to sign and membership-MAC the + // outbound PublicMessage (RFC 9420 §6.1 / §6.2). The signature and + // membership_tag are computed under the epoch in which the commit + // is sent — the one we're about to leave. Receivers (openmls/mdk) + // strict-verify both, so we must use the leaf signing key that's + // still in the pre-commit tree and the membership_key derived from + // the pre-commit epoch secrets. Extensions need explicit rewind to + // pre-proposal state for GroupContextExtensions commits. + val preCommitContextBytes = + groupContext.copy(extensions = preCommitExtensions).toTlsBytes() + val preCommitMembershipKey = epochSecrets.membershipKey + val preCommitSigningKey = signingPrivateKey + + // Sign FramedContentTBS BEFORE folding the commit into the transcript + // hash. RFC 9420 §8.2: ConfirmedTranscriptHashInput contains the real + // signature — using `ByteArray(0)` here produces a confirmed_transcript_hash + // that the receiver cannot reproduce, so strict peers reject the commit + // with `ConfirmationTagMismatch`. + val commitTbsBytes = + buildCommitFramedContentTbs( + groupId = preCommitGroupId, + epoch = oldEpoch, + senderLeafIndex = committerLeafIndex, + commitBytes = commitBytes, + groupContextBytes = preCommitContextBytes, + ) + val commitSignature = + MlsCryptoProvider.signWithLabel(preCommitSigningKey, "FramedContentTBS", commitTbsBytes) + + // Update transcript hashes (RFC 9420 §8.2) with the real signature. + val confirmedTranscriptHashInput = + buildConfirmedTranscriptHashInput(commit, myLeafIndex, commitSignature) + val confirmedInput = TlsWriter() + confirmedInput.putBytes(interimTranscriptHash) + confirmedInput.putBytes(confirmedTranscriptHashInput) + val newConfirmedTranscriptHash = MlsCryptoProvider.hash(confirmedInput.toByteArray()) + groupContext = groupContext.copy( epoch = newEpoch, @@ -599,7 +692,6 @@ class MlsGroup private constructor( pendingProposals.clear() sentKeys.clear() - val commitBytes = commit.toTlsBytes() val framedCommitBytes = framePublicMessageCommit( groupId = preCommitGroupId, @@ -607,6 +699,9 @@ class MlsGroup private constructor( senderLeafIndex = committerLeafIndex, commitBytes = commitBytes, confirmationTag = confirmationTag, + signature = commitSignature, + membershipKey = preCommitMembershipKey, + tbsBytes = commitTbsBytes, ) return CommitResult( commitBytes = commitBytes, @@ -791,13 +886,25 @@ class MlsGroup private constructor( "Sender leaf is blank at index $senderLeafIndex (not a group member)" } - // Get the key/nonce for this sender+generation - // If we sent this message ourselves, use the cached key to avoid ratchet conflict + // Get the key/nonce for this sender+generation from the correct ratchet. + // PrivateMessage Application content uses the application ratchet (RFC 9420 + // §6.3.2); PrivateMessage Commit / Proposal handshakes use a separate + // per-sender handshake ratchet. openmls / mdk default to AlwaysCiphertext + // outgoing, so every B→A commit quartz receives lands here with + // content_type == COMMIT. val kng = if (senderLeafIndex == myLeafIndex && sentKeys.containsKey(generation)) { sentKeys.remove(generation)!! } else { - secretTree.applicationKeyNonceForGeneration(senderLeafIndex, generation) + when (privMsg.contentType) { + ContentType.APPLICATION -> { + secretTree.applicationKeyNonceForGeneration(senderLeafIndex, generation) + } + + ContentType.COMMIT, ContentType.PROPOSAL -> { + secretTree.handshakeKeyNonceForGeneration(senderLeafIndex, generation) + } + } } // Apply reuse_guard XOR to nonce (RFC 9420 §6.3.1) @@ -810,48 +917,86 @@ class MlsGroup private constructor( val contentAad = buildPrivateContentAAD(privMsg.groupId, privMsg.epoch, privMsg.contentType, privMsg.authenticatedData) val pmcPlaintext = MlsCryptoProvider.aeadDecrypt(kng.key, guardedNonce, contentAad, privMsg.ciphertext) - // Parse PrivateMessageContent (RFC 9420 §6.3.1) and verify the - // sender's FramedContentTBS signature before returning bytes. - require(privMsg.contentType == ContentType.APPLICATION) { - // Commit/Proposal-via-PrivateMessage decoding is not implemented. - "decrypt() only supports application-content PrivateMessages" - } + // Parse PrivateMessageContent (RFC 9420 §6.3.1). The layout depends on + // content_type — application payloads carry `opaque application_data` + // whereas commit / proposal payloads carry the struct directly (no + // outer length prefix). val pmcReader = TlsReader(pmcPlaintext) - val applicationData = pmcReader.readOpaqueVarInt() - val signature = pmcReader.readOpaqueVarInt() - // Remaining bytes are padding; all must be zero per §6.3.1. - while (pmcReader.hasRemaining) { - require(pmcReader.readBytes(1)[0] == 0.toByte()) { - "PrivateMessageContent padding must be zero" + when (privMsg.contentType) { + ContentType.APPLICATION -> { + val applicationData = pmcReader.readOpaqueVarInt() + val signature = pmcReader.readOpaqueVarInt() + while (pmcReader.hasRemaining) { + require(pmcReader.readBytes(1)[0] == 0.toByte()) { + "PrivateMessageContent padding must be zero" + } + } + + val senderLeaf = + requireNotNull(tree.getLeaf(senderLeafIndex)) { + "Sender leaf is blank at index $senderLeafIndex" + } + require( + MlsCryptoProvider.verifyWithLabel( + senderLeaf.signatureKey, + "FramedContentTBS", + buildApplicationFramedContentTbs( + groupId = privMsg.groupId, + epoch = privMsg.epoch, + senderLeafIndex = senderLeafIndex, + authenticatedData = privMsg.authenticatedData, + applicationData = applicationData, + groupContext = groupContext, + ), + signature, + ), + ) { "FramedContentTBS signature verification failed" } + + return DecryptedMessage( + senderLeafIndex = senderLeafIndex, + contentType = privMsg.contentType, + content = applicationData, + epoch = privMsg.epoch, + ) + } + + ContentType.COMMIT -> { + // PrivateMessageContent for a Commit: the Commit struct + // (no length prefix) followed by signature and + // confirmation_tag, then zero padding. The caller drives + // processCommit with (commitBytes, signature, confirmationTag) + // — all available here once we re-serialize the parsed Commit + // back to bytes (openmls does the same round-trip). + val commit = Commit.decodeTls(pmcReader) + val commitWriter = TlsWriter() + commit.encodeTls(commitWriter) + val commitBytes = commitWriter.toByteArray() + val signature = pmcReader.readOpaqueVarInt() + val confirmationTag = pmcReader.readOpaqueVarInt() + while (pmcReader.hasRemaining) { + require(pmcReader.readBytes(1)[0] == 0.toByte()) { + "PrivateMessageContent padding must be zero" + } + } + // Apply the commit inline — after this returns, the caller + // only needs to know the epoch advanced. The signature + wire + // format ride into the transcript-hash computation inside + // processCommit (RFC 9420 §8.2 requires the real wire format + // for ConfirmedTranscriptHashInput). + processCommit(commitBytes, senderLeafIndex, confirmationTag, signature, WireFormat.PRIVATE_MESSAGE) + + return DecryptedMessage( + senderLeafIndex = senderLeafIndex, + contentType = privMsg.contentType, + content = commitBytes, + epoch = privMsg.epoch, + ) + } + + ContentType.PROPOSAL -> { + throw IllegalStateException("Standalone PrivateMessage proposals not yet supported") } } - - val senderLeaf = - requireNotNull(tree.getLeaf(senderLeafIndex)) { - "Sender leaf is blank at index $senderLeafIndex" - } - require( - MlsCryptoProvider.verifyWithLabel( - senderLeaf.signatureKey, - "FramedContentTBS", - buildApplicationFramedContentTbs( - groupId = privMsg.groupId, - epoch = privMsg.epoch, - senderLeafIndex = senderLeafIndex, - authenticatedData = privMsg.authenticatedData, - applicationData = applicationData, - groupContext = groupContext, - ), - signature, - ), - ) { "FramedContentTBS signature verification failed" } - - return DecryptedMessage( - senderLeafIndex = senderLeafIndex, - contentType = privMsg.contentType, - content = applicationData, - epoch = privMsg.epoch, - ) } /** @@ -897,11 +1042,59 @@ class MlsGroup private constructor( * @param commitBytes TLS-serialized Commit * @param senderLeafIndex the sender's leaf index in the ratchet tree * @param confirmationTag optional confirmation tag from the PublicMessage for verification + * @param signature FramedContentAuthData.signature from the PublicMessage wrapper; + * required to reproduce the ConfirmedTranscriptHashInput (RFC 9420 §8.2) exactly. + * Callers that don't have access to the PublicMessage envelope (test fixtures) + * may pass `ByteArray(0)` — that path only interoperates with senders that + * also pass an empty signature. */ fun processCommit( commitBytes: ByteArray, senderLeafIndex: Int, confirmationTag: ByteArray, + signature: ByteArray = ByteArray(0), + wireFormat: WireFormat = WireFormat.PUBLIC_MESSAGE, + ) { + // Snapshot all mutable state BEFORE applying any proposals or + // UpdatePath mutations. If any step throws (bad signature, parent + // hash mismatch, decrypt failure, confirmation-tag divergence), we + // restore the snapshot so callers observe the group in the same + // state as if processCommit had never run. Without this rollback + // a partial mutation leaves the tree one epoch ahead of + // `groupContext.epoch` and every subsequent decrypt fails with + // `Message epoch X doesn't match current epoch Y`. + val treeSnapshot = tree.snapshot() + val ctxSnapshot = groupContext + val secretsSnapshot = epochSecrets + val initSnapshot = initSecret + val secretTreeSnapshot = secretTree + val interimSnapshot = interimTranscriptHash + val pendingSnapshot = pendingProposals.toList() + val sentKeysSnapshot = sentKeys.toMap() + + try { + processCommitInner(commitBytes, senderLeafIndex, confirmationTag, signature, wireFormat) + } catch (t: Throwable) { + tree.restoreFrom(treeSnapshot) + groupContext = ctxSnapshot + epochSecrets = secretsSnapshot + initSecret = initSnapshot + secretTree = secretTreeSnapshot + interimTranscriptHash = interimSnapshot + pendingProposals.clear() + pendingProposals.addAll(pendingSnapshot) + sentKeys.clear() + sentKeys.putAll(sentKeysSnapshot) + throw t + } + } + + private fun processCommitInner( + commitBytes: ByteArray, + senderLeafIndex: Int, + confirmationTag: ByteArray, + signature: ByteArray, + wireFormat: WireFormat, ) { val commit = Commit.decodeTls(TlsReader(commitBytes)) @@ -925,13 +1118,49 @@ class MlsGroup private constructor( } } - // Apply proposals (resolve references from pending pool) - // Collect all resolved proposals for key schedule computation + // Verify the FramedContentTBS signature (RFC 9420 §6.1) against + // the sender's pre-commit leaf signature_key. Any non-external + // commit MUST carry a non-empty signature from the sender's leaf + // — without this check anyone with the outer exporter key (in a + // compromised relay scenario) could forge commits. + if (!isExternalCommit) { + require(signature.isNotEmpty()) { + "FramedContentTBS signature missing on commit from leaf $senderLeafIndex" + } + val senderLeaf = + requireNotNull(tree.getLeaf(senderLeafIndex)) { + "Sender leaf is blank at index $senderLeafIndex" + } + val tbs = + buildCommitFramedContentTbs( + groupId = groupContext.groupId, + epoch = groupContext.epoch, + senderLeafIndex = senderLeafIndex, + commitBytes = commitBytes, + groupContextBytes = groupContext.toTlsBytes(), + wireFormat = wireFormat, + ) + require(MlsCryptoProvider.verifyWithLabel(senderLeaf.signatureKey, "FramedContentTBS", tbs, signature)) { + "Invalid FramedContentTBS signature on commit from leaf $senderLeafIndex" + } + } + + // 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>() for (proposalOrRef in commit.proposals) { when (proposalOrRef) { is ProposalOrRef.Inline -> { - applyProposal(proposalOrRef.proposal, senderLeafIndex) + if (proposalOrRef.proposal is Proposal.Add) { + inlineAdds.add(proposalOrRef.proposal) + } else { + applyProposal(proposalOrRef.proposal, senderLeafIndex) + } resolvedProposals.add(proposalOrRef.proposal) } @@ -947,11 +1176,22 @@ class MlsGroup private constructor( requireNotNull(resolved) { "Commit references unknown proposal (ref not found in pending proposals)" } - applyProposal(resolved.proposal, resolved.senderLeafIndex) + if (resolved.proposal is Proposal.Add) { + referenceAddSenders.add(resolved.proposal to resolved.senderLeafIndex) + } else { + applyProposal(resolved.proposal, resolved.senderLeafIndex) + } resolvedProposals.add(resolved.proposal) } } } + val newLeavesInCommit = mutableSetOf() + for (add in inlineAdds) { + newLeavesInCommit.add(applyProposalAdd(add)) + } + for ((add, _) in referenceAddSenders) { + newLeavesInCommit.add(applyProposalAdd(add)) + } // Process UpdatePath var commitSecret = ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) @@ -961,6 +1201,17 @@ class MlsGroup private constructor( require(verifyLeafNodeSignature(updatePath.leafNode, groupId, senderLeafIndex)) { "Invalid LeafNode signature in UpdatePath" } + // RFC 9420 §8.4: LeafNode lifetime bounds must be current. + // An expired leaf is a revoked key — accepting it here would + // let a peer replay old UpdatePath material past the window + // the signer authorized. + val lifetime = updatePath.leafNode.lifetime + if (lifetime != null) { + val now = TimeUtils.now() + require(now >= lifetime.notBefore && now <= lifetime.notAfter) { + "LeafNode lifetime expired or not yet valid in UpdatePath" + } + } // For external commits the sender's leaf is not yet in the tree. // Grow the tree with a blank slot so directPath / sibling-hash @@ -1004,12 +1255,26 @@ class MlsGroup private constructor( val directPath = BinaryTree.directPath(senderLeafIndex, tree.leafCount) val myPath = BinaryTree.directPath(myLeafIndex, tree.leafCount) + // Path-decryption context (RFC 9420 §7.6): matches what the + // committer used to encrypt — post-tree-mutation tree_hash and + // bumped epoch, but pre-commit confirmed_transcript_hash. + val pathDecContextBytes = + groupContext + .copy( + epoch = groupContext.epoch + 1, + treeHash = tree.treeHash(), + ).toTlsBytes() + // Find the common ancestor val commonAncestorIdx = directPath.indexOfFirst { it in myPath } if (commonAncestorIdx >= 0 && commonAncestorIdx < updatePath.nodes.size) { val pathNode = updatePath.nodes[commonAncestorIdx] val copathNodeIdx = BinaryTree.copath(senderLeafIndex, tree.leafCount)[commonAncestorIdx] - val resolution = tree.resolution(copathNodeIdx) + 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) @@ -1021,28 +1286,30 @@ class MlsGroup private constructor( MlsCryptoProvider.decryptWithLabel( encryptionPrivateKey, "UpdatePathNode", - groupContext.toTlsBytes(), + pathDecContextBytes, ct.kemOutput, ct.ciphertext, ) - // Derive remaining path secrets from common ancestor up to root. - // pathSecret is the secret AT commonAncestorIdx, so we derive - // (directPath.size - commonAncestorIdx - 1) more steps to reach root. - val remainingSteps = directPath.size - commonAncestorIdx - 1 + // 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. + val stepsToRoot = directPath.size - commonAncestorIdx - 1 var currentSecret = pathSecret - repeat(remainingSteps) { + repeat(stepsToRoot) { currentSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") } - commitSecret = currentSecret + commitSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") } } } // Update transcript hashes (RFC 9420 Section 8.2) // ConfirmedTranscriptHashInput = wire_format || FramedContent || signature - // Simplified: use commit TLS bytes as the transcript input - val confirmedTranscriptHashInput = buildConfirmedTranscriptHashInput(commit, senderLeafIndex) + val confirmedTranscriptHashInput = buildConfirmedTranscriptHashInput(commit, senderLeafIndex, signature, wireFormat) val confirmedInput = TlsWriter() confirmedInput.putBytes(interimTranscriptHash) confirmedInput.putBytes(confirmedTranscriptHashInput) @@ -1077,17 +1344,17 @@ class MlsGroup private constructor( initSecret = epochSecrets.initSecret secretTree = SecretTree(epochSecrets.encryptionSecret, tree.leafCount) - // Verify confirmation tag (RFC 9420 Section 6.1). In real message - // flows the tag is carried on the wrapping PublicMessage and must be - // verified. Callers that process a raw commit payload and have - // verified the tag externally (or for test harnesses that work with - // `Commit.toTlsBytes()` directly) pass `ByteArray(0)`; in that mode - // we skip the comparison. Any non-empty tag is still checked. + // Verify confirmation tag (RFC 9420 Section 6.1). Every commit on + // the wire MUST carry a confirmation_tag that matches what the + // receiver derives from the post-commit confirmation_key — this + // is the last line of defense against a committer with the + // correct signing key but a forged tree state. val expectedTag = computeConfirmationTag(epochSecrets.confirmationKey, newConfirmedTranscriptHash) - if (confirmationTag.isNotEmpty()) { - require(constantTimeEquals(confirmationTag, expectedTag)) { - "Confirmation tag verification failed" - } + require(confirmationTag.isNotEmpty()) { + "Confirmation tag missing on commit from leaf $senderLeafIndex" + } + require(constantTimeEquals(confirmationTag, expectedTag)) { + "Confirmation tag verification failed" } // Update interim_transcript_hash for next epoch (reuse verified expectedTag) @@ -1219,7 +1486,9 @@ class MlsGroup private constructor( private fun buildConfirmedTranscriptHashInput( commit: Commit, senderLeafIndex: Int, - ): ByteArray = buildConfirmedTranscriptHashInput(commit, senderLeafIndex, groupId, epoch) + signature: ByteArray = ByteArray(0), + wireFormat: WireFormat = WireFormat.PUBLIC_MESSAGE, + ): ByteArray = buildConfirmedTranscriptHashInput(commit, senderLeafIndex, groupId, epoch, signature, wireFormat) /** * Compute the RFC 9420 §7.9.2 parent_hash chain for a commit we are @@ -1400,6 +1669,33 @@ class MlsGroup private constructor( return constantTimeEquals(expectedTag, membershipTag) } + /** + * Verify RFC 9420 §6.2 membership_tag on an inbound PublicMessage Commit. + * The tag binds the whole `(TBS || FramedContentAuthData)` payload to + * the sender's epoch — if it's missing or wrong, the sender either + * isn't a member or the message was tampered with. Returns false for + * either case; callers should reject the commit before advancing state. + */ + fun verifyPublicMessageCommitMembershipTag(pubMsg: PublicMessage): Boolean { + val membershipTag = pubMsg.membershipTag ?: return false + if (membershipTag.isEmpty()) return false + val confirmationTag = pubMsg.confirmationTag ?: return false + val tbs = + buildCommitFramedContentTbs( + groupId = pubMsg.groupId, + epoch = pubMsg.epoch, + senderLeafIndex = pubMsg.sender.leafIndex, + commitBytes = pubMsg.content, + groupContextBytes = groupContext.toTlsBytes(), + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + val tbmWriter = TlsWriter() + tbmWriter.putBytes(tbs) + tbmWriter.putOpaqueVarInt(pubMsg.signature) + tbmWriter.putOpaqueVarInt(confirmationTag) + return verifyMembershipTag(tbmWriter.toByteArray(), membershipTag) + } + /** * Build PrivateContentAAD (RFC 9420 §6.3.2): * ``` @@ -1447,22 +1743,6 @@ class MlsGroup private constructor( return writer.toByteArray() } - /** - * Constant-time byte array comparison to prevent timing side-channels. - * Returns true only if both arrays have the same length and contents. - */ - private fun constantTimeEquals( - a: ByteArray, - b: ByteArray, - ): Boolean { - if (a.size != b.size) return false - var result = 0 - for (i in a.indices) { - result = result or (a[i].toInt() xor b[i].toInt()) - } - return result == 0 - } - /** * Apply an Add proposal and return the assigned leaf index. */ @@ -1740,22 +2020,66 @@ class MlsGroup private constructor( private const val RATCHET_TREE_EXTENSION_TYPE = 0x0002 /** - * Wrap a raw [Commit] (as [commitBytes]) in an MlsMessage(PublicMessage(...)) - * envelope so it can be published on the wire (RFC 9420 §6 / §6.2). + * Build the FramedContentTBS bytes for a member-sender commit + * (RFC 9420 §6.1). The signature over this value is the + * `FramedContentAuthData.signature` that rides both in the + * on-the-wire PublicMessage/PrivateMessage AND in the + * ConfirmedTranscriptHashInput. * - * The receiver uses the sender's leaf index and the confirmation_tag from - * the [PublicMessage] header to drive [MlsGroup.processCommit]. The - * `signature` and `membership_tag` opaque fields are intentionally empty — - * the current implementation does not verify them on inbound commits, - * but the TLS structure must still be present so decoding succeeds. + * The wire_format argument MUST match the envelope actually used; + * mixing PUBLIC_MESSAGE here with a PRIVATE_MESSAGE envelope (or + * vice versa) produces a signature that receivers can't verify + * because they recompute the TBS with the real wire_format byte. */ + internal fun buildCommitFramedContentTbs( + groupId: ByteArray, + epoch: Long, + senderLeafIndex: Int, + commitBytes: ByteArray, + groupContextBytes: ByteArray, + wireFormat: WireFormat = WireFormat.PUBLIC_MESSAGE, + ): ByteArray { + val writer = TlsWriter() + writer.putUint16(MlsMessage.MLS_VERSION_10) + writer.putUint16(wireFormat.value) + writer.putOpaqueVarInt(groupId) + writer.putUint64(epoch) + encodeSender(writer, Sender(SenderType.MEMBER, senderLeafIndex)) + writer.putOpaqueVarInt(ByteArray(0)) // authenticated_data + writer.putUint8(ContentType.COMMIT.value) + writer.putBytes(commitBytes) // Commit struct — no outer length prefix + writer.putBytes(groupContextBytes) // member sender appends context raw + return writer.toByteArray() + } + internal fun framePublicMessageCommit( groupId: ByteArray, epoch: Long, senderLeafIndex: Int, commitBytes: ByteArray, confirmationTag: ByteArray, + signature: ByteArray, + membershipKey: ByteArray, + tbsBytes: ByteArray, ): ByteArray { + // AuthenticatedContentTBM = TBS || FramedContentAuthData, where + // FramedContentAuthData = signature || (for commits) confirmation_tag. + // Caller already signed the TBS so we reuse the exact bytes they + // produced — any drift here would desync the membership_tag from + // the signature. + val tbmWriter = TlsWriter() + tbmWriter.putBytes(tbsBytes) + tbmWriter.putOpaqueVarInt(signature) + tbmWriter.putOpaqueVarInt(confirmationTag) + val tbm = tbmWriter.toByteArray() + + // membership_tag = MAC(membership_key, TBM) — HMAC-SHA256 for + // ciphersuite 0x0001. Length = 32; openmls's equal_ct logs + // "Incompatible values" when an empty tag is compared to this. + val macInstance = MacInstance("HmacSHA256", membershipKey) + macInstance.update(tbm) + val membershipTag = macInstance.doFinal() + val publicMessage = PublicMessage( groupId = groupId, @@ -1764,9 +2088,9 @@ class MlsGroup private constructor( authenticatedData = ByteArray(0), contentType = ContentType.COMMIT, content = commitBytes, - signature = ByteArray(0), + signature = signature, confirmationTag = confirmationTag, - membershipTag = ByteArray(0), + membershipTag = membershipTag, ) return MlsMessage.fromPublicMessage(publicMessage).toTlsBytes() } @@ -1780,9 +2104,18 @@ class MlsGroup private constructor( senderLeafIndex: Int, groupId: ByteArray, epoch: Long, + signature: ByteArray = ByteArray(0), + wireFormat: WireFormat = WireFormat.PUBLIC_MESSAGE, ): ByteArray { + // RFC 9420 §8.2: ConfirmedTranscriptHashInput carries the wire_format + // of the actual AuthenticatedContent — openmls passes + // `mls_content.wire_format()` through. When B sends a commit as a + // PrivateMessage (mdk default = AlwaysCiphertext outgoing), amy + // MUST recompute the transcript hash with wire_format=2, or the + // resulting confirmed_transcript_hash — and thus the + // confirmation_tag derived from it — silently diverges from B's. val writer = TlsWriter() - writer.putUint16(WireFormat.PUBLIC_MESSAGE.value) + writer.putUint16(wireFormat.value) writer.putOpaqueVarInt(groupId) writer.putUint64(epoch) writer.putUint8(1) // SenderType.MEMBER @@ -1790,7 +2123,7 @@ class MlsGroup private constructor( writer.putOpaqueVarInt(ByteArray(0)) // authenticated_data writer.putUint8(ContentType.COMMIT.value) commit.encodeTls(writer) - writer.putOpaqueVarInt(ByteArray(0)) // signature placeholder + writer.putOpaqueVarInt(signature) // FramedContentAuthData.signature return writer.toByteArray() } @@ -1929,6 +2262,7 @@ class MlsGroup private constructor( fun create( identity: ByteArray, signingKey: ByteArray? = null, + initialExtensions: List = emptyList(), ): MlsGroup { val sigKp = signingKey?.let { key -> @@ -1953,13 +2287,18 @@ class MlsGroup private constructor( tree.setLeaf(0, leafNode) val treeHash = tree.treeHash() + // Start with required_capabilities + whatever the caller wants to + // bake into epoch 0 (e.g. the MIP-01 MarmotGroupData extension so + // new peers who join later can see the group name without first + // decrypting a pre-membership bootstrap commit — see MIP-03). + val baseExtensions = listOf(buildMarmotRequiredCapabilitiesExtension()) val groupContext = GroupContext( groupId = groupId, epoch = 0, treeHash = treeHash, confirmedTranscriptHash = ByteArray(0), - extensions = listOf(buildMarmotRequiredCapabilitiesExtension()), + extensions = baseExtensions + initialExtensions, ) // Initial key schedule with zero secrets @@ -2086,6 +2425,15 @@ class MlsGroup private constructor( "Invalid GroupInfo signature" } + // RFC 9420 §12.4.3.1: the ratchet_tree extension the joiner + // reconstructs MUST hash to the tree_hash committed in the + // signed GroupContext. Otherwise a compromised signer could + // feed the joiner a tree different from the one encoded in + // the signed context, silently diverging their key schedule. + require(tree.treeHash().contentEquals(groupContext.treeHash)) { + "GroupInfo tree_hash does not match ratchet_tree extension" + } + // Derive epoch secrets directly from memberSecret (RFC 9420 Section 8.3) // For Welcome, epoch_secret = ExpandWithLabel(member_secret, "epoch", GroupContext, Nh) val epochSecret = @@ -2124,6 +2472,19 @@ class MlsGroup private constructor( val confirmMac = MacInstance("HmacSHA256", epochSecrets.confirmationKey) confirmMac.update(groupContext.confirmedTranscriptHash) val confirmationTag = confirmMac.doFinal() + + // RFC 9420 §12.4.3.1: the confirmation_tag the joiner would + // derive from the joiner_secret-sourced confirmation_key MUST + // match the confirmation_tag embedded in the signed GroupInfo. + // Without this check a tampered GroupInfo could supply a + // mismatched confirmed_transcript_hash that still passes the + // signature-over-(context, extensions, tag, signer) as long + // as the attacker controls the signer — the confirmation_tag + // binds the epoch secrets to the transcript. + require(constantTimeEquals(groupInfo.confirmationTag, confirmationTag)) { + "GroupInfo confirmation_tag does not match joiner-derived confirmation_key" + } + val interimInput = TlsWriter() interimInput.putBytes(groupContext.confirmedTranscriptHash) interimInput.putOpaqueVarInt(confirmationTag) @@ -2177,6 +2538,14 @@ class MlsGroup private constructor( "Invalid GroupInfo signature in externalJoin" } + // RFC 9420 §12.4.3.1: enforce that the reconstructed + // ratchet_tree hashes to the signed tree_hash. Without this, + // a malicious signer could serve an externalJoin consumer + // a tree that diverges from the signed context. + require(tree.treeHash().contentEquals(groupContext.treeHash)) { + "externalJoin tree_hash does not match ratchet_tree extension" + } + // Extract external_pub from extensions val externalPubExt = groupInfo.extensions.find { it.extensionType == EXTERNAL_PUB_EXTENSION_TYPE } @@ -2303,10 +2672,11 @@ class MlsGroup private constructor( ) val commitBytes = commit.toTlsBytes() - // Derive epoch secrets using external init_secret + // Derive epoch secrets using external init_secret. + // commit_secret = DeriveSecret(root_path_secret, "path") (RFC 9420 §9.2). val commitSecret = if (pathSecrets.isNotEmpty()) { - pathSecrets.last().pathSecret + MlsCryptoProvider.deriveSecret(pathSecrets.last().pathSecret, "path") } else { ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) } @@ -2444,17 +2814,75 @@ class MlsGroup private constructor( } /** - * Remove self from the group. + * Build a standalone SelfRemove proposal framed as a PublicMessage MLS + * message (RFC 9420 §6.2 + draft-ietf-mls-extensions). * - * Per MIP-01/MIP-03, admins must self-demote first; this helper rejects - * calls from a member currently listed in `admin_pubkeys`. + * openmls/mdk explicitly treat SelfRemove as a STANDALONE proposal + * message, not a commit body: a non-admin committer can't self-remove + * (openmls returns `RequiredPathNotFound`/`AttemptedSelfRemoval`) so + * quartz must publish it as a plain PROPOSAL instead. Admin receivers + * (wn's mdk auto-commit path) pick up the staged proposal and fold it + * into their next commit, which is what actually removes the sender. + * + * The bytes returned are the full `MlsMessage(PublicMessage(proposal))` + * ready for outer ChaCha20 wrapping as kind:445 content. The second + * return value is the epoch this message must be outer-encrypted under. */ - fun selfRemove(): ByteArray { + fun buildSelfRemoveProposalMessage(): Pair { check(!isLocalAdmin()) { "Admin must self-demote via GroupContextExtensions before SelfRemove (MIP-01)" } + + val preCommitExporterSecret = + exporterSecret("marmot", "group-event".encodeToByteArray(), 32) + val proposal = Proposal.SelfRemove() - return proposal.toTlsBytes() + val proposalBytes = proposal.toTlsBytes() + val ctx = groupContext + val ctxBytes = ctx.toTlsBytes() + val preCommitMembershipKey = epochSecrets.membershipKey + val preCommitSigningKey = signingPrivateKey + + // FramedContentTBS for a member-sender PROPOSAL over PublicMessage: + // version || wire_format || FramedContent || serialized_context + val tbsWriter = TlsWriter() + tbsWriter.putUint16(MlsMessage.MLS_VERSION_10) + tbsWriter.putUint16(WireFormat.PUBLIC_MESSAGE.value) + tbsWriter.putOpaqueVarInt(ctx.groupId) + tbsWriter.putUint64(ctx.epoch) + encodeSender(tbsWriter, Sender(SenderType.MEMBER, myLeafIndex)) + tbsWriter.putOpaqueVarInt(ByteArray(0)) // authenticated_data + tbsWriter.putUint8(ContentType.PROPOSAL.value) + tbsWriter.putBytes(proposalBytes) // proposal struct, no outer length prefix + tbsWriter.putBytes(ctxBytes) // member sender appends context + val tbs = tbsWriter.toByteArray() + + val signature = MlsCryptoProvider.signWithLabel(preCommitSigningKey, "FramedContentTBS", tbs) + + // TBM = TBS || FramedContentAuthData. For PROPOSAL there's no + // confirmation_tag, just the signature. + val tbmWriter = TlsWriter() + tbmWriter.putBytes(tbs) + tbmWriter.putOpaqueVarInt(signature) + val tbm = tbmWriter.toByteArray() + + val macInstance = MacInstance("HmacSHA256", preCommitMembershipKey) + macInstance.update(tbm) + val membershipTag = macInstance.doFinal() + + val publicMessage = + PublicMessage( + groupId = ctx.groupId, + epoch = ctx.epoch, + sender = Sender(SenderType.MEMBER, myLeafIndex), + authenticatedData = ByteArray(0), + contentType = ContentType.PROPOSAL, + content = proposalBytes, + signature = signature, + confirmationTag = null, + membershipTag = membershipTag, + ) + return MlsMessage.fromPublicMessage(publicMessage).toTlsBytes() to preCommitExporterSecret } } 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 7fddab22b..ddab3c19d 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 @@ -174,10 +174,11 @@ class MlsGroupManager( nostrGroupId: HexKey, identity: ByteArray, signingKey: ByteArray? = null, + initialExtensions: List = emptyList(), ): MlsGroup = mutex.withLock { Log.d(TAG) { "createGroup($nostrGroupId): creating new MLS group" } - val group = MlsGroup.create(identity, signingKey) + val group = MlsGroup.create(identity, signingKey, initialExtensions) groups[nostrGroupId] = group persistGroup(nostrGroupId) Log.d(TAG) { "createGroup($nostrGroupId): done, in-memory group count=${groups.size}" } @@ -287,6 +288,8 @@ class MlsGroupManager( commitBytes: ByteArray, senderLeafIndex: Int, confirmationTag: ByteArray, + signature: ByteArray = ByteArray(0), + wireFormat: com.vitorpamplona.quartz.marmot.mls.framing.WireFormat = com.vitorpamplona.quartz.marmot.mls.framing.WireFormat.PUBLIC_MESSAGE, ) = mutex.withLock { val group = requireGroup(nostrGroupId) @@ -297,7 +300,7 @@ class MlsGroupManager( // the current epoch key, wasting the finite retention slots. val retainedBefore = group.retainedSecrets() - group.processCommit(commitBytes, senderLeafIndex, confirmationTag) + group.processCommit(commitBytes, senderLeafIndex, confirmationTag, signature, wireFormat) pushRetainedEpoch(nostrGroupId, retainedBefore) persistGroup(nostrGroupId) @@ -331,19 +334,40 @@ class MlsGroupManager( mutex.withLock { val group = requireGroup(nostrGroupId) - // Try current epoch - val current = group.decryptOrNull(messageBytes) - if (current != null) return@withLock current + // Try current epoch. If we hit an exception here we MUST surface + // it — commits that throw mid-processCommit leave the in-memory + // group half-mutated, and a retry via `group.decrypt(...)` will + // just report a stale "epoch mismatch" from the partial advance, + // hiding the real bug. Capture the original throwable, try + // retained epochs as a fallback, and re-raise the captured one + // if nothing decrypts. + val retainedBefore = group.retainedSecrets() + val preEpoch = group.epoch + val currentFailure: Throwable? = + try { + val result = group.decrypt(messageBytes) + // PrivateMessage commits apply inline through + // `MlsGroup.decrypt` → `processCommit`; the epoch advances + // in memory but the CLI reopens a fresh Context on every + // command, so we MUST persist here or reloaded state + // silently reverts to the pre-commit extensions (including + // admin list). + if (result.contentType == com.vitorpamplona.quartz.marmot.mls.framing.ContentType.COMMIT && group.epoch != preEpoch) { + pushRetainedEpoch(nostrGroupId, retainedBefore) + persistGroup(nostrGroupId) + } + return@withLock result + } catch (t: Throwable) { + t + } - // Try retained epochs val retained = retainedEpochs[nostrGroupId] ?: emptyList() for (epochSecrets in retained) { val result = tryDecryptWithRetainedEpoch(messageBytes, epochSecrets) if (result != null) return@withLock result } - // No epoch could decrypt — rethrow from current epoch for diagnostics - group.decrypt(messageBytes) + throw currentFailure ?: IllegalStateException("Decrypt failed without captured cause") } /** @@ -441,16 +465,17 @@ class MlsGroupManager( } /** - * Leave a group (self-remove). - * Returns the SelfRemove proposal bytes to publish, then removes - * local state. + * Leave a group by publishing a standalone PublicMessage SelfRemove + * proposal (draft-ietf-mls-extensions). Admin receivers auto-commit + * the pending proposal; the caller publishes the framed bytes as the + * kind:445 content, outer-encrypted with the returned exporter key. */ - suspend fun leaveGroup(nostrGroupId: HexKey): ByteArray = + suspend fun leaveGroup(nostrGroupId: HexKey): Pair = mutex.withLock { val group = requireGroup(nostrGroupId) - val proposalBytes = group.selfRemove() + val result = group.buildSelfRemoveProposalMessage() removeGroupStateUnlocked(nostrGroupId) - proposalBytes + result } /** diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Proposal.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Proposal.kt index 8e25297ac..67ab80be9 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Proposal.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/messages/Proposal.kt @@ -45,8 +45,11 @@ enum class ProposalType( EXTERNAL_INIT(6), GROUP_CONTEXT_EXTENSIONS(7), - // Marmot custom proposal types (private-use range 0xF000-0xFFFF) - SELF_REMOVE(0xF001), + // SelfRemove is standardized in MLS Extensions draft-ietf-mls-extensions + // as IANA proposal type 0x000A, NOT a Marmot private-use value. + // openmls / mdk encode it as 0x000A on the wire; quartz was writing + // 0xF001, which strict receivers reject as "Unknown ProposalType". + SELF_REMOVE(0x000A), ; companion object { 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 5c3c28e66..596b80262 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 @@ -50,17 +50,21 @@ class SecretTree( /** Per-sender ratchet state: (handshake generation, handshake secret, app generation, app secret) */ private val senderState = mutableMapOf() - /** Consumed (sender, generation) pairs for replay detection (RFC 9420 Section 9.1) */ + /** Consumed (sender, generation) pairs for replay detection on the APPLICATION ratchet. */ private val consumedGenerations = mutableMapOf>() + /** Same replay tracker, but for the HANDSHAKE ratchet (commits / proposals). */ + private val consumedHandshakeGenerations = mutableMapOf>() + /** - * Cache of key/nonce pairs for skipped generations. + * Cache of key/nonce pairs for skipped APPLICATION generations. * Key: (leafIndex, generation) -> derived KeyNonceGeneration. - * When fast-forwarding a ratchet, intermediate generations are saved here - * so that out-of-order messages arriving later can still be decrypted. */ private val skippedKeys = mutableMapOf, KeyNonceGeneration>() + /** Same cache for the HANDSHAKE ratchet. */ + private val handshakeSkippedKeys = mutableMapOf, KeyNonceGeneration>() + private companion object { /** Maximum number of skipped key entries to retain (prevents unbounded memory growth). */ const val MAX_SKIPPED_KEYS = 1000 @@ -186,6 +190,70 @@ class SecretTree( return result } + /** + * Handshake-ratchet counterpart to [applicationKeyNonceForGeneration]. + * + * PrivateMessage commits / proposals (RFC 9420 §6.3.2) are encrypted + * with a separate handshake ratchet per sender leaf — NOT the + * application ratchet. openmls / mdk use this path by default + * (`MIXED_CIPHERTEXT_WIRE_FORMAT_POLICY`), so quartz must also + * ratchet-forward the handshake chain when decrypting an inbound + * PrivateMessage commit. + */ + fun handshakeKeyNonceForGeneration( + leafIndex: Int, + generation: Int, + ): KeyNonceGeneration { + val cachedKey = handshakeSkippedKeys.remove(Pair(leafIndex, generation)) + if (cachedKey != null) { + val senderConsumed = consumedHandshakeGenerations.getOrPut(leafIndex) { mutableSetOf() } + require(generation !in senderConsumed) { + "Replay detected: handshake generation $generation from sender $leafIndex already consumed" + } + senderConsumed.add(generation) + return cachedKey + } + + val state = getOrInitSender(leafIndex) + + require(generation >= state.handshakeGeneration) { + "Handshake generation $generation already consumed (current: ${state.handshakeGeneration})" + } + + val senderConsumed = consumedHandshakeGenerations.getOrPut(leafIndex) { mutableSetOf() } + require(generation !in senderConsumed) { + "Replay detected: handshake generation $generation from sender $leafIndex already consumed" + } + senderConsumed.add(generation) + + if (senderConsumed.size > MAX_CONSUMED_GENERATIONS_PER_SENDER) { + val minGeneration = state.handshakeGeneration + senderConsumed.removeAll { it < minGeneration } + } + + var secret = state.handshakeSecret + var gen = state.handshakeGeneration + while (gen < generation) { + val intermediateKng = deriveKeyNonce(secret, gen) + val cacheKey = Pair(leafIndex, gen) + if (handshakeSkippedKeys.size < MAX_SKIPPED_KEYS) { + handshakeSkippedKeys[cacheKey] = intermediateKng + } + secret = MlsCryptoProvider.expandWithLabel(secret, "secret", generationContext(gen), MlsCryptoProvider.HASH_OUTPUT_LENGTH) + gen++ + } + + val result = deriveKeyNonce(secret, generation) + val nextSecret = MlsCryptoProvider.expandWithLabel(secret, "secret", generationContext(generation), MlsCryptoProvider.HASH_OUTPUT_LENGTH) + senderState[leafIndex] = + state.copy( + handshakeSecret = nextSecret, + handshakeGeneration = generation + 1, + ) + + return result + } + /** * Encode a generation counter as a 4-byte big-endian uint32 for DeriveTreeSecret context. */ diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt index 729186470..39f70a786 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt @@ -24,7 +24,6 @@ import com.vitorpamplona.quartz.marmot.mls.codec.TlsReader import com.vitorpamplona.quartz.marmot.mls.codec.TlsSerializable import com.vitorpamplona.quartz.marmot.mls.codec.TlsWriter import com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider -import com.vitorpamplona.quartz.marmot.mls.crypto.X25519 /** * MLS Ratchet Tree (RFC 9420 Section 7). @@ -157,18 +156,56 @@ class RatchetTree( /** * Remove a member by blanking their leaf and all parent nodes on the direct path. + * + * RFC 9420 §7.8: trailing blank leaves MUST be trimmed so every participant + * agrees on `leaf_count` (and therefore on `direct_path` lengths). Openmls + * does this after every leaf blank; without the trim, a committer that had + * the removed leaf at the far right end of the tree sends an UpdatePath one + * entry longer than the receiver's tree layout accepts, and the receiver + * errors out with `UpdatePathError(PathLengthMismatch)`. */ fun removeLeaf(leafIndex: Int) { setLeaf(leafIndex, null) - // Blank the direct path val directPath = BinaryTree.directPath(leafIndex, _leafCount) for (nodeIdx in directPath) { if (nodeIdx < nodes.size) { nodes[nodeIdx] = null } } + // Shrink leafCount past any trailing blanks. Parent nodes that become + // orphaned by the shrink are dropped from the `nodes` list so + // treeHash() / resolution() / direct_path() agree with openmls on the + // new tree shape. + while (_leafCount > 0 && getLeaf(_leafCount - 1) == null) { + _leafCount-- + } + val effectiveNodeCount = if (_leafCount > 0) BinaryTree.nodeCount(_leafCount) else 0 + while (nodes.size > effectiveNodeCount) { + nodes.removeAt(nodes.size - 1) + } } + /** + * Snapshot the mutable tree state so callers can roll back after a + * failed commit application. `TreeNode`, `LeafNode`, and `ParentNode` + * are all immutable data classes — copying the `nodes` list is enough + * to isolate future edits. + */ + fun snapshot(): Snapshot = Snapshot(nodes.toList(), _leafCount) + + /** Restore the mutable tree state produced by an earlier [snapshot]. */ + fun restoreFrom(snapshot: Snapshot) { + nodes.clear() + nodes.addAll(snapshot.nodes) + _leafCount = snapshot.leafCount + } + + /** Opaque capture of the ratchet tree's mutable state. */ + class Snapshot internal constructor( + internal val nodes: List, + internal val leafCount: Int, + ) + /** * Compute the tree hash for this ratchet tree (RFC 9420 Section 7.9). * Used in GroupContext to bind the group state to the tree. @@ -278,15 +315,20 @@ class RatchetTree( var currentSecret = leafSecret for (nodeIdx in directPath) { - // RFC 9420 Section 7.4: path_secret[0] = leafSecret, - // node_secret[n] = DeriveSecret(path_secret[n], "node") + // RFC 9420 §7.4: path_secret[0] = leafSecret, + // node_secret[n] = DeriveSecret(path_secret[n], "node"), + // node's HPKE keypair = DeriveKeyPair(node_secret). + // DeriveKeyPair is HPKE's (RFC 9180 §7.1.3), NOT an MLS ExpandWithLabel — + // it uses the "HPKE-v1" + KEM suite_id labels, and produces a keypair + // that openmls/mdk can reproduce from the same path_secret. If we derive + // with a different formula, receivers compute different public keys and + // reject the UpdatePath with `UpdatePathError(PathMismatch)`. val nodeSecret = MlsCryptoProvider.deriveSecret(currentSecret, "node") + val kp = + com.vitorpamplona.quartz.marmot.mls.crypto.Hpke + .deriveKeyPair(nodeSecret) - // Derive HPKE key pair from node_secret - val privateKey = MlsCryptoProvider.expandWithLabel(nodeSecret, "hpke", ByteArray(0), 32) - val publicKey = X25519.publicFromPrivate(privateKey) - - results.add(PathSecretAndKey(currentSecret, privateKey, publicKey)) + results.add(PathSecretAndKey(currentSecret, kp.privateKey, kp.publicKey)) // path_secret[n+1] = DeriveSecret(path_secret[n], "path") currentSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path") diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupNegativeTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupNegativeTest.kt new file mode 100644 index 000000000..4ebd900fe --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupNegativeTest.kt @@ -0,0 +1,291 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.marmot.mls.group + +import com.vitorpamplona.quartz.marmot.mls.codec.TlsReader +import com.vitorpamplona.quartz.marmot.mls.framing.MlsMessage +import com.vitorpamplona.quartz.marmot.mls.framing.PublicMessage +import com.vitorpamplona.quartz.marmot.mls.framing.WireFormat +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertNotEquals +import kotlin.test.assertTrue + +/** + * Negative tests: every post-RFC-9420-§6 authenticity check that + * [MlsGroup.processCommit] performs should reject the tampered input AND + * leave the group in its pre-commit state (atomic rollback — no partial + * epoch advance, no mutated tree, no dangling pending proposals). + */ +class MlsGroupNegativeTest { + private data class TwoMemberFixture( + val alice: MlsGroup, + val bob: MlsGroup, + val charliePkg: ByteArray, + ) + + /** + * Produce a 2-member group (alice creator, bob joined via Welcome) + * plus a spare KeyPackage for Charlie. Callers use Charlie's bundle to + * drive Alice into producing a fresh Add commit that Bob will process. + */ + private fun twoMemberGroup(): TwoMemberFixture { + val alice = MlsGroup.create(identity = "alice".encodeToByteArray()) + + val bobBundle = alice.createKeyPackage(identity = "bob".encodeToByteArray(), signingKey = ByteArray(32) { 1 }) + val addBob = alice.addMember(bobBundle.keyPackage.toTlsBytes()) + val bob = MlsGroup.processWelcome(addBob.welcomeBytes!!, bobBundle) + + val charlieBundle = + alice.createKeyPackage(identity = "charlie".encodeToByteArray(), signingKey = ByteArray(32) { 2 }) + + return TwoMemberFixture(alice, bob, charlieBundle.keyPackage.toTlsBytes()) + } + + /** + * Decode a framedCommitBytes (MlsMessage(PublicMessage(commit))) into + * the fields [MlsGroup.processCommit] consumes. Tests re-use this to + * mutate individual fields before re-invoking. + */ + private data class CommitParts( + val content: ByteArray, + val senderLeafIndex: Int, + val confirmationTag: ByteArray, + val signature: ByteArray, + val pubMsg: PublicMessage, + ) + + private fun parseCommit(framedBytes: ByteArray): CommitParts { + val mlsMsg = MlsMessage.decodeTls(TlsReader(framedBytes)) + val pub = PublicMessage.decodeTls(TlsReader(mlsMsg.payload)) + return CommitParts( + content = pub.content, + senderLeafIndex = pub.sender.leafIndex, + confirmationTag = pub.confirmationTag!!, + signature = pub.signature, + pubMsg = pub, + ) + } + + /** Baseline: an honest commit applies and advances Bob's epoch. */ + @Test + fun honestCommitIsAccepted() { + val fx = twoMemberGroup() + val bobEpochBefore = fx.bob.epoch + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = parts.confirmationTag, + signature = parts.signature, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + assertEquals(bobEpochBefore + 1, fx.bob.epoch) + } + + /** + * Tampered confirmation_tag — flipping a single bit must fail the + * HMAC compare, throw, and leave Bob on the original epoch. + */ + @Test + fun tamperedConfirmationTagIsRejectedAndStateRolledBack() { + val fx = twoMemberGroup() + val epochBefore = fx.bob.epoch + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + val tamperedTag = parts.confirmationTag.copyOf().apply { this[0] = (this[0].toInt() xor 0x01).toByte() } + + assertFailsWith { + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = tamperedTag, + signature = parts.signature, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + } + assertEquals(epochBefore, fx.bob.epoch) + + // And the honest commit still applies cleanly afterwards — proving + // no mutable state leaked across the failed attempt. + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = parts.confirmationTag, + signature = parts.signature, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + assertEquals(epochBefore + 1, fx.bob.epoch) + } + + /** + * Tampered FramedContentTBS signature — receiver reconstructs the + * exact same TBS bytes the sender signed; any bit-flip in the + * signature fails Ed25519 verification. + */ + @Test + fun tamperedSignatureIsRejectedAndStateRolledBack() { + val fx = twoMemberGroup() + val epochBefore = fx.bob.epoch + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + val tamperedSig = parts.signature.copyOf().apply { this[0] = (this[0].toInt() xor 0x80).toByte() } + + assertFailsWith { + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = parts.confirmationTag, + signature = tamperedSig, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + } + assertEquals(epochBefore, fx.bob.epoch) + } + + /** + * Claiming the commit came from the wrong leaf index — the signature + * was bound to the original sender leaf, so verifying against a + * different leaf's pre-commit signatureKey fails. + */ + @Test + fun spoofedSenderLeafIndexIsRejected() { + val fx = twoMemberGroup() + val epochBefore = fx.bob.epoch + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + // Alice is leaf 0, Bob is leaf 1. Swap. + val wrongLeaf = if (parts.senderLeafIndex == 0) 1 else 0 + + assertFailsWith { + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = wrongLeaf, + confirmationTag = parts.confirmationTag, + signature = parts.signature, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + } + assertEquals(epochBefore, fx.bob.epoch) + } + + /** + * Declaring the wrong wire_format at the receiver — the FramedContentTBS + * hash diverges and signature verification fails, because the sender + * mixed wire_format=PUBLIC_MESSAGE into their TBS bytes. + */ + @Test + fun wrongWireFormatIsRejected() { + val fx = twoMemberGroup() + val epochBefore = fx.bob.epoch + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + + assertFailsWith { + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = parts.confirmationTag, + signature = parts.signature, + wireFormat = WireFormat.PRIVATE_MESSAGE, + ) + } + assertEquals(epochBefore, fx.bob.epoch) + } + + /** + * Empty confirmation_tag is rejected explicitly — RFC 9420 §6 requires + * every commit to carry a confirmation_tag, and an omitted tag means + * the receiver has no authentication of the post-commit epoch secrets. + */ + @Test + fun emptyConfirmationTagIsRejected() { + val fx = twoMemberGroup() + val epochBefore = fx.bob.epoch + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + + assertFailsWith { + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = ByteArray(0), + signature = parts.signature, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + } + assertEquals(epochBefore, fx.bob.epoch) + } + + /** + * Tampered membership_tag — PublicMessage commits from a member must + * carry an HMAC(membership_key, TBM) that matches what the receiver + * can reconstruct from the current epoch's membership_key. Without + * this check, an outsider that learned the outer exporter secret + * could forge arbitrary commit bodies. + * + * This is enforced at the MarmotInboundProcessor layer before + * [MlsGroup.processCommit] is called; the group exposes + * [MlsGroup.verifyPublicMessageCommitMembershipTag] so the processor + * can short-circuit on tag mismatch. + */ + @Test + fun tamperedMembershipTagFailsPublicMessageCheck() { + val fx = twoMemberGroup() + + val commit = fx.alice.addMember(fx.charliePkg) + val parts = parseCommit(commit.framedCommitBytes) + + // Honest tag is valid. + assertTrue(fx.bob.verifyPublicMessageCommitMembershipTag(parts.pubMsg)) + + // Flip one bit in the wire tag — must be rejected. + val original = parts.pubMsg.membershipTag!! + val tampered = original.copyOf().apply { this[0] = (this[0].toInt() xor 0x01).toByte() } + val badPub = parts.pubMsg.copy(membershipTag = tampered) + assertFalse(fx.bob.verifyPublicMessageCommitMembershipTag(badPub)) + + // A missing tag is also rejected. + val missingPub = parts.pubMsg.copy(membershipTag = null) + assertFalse(fx.bob.verifyPublicMessageCommitMembershipTag(missingPub)) + + // And the honest commit still applies — nothing got mutated by the checks. + val epochBefore = fx.bob.epoch + fx.bob.processCommit( + commitBytes = parts.content, + senderLeafIndex = parts.senderLeafIndex, + confirmationTag = parts.confirmationTag, + signature = parts.signature, + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + assertEquals(epochBefore + 1, fx.bob.epoch) + assertNotEquals(epochBefore, fx.bob.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 f8a2cc345..cedb62ea3 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -173,7 +173,7 @@ class MarmotMipBehaviorTest { val alice = manager.getGroup(groupId)!! assertFailsWith { alice.proposeSelfRemove() } - assertFailsWith { alice.selfRemove() } + assertFailsWith { alice.buildSelfRemoveProposalMessage() } } @Test @@ -187,8 +187,8 @@ class MarmotMipBehaviorTest { manager.updateGroupExtensions(groupId, listOf(strangerAdmin.toExtension())) val alice = manager.getGroup(groupId)!! - // selfRemove (standalone proposal helper) should succeed for a non-admin. - val bytes = alice.selfRemove() + // Standalone SelfRemove proposal helper should succeed for a non-admin. + val (bytes, _) = alice.buildSelfRemoveProposalMessage() assertTrue(bytes.isNotEmpty()) } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupTest.kt index 2dc3e3f64..57ded76b7 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupTest.kt @@ -272,7 +272,7 @@ class MlsGroupTest { @Test fun testSelfRemove() { val group = MlsGroup.create("alice".encodeToByteArray()) - val selfRemoveBytes = group.selfRemove() + val (selfRemoveBytes, _) = group.buildSelfRemoveProposalMessage() assertTrue(selfRemoveBytes.isNotEmpty()) } diff --git a/tools/marmot-interop/headless/patches/whitenoise-defaults-env.patch b/tools/marmot-interop/headless/patches/whitenoise-defaults-env.patch new file mode 100644 index 000000000..57cc16cd4 --- /dev/null +++ b/tools/marmot-interop/headless/patches/whitenoise-defaults-env.patch @@ -0,0 +1,24 @@ +--- a/src/whitenoise/relays.rs ++++ b/src/whitenoise/relays.rs +@@ -98,6 +98,21 @@ + } + + pub(crate) fn defaults() -> Vec { ++ // marmot-interop-headless patch: honour $WHITENOISE_DISCOVERY_RELAYS ++ // (comma-separated list) when present so newly created accounts only ++ // ever get our loopback relay baked into their NIP-65 / inbox / ++ // key-package lists. Without this override, `create-identity` stamps ++ // the hard-coded public set into the account's relay lists, and every ++ // later activate / publish burns connection budget on unreachable ++ // sockets — enough to break inbox-plane activation and drop kind:1059. ++ if let Ok(from_env) = std::env::var("WHITENOISE_DISCOVERY_RELAYS") { ++ let parsed: Vec = from_env ++ .split(',').map(str::trim).filter(|s| !s.is_empty()) ++ .filter_map(|u| RelayUrl::parse(u).ok()) ++ .map(|url| Relay::new(&url)) ++ .collect(); ++ if !parsed.is_empty() { return parsed; } ++ } + let urls: &[&str] = if cfg!(debug_assertions) { + &["ws://localhost:8080", "ws://localhost:7777"] + } else { diff --git a/tools/marmot-interop/headless/patches/whitenoise-mdk-plaintext-policy.patch b/tools/marmot-interop/headless/patches/whitenoise-mdk-plaintext-policy.patch new file mode 100644 index 000000000..9c3f9e97e --- /dev/null +++ b/tools/marmot-interop/headless/patches/whitenoise-mdk-plaintext-policy.patch @@ -0,0 +1,11 @@ +--- a/../../../.cargo/git/checkouts/mdk-7d5a3a2420b194f5/8a8d06c/crates/mdk-core/src/groups.rs ++++ b/../../../.cargo/git/checkouts/mdk-7d5a3a2420b194f5/8a8d06c/crates/mdk-core/src/groups.rs +@@ -1215,7 +1215,7 @@ + ); + let group_config = MlsGroupCreateConfig::builder() + .ciphersuite(self.ciphersuite) +- .wire_format_policy(MIXED_CIPHERTEXT_WIRE_FORMAT_POLICY) ++ .wire_format_policy(MIXED_PLAINTEXT_WIRE_FORMAT_POLICY) + .use_ratchet_tree_extension(true) + .capabilities(capabilities) + .with_group_context_extensions(extensions) diff --git a/tools/marmot-interop/headless/patches/whitenoise-skip-unprocessable-retry.patch b/tools/marmot-interop/headless/patches/whitenoise-skip-unprocessable-retry.patch new file mode 100644 index 000000000..c4caaa3d6 --- /dev/null +++ b/tools/marmot-interop/headless/patches/whitenoise-skip-unprocessable-retry.patch @@ -0,0 +1,26 @@ +--- a/src/whitenoise/event_processor/account_event_processor.rs ++++ b/src/whitenoise/event_processor/account_event_processor.rs +@@ -178,7 +178,23 @@ + } + Err(e) => { + // Handle retry logic for actual processing errors +- if retry_info.should_retry() { ++ // marmot-interop-headless patch: MLS errors that come from ++ // mdk are ALREADY terminal — mdk doesn't retry internally, so ++ // any Err it returns (Unprocessable, PreviouslyFailed, decrypt ++ // failure, group-not-found, etc.) is provably permanent. ++ // Retrying those 10 times with exponential backoff (total ++ // ~17 min) just blocks later decryptable commits behind a ++ // queue of doomed retries, so every later join / rename / ++ // leave propagation races the test timeout. Treat them all ++ // as one-shot: log once, move on. ++ let is_terminal = matches!( ++ e, ++ WhitenoiseError::MlsMessageUnprocessable(_) ++ | WhitenoiseError::MlsMessagePreviouslyFailed ++ | WhitenoiseError::MdkCoreError(_), ++ ); ++ if !is_terminal && retry_info.should_retry() { + self.schedule_retry(event, source, retry_info, e); + } else { + tracing::error!( diff --git a/tools/marmot-interop/headless/setup.sh b/tools/marmot-interop/headless/setup.sh index 635e10de1..c58658a65 100644 --- a/tools/marmot-interop/headless/setup.sh +++ b/tools/marmot-interop/headless/setup.sh @@ -39,7 +39,7 @@ preflight() { 2>&1 | tee -a "$LOG_FILE" fi - # Two harness-only patches to wnd so it runs fully offline / in + # Four harness-only patches to wnd so it runs fully offline / in # sandboxes that block outbound + kernel keyring: # 1. discovery-env: honour $WHITENOISE_DISCOVERY_RELAYS so we can # point wnd at our loopback relay instead of the baked-in public @@ -47,9 +47,25 @@ preflight() { # 2. mock-keyring: honour $WHITENOISE_MOCK_KEYRING so wnd uses the # integration-tests mock keyring store when the kernel keyutils # syscalls are blocked (common in containers / CI). + # 3. defaults-env: reuse the same env var so `Relay::defaults()` + # (what `create-identity` stamps into the new account's NIP-65 / + # inbox / key-package lists) points at the loopback relay too. + # Without it every account wnd creates carries damus.io / + # primal.net / nos.lol, and every later activate / publish burns + # connection budget on unreachable sockets — enough to break the + # account-inbox subscription plane and drop kind:1059 delivery. + # 4. skip-unprocessable-retry: when mdk-core returns + # `MlsMessageUnprocessable` (pre-membership commit, too-old epoch) + # the message is provably undecryptable — retrying it ten times + # with exponential backoff (total ~17 min) just blocks later + # decryptable commits behind a queue of doomed retries, which in + # the harness manifests as "A already left" / "name unchanged" + # timeouts. The patch treats that error as terminal. local -a patches=( "whitenoise-discovery-env.patch" "whitenoise-mock-keyring.patch" + "whitenoise-defaults-env.patch" + "whitenoise-skip-unprocessable-retry.patch" ) for name in "${patches[@]}"; do local marker="$WN_REPO/.headless-patched-${name%.patch}" @@ -267,4 +283,9 @@ configure_relays() { step "publishing A's KeyPackage" amy_a marmot key-package publish >>"$LOG_FILE" 2>&1 || warn "amy marmot key-package publish failed" + # Give nostr-rs-relay a breath to fsync the kind:10002 / 10050 / 30443 + # writes and push them out on the discovery subscription so that the + # first `wn keys check` that follows actually sees them instead of + # racing the relay's WAL flush. + sleep 2 } diff --git a/tools/marmot-interop/headless/tests-create.sh b/tools/marmot-interop/headless/tests-create.sh index 702d6cdab..33c898c35 100644 --- a/tools/marmot-interop/headless/tests-create.sh +++ b/tools/marmot-interop/headless/tests-create.sh @@ -33,12 +33,19 @@ test_02_a_creates_group() { banner "Test 02 — A creates group, invites B" local id="02 A->B create+invite" - local gid - gid=$(amy_field '.group_id' marmot group create --name "Interop-02") || { + # amy keys groups by the MIP-01 nostr_group_id (`group_id`), wn (via + # mdk-core) keys by the MLS GroupContext groupId (`mls_group_id`). Both + # are 32 random bytes and they are NOT the same. We need both: amy calls + # use `gid`, wn calls use `mls_gid`. + local out gid mls_gid + out=$(amy_json marmot group create --name "Interop-02") || { record_result "$id" fail "create returned no group_id"; return } + gid=$(printf '%s' "$out" | jq -r '.group_id') + mls_gid=$(printf '%s' "$out" | jq -r '.mls_group_id') save_state GROUP_02 "$gid" - info "created group $gid" + save_state GROUP_02_MLS "$mls_gid" + info "created group nostr=$gid mls=$mls_gid" amy_json marmot group add "$gid" "$B_NPUB" >/dev/null || { record_result "$id" fail "amy group add failed"; return @@ -52,16 +59,17 @@ test_02_a_creates_group() { wn_b groups accept "$b_gid" >/dev/null 2>&1 || true info "B joined $b_gid" - # A -> B message + # A -> B message. amy takes the nostr id; wait_for_message calls + # `wn messages list` which needs the MLS id. amy_json marmot message send "$gid" "hello from amethyst" >/dev/null || { record_result "$id" fail "amy send failed"; return } - if ! wait_for_message B "$gid" "hello from amethyst" 30; then + if ! wait_for_message B "$mls_gid" "hello from amethyst" 90; then record_result "$id" fail "B didn't receive A's message"; return fi # B -> A message - wn_b messages send "$gid" "hello from wn" >/dev/null 2>&1 || warn "wn send returned nonzero" + wn_b messages send "$mls_gid" "hello from wn" >/dev/null 2>&1 || warn "wn send returned nonzero" if ! amy_json marmot await message "$gid" --match "hello from wn" --timeout 30 >/dev/null; then record_result "$id" fail "A didn't receive B's reply"; return fi @@ -72,33 +80,41 @@ test_03_b_creates_group() { banner "Test 03 — B creates group, invites A" local id="03 B->A create+invite" - local out gid + # `wn groups create` returns the MLS group id (what wn's messages API + # expects). amy indexes by the MIP-01 nostr_group_id, which we only learn + # once A has processed the welcome and we can ask `amy await group` for + # it. Keep them distinct so each CLI gets the id it understands. + local out mls_gid out=$(wn_b --json groups create "Interop-03" "$A_NPUB" 2>>"$LOG_FILE") || { record_result "$id" fail "wn groups create failed"; return } - gid=$(printf '%s' "$out" | jq_group_id) - if [[ -z "$gid" ]]; then + mls_gid=$(printf '%s' "$out" | jq_group_id) + if [[ -z "$mls_gid" ]]; then record_result "$id" fail "could not parse group_id"; return fi - save_state GROUP_03 "$gid" - info "group_id: $gid" + save_state GROUP_03_MLS "$mls_gid" + info "mls_group_id: $mls_gid" - # A: poll until it joins. - if ! amy_json marmot await group --name "Interop-03" --timeout 30 >/dev/null; then + # A: poll until it joins, and capture its nostr_group_id. + local a_out a_gid + a_out=$(amy_json marmot await group --name "Interop-03" --timeout 30) || { record_result "$id" fail "A never joined B's group"; return - fi + } + a_gid=$(printf '%s' "$a_out" | jq -r '.group_id') + save_state GROUP_03 "$a_gid" + info "A joined as nostr_group_id=$a_gid" # B -> A message - wn_b messages send "$gid" "ping from wn" >/dev/null 2>&1 || true - if ! amy_json marmot await message "$gid" --match "ping from wn" --timeout 30 >/dev/null; then + wn_b messages send "$mls_gid" "ping from wn" >/dev/null 2>&1 || true + if ! amy_json marmot await message "$a_gid" --match "ping from wn" --timeout 30 >/dev/null; then record_result "$id" fail "A didn't see B's ping"; return fi # A -> B reply - amy_json marmot message send "$gid" "pong from amethyst" >/dev/null || { + amy_json marmot message send "$a_gid" "pong from amethyst" >/dev/null || { record_result "$id" fail "amy send pong failed"; return } - if wait_for_message B "$gid" "pong from amethyst" 30; then + if wait_for_message B "$mls_gid" "pong from amethyst" 90; then record_result "$id" pass else record_result "$id" fail "B didn't see A's pong" @@ -112,7 +128,9 @@ test_04_three_member_group() { wn_c keys publish >/dev/null 2>&1 || true sleep 3 - local gid; gid=$(load_state GROUP_02 || true) + local gid mls_gid + gid=$(load_state GROUP_02 || true) + mls_gid=$(load_state GROUP_02_MLS || true) if [[ -z "${gid:-}" ]]; then record_result "$id" skip "no GROUP_02"; return fi @@ -131,8 +149,14 @@ test_04_three_member_group() { amy_json marmot message send "$gid" "hello three-member world" >/dev/null || { record_result "$id" fail "amy send failed"; return } - if wait_for_message B "$gid" "hello three-member world" 30 \ - && wait_for_message C "$c_gid" "hello three-member world" 30; then + # wn's event_processor retries undecryptable pre-membership commits with + # exponential backoff (2+4+8+16=~30s), and kind:445 processing is serial + # per-account; a fresh joiner routinely needs ~60s to burn through that + # retry queue before the add-C commit is applied and "hello three-member + # world" decrypts. The 30s we used for the inline A<->B send/recv + # wouldn't clear that. + if wait_for_message B "$mls_gid" "hello three-member world" 90 \ + && wait_for_message C "$c_gid" "hello three-member world" 90; then record_result "$id" pass else record_result "$id" fail "B or C missed A's post-add message" @@ -166,8 +190,8 @@ test_05_b_adds_a_existing() { amy_json marmot message send "$gid" "joined from amethyst" >/dev/null || { record_result "$id" fail "amy send failed"; return } - if wait_for_message B "$gid" "joined from amethyst" 30 \ - && wait_for_message C "$gid" "joined from amethyst" 30; then + if wait_for_message B "$gid" "joined from amethyst" 90 \ + && wait_for_message C "$gid" "joined from amethyst" 90; then record_result "$id" pass else record_result "$id" fail "B or C didn't see A's message" diff --git a/tools/marmot-interop/headless/tests-extras.sh b/tools/marmot-interop/headless/tests-extras.sh index 2f147c83e..6e8495856 100644 --- a/tools/marmot-interop/headless/tests-extras.sh +++ b/tools/marmot-interop/headless/tests-extras.sh @@ -7,35 +7,37 @@ test_09_reply_react_unreact() { banner "Test 09 — reply / react / unreact" local id="09 reply/react" - local gid; gid=$(load_state GROUP_02 || true) + local gid mls_gid + gid=$(load_state GROUP_02 || true) + mls_gid=$(load_state GROUP_02_MLS || true) if [[ -z "${gid:-}" ]]; then record_result "$id" skip "no GROUP_02"; return fi # B anchors. Needs a member to be present — if Test 11 already ran and A left, # skip cleanly so we don't double-fail. - if ! wn_b --json groups members "$gid" 2>/dev/null \ - | jq -e --arg p "$A_HEX" '.[]? | select((.pubkey // .public_key) == $p)' \ + if ! wn_b --json groups members "$mls_gid" 2>/dev/null \ + | jq -e --arg p "$A_HEX" '(.result // .) | .[]? | select((.pubkey // .public_key) == $p)' \ >/dev/null 2>&1; then record_result "$id" skip "A already left GROUP_02"; return fi - wn_b messages send "$gid" "anchor for reactions" >/dev/null 2>&1 || true + wn_b messages send "$mls_gid" "anchor for reactions" >/dev/null 2>&1 || true sleep 3 local msg_id - msg_id=$(wn_b --json messages list "$gid" --limit 10 2>/dev/null \ - | jq -r '[.[]? | select((.content // .text // "") == "anchor for reactions")][0].id // empty') + msg_id=$(wn_b --json messages list "$mls_gid" --limit 10 2>/dev/null \ + | jq -r '[(.result // .) | .[]? | select((.content // .text // "") == "anchor for reactions")][0].id // empty') if [[ -z "$msg_id" || "$msg_id" == "null" ]]; then record_result "$id" fail "couldn't find anchor message id"; return fi - wn_b messages react "$gid" "$msg_id" "🌮" >/dev/null 2>&1 || true + wn_b messages react "$mls_gid" "$msg_id" "🌮" >/dev/null 2>&1 || true sleep 3 # amy reply amy_json marmot message send "$gid" "replying via amy" >/dev/null || { record_result "$id" fail "amy send reply failed"; return } - if wait_for_message B "$gid" "replying via amy" 30; then + if wait_for_message B "$mls_gid" "replying via amy" 90; then record_result "$id" pass else record_result "$id" fail "B didn't receive reply" @@ -48,12 +50,14 @@ test_10_concurrent_commits() { banner "Test 10 — Concurrent commits race" local id="10 concurrent commits" - local gid; gid=$(load_state GROUP_02 || true) + local gid mls_gid + gid=$(load_state GROUP_02 || true) + mls_gid=$(load_state GROUP_02_MLS || true) if [[ -z "${gid:-}" ]]; then record_result "$id" skip "no GROUP_02"; return fi - if ! wn_b --json groups members "$gid" 2>/dev/null \ - | jq -e --arg p "$A_HEX" '.[]? | select((.pubkey // .public_key) == $p)' \ + if ! wn_b --json groups members "$mls_gid" 2>/dev/null \ + | jq -e --arg p "$A_HEX" '(.result // .) | .[]? | select((.pubkey // .public_key) == $p)' \ >/dev/null 2>&1; then record_result "$id" skip "A already left GROUP_02"; return fi @@ -62,21 +66,21 @@ test_10_concurrent_commits() { # guarantees a deterministic outcome. ( amy_json marmot group rename "$gid" "race-from-amethyst" >/dev/null ) & local a_pid=$! - ( wn_b groups rename "$gid" "race-from-wn" >/dev/null 2>&1 ) & + ( wn_b groups rename "$mls_gid" "race-from-wn" >/dev/null 2>&1 ) & local b_pid=$! wait "$a_pid" "$b_pid" 2>/dev/null || true sleep 10 local b_name - b_name=$(wn_b --json groups show "$gid" 2>/dev/null | jq -r '.name // empty') + b_name=$(wn_b --json groups show "$mls_gid" 2>/dev/null | jq -r '(.result // .) | .name // empty') local a_name a_name=$(amy_field '.name' marmot group show "$gid" 2>/dev/null || echo "") if [[ -n "$a_name" && "$a_name" == "$b_name" ]]; then info "race converged: both sides see \"$a_name\"" # Verify encryption still works. - wn_b messages send "$gid" "post-race ping" >/dev/null 2>&1 || true - if amy_json marmot await message "$gid" --match "post-race ping" --timeout 15 >/dev/null; then + wn_b messages send "$mls_gid" "post-race ping" >/dev/null 2>&1 || true + if amy_json marmot await message "$gid" --match "post-race ping" --timeout 90 >/dev/null; then record_result "$id" pass else record_result "$id" fail "encryption broken after race" @@ -90,35 +94,39 @@ test_12_offline_catchup() { banner "Test 12 — Offline catch-up" local id="12 offline catchup" - # Fresh group so we don't collide with other tests. - local out gid + # wn-side keys groups by mls_group_id; amy-side by nostr_group_id. Learn + # the amy side from `amy await group` so later amy calls target the right + # group. + local out mls_gid a_out a_gid out=$(wn_b --json groups create "Interop-12" "$A_NPUB" 2>>"$LOG_FILE") - gid=$(printf '%s' "$out" | jq_group_id) - [[ -n "$gid" ]] || { record_result "$id" fail "couldn't create Interop-12"; return; } - save_state GROUP_12 "$gid" + mls_gid=$(printf '%s' "$out" | jq_group_id) + [[ -n "$mls_gid" ]] || { record_result "$id" fail "couldn't create Interop-12"; return; } + save_state GROUP_12_MLS "$mls_gid" # A joins. - amy_json marmot await group --name "Interop-12" --timeout 30 >/dev/null || { + a_out=$(amy_json marmot await group --name "Interop-12" --timeout 30) || { record_result "$id" fail "A never received Interop-12 invite"; return } + a_gid=$(printf '%s' "$a_out" | jq -r '.group_id') + save_state GROUP_12 "$a_gid" # "Go offline" == don't invoke amy. Meanwhile B sends 5 messages + adds C + sends 3 more + rename. for i in 1 2 3 4 5; do - wn_b messages send "$gid" "offline-msg-$i" >/dev/null 2>&1 || true + wn_b messages send "$mls_gid" "offline-msg-$i" >/dev/null 2>&1 || true sleep 1 done - wn_b groups add-members "$gid" "$C_NPUB" >/dev/null 2>&1 || true - wait_for_invite C 30 >/dev/null && wn_c groups accept "$gid" >/dev/null 2>&1 || true + wn_b groups add-members "$mls_gid" "$C_NPUB" >/dev/null 2>&1 || true + wait_for_invite C 30 >/dev/null && wn_c groups accept "$mls_gid" >/dev/null 2>&1 || true for i in 6 7 8; do - wn_b messages send "$gid" "offline-msg-$i" >/dev/null 2>&1 || true + wn_b messages send "$mls_gid" "offline-msg-$i" >/dev/null 2>&1 || true sleep 1 done - wn_b groups rename "$gid" "Interop-12-renamed" >/dev/null 2>&1 || true + wn_b groups rename "$mls_gid" "Interop-12-renamed" >/dev/null 2>&1 || true sleep 3 # A comes back online — single sync pulls everything. local show - show=$(amy_json marmot group show "$gid") || { + show=$(amy_json marmot group show "$a_gid") || { record_result "$id" fail "amy group show failed"; return } local name; name=$(printf '%s' "$show" | jq -r '.name') @@ -127,7 +135,7 @@ test_12_offline_catchup() { fi # All 8 messages should be locally stored. - local msgs; msgs=$(amy_json marmot message list "$gid" --limit 100) + local msgs; msgs=$(amy_json marmot message list "$a_gid" --limit 100) local missing=0 for i in 1 2 3 4 5 6 7 8; do if ! printf '%s' "$msgs" | jq -e --arg t "offline-msg-$i" \ @@ -170,3 +178,57 @@ test_13_keypackage_rotation() { record_result "$id" fail "no new KP event_id observed" fi } + +# -- Inverted-role tests ---------------------------------------------------- +# Tests 01–13 mostly exercise amethyst as the committer and wn as the +# receiver. The scenarios below are complementary: wn drives the state +# change and amy is the receiver that must accept, verify and apply it. +# This widens coverage for the post-fix inbound authenticity checks +# (membership_tag, FramedContentTBS signature, LeafNode lifetime, +# confirmation_tag) that now run on every commit amy processes. + +test_14_wn_removes_a() { + banner "Test 14 — wn (admin) removes A; amy processes Remove" + local id="14 wn removes amy" + + # Known gap: wn (mdk-core/openmls) emits the filtered direct-path + # form of UpdatePath on Remove commits per RFC 9420 §7.7 — when the + # copath of a direct-path node has an empty resolution (every leaf + # under it is blank) the corresponding UpdatePathNode is omitted. + # Quartz's RatchetTree.applyUpdatePath currently requires the + # unfiltered node count (`pathNodes.size == directPath.size`) so + # every wn->amy Remove triggers + # "UpdatePath node count (N) doesn't match direct path length (N+k)". + # That's a pre-existing quartz conformance bug, out of scope for + # this branch; the harness carries the test so it starts passing + # the moment the filtered-path path is wired up. + record_result "$id" skip "pending filtered_direct_path support in applyUpdatePath" +} + +test_15_wn_member_leaves() { + banner "Test 15 — wn_c leaves; amy + wn_b process SelfRemove" + local id="15 wn_c leaves" + + # Same filtered_direct_path gap as test 14: when wn_c leaves a + # 3-member group, wn_b folds the SelfRemove into a commit whose + # UpdatePath uses RFC 9420 §7.7 filtering, and amy's strict + # applyUpdatePath rejects it. Skip until quartz handles the + # filtered form on inbound. + record_result "$id" skip "pending filtered_direct_path support in applyUpdatePath" +} + +test_16_wn_keypackage_rotation() { + banner "Test 16 — wn rotates KeyPackage; amy discovers new KP" + local id="16 wn keypackage rotation" + + # amy's KeyPackageFetcher.fetchKeyPackage calls client.fetchFirst, + # which returns the first matching event a relay sends — nostr-rs-relay + # typically serves kind:443 events in storage order, not created_at + # order, so after a rotation amy may keep seeing the older event_id + # depending on which arrives first. Making this test deterministic + # requires a "fetch latest by created_at" KeyPackage fetcher; until + # then the check flaps. The inverse direction (test 13, amy rotates + # and wn sees via `wn keys check` which is an addressable index) is + # the reliable one. + record_result "$id" skip "pending createdAt-sorted KeyPackage fetch path" +} diff --git a/tools/marmot-interop/headless/tests-manage.sh b/tools/marmot-interop/headless/tests-manage.sh index 509313d58..fd14c49f9 100644 --- a/tools/marmot-interop/headless/tests-manage.sh +++ b/tools/marmot-interop/headless/tests-manage.sh @@ -7,9 +7,17 @@ test_06_member_removal() { banner "Test 06 — Member removal + forward secrecy" local id="06 member removal" - local gid; gid=$(load_state GROUP_05 || true) - if [[ -z "${gid:-}" ]]; then - record_result "$id" skip "no GROUP_05"; return + # MIP-03 only admins may commit Remove proposals. In GROUP_05 (wn-created + # by B, A joined later) A is not an admin, so the test used to fail with + # `IllegalStateException: non-admin members may only commit...`. Test on + # GROUP_02 instead, where amy is the creator and therefore sole admin, + # and where test 04 has already added C. amy calls use the nostr id, + # wn calls use the MLS id. + local gid mls_gid + gid=$(load_state GROUP_02 || true) + mls_gid=$(load_state GROUP_02_MLS || true) + if [[ -z "${gid:-}" || -z "${mls_gid:-}" ]]; then + record_result "$id" skip "no GROUP_02"; return fi amy_json marmot group remove "$gid" "$C_NPUB" >/dev/null || { @@ -17,10 +25,10 @@ test_06_member_removal() { } # C should no longer see the group on its own member view. - local deadline=$(( $(date +%s) + 60 )) removed=0 + local deadline=$(( $(date +%s) + 120 )) removed=0 while [[ $(date +%s) -lt $deadline ]]; do - if ! wn_c --json groups members "$gid" 2>/dev/null \ - | jq -e --arg p "$C_HEX" '.[]? | select((.pubkey // .public_key) == $p)' \ + if ! wn_c --json groups members "$mls_gid" 2>/dev/null \ + | jq -e --arg p "$C_HEX" '(.result // .) | .[]? | select((.pubkey // .public_key) == $p)' \ >/dev/null 2>&1; then removed=1; break fi @@ -33,13 +41,13 @@ test_06_member_removal() { amy_json marmot message send "$gid" "after removing C" >/dev/null || { record_result "$id" fail "amy send failed"; return } - wait_for_message B "$gid" "after removing C" 30 || { + wait_for_message B "$mls_gid" "after removing C" 90 || { record_result "$id" fail "B lost access after C's removal"; return } # Forward secrecy: C must NOT see the post-removal message. sleep 5 - if wait_for_message C "$gid" "after removing C" 10; then + if wait_for_message C "$mls_gid" "after removing C" 10; then record_result "$id" fail "C still decrypted a post-removal message" else record_result "$id" pass @@ -50,8 +58,13 @@ test_07_metadata_rename() { banner "Test 07 — Metadata rename round-trip (MIP-01)" local id="07 metadata rename" - local gid; gid=$(load_state GROUP_02 || true) - if [[ -z "${gid:-}" ]]; then + # amy was the creator of GROUP_02 so its own nostr_group_id is saved as + # GROUP_02; wn keys its copy by the MLS group id saved as GROUP_02_MLS. + # Pass each CLI the id it understands. + local gid mls_gid + gid=$(load_state GROUP_02 || true) + mls_gid=$(load_state GROUP_02_MLS || true) + if [[ -z "${gid:-}" || -z "${mls_gid:-}" ]]; then record_result "$id" skip "no GROUP_02"; return fi @@ -59,9 +72,9 @@ test_07_metadata_rename() { record_result "$id" fail "amy rename failed"; return } - local deadline=$(( $(date +%s) + 60 )) seen="" + local deadline=$(( $(date +%s) + 120 )) seen="" while [[ $(date +%s) -lt $deadline ]]; do - seen=$(wn_b --json groups show "$gid" 2>/dev/null | jq -r '.name // empty') + seen=$(wn_b --json groups show "$mls_gid" 2>/dev/null | jq -r '(.result // .) | .name // empty') [[ "$seen" == "Interop-02-renamed" ]] && break sleep 3 done @@ -69,9 +82,23 @@ test_07_metadata_rename() { record_result "$id" fail "B saw name=\"$seen\" not \"Interop-02-renamed\""; return } + # MIP-01: only admins may rename. GROUP_02 was created by amy (sole + # admin), so for B's rename to be accepted by wn's MIP-01 check amy + # must first promote B. amy adds B to admin_pubkeys via GCE, which + # the harness consumes via `marmot group promote` — equivalent to + # `wn groups promote` but issued by the quartz side. Without this + # step B's own wn silently refuses the rename on its MIP-01 + # `ensure_account_is_group_admin` check, and the kind:445 is never + # published. + amy_json marmot group promote "$gid" "$B_NPUB" >/dev/null 2>&1 || { + record_result "$id" fail "amy promote-B failed"; return + } + # Let wn apply the promote commit before issuing the rename. + sleep 3 + # Now B renames back and A should pick it up. - wn_b groups rename "$gid" "Interop-02-reverse" >/dev/null 2>&1 || true - if amy_json marmot await rename "$gid" --name "Interop-02-reverse" --timeout 60 >/dev/null; then + wn_b groups rename "$mls_gid" "Interop-02-reverse" >/dev/null 2>&1 || true + if amy_json marmot await rename "$gid" --name "Interop-02-reverse" --timeout 120 >/dev/null; then record_result "$id" pass else record_result "$id" fail "A did not pick up B's rename" @@ -82,39 +109,45 @@ test_08_admin_promote_demote() { banner "Test 08 — Admin promote / demote" local id="08 admin promote/demote" - local gid; gid=$(load_state GROUP_03 || true) - if [[ -z "${gid:-}" ]]; then + # GROUP_03 was created by wn so both sides need different ids: + # GROUP_03 → amy's nostr_group_id (captured in test 03 after + # `amy await group` returned `.group_id`) + # GROUP_03_MLS → wn's mls_group_id (wn's `groups create` output) + local a_gid mls_gid + a_gid=$(load_state GROUP_03 || true) + mls_gid=$(load_state GROUP_03_MLS || true) + if [[ -z "${mls_gid:-}" ]]; then record_result "$id" skip "no GROUP_03"; return fi # Ensure 3 members (add C if missing). wn_c keys publish >/dev/null 2>&1 || true sleep 2 - wn_b groups add-members "$gid" "$C_NPUB" >/dev/null 2>&1 || true - wait_for_invite C 30 >/dev/null && wn_c groups accept "$gid" >/dev/null 2>&1 || true + wn_b groups add-members "$mls_gid" "$C_NPUB" >/dev/null 2>&1 || true + wait_for_invite C 30 >/dev/null && wn_c groups accept "$mls_gid" >/dev/null 2>&1 || true # B promotes A. - wn_b groups promote "$gid" "$A_NPUB" >/dev/null 2>&1 || { + wn_b groups promote "$mls_gid" "$A_NPUB" >/dev/null 2>&1 || { record_result "$id" fail "wn promote failed"; return } # A should reflect the new admin set — poll via amy. - if ! amy_json marmot await admin "$gid" "$A_NPUB" --timeout 30 >/dev/null; then + if ! amy_json marmot await admin "$a_gid" "$A_NPUB" --timeout 90 >/dev/null; then record_result "$id" fail "A never saw itself promoted"; return fi # A now commits a rename — only possible if we're admin. - amy_json marmot group rename "$gid" "Interop-03-by-A" >/dev/null || { + amy_json marmot group rename "$a_gid" "Interop-03-by-A" >/dev/null || { record_result "$id" fail "A (now admin) could not rename"; return } # B demotes A. - wn_b groups demote "$gid" "$A_NPUB" >/dev/null 2>&1 || warn "demote returned nonzero" + wn_b groups demote "$mls_gid" "$A_NPUB" >/dev/null 2>&1 || warn "demote returned nonzero" sleep 5 local admins - admins=$(wn_b --json groups admins "$gid" 2>/dev/null \ - | jq -r '.[].pubkey // .[].public_key // .[]' | tr '\n' ' ') + admins=$(wn_b --json groups admins "$mls_gid" 2>/dev/null \ + | jq -r '(.result // .) | .[]?.pubkey // .[]?.public_key // .[]?' | tr '\n' ' ') if [[ "$admins" == *"$A_HEX"* ]]; then record_result "$id" fail "A still admin after demote" else @@ -126,7 +159,9 @@ test_11_leave_group() { banner "Test 11 — Leave group" local id="11 leave group" - local gid; gid=$(load_state GROUP_02 || true) + local gid mls_gid + gid=$(load_state GROUP_02 || true) + mls_gid=$(load_state GROUP_02_MLS || true) if [[ -z "${gid:-}" ]]; then record_result "$id" skip "no GROUP_02"; return fi @@ -135,10 +170,10 @@ test_11_leave_group() { record_result "$id" fail "amy leave failed"; return } - local deadline=$(( $(date +%s) + 60 )) gone=0 + local deadline=$(( $(date +%s) + 120 )) gone=0 while [[ $(date +%s) -lt $deadline ]]; do - if ! wn_b --json groups members "$gid" 2>/dev/null \ - | jq -e --arg p "$A_HEX" '.[]? | select((.pubkey // .public_key) == $p)' \ + if ! wn_b --json groups members "$mls_gid" 2>/dev/null \ + | jq -e --arg p "$A_HEX" '(.result // .) | .[]? | select((.pubkey // .public_key) == $p)' \ >/dev/null 2>&1; then gone=1; break fi diff --git a/tools/marmot-interop/lib.sh b/tools/marmot-interop/lib.sh index 744e9ff95..0196117b1 100644 --- a/tools/marmot-interop/lib.sh +++ b/tools/marmot-interop/lib.sh @@ -150,6 +150,10 @@ expect_contains() { # - plain hex string (from `groups list`) # - {"value":{"vec":[...]}} serde struct (from `groups create`) # - flat byte array [n, ...] (from some responses) +# Plus the three wrapper shapes wn actually uses: +# - {"result": {"mls_group_id": ...}} (groups create) +# - {"group": {"mls_group_id": ...}, "membership": ...} (groups invites[0]) +# - {"mls_group_id": ...} (bare) # Input: JSON string via stdin; optional 2nd arg = field name (default: mls_group_id) jq_group_id() { local field="${1:-mls_group_id}" @@ -159,7 +163,8 @@ jq_group_id() { [($n / 16 | floor), ($n % 16)] | map(if . < 10 then (48 + .) else (87 + .) end) | implode; - (.result // .) | + (.group // .result // .) | + (.group // .) | .[$f] | if type == "string" then . elif (type == "object" and (.value.vec != null)) then @@ -190,8 +195,11 @@ wait_for_invite() { deadline=$(( start + timeout )) last_hb=$start while [[ $(date +%s) -lt $deadline ]]; do + # Post-v0.2 `wn --json groups invites` returns `{"result": [...]}` + # (older builds returned the bare array). Peel the wrapper when + # present so a pending invite is actually detected. gid=$("$wnfn" --json groups invites 2>/dev/null \ - | jq -c '.[0] // empty' 2>/dev/null | jq_group_id || true) + | jq -c '(.result // .) | .[0] // empty' 2>/dev/null | jq_group_id || true) if [[ -n "${gid:-}" ]]; then printf '%s\n' "$gid" return 0 @@ -203,7 +211,7 @@ wait_for_invite() { local elapsed=$(( now - start )) remaining=$(( deadline - now )) local pending pending=$("$wnfn" --json groups invites 2>/dev/null \ - | jq 'length' 2>/dev/null || echo "?") + | jq '(.result // .) | length' 2>/dev/null || echo "?") local recent="" if [[ -f "$data_dir/logs/stderr.log" ]]; then recent=$(tail -n 200 "$data_dir/logs/stderr.log" 2>/dev/null \ @@ -233,7 +241,7 @@ wait_for_message() { fi if [[ -n "${payload:-}" ]] && \ printf '%s' "$payload" | jq -e --arg n "$needle" \ - '.[]? | select((.content // .text // "") | contains($n))' \ + '(.result // .) | .[]? | select((.content // .text // "") | contains($n))' \ >/dev/null 2>&1; then return 0 fi @@ -254,7 +262,7 @@ wait_for_member() { payload=$(wn_c_json groups members "$gid" 2>/dev/null || true) fi if printf '%s' "${payload:-}" | jq -e --arg p "$pubkey" \ - '.[]? | select((.pubkey // .public_key // "") == $p)' \ + '(.result // .) | .[]? | select((.pubkey // .public_key // "") == $p)' \ >/dev/null 2>&1; then return 0 fi diff --git a/tools/marmot-interop/marmot-interop-headless.sh b/tools/marmot-interop/marmot-interop-headless.sh index 0cff73d83..bd6ac542d 100755 --- a/tools/marmot-interop/marmot-interop-headless.sh +++ b/tools/marmot-interop/marmot-interop-headless.sh @@ -105,3 +105,6 @@ test_10_concurrent_commits test_11_leave_group test_12_offline_catchup test_13_keypackage_rotation +test_14_wn_removes_a +test_15_wn_member_leaves +test_16_wn_keypackage_rotation