Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds MIP-06 multi-device support: new spec ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
alltheseas
left a comment
There was a problem hiding this comment.
Review Summary
Strong specification with well-designed architecture (External Commits, three-layer auth chain, capability gating). The 22931b5 revision addressed several issues from an earlier review round — forward-compat TLS framing is now fixed, pairing secrets are ciphersuite-length-aware, ephemeral key freshness is explicit, and capability gating via 0xF2F0 prevents silent group forks.
One blocker remains: the HKDF hash function for pairing encryption is not pinned, which will cause interop splits between implementations.
See inline comments for the blocker and additional non-blocking findings.
Cross-file issue not addressable inline
03.md:91 still reads as an unconditional MUST-reject for all non-admin non-self-update Commits. The 0xF2F0 capability gating prevents silent forks (non-MIP-06 clients can't join groups requiring 0xF2F0), so this is a normative clarity gap rather than a hard protocol blocker. But 03.md:91 should still be amended to reference the MIP-06 External Commit exception — a client reading both specs currently encounters a contradiction.
Suggested wording for 03.md:91: add "Exception: when sender_type is new_member_commit and marmot_multi_device (0xF2F0) is present in GroupContext.extensions, apply MIP-06 External Commit authorization rules instead."
|
What can happen if one of the apps I am testing with real groups before switching to is compromised? |
|
Does the device name extension help with deduping keypackages? Or is that just for the sake of display in the user's own apps. Big UX challenge we're trying to solve is the fact that the newest device to publish the keypackage gets all the invites rn. |
I'm not sure what you mean by this @vitorpamplona |
@futurepaul we talked a lot about this recently with the team. We're strongly considering moving to replaceable events for key packages. I'd love to hear your thoughts on that. you too @vitorpamplona Benefits:
|
Users test a LOT of nostr apps at the same time, with production nsecs and production groups they participate in. Usually via Signers, but still, the culture in Nostr is that users get to pick their own clients and frequently change them all the time. If I am in a group with 5 people and all of us are frequently (every week or so) testing new clients to see if they like them, what could happen to the privacy/security of the group? This is a security issue that is unique to Nostr because of our own protocol/culture of fully interoperable clients. |
|
On the replaceable KP, sounds ok. Deletion of non-replaceable events works quite well in most relays, so I am not sure what the issue is with the current approach. |
The main issue is that we'd originally wanted to use Also - with multi-device it means that we're going to have lots more valid KPs in existence at any given time so replaceable makes it a little easier to track these (especially if a client adds a client tag to the KP itself) |
Yeah - this is a real security risk to the group if the clients they are testing aren't implementing things properly. 🤷♂️ I'm not really sure that that is something we can completely solve for. |
That's why I was leaning more towards a Marmot signer. The goal is to minimize the risk of random clients leaking control over groups. |
Define how users operate Marmot on multiple devices via MLS External Commits with a three-layer authentication chain: exporter_secret knowledge, Resumption PSK, and Nostr identity proof in FramedContent.authenticated_data. Includes device pairing flow, TLS-serialized payload format, size estimates for implementors, transfer channel guidance, optional encrypted device name extension (0xF2EF), and client recommendations.
22931b5 to
5dd0cc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@00.md`:
- Line 74: The sentence describing the relays tag was narrowed incorrectly;
update the description for the `relays` tag so it states that it lists Relay
URLs where the KeyPackage publisher can receive Welcome events (e.g., “Relay
URLs where the KeyPackage publisher can receive Welcome events; MUST contain at
least one valid WebSocket URL (ws:// or wss://)”), and clarify that Welcome
events are delivered via the recipient’s inbox relays rather than implying the
KeyPackage is published there; edit the `relays` line and any nearby instances
that currently say “where the KeyPackage is published” to use the
publisher/receive wording instead.
In `@06.md`:
- Around line 476-479: The fenced code block containing the NIP44 example should
include a language identifier to satisfy markdownlint MD040; edit the block that
contains "conversation_key = NIP44.derive_conversation_key(nostr_privkey,
nostr_pubkey)" and "encrypted_name = NIP44.encrypt(conversation_key,
device_name_utf8)" to start with a language tag (e.g., ```text or ```python) so
the fence explicitly declares the language.
- Around line 75-83: The acceptance logic for MIP-06 must validate the full
signaling gate, not just presence in GroupContext.extensions: when evaluating an
External Commit (the checks around "marmot_multi_device" in
GroupContext.extensions), additionally require that
GroupContext.required_capabilities includes the multi-device capability and that
at least one existing member advertises that capability (e.g., check each
member's advertised capabilities field such as member.capabilities or
equivalent). Update the acceptance checks referenced in the MIP-06 section (the
logic driving the bullets around `marmot_multi_device`,
`GroupContext.extensions`, and related checks) to require all three conditions
(GroupContext.extensions contains `marmot_multi_device`,
GroupContext.required_capabilities lists the capability, and an existing member
advertises it) before accepting an External Commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ce946fe-e2da-479d-8fee-d6e86aa869c3
📒 Files selected for processing (6)
00.md01.md03.md06.mdREADME.mdthreat_model.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
00.md (1)
74-74:⚠️ Potential issue | 🟡 MinorRelays tag description should focus on Welcome delivery.
Line 74 frames
relaysas "where this KeyPackage is published," which breaks the intended universal meaning for direct device-to-device shared KeyPackages. The field should describe where the KeyPackage publisher can receive Welcome events.Based on learnings: "the
relaystag ... should be described as 'Relay URLs where the KeyPackage publisher can receive Welcome events' — not as 'where the KeyPackage is published.'" This framing makes the MUST requirement universally valid for both relay-published and direct-shared KeyPackages, since in both cases the inviter needs relay URLs to deliver the Welcome message.Suggested wording fix
-- **`relays`**: Relay URLs where this KeyPackage is published. MUST contain at least one valid WebSocket URL (ws:// or wss://). Used for KeyPackage discovery and deletion. +- **`relays`**: Relay URLs where the KeyPackage publisher can receive Welcome events. MUST contain at least one valid WebSocket URL (ws:// or wss://). This applies whether the KeyPackage is relay-published or shared directly device-to-device.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@00.md` at line 74, Update the `relays` tag description to focus on Welcome delivery: change the phrasing that currently says "Relay URLs where this KeyPackage is published" to something like "Relay URLs where the KeyPackage publisher can receive Welcome events" and ensure the MUST requirement (at least one valid WebSocket URL) is preserved so it applies both to relay-published and direct device-to-device shared KeyPackages; locate the `relays` tag description in the document and replace the sentence accordingly while keeping the rest of the constraints intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@00.md`:
- Line 74: Update the `relays` tag description to focus on Welcome delivery:
change the phrasing that currently says "Relay URLs where this KeyPackage is
published" to something like "Relay URLs where the KeyPackage publisher can
receive Welcome events" and ensure the MUST requirement (at least one valid
WebSocket URL) is preserved so it applies both to relay-published and direct
device-to-device shared KeyPackages; locate the `relays` tag description in the
document and replace the sentence accordingly while keeping the rest of the
constraints intact.
There was a problem hiding this comment.
Should we have some configuration on the group that limits the number of devices per user? Just wondering whether it could be a problem to leave it 'unbounded'
If we want to cap, it has to be included at the protocol level because if we leave it to the implementations then they might disagree on the limit and end up forking groups
Perhaps on the same MarmotMultiDevice extension?
struct {
uint16 version; // current: 1
uint8 max_devices_per_user; // 0 = unlimited, 1-255 = cap
} MarmotMultiDevice;
There was a problem hiding this comment.
Added a Device Count Limits section.
I agree this is consensus-critical if we ever add it, because implementation-local caps could make clients disagree about whether an otherwise-valid External Commit advances the group. For v1, I think the safest thing is to explicitly define no protocol-level per-user cap and require implementations not to reject valid MIP-06 joins based only on a local per-user device-count policy. If we decide Marmot needs that policy later, a future version of the marmot_multi_device extension can add a consensus-enforced cap.
dannym-arx
left a comment
There was a problem hiding this comment.
We may have a problem with the spec. Still need to investigate properly, but during my implementation PR I ran into an issue.
The following is a manually cleaned up AI-generated summary. I'm pretty confident it's correct, but it is AI, so... grain of salt.
TL;DR: A new device joining via external commit cannot look up the resumption PSK because ResumptionPskStore starts empty and OpenMLS exposes no way to pre-populate it.
The Problem
1. Normal Path (Existing Member Commits)
When an existing group member creates a commit with a PSK proposal, the group already has state, including a populated resumption_psk_store:
graph LR
A["<b>MlsGroup</b><br/><i>has group state</i>"] --> B["<b>group_epoch_secrets</b>"]
B --> C["<b>ResumptionPskStore</b>"]
C --> D["Vec‹GroupEpoch, ResumptionPskSecret›<br/><i>populated by MLS key schedule</i>"]
style A fill:#2d6a4f,stroke:#1b4332,color:#fff
style B fill:#40916c,stroke:#2d6a4f,color:#fff
style C fill:#52b788,stroke:#40916c,color:#fff
style D fill:#95d5b2,stroke:#52b788,color:#1b4332
Commit creation follows this chain:
commit_builder
.add_psk_proposal(psk)
.load_psks(storage)
.build(...)
.finalize(...)load_psks() at commit_builder.rs:336 resolves through psk.rs:520–535:
match &psk_id.psk {
Psk::Resumption(resumption) => {
if let Some(psk_bundle) = resumption_psk_store.get(resumption.psk_epoch()) {
psk_bundles.push((psk_id, psk_bundle.secret.clone())); // ✅ found
} else {
return Err(PskError::KeyNotFound);
}
}
// ...The store is a ring buffer populated epoch-by-epoch as the group evolves, so lookups always succeed for existing members.
2. External Commit Path (New Device Joining)
When a new device calls ExternalCommitBuilder::build_group(), the group is constructed from scratch:
// line 290
let mut mls_group = MlsGroup {
// ...
resumption_psk_store: ResumptionPskStore::new(32), // ⚠️ empty
// ...
};The new device has no epoch history. The store starts empty. Then the commit flow runs:
commit_builder
.add_psk_proposal(resumption_psk_proposal) // required by MIP-06
.load_psks(provider.storage()) // ❌ searches empty store → KeyNotFoundgraph LR
A["<b>New Device</b><br/>ExternalCommitBuilder"] --> B["<b>build_group()</b><br/>ResumptionPskStore::new(32)"]
B --> C["<b>load_psks()</b><br/>searches empty store"]
C --> D["<b>PskError::KeyNotFound</b>"]
style A fill:#264653,stroke:#1d3557,color:#fff
style B fill:#e76f51,stroke:#c1440e,color:#fff
style C fill:#e76f51,stroke:#c1440e,color:#fff
style D fill:#9b2226,stroke:#641220,color:#fff
3. Why the Existing Device Can't Help
The existing device does know the resumption PSK (it's in the pairing payload), but there is no mechanism to inject it into the new device's store:
| # | Barrier | Detail |
|---|---|---|
| 1 | ResumptionPskStore is pub(crate) |
Cannot be constructed outside openmls |
| 2 | ResumptionPskStore::add() is pub(crate) |
Cannot insert entries externally |
| 3 | No MlsGroup injection method |
No public API to add a PSK to the store |
| 4 | Store is not in StorageProvider |
In-memory struct owned by MlsGroup, no storage workaround |
4. What Would Fix It
OpenMLS needs to expose one of:
| Option | API | Tradeoff |
|---|---|---|
| A | MlsGroup::inject_resumption_psk(epoch, secret) |
Simple but exposes internal store mutation |
| B | Public ResumptionPskStore constructor + setter on MlsGroup |
Flexible but leaks implementation details |
| C | ExternalCommitBuilder::with_resumption_psk(epoch, secret) |
Cleanest: keeps store private, scoped to external commit path |
Recommended: Option C. It keeps the internal store private while allowing the external commit path to supply PSK material received out-of-band.
|
@dannym-arx you were right on the resumption PSKs - I dug in a bit more and changed to using external PSKs which don't have the same limits but serve the same purpose for us which is to prove that the newly joining device has valid current group/epoch secret material. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
06.md (1)
73-85:⚠️ Potential issue | 🟠 MajorResolve cross-spec authorization trigger mismatch with MIP-03.
06.mdrequires the full 3-part signaling gate before applying the MIP-06 carve-out, but03.md:109-119(context snippet) still says the carve-out applies when0xF2F0is present inGroupContext.extensionsalone. This creates conflicting normative behavior across specs and can cause interop divergence on External Commit acceptance. Please align both docs to one authoritative rule (preferably the full 3-part gate).Also applies to: 101-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@06.md` around lines 73 - 85, Docs conflict: 06.md requires the full 3-part signaling gate (GroupContext.extensions contains marmot_multi_device 0xF2F0, GroupContext.required_capabilities requires 0xF2F0, and every current non-blank leaf advertises 0xF2F0) before applying the MIP-06 carve-out, but 03.md still treats presence in GroupContext.extensions alone as sufficient. Update 03.md (and the other occurrences around lines 101-108) to match 06.md’s authoritative rule by replacing the single-extension check with the full three-part gate (references: GroupContext.extensions, GroupContext.required_capabilities, LeafNode.capabilities.extensions, and the MIP-06 join PSK/ExternalInit carve-out language) so both specs require the same conditions for accepting External Commits under MIP-06.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@06.md`:
- Around line 73-85: Docs conflict: 06.md requires the full 3-part signaling
gate (GroupContext.extensions contains marmot_multi_device 0xF2F0,
GroupContext.required_capabilities requires 0xF2F0, and every current non-blank
leaf advertises 0xF2F0) before applying the MIP-06 carve-out, but 03.md still
treats presence in GroupContext.extensions alone as sufficient. Update 03.md
(and the other occurrences around lines 101-108) to match 06.md’s authoritative
rule by replacing the single-extension check with the full three-part gate
(references: GroupContext.extensions, GroupContext.required_capabilities,
LeafNode.capabilities.extensions, and the MIP-06 join PSK/ExternalInit carve-out
language) so both specs require the same conditions for accepting External
Commits under MIP-06.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bac1cba-6cc6-4cdb-ad36-5a66ef4c6ab2
📒 Files selected for processing (2)
06.mdthreat_model.md
🚧 Files skipped from review as they are similar to previous changes (1)
- threat_model.md
|
@erskingardner great, I'll change my impl to use external PSKs on monday |
…ignature_key instead of full LeafNode The full serialized LeafNode contains encryption_key and leaf_node_source which are generated inside build() after authenticated_data must already be set via with_aad(). Replace the challenge hash input with the two identity-relevant fields (credential_identity, signature_key) that are known before commit construction and are already sufficient for identity binding — the remaining LeafNode fields are identity-irrelevant and already covered by MLS confirmation_tag validation. Updates Proof Construction, Proof Verification, Security Properties, and Replay Protection sections.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@06.md`:
- Around line 36-40: Update the version handling for the marmot_multi_device
extension to follow the forward-compatible contract used in 01.md (see
01.md:228-260) and the marmot_group_data guidance: reject version 0, accept
version 1 through MAX_SUPPORTED_VERSION (i.e., treat any supported range as
valid), and for higher/unknown versions parse known fields in order and ignore
any unknown trailing fields rather than rejecting the message; change the strict
“reject higher versions” rule to this forward-compatible pattern so
implementations that only understand v1 can still process v2+ payloads that only
add optional trailing fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| Version rules: | ||
|
|
||
| - Version `0` is reserved and MUST be rejected. | ||
| - Version `1` is the current format. | ||
| - Higher versions MUST be rejected unless explicitly supported. |
There was a problem hiding this comment.
Version handling deviates from established forward-compatibility contract.
Lines 38-40 specify a strict versioning pattern: reject v0, accept v1, reject higher versions. This diverges from the established extension versioning contract defined in 01.md:228-260, which requires all extensions to accept versions 1 through MAX_SUPPORTED_VERSION and handle unknown future versions with forward compatibility by parsing known fields in order and ignoring unknown trailing fields. The marmot_group_data extension (01.md:292-310) explicitly requires append-only forward compatibility.
This deviation means:
- Implementations supporting only v1 will reject v2+
marmot_multi_deviceextensions entirely, even if v2 only adds optional trailing fields - Groups cannot gracefully roll out newer extension versions
- Version bumps become hard protocol forks instead of smooth upgrades
📋 Recommended alignment with standard versioning pattern
Replace lines 36-40 with the standard forward-compatible pattern:
-Version rules:
-
-- Version `0` is reserved and MUST be rejected.
-- Version `1` is the current format.
-- Higher versions MUST be rejected unless explicitly supported.
+Version rules (following the standard Marmot extension versioning contract from [MIP-01](01.md)):
+
+- Version `0` is reserved and MUST be rejected.
+- Versions `1` through the implementation's `MAX_SUPPORTED_VERSION` MUST be accepted if the structure is valid.
+- Unknown future versions (higher than `MAX_SUPPORTED_VERSION`) SHOULD be handled with forward compatibility: parse known fields in order (currently only `version`), ignore unknown trailing fields, log a warning, and continue operation.
+- Current `MAX_SUPPORTED_VERSION` for `marmot_multi_device` is `1`.The structure is already forward-compatible by design (single uint16 field with no complex nested data), so this change preserves all current behavior while enabling smooth future upgrades.
Based on learnings: "Applies to {00,01,02,03,04,threat_model,data_flows,dependency_reqs}.md : Use precise MLS and Nostr terminology in protocol specifications" — ensure version handling follows established protocol patterns for consistency.
🧰 Tools
🪛 LanguageTool
[style] ~39-~39: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...0 is reserved and MUST be rejected. - Version 1 is the current format. - Higher ver...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@06.md` around lines 36 - 40, Update the version handling for the
marmot_multi_device extension to follow the forward-compatible contract used in
01.md (see 01.md:228-260) and the marmot_group_data guidance: reject version 0,
accept version 1 through MAX_SUPPORTED_VERSION (i.e., treat any supported range
as valid), and for higher/unknown versions parse known fields in order and
ignore any unknown trailing fields rather than rejecting the message; change the
strict “reject higher versions” rule to this forward-compatible pattern so
implementations that only understand v1 can still process v2+ payloads that only
add optional trailing fields.

Summary
Adds MIP-06: Multi-Device Support, plus the cross-MIP clarifications needed to make it operationally consistent with the current Marmot spec set.
This defines how one Nostr identity can participate in the same MLS group from multiple devices, where each device is an independent leaf that joins via External Commits.
Closes #43
Files Updated
06.md— full MIP-06 specification01.md— clarified Marmot's TLS-notation + QUIC-varint wire-format convention03.md— MIP-06 commit exception, leaf-scoped SelfRemove guidance, andauthenticated_datarulesthreat_model.md— expanded multi-device-specific threats and validation requirementsREADME.md— MIP table entryCurrent Design
marmot_multi_device(0xF2F0) is present inGroupContext.extensions, required inrequired_capabilities, and advertised in member capabilities0xF2F0at the group levelFramedContent.authenticated_datausage = application)Removeproposals (join-style External Commits only)kind: 450) signed using standard Nostr event signing and carried inauthenticated_data; the proof event MUST NOT be published to relaysgroup_event_key = MLS-Exporter("marmot", "group-event", 32); pairing payload transfers the exact current-epochgroup_event_keyplusresumption_pskandGroupInfo0xF2EF)Additional Spec Tightening Included
groupsvector to2^32-1bytes to support large multi-group sessionsauthenticated_dataempty by default unless another Marmot spec defines otherwiseOut of Scope / Deferred
Summary by CodeRabbit
New Features
Documentation
Updates