Skip to content

feat(MIP-06): Multi-Device Support#44

Open
erskingardner wants to merge 9 commits intomasterfrom
mip-06-multi-device
Open

feat(MIP-06): Multi-Device Support#44
erskingardner wants to merge 9 commits intomasterfrom
mip-06-multi-device

Conversation

@erskingardner
Copy link
Copy Markdown
Member

@erskingardner erskingardner commented Feb 19, 2026

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 specification
  • 01.md — clarified Marmot's TLS-notation + QUIC-varint wire-format convention
  • 03.md — MIP-06 commit exception, leaf-scoped SelfRemove guidance, and authenticated_data rules
  • threat_model.md — expanded multi-device-specific threats and validation requirements
  • README.md — MIP table entry

Current Design

  • Join mechanism: MLS External Commits (RFC 9420 §12.4.3.2)
  • Group gating: MIP-06 is enabled only when marmot_multi_device (0xF2F0) is present in GroupContext.extensions, required in required_capabilities, and advertised in member capabilities
  • Existing-group migration: current leaves update capabilities first; only then can admins enable 0xF2F0 at the group level
  • Authorization checks:
    • joining credential identity matches an existing member identity
    • valid Nostr identity proof in FramedContent.authenticated_data
    • exactly one Resumption PSK proposal (usage = application)
    • no Remove proposals (join-style External Commits only)
    • normal MLS / LeafNode validation
  • Identity proof: canonical unsigned Nostr proof event (kind: 450) signed using standard Nostr event signing and carried in authenticated_data; the proof event MUST NOT be published to relays
  • Outer encryption: group_event_key = MLS-Exporter("marmot", "group-event", 32); pairing payload transfers the exact current-epoch group_event_key plus resumption_psk and GroupInfo
  • Pairing transport: X25519 + HKDF-SHA256 + ChaCha20-Poly1305; Phase 1 bootstrap must use an authenticated channel or explicit key verification
  • Retry model: incremental refresh uses the original new-device recipient key plus a fresh existing-device ephemeral keypair for each refresh message
  • Optional device metadata: encrypted device-name extension (0xF2EF)
  • Admin model: admin status remains identity-based, so new leaves for an admin identity inherit admin privileges immediately

Additional Spec Tightening Included

  • Uses Marmot's established TLS-notation / QUIC-varint serialization convention consistently with MIP-01
  • Simplifies pairing-payload versioning to a single outer payload version
  • Expands the payload groups vector to 2^32-1 bytes to support large multi-group sessions
  • Makes non-MIP-06 Commit authenticated_data empty by default unless another Marmot spec defines otherwise
  • Clarifies that SelfRemove is leaf-scoped in multi-device groups
  • Adds client guidance to surface device additions, especially for admin identities and suspicious repeated same-identity joins
  • Adds threat-model coverage for:
    • External Commit flooding / epoch-churn DoS
    • Phase-1 bootstrap key substitution / MITM

Out of Scope / Deferred

  • History synchronization between devices
  • Mandatory stable cross-group device identifier / guaranteed “remove this device from every group” semantics

Summary by CodeRabbit

  • New Features

    • MIP-06 multi-device support, device pairing flow, and multi-device External Commit rules
  • Documentation

    • Added MIP-06 spec and README entry; expanded threat model and security guidance
    • Clarified relays tag: used for KeyPackage discovery and deletion (welcome delivery statement removed)
  • Updates

    • Require marmot_multi_device signaling/capabilities for MIP-06 groups
    • Renamed encryption key concept to group_event_key; updated serialization, nonce and encoding conventions
    • Adjusted commit authorization and SelfRemove semantics for MIP-06 groups

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 19, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds MIP-06 multi-device support: new spec (06.md) defining marmot_multi_device (0xF2F0) signaling, KeyPackage/group capability requirements, revised group-event encryption derived from MLS-Exporter as group_event_key, MIP-06 External Commit authorization and pairing flows, updated SelfRemove semantics, threat-model expansions, and clarifications to KeyPackage relays usage.

Changes

