From a89f1d198f817aa372694ca0081edb009d7013e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 01:12:59 +0000 Subject: [PATCH] fix(cordn): enforce admin_pubkeys on add, remove and metadata commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reference client gates exactly three operations behind the group's admin list — addMember, removeMember, updateGroupMetadata — and because MLS has no server that can police membership, it does so on both sides: it refuses to build one of those commits unless the local member is an admin, and it rejects an inbound commit carrying add, remove or group_context_extensions from a member who is not. We treated admin_pubkeys as presentation metadata and left authorizeCommit at the default. The class KDoc said "neither the spec nor the reference coordinator restricts who may commit", which was the wrong component to look at: the coordinator cannot restrict it, the client is where it happens, and the reference client does. Accepting a commit the rest of the group rejects forks the epoch, and MLS does not recover from a fork. authorizeCommit now implements that rule. The engine calls it in both directions, so one check covers both. The gate is the reference's three proposal types and nothing else — an Update, a SelfRemove or a PSK from any member stays allowed, so nobody is trapped in a group they may not leave. Being stricter would fork the epoch in the other direction. An empty admin list stays egalitarian: the gate opens rather than closes. One trap worth recording, because it would have inverted the check rather than disabling it. Marmot's credential holds the account key as raw bytes, so GroupView.memberIdentityHex is the pubkey. A cordn credential holds it as 64 ASCII hex characters, so the same accessor returns 128 characters that are never a pubkey — comparing an admin pubkey against it matches nothing and rejects every commit in a group that names admins, which would have read as "admins can never commit" rather than as a decoding bug. The comparison therefore happens in credential-bytes space, mapping the admin list forwards rather than decoding a leaf backwards, which also keeps untrusted bytes out of the decoder. The test for it fails if that mapping is removed. Groups Amethyst creates are all egalitarian today, so this changes nothing for them; it matters for a group created elsewhere that names admins. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012BfD4txdnsaPRXmNXbup9n --- .../quartz/cordn/groups/CordnGroupPolicy.kt | 115 ++++++++++++--- .../cordn/groups/CordnAdminPolicyTest.kt | 135 ++++++++++++++++++ 2 files changed, 233 insertions(+), 17 deletions(-) create mode 100644 quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt index 36fc86045d..a83066cf05 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt @@ -30,9 +30,12 @@ import com.vitorpamplona.quartz.mls.group.MlsExporterLabel import com.vitorpamplona.quartz.mls.group.MlsGroup import com.vitorpamplona.quartz.mls.group.MlsGroupPolicy import com.vitorpamplona.quartz.mls.group.PendingProposal +import com.vitorpamplona.quartz.mls.messages.Proposal import com.vitorpamplona.quartz.mls.tree.Capabilities import com.vitorpamplona.quartz.mls.tree.Credential import com.vitorpamplona.quartz.mls.tree.Extension +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.core.toHexKey /** * cordn's profile, as an [MlsGroupPolicy]. @@ -49,20 +52,25 @@ import com.vitorpamplona.quartz.mls.tree.Extension * ) * ``` * - * ## Why there is no authorization hook + * ## Admin authorization * - * cordn has no equivalent of Marmot's MIP-03. `spec/01.md` §5.3 defines - * admins as *presentation* metadata — who the application shows a settings - * button to — and neither the spec nor the reference coordinator restricts who - * may commit. Empty `admin_pubkeys` is egalitarian mode, a deliberate and - * permanent choice, not a bootstrap window. + * `spec/01.md` §5.3 leaves the meaning of `admin_pubkeys` to the application + * and says only that an EMPTY list is egalitarian mode. The reference client + * fills that in, and because MLS has no server that can police membership, + * it does so on both sides of every commit: it refuses to build an add, a + * remove or a metadata change unless the local member is an admin, and it + * rejects an inbound commit carrying one of those from a member who is not. * - * So [authorizeCommit] is left at the default. That is a real statement about - * cordn, not an omission: **any member of a cordn group can commit anything - * MLS itself permits, including removing other members.** A UI that presents a - * cordn group's admin list as an access-control boundary would be lying. If - * `spec/01.md` later gives admins enforcement teeth, that rule belongs here, - * where an admin change can be checked against the post-commit extensions. + * [authorizeCommit] implements exactly that rule, and the engine calls it in + * both directions, so one check covers both. Matching it is not cosmetic — + * accepting a commit the rest of the group rejects forks the epoch, and MLS + * does not recover from a fork. Being *stricter* than the reference would fork + * it the other way, which is why the gate is the reference's three proposal + * types and nothing else: an Update, a SelfRemove or a PSK from any member + * stays allowed, so nobody can be trapped in a group they may not leave. + * + * Egalitarian mode is a permanent choice, not a bootstrap window: an empty + * list means every member administers, so the gate opens rather than closes. */ object CordnGroupPolicy : MlsGroupPolicy { /** @@ -169,15 +177,88 @@ object CordnGroupPolicy : MlsGroupPolicy { ) /** - * Unused today; kept so the shape of a future rule is obvious. + * Refuses a commit that adds, removes or rewrites metadata on behalf of a + * member `admin_pubkeys` does not name. See the class KDoc for why this + * matches the reference client exactly rather than approximately. * - * If cordn ever gives `admin_pubkeys` enforcement teeth, the check belongs - * here — [group] carries the pre-commit extensions and [proposals] the - * replacement, which is exactly what deciding an admin change needs. + * [committerLeafIndex] is the committer, which is who the reference checks + * for a commit. A proposal one member sent by reference and an admin then + * committed is therefore allowed — on both implementations, the admin who + * committed it is the one answering for it. */ override fun authorizeCommit( group: GroupView, proposals: List, committerLeafIndex: Int, - ) = Unit + ) { + if (proposals.isEmpty()) return + + if (adminIdentitiesIn(group.extensions).isEmpty()) return + if (proposals.none { it.proposal.needsAdmin() }) return + + check(isAdminLeaf(group, committerLeafIndex)) { + "cordn: only admin_pubkeys may add, remove or rewrite group metadata; leaf " + + "$committerLeafIndex is not an admin" + } + } + + /** + * The admin set [extensions] names, or empty for egalitarian. + * + * A metadata extension too malformed to decode reads as egalitarian rather + * than taking the group down with it — the same call Marmot's policy makes + * about its own components. It is not a way in: installing metadata takes a + * GroupContextExtensions commit, which this gate already covers, so nobody + * outside the admin set can put a broken extension there in the first + * place. A group whose metadata was malformed from creation was never + * administrable by anyone. + */ + fun adminIdentitiesIn(extensions: List): Set = + runCatching { CordnGroupMetadata.fromExtensions(extensions)?.adminPubkeys } + .getOrNull() + .orEmpty() + .toSet() + + /** True if the account [pubKey] may add, remove or rewrite metadata here. */ + fun isAdmin( + group: GroupView, + pubKey: HexKey, + ): Boolean { + val admins = adminIdentitiesIn(group.extensions) + return admins.isEmpty() || pubKey in admins + } + + /** + * True if the member at [leafIndex] may. + * + * The comparison happens in credential-bytes space, not account space, and + * deliberately: [GroupView.memberIdentityHex] hexes whatever the credential + * holds, and a cordn credential holds the account key as its 64 ASCII hex + * characters (see [CordnCredential]), so that accessor returns 128 + * characters which are never a pubkey. Marmot stores raw bytes and can + * compare directly; comparing an account pubkey against this without + * converting would match nothing and reject every commit in a group that + * names admins. Mapping the admin list forwards rather than decoding the + * leaf backwards keeps untrusted bytes out of the decoder entirely. + */ + fun isAdminLeaf( + group: GroupView, + leafIndex: Int, + ): Boolean { + val admins = adminIdentitiesIn(group.extensions) + if (admins.isEmpty()) return true + + val credentialHex = group.memberIdentityHex(leafIndex) ?: return false + return credentialHex in admins.mapTo(mutableSetOf()) { it.encodeToByteArray().toHexKey() } + } + + /** True if the local member may. */ + fun isLocalAdmin(group: GroupView): Boolean = isAdminLeaf(group, group.myLeafIndex) + + /** + * The three proposal types the reference client gates, and only those: + * `add`, `remove` and `group_context_extensions`, matching its + * `addMember` / `removeMember` / `updateGroupMetadata`. + */ + private fun Proposal.needsAdmin(): Boolean = this is Proposal.Add || this is Proposal.Remove || this is Proposal.GroupContextExtensions } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt new file mode 100644 index 0000000000..5d830daf33 --- /dev/null +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt @@ -0,0 +1,135 @@ +/* + * 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.cordn.groups + +import com.vitorpamplona.quartz.cordn.spec01GroupMetadata.CordnGroupMetadata +import com.vitorpamplona.quartz.mls.group.MlsGroup +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * That `admin_pubkeys` actually gates commits, the way the reference client + * gates its `addMember` / `removeMember` / `updateGroupMetadata`. + * + * The engine calls [CordnGroupPolicy.authorizeCommit] before building a local + * commit AND before applying an inbound one, so these tests cover both + * directions at once: a commit this policy refuses to build is the same commit + * it refuses to apply from someone else. That matters more than the local error + * — applying a commit the rest of the group rejects forks the epoch. + */ +class CordnAdminPolicyTest { + private val alice = "a1".repeat(32) + private val bob = "b2".repeat(32) + + private fun identity(pubKeyHex: String) = CordnCredential.of(pubKeyHex).identity + + /** A group created by [creator] whose metadata names [admins]. */ + private fun group( + creator: String, + admins: List, + ): MlsGroup = + MlsGroup.create( + identity = identity(creator), + policy = CordnGroupPolicy, + initialExtensions = listOf(CordnGroupMetadata(name = "Admin test", adminPubkeys = admins).toExtension()), + ) + + @Test + fun anEmptyAdminListLetsAnyMemberRewriteMetadata() { + val egalitarian = group(creator = alice, admins = emptyList()) + val before = egalitarian.epoch + + // spec/01.md §5.3: empty means egalitarian, permanently — not a + // bootstrap window that later closes. + egalitarian.proposeGroupContextExtensions(egalitarian.extensions) + egalitarian.commit() + + assertEquals(before + 1, egalitarian.epoch) + assertTrue(CordnGroupPolicy.isLocalAdmin(egalitarian.view())) + } + + @Test + fun aNonAdminCannotRewriteMetadata() { + val group = group(creator = alice, admins = listOf(bob)) + assertFalse(CordnGroupPolicy.isLocalAdmin(group.view()), "alice must not be an admin or this proves nothing") + + group.proposeGroupContextExtensions(group.extensions) + assertFailsWith { group.commit() } + } + + @Test + fun anAdminCan() { + val group = group(creator = alice, admins = listOf(alice)) + assertTrue(CordnGroupPolicy.isLocalAdmin(group.view())) + val before = group.epoch + + group.proposeGroupContextExtensions(group.extensions) + group.commit() + + assertEquals(before + 1, group.epoch) + } + + /** + * The regression that matters most: a cordn credential stores the account + * key as 64 ASCII hex characters, so hexing the credential bytes yields 128 + * characters. An admin check that compared an account pubkey against that + * would match nothing — and since the gate only fires when a commit needs + * an admin, the failure would look like "admins can never commit" rather + * than like a decoding bug. + */ + @Test + fun theAdminListIsComparedInTheCredentialsOwnEncoding() { + val group = group(creator = alice, admins = listOf(alice)) + + val credentialHex = group.view().let { it.memberIdentityHex(it.myLeafIndex) } + assertEquals(128, credentialHex?.length, "a cordn credential hexes to twice the pubkey length") + assertTrue(credentialHex != alice, "if these were equal this test would be vacuous") + + assertTrue(group.view().let { CordnGroupPolicy.isAdminLeaf(it, it.myLeafIndex) }) + } + + @Test + fun anUpdateIsNotAnAdminAction() { + val group = group(creator = alice, admins = listOf(bob)) + val before = group.epoch + + // Only add, remove and group_context_extensions are gated. Keeping the + // rest open is what stops a non-admin being trapped in a group whose + // keys they may never rotate. + group.proposeSigningKeyRotation() + group.commit() + + assertEquals(before + 1, group.epoch) + } + + @Test + fun anEmptyCommitIsNotAnAdminAction() { + val group = group(creator = alice, admins = listOf(bob)) + val before = group.epoch + + group.commit() + + assertEquals(before + 1, group.epoch) + } +}