From 46845d9058aacdbfa872c23cb7fb7c8e7dececaa Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 23:11:02 +0000 Subject: [PATCH] =?UTF-8?q?fix(marmot):=20frame=20published=20KeyPackages?= =?UTF-8?q?=20as=20MLSMessage=20=E2=80=94=20interop=20test=2001=20passes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test 01 (bidirectional KeyPackage discovery with MDK) now passes. It was failing on a four-byte omission. `foundation/key-packages.md`: "a transport publication is unambiguously the framed MLSMessage, not a bare KeyPackage struct." We published bare bytes. That is not a cosmetic difference — a reader expecting the envelope reads a bare KeyPackage's leading 0x0001 0x0001 as version 1, wire format 1 (mls_public_message), parses on as a PublicMessage, and dies several fields later on a byte that means nothing. MDK reported `UnknownValue(112)`, a number that appears nowhere in a KeyPackage, which is why this was invisible from our side: our own decoder round-tripped our own bytes perfectly. Only a second implementation could find it. The KeyPackageRef stays over the INNER KeyPackage, as RFC 9420 MakeKeyPackageRef defines — framing the ref too would make our `i` tag disagree with everyone else's. Bare bytes are still accepted on read: every KeyPackage we published before this is bare and still inside its lifetime, and refusing them would leave our own users unable to invite each other until all of them rotated. With framing fixed MDK got one field further and rejected the next thing: "mls_extensions tag does not exactly match decoded KeyPackage metadata". Those id-list tags duplicate metadata already inside the KeyPackage, and we were writing them by hand — so adding one leaf capability (the legacy 0xF2EE, added so our KeyPackages stay addable to existing groups) silently invalidated every KeyPackage we published. They are now derived from the KeyPackage itself and cannot drift. `app_components` lists the Marmot registry ids only; the upstream MLS-extensions component ids below 0x8000 are not app components being advertised. The harness needed one more fix: MDK 0.9.x reports a found KeyPackage under `result.key_package`, and the harness probed two older shapes, so a successful check read as a failure. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016kCuA6tc4JQzHPCDd39GHq --- cli/tests/marmot/tests-create.sh | 4 +- .../amethyst/commons/marmot/MarmotManager.kt | 48 ++++---- .../mip00KeyPackages/KeyPackageEvent.kt | 71 +++++++++++- .../mip00KeyPackages/KeyPackageUtils.kt | 51 ++++++++- .../mip00KeyPackages/KeyPackageFramingTest.kt | 104 ++++++++++++++++++ 5 files changed, 253 insertions(+), 25 deletions(-) create mode 100644 quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageFramingTest.kt diff --git a/cli/tests/marmot/tests-create.sh b/cli/tests/marmot/tests-create.sh index 218851be04..b868380ef9 100644 --- a/cli/tests/marmot/tests-create.sh +++ b/cli/tests/marmot/tests-create.sh @@ -10,7 +10,9 @@ test_01_keypackage_discovery() { # B finds A's KP local raw ev raw=$(wn_b --json keys check "$A_NPUB" 2>>"$LOG_FILE" || true) - ev=$(printf '%s' "$raw" | jq -r '.result.event_id // .event_id // empty') + # MDK 0.9.x reports the found KeyPackage under result.key_package; the two + # older shapes are kept so this still reads a pre-0.9 daemon. + ev=$(printf '%s' "$raw" | jq -r '.result.key_package.key_package_event_id // .result.event_id // .event_id // empty') if [[ -z "$ev" || "$ev" == "null" ]]; then record_result "$id (B->A)" fail "wn couldn't find A's KP"; return fi 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 f61b8db7d5..9b16da3617 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 @@ -418,12 +418,11 @@ class MarmotManager( keyPackageEventId: HexKey, relays: List, ): Pair { - // Verify that the KeyPackage credential matches the expected member pubkey - val kp = - com.vitorpamplona.quartz.marmot.mls.messages.MlsKeyPackage.decodeTls( - com.vitorpamplona.quartz.marmot.mls.codec - .TlsReader(keyPackageBytes), - ) + // Verify that the KeyPackage credential matches the expected member + // pubkey. Accepts either framing — a peer's published KeyPackage is an + // MLSMessage, and bare bytes still arrive from our own pre-fix + // publications sitting on relays. + val kp = KeyPackageUtils.decodeKeyPackage(keyPackageBytes) val credential = kp.leafNode.credential require(credential is Credential.Basic) { "KeyPackage must use BasicCredential" @@ -439,7 +438,9 @@ class MarmotManager( // that key. val publication = commitAndPublish(nostrGroupId, relays) { - groupManager.stageAddMember(nostrGroupId, keyPackageBytes) + // The BARE KeyPackage, not the bytes as published. Transport + // framing is the Marmot layer's business; MLS takes the struct. + groupManager.stageAddMember(nostrGroupId, kp.toTlsBytes()) } // The Welcome is a SEPARATE, retryable per-invitee delivery obligation @@ -836,24 +837,32 @@ class MarmotManager( keyPackageRotationManager.generateKeyPackage(identity, dTag) } - val keyPackageBytes = bundle.keyPackage.toTlsBytes() - val keyPackageBase64 = Base64.encode(keyPackageBytes) + // Framed as an MLSMessage, never bare: `foundation/key-packages.md` + // says a transport publication IS the framed message. The ref stays + // over the INNER KeyPackage, which is what RFC 9420 MakeKeyPackageRef + // hashes — framing the ref too would make our `i` tag disagree with + // every other implementation's. + val keyPackageBase64 = Base64.encode(KeyPackageUtils.frameKeyPackage(bundle.keyPackage)) val keyPackageRef = bundle.keyPackage.reference().toHexKey() val template = if (currentProfile) { + // Every id-list tag is derived from the KeyPackage itself. + // They duplicate metadata already inside it, so writing them by + // hand is a second source of truth — and a receiver that + // compares them (MDK does, exactly) rejects the KeyPackage the + // moment the two disagree. + // // Deliberately no `relays` tag and no `encoding` tag. // `transports/nostr.md`: a KeyPackage is fetched from the - // account's own inbox relay set, so repeating them here would - // be a second, drifting source of truth; and the binding - // forbids an `encoding` tag outright, because a receiver that - // switched decoders on one could be steered into a different - // parse of the same bytes. - KeyPackageEvent.buildCurrentProfile( - keyPackageBase64 = keyPackageBase64, + // account's own NIP-65 write set, so repeating relays here + // would be another drifting duplicate; and the binding forbids + // an `encoding` tag outright, because a receiver that switched + // decoders on one could be steered into a different parse of + // the same bytes. + KeyPackageEvent.buildCurrentProfileFrom( + keyPackage = bundle.keyPackage, dTagSlot = dTag, - keyPackageRef = keyPackageRef, - appComponentIds = emptyList(), ) } else { KeyPackageEvent.build( @@ -884,8 +893,7 @@ class MarmotManager( val identity = signer.pubKey.hexToByteArray() return pendingSlots.map { slot -> val bundle = keyPackageRotationManager.rotateSlot(identity, slot) - val keyPackageBytes = bundle.keyPackage.toTlsBytes() - val keyPackageBase64 = Base64.encode(keyPackageBytes) + val keyPackageBase64 = Base64.encode(KeyPackageUtils.frameKeyPackage(bundle.keyPackage)) val keyPackageRef = bundle.keyPackage.reference().toHexKey() val template = diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageEvent.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageEvent.kt index 76b4238187..5686cd69d1 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageEvent.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageEvent.kt @@ -24,13 +24,19 @@ import androidx.compose.runtime.Immutable import com.vitorpamplona.quartz.marmot.mip00KeyPackages.tags.AppComponentsTag import com.vitorpamplona.quartz.marmot.mip00KeyPackages.tags.EncodingTag import com.vitorpamplona.quartz.marmot.mip00KeyPackages.tags.MlsProposalsTag +import com.vitorpamplona.quartz.marmot.mls.components.AppDataDictionary +import com.vitorpamplona.quartz.marmot.mls.components.ComponentsList +import com.vitorpamplona.quartz.marmot.mls.messages.MlsKeyPackage import com.vitorpamplona.quartz.nip01Core.core.BaseAddressableEvent import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.core.TagArrayBuilder +import com.vitorpamplona.quartz.nip01Core.core.toHexKey import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.signers.eventTemplate import com.vitorpamplona.quartz.nip01Core.tags.dTag.dTag import com.vitorpamplona.quartz.utils.TimeUtils +import kotlin.io.encoding.Base64 +import kotlin.io.encoding.ExperimentalEncodingApi /** * Marmot KeyPackage Event (MIP-00) — kind 30443. @@ -125,6 +131,8 @@ class KeyPackageEvent( keyPackageRef: HexKey, appComponentIds: List, ciphersuite: String = "0x0001", + mlsExtensionIds: List = listOf(CURRENT_PROFILE_EXTENSION), + mlsProposalIds: List = listOf(APP_DATA_UPDATE_PROPOSAL, MlsProposalsTag.SELF_REMOVE), clientName: String? = null, createdAt: Long = TimeUtils.now(), initializer: TagArrayBuilder.() -> Unit = {}, @@ -132,8 +140,8 @@ class KeyPackageEvent( dTag(dTagSlot) mlsProtocolVersion() mlsCiphersuite(ciphersuite) - mlsExtensions(listOf(CURRENT_PROFILE_EXTENSION)) - mlsProposals(listOf(APP_DATA_UPDATE_PROPOSAL, MlsProposalsTag.SELF_REMOVE)) + mlsExtensions(mlsExtensionIds.distinct().sorted()) + mlsProposals(mlsProposalIds.distinct().sorted()) appComponents( (appComponentIds + AppComponentsTag.ACCOUNT_IDENTITY_PROOF_V2).distinct().sorted(), ) @@ -142,6 +150,65 @@ class KeyPackageEvent( initializer() } + /** + * Build the event from the KeyPackage itself, deriving every id-list + * tag from the bytes it advertises. + * + * The tags duplicate metadata that is already inside the KeyPackage, so + * writing them by hand is writing a second source of truth — and MDK + * rejects a KeyPackage whose `mls_extensions` tag "does not exactly + * match decoded KeyPackage metadata". Adding one leaf capability and + * forgetting the tag is enough to make every one of our KeyPackages + * unusable, which is exactly what happened. + * + * `app_components` lists the Marmot registry ids only. The + * `app_components` component itself (`0x0001`) and the other upstream + * MLS-extensions component ids live below `0x8000` and are not app + * components being advertised. + */ + @OptIn(ExperimentalEncodingApi::class) + fun buildCurrentProfileFrom( + keyPackage: MlsKeyPackage, + dTagSlot: String, + clientName: String? = null, + createdAt: Long = TimeUtils.now(), + initializer: TagArrayBuilder.() -> Unit = {}, + ) = buildCurrentProfile( + keyPackageBase64 = Base64.encode(KeyPackageUtils.frameKeyPackage(keyPackage)), + dTagSlot = dTagSlot, + keyPackageRef = keyPackage.reference().toHexKey(), + appComponentIds = advertisedAppComponents(keyPackage).map(::idHex), + ciphersuite = idHex(keyPackage.cipherSuite), + mlsExtensionIds = + keyPackage.leafNode.capabilities.extensions + .map(::idHex), + mlsProposalIds = + keyPackage.leafNode.capabilities.proposals + .map(::idHex), + clientName = clientName, + createdAt = createdAt, + initializer = initializer, + ) + + /** `0x`-prefixed lowercase hex of a 16-bit id, zero-padded to four digits. */ + fun idHex(id: Int): String { + val hex = id.toString(16) + return "0x" + "0".repeat(4 - hex.length) + hex + } + + /** Marmot app-component ids the leaf advertises, from its `app_components` component. */ + private fun advertisedAppComponents(keyPackage: MlsKeyPackage): List = + try { + val dictionary = AppDataDictionary.fromExtensionsOrEmpty(keyPackage.leafNode.extensions) + val list = dictionary[ComponentsList.APP_COMPONENTS_ID] ?: return emptyList() + ComponentsList.decode(list).filter { it >= MARMOT_COMPONENT_RANGE_START } + } catch (_: Exception) { + emptyList() + } + + /** Marmot's own component registry starts here; lower ids are upstream MLS-extensions ones. */ + private const val MARMOT_COMPONENT_RANGE_START = 0x8000 + /** MIP-era builder, kept for legacy groups already on disk. */ fun build( keyPackageBase64: String, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageUtils.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageUtils.kt index e5617a900f..a5fd89b6ee 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageUtils.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageUtils.kt @@ -33,6 +33,8 @@ import com.vitorpamplona.quartz.marmot.mls.codec.TlsReader import com.vitorpamplona.quartz.marmot.mls.components.AppDataDictionary import com.vitorpamplona.quartz.marmot.mls.components.ComponentsList import com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider +import com.vitorpamplona.quartz.marmot.mls.framing.MlsMessage +import com.vitorpamplona.quartz.marmot.mls.framing.WireFormat import com.vitorpamplona.quartz.marmot.mls.messages.MlsKeyPackage import com.vitorpamplona.quartz.marmot.mls.tree.Credential import com.vitorpamplona.quartz.nip01Core.core.Event @@ -59,6 +61,52 @@ object KeyPackageUtils { */ const val MAX_LIFETIME_SECONDS = 7_261_200L + /** + * Frame a KeyPackage for publication. + * + * `foundation/key-packages.md` is explicit: "a transport publication is + * unambiguously the framed `MLSMessage`, not a bare `KeyPackage` struct." + * The envelope is only four bytes — `ProtocolVersion` then + * `WireFormat = mls_key_package` — but omitting it is not a cosmetic + * difference. A reader that expects the envelope reads a bare KeyPackage's + * leading `0x0001 0x0001` as version 1, wire format 1 (`mls_public_message`) + * and then parses the rest as a PublicMessage, desyncing a few fields in + * and failing on whatever byte it lands on. That is exactly how MDK + * rejected every KeyPackage we published, with a decode error naming a + * value that appears nowhere in the structure. + */ + fun frameKeyPackage(keyPackage: MlsKeyPackage): ByteArray = + MlsMessage( + wireFormat = WireFormat.KEY_PACKAGE, + payload = keyPackage.toTlsBytes(), + ).toTlsBytes() + + /** + * Decode published KeyPackage bytes, framed or bare. + * + * Framed is what the spec requires and what we now publish. The bare form + * is accepted because every KeyPackage this client published before the fix + * is bare, and those are still sitting on relays inside their publication + * lifetime; refusing them would make our own users un-invitable by each + * other until every one of them rotated. + * + * The two are told apart by the envelope rather than by trial and error: a + * framed message starts with `ProtocolVersion = 1` and + * `WireFormat = mls_key_package`, and a bare KeyPackage's second field is + * its ciphersuite, which is never 5 for any suite Marmot uses. + */ + fun decodeKeyPackage(bytes: ByteArray): MlsKeyPackage { + if (bytes.size >= 4) { + val version = ((bytes[0].toInt() and 0xFF) shl 8) or (bytes[1].toInt() and 0xFF) + val wireFormat = ((bytes[2].toInt() and 0xFF) shl 8) or (bytes[3].toInt() and 0xFF) + if (version == MlsMessage.MLS_VERSION_10 && wireFormat == WireFormat.KEY_PACKAGE.value) { + val message = MlsMessage.decodeTls(TlsReader(bytes)) + return MlsKeyPackage.decodeTls(TlsReader(message.payload)) + } + } + return MlsKeyPackage.decodeTls(TlsReader(bytes)) + } + /** Legacy non-addressable KeyPackage kind (pre-migration) */ const val LEGACY_KIND = 443 @@ -190,8 +238,7 @@ object KeyPackageUtils { val iTag = event.keyPackageRef() ?: return false val keyPackage = try { - val bytes = Base64.decode(event.content) - MlsKeyPackage.decodeTls(TlsReader(bytes)) + decodeKeyPackage(Base64.decode(event.content)) } catch (_: Throwable) { return false } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageFramingTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageFramingTest.kt new file mode 100644 index 0000000000..26619fe6f9 --- /dev/null +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageFramingTest.kt @@ -0,0 +1,104 @@ +/* + * 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.mip00KeyPackages + +import com.vitorpamplona.quartz.marmot.mls.framing.MlsMessage +import com.vitorpamplona.quartz.marmot.mls.framing.WireFormat +import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * `foundation/key-packages.md`: "a transport publication is unambiguously the + * framed `MLSMessage`, not a bare `KeyPackage` struct." + * + * We published bare bytes, and it cost us every invitation. A reader expecting + * the envelope reads a bare KeyPackage's leading `0x0001 0x0001` as + * version 1 / wire format 1 (`mls_public_message`), parses on as a + * PublicMessage, and dies several fields later on a byte that means nothing — + * MDK reported `UnknownValue(112)`, a number that appears nowhere in a + * KeyPackage. A four-byte envelope is not a detail when its absence is + * indistinguishable from a different message type. + */ +class KeyPackageFramingTest { + private fun aKeyPackage() = + MlsGroup + .create(identity = ByteArray(32) { 0x0a }) + .createKeyPackage(identity = ByteArray(32) { 0x0b }, signingKey = ByteArray(32) { 1 }) + .keyPackage + + @Test + fun framesAsAnMlsMessageWithTheKeyPackageWireFormat() { + val kp = aKeyPackage() + val framed = KeyPackageUtils.frameKeyPackage(kp) + + assertEquals(MlsMessage.MLS_VERSION_10, ((framed[0].toInt() and 0xFF) shl 8) or (framed[1].toInt() and 0xFF)) + assertEquals(WireFormat.KEY_PACKAGE.value, ((framed[2].toInt() and 0xFF) shl 8) or (framed[3].toInt() and 0xFF)) + assertEquals(kp.toTlsBytes().size + 4, framed.size) + } + + @Test + fun decodesItsOwnFraming() { + val kp = aKeyPackage() + val decoded = KeyPackageUtils.decodeKeyPackage(KeyPackageUtils.frameKeyPackage(kp)) + assertContentEquals(kp.toTlsBytes(), decoded.toTlsBytes()) + } + + /** + * Bare bytes still decode. Every KeyPackage this client published before + * the fix is bare and still inside its publication lifetime; refusing them + * would leave our own users unable to invite each other until all of them + * rotated. + */ + @Test + fun stillDecodesTheBareLegacyForm() { + val kp = aKeyPackage() + val decoded = KeyPackageUtils.decodeKeyPackage(kp.toTlsBytes()) + assertContentEquals(kp.toTlsBytes(), decoded.toTlsBytes()) + } + + /** + * The two forms are told apart by the envelope, not by trial and error: a + * bare KeyPackage's second field is its ciphersuite, which is never 5. + */ + @Test + fun tellsTheFormsApartByTheEnvelopeNotByGuessing() { + val kp = aKeyPackage() + val bare = kp.toTlsBytes() + assertEquals(MlsMessage.MLS_VERSION_10, ((bare[0].toInt() and 0xFF) shl 8) or (bare[1].toInt() and 0xFF)) + val suite = ((bare[2].toInt() and 0xFF) shl 8) or (bare[3].toInt() and 0xFF) + assertTrue(suite != WireFormat.KEY_PACKAGE.value, "a real ciphersuite must not collide with the wire format") + } + + /** + * `KeyPackageRef` is computed over the INNER KeyPackage, never the + * envelope — framing it too would make our `i` tag disagree with every + * other implementation's. + */ + @Test + fun theReferenceIsOverTheInnerKeyPackage() { + val kp = aKeyPackage() + val framed = KeyPackageUtils.frameKeyPackage(kp) + assertContentEquals(kp.reference(), KeyPackageUtils.decodeKeyPackage(framed).reference()) + } +}