Cohort / File(s) Summary
MIP-06 Multi-Device Spec
06.md
New MIP-06 document specifying marmot_multi_device (0xF2F0) extension/versioning, strict External Commit acceptance rules (Nostr identity proof, PSK constraints, group signaling/capability checks), out-of-band pairing flow, TLS pairing payloads, and optional device-name extension (0xF2EF).
KeyPackage & Group Context
00.md, 01.md
Docs updated: KeyPackage mls_extensions rules and relays tag clarified (discovery and deletion). Groups enabling multi-device MUST include 0xF2F0 in GroupContext.extensions and required_capabilities; members must advertise it in LeafNode.capabilities.extensions.
Encryption & Commit/Auth Semantics
03.md
Outer-group-event AEAD now derived as group_event_key = MLS-Exporter("marmot","group-event",32) and used with ChaCha20-Poly1305; ciphertext remains `base64(nonce
Threat Model & Security Docs
threat_model.md
Threat entries revised/expanded: rename to group-event-key compromise, add T.13.8–T.13.10 (retry/epoch-refresh, External Commit flooding, pairing key substitution), MIP-06 CRITICAL validation checklist, and updated countermeasures/implementation requirements.
Project Metadata / README
README.md
Added MIP-06 row to MIPs table linking to 06.md (marked Draft / not required).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(MIP-06): Multi-Device Support' clearly and concisely summarizes the main change—adding MIP-06 specification for multi-device support—and is directly related to the primary content of this PR.
Linked Issues check ✅ Passed The PR closes issue #43 (Multi-Device Support) by fully implementing the MIP-06 specification with comprehensive documentation covering join mechanisms, authorization, identity proofs, encryption, pairing flows, and threat model updates.
Out of Scope Changes check ✅ Passed All changes are within scope: new 06.md defines MIP-06 multi-device spec; 01.md, 03.md, threat_model.md updated to integrate MIP-06 requirements; README.md updated with MIP-06 entry; no unrelated modifications present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mip-06-multi-device

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@alltheseas alltheseas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Comment thread 06.md Outdated
Comment thread 06.md Outdated
Comment thread 06.md
Comment thread 06.md Outdated
Comment thread 06.md Outdated
Comment thread 06.md Outdated
Comment thread 06.md
Comment thread threat_model.md Outdated
@vitorpamplona
Copy link
Copy Markdown

What can happen if one of the apps I am testing with real groups before switching to is compromised?

@futurepaul
Copy link
Copy Markdown

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.

@erskingardner
Copy link
Copy Markdown
Member Author

What can happen if one of the apps I am testing with real groups before switching to is compromised?

I'm not sure what you mean by this @vitorpamplona

@erskingardner
Copy link
Copy Markdown
Member Author

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.

@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:

  • One KP per client out there in the wild at a time.
  • No difficulty in cleaning up old key packages. Delete's are mostly respected but the fact that KPs and get rebroadcast to many other relays (outside your KP listed ones) means that you might not know all the places you have to send kind: 5s.

@vitorpamplona
Copy link
Copy Markdown

vitorpamplona commented Mar 17, 2026

What can happen if one of the apps I am testing with real groups before switching to is compromised?

I'm not sure what you mean by this @vitorpamplona

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.

@vitorpamplona
Copy link
Copy Markdown

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.

@erskingardner
Copy link
Copy Markdown
Member Author

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 - protected tags to keep these KPs from leaking out to many other relays but many/most relays don't actually support protected properly so KPs tend to wander and then the owner doesn't know where to send the delete requests and old KPs stick around for longer than necessary.

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)

@erskingardner
Copy link
Copy Markdown
Member Author

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.

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.

@vitorpamplona
Copy link
Copy Markdown

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.
Comment thread 06.md Outdated
Comment thread 06.md
@erskingardner erskingardner marked this pull request as ready for review March 27, 2026 12:20
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b992ca8 and e3731cd.

📒 Files selected for processing (6)
  • 00.md
  • 01.md
  • 03.md
  • 06.md
  • README.md
  • threat_model.md

Comment thread 00.md Outdated
Comment thread 06.md
Comment thread 06.md Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
00.md (1)

74-74: ⚠️ Potential issue | 🟡 Minor

Relays tag description should focus on Welcome delivery.

Line 74 frames relays as "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 relays tag ... 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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b3396e46-209f-401b-b248-883f0bd40780

📥 Commits

Reviewing files that changed from the base of the PR and between 8962b38 and a4b8b4d.

📒 Files selected for processing (2)
  • 00.md
  • 06.md

Comment thread 06.md
Comment thread 06.md Outdated
Comment thread 06.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread 06.md
Comment thread 06.md Outdated
Comment thread 06.md Outdated
Copy link
Copy Markdown
Contributor

@dannym-arx dannym-arx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Loading

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 → KeyNotFound
graph 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
Loading

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.

@erskingardner
Copy link
Copy Markdown
Member Author

@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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
06.md (1)

73-85: ⚠️ Potential issue | 🟠 Major

Resolve cross-spec authorization trigger mismatch with MIP-03.

06.md requires the full 3-part signaling gate before applying the MIP-06 carve-out, but 03.md:109-119 (context snippet) still says the carve-out applies when 0xF2F0 is present in GroupContext.extensions alone. 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

📥 Commits

Reviewing files that changed from the base of the PR and between a4b8b4d and 7110da6.

📒 Files selected for processing (2)
  • 06.md
  • threat_model.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • threat_model.md

@dannym-arx
Copy link
Copy Markdown
Contributor

@erskingardner great, I'll change my impl to use external PSKs on monday

@dannym-arx
Copy link
Copy Markdown
Contributor

Another day another problem, but this time I come bearing gifts in the form of minor spec changes

image

TL;DR: The identity proof challenge hashes serialized_LeafNode, but the LeafNode's HPKE key isn't generated until after authenticated_data must be set. Changing the challenge to hash credential_identity || signature_key instead fixes this. Both are known upfront, and it's a tighter binding anyway.


AI generated and human cleaned description:

The Problem

1. What the Challenge Hash Currently Binds To

The spec defines:

challenge = SHA-256("marmot-external-commit-v1" || serialized_LeafNode || serialized_GroupContext)

A TLS-serialized LeafNode contains six fields:

Field Identity-relevant? Known before External Commit is built?
credential (Nostr pubkey) Yes Yes, duh
signature_key (MLS signing pubkey) Yes Yes
capabilities No Yes
extensions No Yes
encryption_key (HPKE pubkey) No No, generated by compute_path() inside build()
leaf_node_source (parent hash) No No, computed from ratchet tree diff inside build()

The last two fields are generated deep inside OpenMLS during commit construction. Because the challenge includes the full serialized LeafNode, it cannot be computed until after build(), but authenticated_data must be set before build_group(), which runs earlier.

2. The Ordering Constraint

graph LR
    A["<b>with_aad(proof)</b><br/><i>AAD must be set HERE</i>"] --> B["<b>build_group()</b><br/>captures AAD in ExternalCommitInfo"]
    B --> C["<b>add_psk_proposal()</b><br/>load_psks()"]
    C --> D["<b>build()</b><br/>compute_path() creates LeafNode<br/>❌ too late to change AAD"]

    style A fill:#2d6a4f,stroke:#1b4332,color:#fff
    style B fill:#e9c46a,stroke:#e76f51,color:#1b4332
    style C fill:#e9c46a,stroke:#e76f51,color:#1b4332
    style D fill:#e76f51,stroke:#c1440e,color:#fff
Loading

ExternalCommitBuilder::with_aad() must be called before build_group() (captured at external_commits.rs:309), but compute_path() generates the HPKE key and parent hash inside build() (commit_builder.rs:606). There is no public API to set AAD between these two points.

3. Why We Don't Need the Full LeafNode

The identity proof's purpose is to prove the Nostr private key holder authorized this device's signing key to join this group at this epoch. The HPKE encryption key and parent hash are:

  • Identity-irrelevant; they're ephemeral tree-key material, not identity material
  • Already bound by MLS itself; confirmation_tag validation and the tree hash in GroupContext cover these fields

They create a circular dependency with the AAD ordering; the narrower binding is stronger: it targets exactly the identity-relevant fields.

4. The Fix

Change the challenge to:

challenge = SHA-256("marmot-external-commit-v1" || credential_identity || signature_key || serialized_GroupContext)

Where:

  • credential_identity = raw Nostr pubkey bytes from the BasicCredential (32 bytes)
  • signature_key = MLS signature public key bytes from the CredentialWithKey

Both are supplied by the application before build_group() is called. On the verification side, both are extracted from the joining LeafNode in the staged commit.

I've already implemented and tested this in the SDK. The full External Commit flow with identity proof construction, AAD, and cryptographic verification works end-to-end with no upstream OpenMLS changes.

5. Spec Lines to Update

Proof Construction (§Proof Construction, steps 1 and 3):

-1. Serialize the LeafNode from the External Commit's UpdatePath using TLS serialization.
+1. Extract `credential_identity` (raw Nostr pubkey bytes from the BasicCredential)
+   and `signature_key` (MLS signature public key bytes) from the `CredentialWithKey`
+   used to construct the External Commit. Both are known before the commit is built.

-3. Compute the challenge: `challenge = SHA-256(ASCII("marmot-external-commit-v1") || serialized_LeafNode || serialized_GroupContext)`.
+3. Compute the challenge: `challenge = SHA-256(ASCII("marmot-external-commit-v1") || credential_identity || signature_key || serialized_GroupContext)`.

Proof Verification (§Proof Verification, steps 3 and 5):

-3. Serialize the LeafNode from the External Commit's UpdatePath.
+3. Extract `credential_identity` (raw credential identity bytes) and `signature_key`
+   (signature public key bytes) from the LeafNode in the External Commit's UpdatePath.

-5. Compute `challenge = SHA-256(ASCII("marmot-external-commit-v1") || serialized_LeafNode || serialized_GroupContext)`.
+5. Compute `challenge = SHA-256(ASCII("marmot-external-commit-v1") || credential_identity || signature_key || serialized_GroupContext)`.

Security Properties (§Security Properties, "Identity binding"):

-- **Identity binding**: Proves the Nostr private key holder authorized this specific LeafNode to join this specific group at this specific epoch.
+- **Identity binding**: Proves the Nostr private key holder authorized this specific credential and signing key to join this specific group at this specific epoch.

Replay Protection (§Replay Protection):

-Nostr identity proof further binds to specific LeafNode + GroupContext.
+Nostr identity proof further binds to specific credential identity, signature key, and GroupContext.

…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.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 70cee25c-62a7-4788-b659-5b991db8c1ac

📥 Commits

Reviewing files that changed from the base of the PR and between 7110da6 and be63d35.

📒 Files selected for processing (1)
  • 06.md

Comment thread 06.md
Comment on lines +36 to +40
Version rules:

- Version `0` is reserved and MUST be rejected.
- Version `1` is the current format.
- Higher versions MUST be rejected unless explicitly supported.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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_device extensions 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-Device Support

6 participants