test: cover the two decisions that decide who said what
The crypto was tested; the logic that acts on it was not. Both untested pieces were the security-critical ones, and neither fails loudly when it goes wrong -- one silently widens who may impersonate whom, the other silently destroys a message. Extracted MarmotDirectMessage.classify, which decides what an arriving wrap is to this device, from ChatMessage.directMessage, which turns that decision into rows. The decision is pure; only the filing needs a database, and Room-backed code cannot be unit-tested in this project. Same split, and for the same reason, as pulling the wrap/open crypto out of the DAO in the first place. Extracted MarmotInboundManager.mip03Rejection for the same reason. Its kind:1059 exemption is the most dangerous line in this feature: widened to another kind, or stripped of its kind guard, it hands every member of every group the ability to publish events as anybody, and nothing else in the pipeline would notice. There is now a test that walks seven kinds and asserts each is still held to MIP-03. Fifteen cases, the ones worth naming: `our own message is ours, even though we cannot open it` and `ours is decided before anything is opened`. A sender cannot decrypt their own wrap -- the key was discarded -- so by decryption alone this is indistinguishable from a bystander's view, and only the MLS identity separates them. Get it wrong and the inbound path files an empty placeholder over the row sendChatMessage wrote, which holds the only copy of those words. It is the one failure here that loses data rather than rendering something wrong. `words sealed by one member and sent by another are dropped`. The check that replaces MIP-03 for this kind, tested directly rather than described in a comment as it was before. One test asserts something I had wrong. I expected a seal relabelled with another member's pubkey to be caught by the signature check; it never reaches it. NIP-44 derives the conversation key from the pubkey being claimed, so relabelling a seal makes it undecryptable by the person it was encrypted for -- the label is bound to the key, not merely asserted alongside it. The outcome is Unreadable, which is the truth: the recipient genuinely cannot read it. `a seal tampered with after signing is dropped` covers what verify() does catch, using an alteration that survives decryption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
@@ -30,8 +30,6 @@ import press.mantra.compose.nostr.nip30303.TranslationContributorListEvent
|
||||
import press.mantra.compose.nostr.nip30303.TranslationEvent
|
||||
import co.touchlab.kermit.Logger
|
||||
import com.vitorpamplona.quartz.nip01Core.core.firstTagValue
|
||||
import com.vitorpamplona.quartz.nip01Core.crypto.verify
|
||||
import com.vitorpamplona.quartz.nip17Dm.messages.ChatMessageEvent
|
||||
import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent
|
||||
import press.mantra.compose.nostr.MarmotDirectMessage
|
||||
import kotlin.time.Clock
|
||||
@@ -221,23 +219,8 @@ data class ChatMessage(
|
||||
/**
|
||||
* Files one direct message, from whichever side of it this device is on.
|
||||
*
|
||||
* Three outcomes, and the third is the one that destroys data if it is missed:
|
||||
*
|
||||
* **The recipient** opens the wrap and gets the words. The rumor inside is stored
|
||||
* as its own [MarmotInnerEvent] -- keyed on the rumor's id, which is the id the
|
||||
* sender queued -- and the chat message points at that rather than at the wrap.
|
||||
*
|
||||
* **A bystander** cannot open it and gets a line with no content: the group is
|
||||
* meant to see that a private message was sent and to whom, and nothing more.
|
||||
*
|
||||
* **The sender**, on a re-sync, is indistinguishable from a bystander -- the
|
||||
* wrap's key was discarded at send time, so we cannot open our own message -- and
|
||||
* so would file an empty placeholder over the row `sendChatMessage` wrote on the
|
||||
* way out. That row is the only copy of those words that exists. Hence the early
|
||||
* return; `NostrDao.persistInboundChatMessage` carries the same guard, for the
|
||||
* same reason, on the NIP-17 path.
|
||||
*
|
||||
* See docs/marmot-direct-messages.md.
|
||||
* The decision is [MarmotDirectMessage.classify]'s, which is where it is tested;
|
||||
* this turns it into rows. See docs/marmot-direct-messages.md.
|
||||
*/
|
||||
private suspend fun directMessage(
|
||||
database: MantraDatabase,
|
||||
@@ -247,105 +230,91 @@ data class ChatMessage(
|
||||
wrap: Event,
|
||||
senderIdentity: HexKey?
|
||||
): ChatMessage? {
|
||||
if (senderIdentity == null) {
|
||||
logger.e("Dropping direct message ${wrap.id}: no MLS sender identity to attribute it to")
|
||||
return null
|
||||
}
|
||||
|
||||
val recipientPublicKey = wrap.tags.firstTagValue("p")
|
||||
|
||||
// Our own words coming back. See the third outcome above.
|
||||
if (senderIdentity == activeKeyPair.pubKey.toHex()) {
|
||||
logger.d("Direct message ${wrap.id} is ours; the row written on send is the only copy")
|
||||
return null
|
||||
}
|
||||
|
||||
val opened = MarmotDirectMessage.open(wrap, activeKeyPair)
|
||||
|
||||
if (opened == null) {
|
||||
// Relays redeliver and negentropy re-syncs; the wrap yields the same id
|
||||
// every time, so this is what keeps a second delivery from becoming a
|
||||
// second line. ChatMessage.id is autogenerated and would take a duplicate.
|
||||
if (database.chatMessageDao().getChatMessagesByMarmotInnerEventId(wrap.id) != null) {
|
||||
return null
|
||||
return when (val delivery = MarmotDirectMessage.classify(wrap, activeKeyPair, senderIdentity)) {
|
||||
// Our own words coming back. We cannot open our own wrap -- its key was
|
||||
// discarded at send time -- so this is indistinguishable from a
|
||||
// bystander's view by decryption alone, and filing it as one would
|
||||
// replace the words on the row `sendChatMessage` wrote with an empty
|
||||
// placeholder. That row is the only copy of them that exists.
|
||||
// `NostrDao.persistInboundChatMessage` carries the same guard, for the
|
||||
// same reason, on the NIP-17 path.
|
||||
MarmotDirectMessage.Delivery.Ours -> {
|
||||
logger.d("Direct message ${wrap.id} is ours; the row written on send is the only copy")
|
||||
null
|
||||
}
|
||||
|
||||
return ChatMessage(
|
||||
giftWrapPayloadId = null,
|
||||
messageType = TYPE_DIRECT_MESSAGE,
|
||||
marmotGroupEventId = groupEvent.id,
|
||||
marmotInnerEventId = wrap.id,
|
||||
senderPublicKey = senderIdentity,
|
||||
directMessageRecipientPublicKey = recipientPublicKey,
|
||||
isUserMessage = false,
|
||||
chatRoomId = chatRoomId,
|
||||
createdAt = Instant.fromEpochSeconds(wrap.createdAt),
|
||||
content = ""
|
||||
)
|
||||
// A forged or malformed message costs its own line and nothing else. It
|
||||
// is dropped rather than thrown because the caller is inside
|
||||
// storeNostrEvent's transaction, and this must not cost the whole event.
|
||||
is MarmotDirectMessage.Delivery.Rejected -> {
|
||||
logger.e("Dropping direct message ${wrap.id}: ${delivery.reason}")
|
||||
null
|
||||
}
|
||||
|
||||
// Not ours to read. The group is meant to see that a private message was
|
||||
// sent and to whom, and nothing more.
|
||||
MarmotDirectMessage.Delivery.Unreadable -> {
|
||||
// Relays redeliver and negentropy re-syncs; the wrap yields the same
|
||||
// id every time, so this is what keeps a second delivery from becoming
|
||||
// a second line. ChatMessage.id is autogenerated and would take a
|
||||
// duplicate.
|
||||
if (database.chatMessageDao().getChatMessagesByMarmotInnerEventId(wrap.id) != null) {
|
||||
return null
|
||||
}
|
||||
|
||||
ChatMessage(
|
||||
giftWrapPayloadId = null,
|
||||
messageType = TYPE_DIRECT_MESSAGE,
|
||||
marmotGroupEventId = groupEvent.id,
|
||||
marmotInnerEventId = wrap.id,
|
||||
senderPublicKey = senderIdentity ?: wrap.pubKey,
|
||||
directMessageRecipientPublicKey = recipientPublicKey,
|
||||
isUserMessage = false,
|
||||
chatRoomId = chatRoomId,
|
||||
createdAt = Instant.fromEpochSeconds(wrap.createdAt),
|
||||
content = ""
|
||||
)
|
||||
}
|
||||
|
||||
is MarmotDirectMessage.Delivery.Readable -> {
|
||||
val opened = delivery.opened
|
||||
|
||||
if (database.chatMessageDao().getChatMessagesByMarmotInnerEventId(opened.rumor.id) != null) {
|
||||
return null
|
||||
}
|
||||
|
||||
// Keyed on the rumor, not the wrap. This is the id the sender queued,
|
||||
// so both sides of the conversation hold the same message under the
|
||||
// same identity.
|
||||
database.marmotInnerEventDao().upsert(
|
||||
MarmotInnerEvent(
|
||||
id = opened.rumor.id,
|
||||
publicKey = opened.rumor.pubKey,
|
||||
marmotGroupEventId = groupEvent.id,
|
||||
tags = opened.rumor.tags,
|
||||
content = opened.rumor.content,
|
||||
chatRoomId = chatRoomId,
|
||||
kind = opened.rumor.kind,
|
||||
createdAt = Instant.fromEpochSeconds(opened.rumor.createdAt)
|
||||
)
|
||||
)
|
||||
|
||||
ChatMessage(
|
||||
giftWrapPayloadId = null,
|
||||
messageType = TYPE_DIRECT_MESSAGE,
|
||||
marmotGroupEventId = groupEvent.id,
|
||||
marmotInnerEventId = opened.rumor.id,
|
||||
senderPublicKey = opened.seal.pubKey,
|
||||
directMessageRecipientPublicKey = recipientPublicKey,
|
||||
isUserMessage = false,
|
||||
chatRoomId = chatRoomId,
|
||||
createdAt = Instant.fromEpochSeconds(opened.rumor.createdAt),
|
||||
content = opened.rumor.content
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// What replaces MIP-03's pubkey check for kind:1059, and the only thing
|
||||
// standing between the words and being rendered as somebody else's. The seal
|
||||
// is the one layer the sender signs; binding it to the MLS leaf that sent the
|
||||
// message is what stops a member re-wrapping a seal they were sent and
|
||||
// passing it off as its author's. A forged one is dropped, not rendered.
|
||||
if (opened.seal.pubKey != senderIdentity) {
|
||||
logger.e(
|
||||
"Dropping direct message ${wrap.id}: sealed by ${opened.seal.pubKey} " +
|
||||
"but sent by $senderIdentity"
|
||||
)
|
||||
return null
|
||||
}
|
||||
|
||||
if (!opened.seal.verify()) {
|
||||
logger.e("Dropping direct message ${wrap.id}: the seal's signature does not verify")
|
||||
return null
|
||||
}
|
||||
|
||||
if (opened.rumor.pubKey != opened.seal.pubKey) {
|
||||
logger.e(
|
||||
"Dropping direct message ${wrap.id}: rumor by ${opened.rumor.pubKey} " +
|
||||
"inside a seal by ${opened.seal.pubKey}"
|
||||
)
|
||||
return null
|
||||
}
|
||||
|
||||
if (opened.rumor.kind != ChatMessageEvent.KIND) {
|
||||
logger.w("Dropping direct message ${wrap.id}: unsupported rumor kind ${opened.rumor.kind}")
|
||||
return null
|
||||
}
|
||||
|
||||
if (database.chatMessageDao().getChatMessagesByMarmotInnerEventId(opened.rumor.id) != null) {
|
||||
return null
|
||||
}
|
||||
|
||||
// Keyed on the rumor, not the wrap. This is the id the sender queued, so both
|
||||
// sides of the conversation hold the same message under the same identity.
|
||||
database.marmotInnerEventDao().upsert(
|
||||
MarmotInnerEvent(
|
||||
id = opened.rumor.id,
|
||||
publicKey = opened.rumor.pubKey,
|
||||
marmotGroupEventId = groupEvent.id,
|
||||
tags = opened.rumor.tags,
|
||||
content = opened.rumor.content,
|
||||
chatRoomId = chatRoomId,
|
||||
kind = opened.rumor.kind,
|
||||
createdAt = Instant.fromEpochSeconds(opened.rumor.createdAt)
|
||||
)
|
||||
)
|
||||
|
||||
return ChatMessage(
|
||||
giftWrapPayloadId = null,
|
||||
messageType = TYPE_DIRECT_MESSAGE,
|
||||
marmotGroupEventId = groupEvent.id,
|
||||
marmotInnerEventId = opened.rumor.id,
|
||||
senderPublicKey = senderIdentity,
|
||||
directMessageRecipientPublicKey = recipientPublicKey,
|
||||
isUserMessage = false,
|
||||
chatRoomId = chatRoomId,
|
||||
createdAt = Instant.fromEpochSeconds(opened.rumor.createdAt),
|
||||
content = opened.rumor.content
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -37,6 +37,7 @@ import press.mantra.compose.database.GENESIS_AT
|
||||
import press.mantra.compose.database.model.NegentropySynchronizeRequest
|
||||
import press.mantra.compose.database.model.Profile
|
||||
import press.mantra.compose.database.model.types.SynchronizationFilter
|
||||
import press.mantra.compose.nostr.MarmotDirectMessage
|
||||
import press.mantra.compose.nostr.Relays
|
||||
import kotlin.time.Clock
|
||||
|
||||
@@ -62,6 +63,45 @@ object MarmotInboundManager {
|
||||
|
||||
private val commitTracker = CommitOrdering.EpochCommitTracker()
|
||||
|
||||
/**
|
||||
* Why an inner application event may not be accepted from this sender, or null.
|
||||
*
|
||||
* MIP-03: an inner event's `pubkey` MUST equal the MLS sender's credential identity,
|
||||
* so a member cannot mint events claiming a different author.
|
||||
*
|
||||
* A gift wrap is exempt. Its pubkey is a throwaway key by construction -- see
|
||||
* [MarmotDirectMessage] -- so there is no author field to compare and the check
|
||||
* cannot be applied as written. The authorship claim moves inside, to the signed
|
||||
* kind:13 seal, which [MarmotDirectMessage.classify] binds to this same identity
|
||||
* before a word is rendered: a verified signature bound to an MLS leaf, rather than a
|
||||
* plaintext field compared to one.
|
||||
*
|
||||
* The exemption is on kind:1059 alone. Widening it to any other kind, or dropping the
|
||||
* kind guard, hands every member the ability to publish events as anybody.
|
||||
*
|
||||
* Other Marmot clients do not have this carve-out and drop these messages as
|
||||
* impersonation. See docs/marmot-direct-messages.md.
|
||||
*
|
||||
* [senderIdentity] is required rather than merely compared: every sender-derived
|
||||
* field downstream reads from it -- who a message is from, and whether it is ours --
|
||||
* because a gift wrap payload carries no author of its own.
|
||||
*/
|
||||
fun mip03Rejection(
|
||||
innerEventKind: Int,
|
||||
innerEventPubKey: HexKey,
|
||||
senderIdentity: HexKey?,
|
||||
): String? {
|
||||
if (senderIdentity == null) {
|
||||
return "No MLS credential identity for the sender leaf"
|
||||
}
|
||||
|
||||
if (innerEventKind != GiftWrapEvent.KIND && innerEventPubKey != senderIdentity) {
|
||||
return "MIP-03: inner event pubkey ($innerEventPubKey) does not match MLS sender identity ($senderIdentity)"
|
||||
}
|
||||
|
||||
return null
|
||||
}
|
||||
|
||||
suspend fun processGroupMembershipChanges(
|
||||
database: MantraDatabase,
|
||||
localChatRoom: LocalChatRoom,
|
||||
@@ -269,31 +309,8 @@ object MarmotInboundManager {
|
||||
if (innerEvent != null) {
|
||||
val senderIdentity = mlsGroup.memberIdentityHex(decrypted.senderLeafIndex)
|
||||
|
||||
// Required rather than merely compared. Every sender-derived field
|
||||
// downstream now reads from here -- who a message is from, and whether
|
||||
// it is ours -- because a gift wrap payload carries no author of its
|
||||
// own. Without an identity there is nothing to attribute a line to.
|
||||
if (senderIdentity == null) {
|
||||
return GroupEventResult.Error(
|
||||
groupId,
|
||||
"No MLS credential identity for sender leaf ${decrypted.senderLeafIndex}",
|
||||
)
|
||||
}
|
||||
|
||||
// A gift wrap's pubkey is a throwaway key by construction, so there is
|
||||
// no author field here to compare against and MIP-03's check cannot be
|
||||
// applied as written. The authorship claim moves inside, to the signed
|
||||
// kind:13 seal, which ChatMessage.fromGroupEventResult binds to this
|
||||
// same senderIdentity before it renders a word -- a verified signature
|
||||
// bound to an MLS leaf, rather than a plaintext field compared to one.
|
||||
//
|
||||
// Other Marmot clients do not have this carve-out and will drop these
|
||||
// as impersonation. See docs/marmot-direct-messages.md.
|
||||
if (innerEvent.kind != GiftWrapEvent.KIND && innerEvent.pubKey != senderIdentity) {
|
||||
return GroupEventResult.Error(
|
||||
groupId,
|
||||
"MIP-03: inner event pubkey (${innerEvent.pubKey}) does not match MLS sender identity ($senderIdentity)",
|
||||
)
|
||||
mip03Rejection(innerEvent.kind, innerEvent.pubKey, senderIdentity)?.let { reason ->
|
||||
return GroupEventResult.Error(groupId, reason)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -4,11 +4,14 @@ import com.vitorpamplona.quartz.nip01Core.core.Event
|
||||
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
||||
import com.vitorpamplona.quartz.nip01Core.core.Kind
|
||||
import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray
|
||||
import com.vitorpamplona.quartz.nip01Core.core.toHexKey
|
||||
import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
|
||||
import com.vitorpamplona.quartz.nip01Core.crypto.verify
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
|
||||
import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate
|
||||
import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerSync
|
||||
import com.vitorpamplona.quartz.nip44Encryption.Nip44
|
||||
import com.vitorpamplona.quartz.nip17Dm.messages.ChatMessageEvent
|
||||
import com.vitorpamplona.quartz.nip59Giftwrap.rumors.RumorAssembler
|
||||
import com.vitorpamplona.quartz.nip59Giftwrap.seals.SealedRumorEvent
|
||||
import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent
|
||||
@@ -42,6 +45,87 @@ object MarmotDirectMessage {
|
||||
val rumor: Event,
|
||||
)
|
||||
|
||||
/**
|
||||
* What this device should do with an arriving wrap.
|
||||
*
|
||||
* Separated from the filing of it so the decision can be tested: the persistence it
|
||||
* drives needs a database, which cannot be unit-tested in this project, and the
|
||||
* decision includes the check that replaces MIP-03 for kind:1059. That check is the
|
||||
* only thing standing between somebody else's words and being rendered as a member's
|
||||
* own, so it is not somewhere to rely on integration testing that does not exist.
|
||||
*/
|
||||
sealed interface Delivery {
|
||||
/**
|
||||
* Our own message coming back, which we cannot open -- the wrap's key was
|
||||
* discarded at send time -- and so cannot tell apart from [Unreadable] by
|
||||
* decryption alone. Only the MLS sender identity distinguishes them.
|
||||
*
|
||||
* File nothing: the row written on send holds the only copy of these words, and
|
||||
* an unreadable line would replace them with a placeholder.
|
||||
*/
|
||||
data object Ours : Delivery
|
||||
|
||||
/** Somebody else's. The group sees that it happened, and to whom. */
|
||||
data object Unreadable : Delivery
|
||||
|
||||
/** Addressed to us, opened, and consistent with the leaf that sent it. */
|
||||
data class Readable(
|
||||
val opened: Opened,
|
||||
) : Delivery
|
||||
|
||||
/** Something is wrong with it. Drop it; do not render it as anybody's. */
|
||||
data class Rejected(
|
||||
val reason: String,
|
||||
) : Delivery
|
||||
}
|
||||
|
||||
/**
|
||||
* Decides what [wrap] is to this device, given the identity MLS says sent it.
|
||||
*
|
||||
* [senderIdentity] is not optional and is not read from the payload: a wrap names
|
||||
* nobody, so the MLS frame is the only source of who sent it. Absent one there is
|
||||
* nothing to attribute a line to and nothing to check the seal against.
|
||||
*/
|
||||
fun classify(
|
||||
wrap: Event,
|
||||
me: KeyPair,
|
||||
senderIdentity: HexKey?,
|
||||
): Delivery {
|
||||
if (senderIdentity == null) {
|
||||
return Delivery.Rejected("no MLS sender identity to attribute it to")
|
||||
}
|
||||
|
||||
if (senderIdentity == me.pubKey.toHexKey()) return Delivery.Ours
|
||||
|
||||
val opened = open(wrap, me) ?: return Delivery.Unreadable
|
||||
|
||||
// The seal is the one layer the sender signs. Binding it to the leaf MLS says
|
||||
// sent this message is what replaces MIP-03's pubkey check for kind:1059 --
|
||||
// a verified signature bound to a leaf, rather than a plaintext field compared
|
||||
// to one. A divergence here is somebody passing off words as another member's.
|
||||
if (opened.seal.pubKey != senderIdentity) {
|
||||
return Delivery.Rejected(
|
||||
"sealed by ${opened.seal.pubKey} but sent by $senderIdentity",
|
||||
)
|
||||
}
|
||||
|
||||
if (!opened.seal.verify()) {
|
||||
return Delivery.Rejected("the seal's signature does not verify")
|
||||
}
|
||||
|
||||
if (opened.rumor.pubKey != opened.seal.pubKey) {
|
||||
return Delivery.Rejected(
|
||||
"rumor by ${opened.rumor.pubKey} inside a seal by ${opened.seal.pubKey}",
|
||||
)
|
||||
}
|
||||
|
||||
if (opened.rumor.kind != ChatMessageEvent.KIND) {
|
||||
return Delivery.Rejected("unsupported rumor kind ${opened.rumor.kind}")
|
||||
}
|
||||
|
||||
return Delivery.Readable(opened)
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the payload for a direct message to [recipientPublicKey].
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user