MIP-06 multi-device support#251
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:
📝 WalkthroughWalkthroughAdds MIP‑06 multi‑device support behind a new Changes
Sequence DiagramsequenceDiagram
participant Device
participant MDK
participant MLS
participant Crypto
participant Proof
Device->>MDK: Request PairingPayload (group IDs)
MDK->>MLS: Export GroupInfo / GroupContext
MDK->>MDK: Derive join PSK & group_event_key
MDK-->>Device: Return PairingPayload
Device->>Crypto: Generate ephemeral X25519 keypair
Device->>Device: Build External Commit (uses join PSK)
Device->>Proof: Compute challenge from GroupContext
Device->>Proof: Sign challenge -> Nostr identity proof
Device->>Device: Embed proof into authenticated_data
Device-->>MDK: Send External Commit + authenticated_data
MDK->>Proof: Parse & verify identity proof from AAD
MDK->>MLS: Process staged external commit
MDK->>MDK: Validate joining identity matches existing member
MDK->>MLS: Merge staged commit
MDK->>MDK: Register join PSK for new epoch
MDK-->>Device: Confirm join
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
UniFFI Binding Coverage1 public method(s) not bound in UniFFI Coverage: 50/51 bindable methods (98.0%)
|
| Method | Source |
|---|---|
join_group_via_external_commit |
crates/mdk-core/src/groups.rs:1725 |
✅ Bound methods (50)
| Method | Source |
|---|---|
accept_welcome |
crates/mdk-core/src/welcomes.rs:315 |
add_device_to_groups |
crates/mdk-core/src/groups.rs:1584 |
add_members |
crates/mdk-core/src/groups.rs:901 |
build_pairing_payload |
crates/mdk-core/src/groups.rs:1675 |
clear_pending_commit |
crates/mdk-core/src/groups.rs:2323 |
coalesced_members |
crates/mdk-core/src/groups.rs:653 |
create_group |
crates/mdk-core/src/groups.rs:1346 |
create_group_with_multi_device |
crates/mdk-core/src/groups.rs:1372 |
create_key_package_for_event |
crates/mdk-core/src/key_packages.rs:88 |
create_key_package_for_event_with_options |
crates/mdk-core/src/key_packages.rs:127 |
create_message |
crates/mdk-core/src/messages/create.rs:110 |
decline_welcome |
crates/mdk-core/src/welcomes.rs:350 |
decrypt_group_image |
crates/mdk-core/src/extension/group_image.rs:229 |
delete_group |
crates/mdk-core/src/groups.rs:2302 |
delete_key_package_from_storage |
crates/mdk-core/src/key_packages.rs:731 |
delete_key_package_from_storage_by_hash_ref |
crates/mdk-core/src/key_packages.rs:746 |
delete_messages_for_group |
crates/mdk-core/src/groups.rs:2282 |
derive_upload_keypair |
crates/mdk-core/src/extension/group_image.rs:334 |
enable_multi_device |
crates/mdk-core/src/groups.rs:1525 |
get_group |
crates/mdk-core/src/groups.rs:588 |
get_groups |
crates/mdk-core/src/groups.rs:600 |
get_last_message |
crates/mdk-core/src/messages/mod.rs:289 |
get_members |
crates/mdk-core/src/groups.rs:634 |
get_message |
crates/mdk-core/src/messages/mod.rs:222 |
get_messages |
crates/mdk-core/src/messages/mod.rs:261 |
get_pending_welcomes |
crates/mdk-core/src/welcomes.rs:72 |
get_ratchet_tree_info |
crates/mdk-core/src/groups.rs:731 |
get_relays |
crates/mdk-core/src/groups.rs:1292 |
get_welcome |
crates/mdk-core/src/welcomes.rs:40 |
group_leaf_map |
crates/mdk-core/src/groups.rs:697 |
groups_needing_self_update |
crates/mdk-core/src/groups.rs:618 |
leave_group |
crates/mdk-core/src/groups.rs:2234 |
merge_pending_commit |
crates/mdk-core/src/groups.rs:2374 |
own_device_leaves |
crates/mdk-core/src/groups.rs:667 |
own_leaf_index |
crates/mdk-core/src/groups.rs:688 |
parse_key_package |
crates/mdk-core/src/key_packages.rs:330 |
pending_added_members_pubkeys |
crates/mdk-core/src/groups.rs:787 |
pending_member_changes |
crates/mdk-core/src/groups.rs:855 |
pending_removed_members_pubkeys |
crates/mdk-core/src/groups.rs:820 |
prepare_group_image_for_upload |
crates/mdk-core/src/extension/group_image.rs:416 |
prepare_group_image_for_upload_with_options |
crates/mdk-core/src/extension/group_image.rs:463 |
process_message |
crates/mdk-core/src/messages/process.rs:340 |
process_message_with_context |
crates/mdk-core/src/messages/process.rs:347 |
process_welcome |
crates/mdk-core/src/welcomes.rs:178 |
register_join_psk |
crates/mdk-core/src/groups.rs:1632 |
remove_members |
crates/mdk-core/src/groups.rs:1002 |
self_demote |
crates/mdk-core/src/groups.rs:2180 |
self_update |
crates/mdk-core/src/groups.rs:2010 |
sync_group_metadata_from_mls |
crates/mdk-core/src/groups.rs:2451 |
update_group_data |
crates/mdk-core/src/groups.rs:1226 |
➖ Intentionally excluded (5)
| Method | Reason |
|---|---|
builder |
Wrapped by new_mdk(), new_mdk_with_key(), new_mdk_unencrypted() free functions |
load_mls_group |
Only available behind debug-examples feature flag, not for production use |
media_manager |
Returns borrowed reference with lifetime — cannot cross FFI boundary |
migrate_group_image_v1_to_v2 |
Migration utility for internal use only, not needed in client bindings |
new |
Wrapped by new_mdk(), new_mdk_with_key(), new_mdk_unencrypted() free functions |
❌ Coverage: 93.7% → 93.1% (-0.60%) |
❌ Coverage: 93.68% → 93.04% (-0.64%) |
❌ Coverage: 93.68% → 92.91% (-0.77%) |
❌ Coverage: 93.68% → 92.99% (-0.69%) |
❌ Coverage: 93.68% → 93.02% (-0.66%) |
❌ Coverage: 93.68% → 92.99% (-0.69%) |
Not finished, but... getting there
dannym-arx
force-pushed
the
multi-device
branch
from
3bee12c to
03096ed
Compare
❌ Coverage: 93.68% → 93% (-0.68%) |
|
@CodeRabbit We need an extremely deep review here. make sure the PR is fully following the spec marmot-protocol/marmot#44 analyze every line of code, and if you find enough issues, you'll get a special golden carrot |
dannym-arx
changed the title
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainThe PR description image shows a charming pixel-art marmot sitting at a wooden desk in a cozy underground burrow, surrounded by three devices (phone, desktop computer, and tablet), all showing encrypted chat messages between "Alice" and "Bob." Glowing padlock icons float between the devices, illustrating the multi-device secure messaging concept. The burrow is lit by lanterns with scrolls and a map on the shelves — a fitting visual for MIP-06 multi-device support. Let me fetch the MIP-06 spec and deeply analyze all the PR code simultaneously. I need to read the actual spec and all the code. Let me fetch them now. 🏁 Script executed: Length of output: 718 🏁 Script executed: Length of output: 140872 🏁 Script executed: Length of output: 4069 🏁 Script executed: Length of output: 718
🔴 Critical Issues (Wire-Level Protocol Deviations)1. Wrong PSK Type: External PSK vs. Resumption PSKThe spec says (marmot-protocol/marmot#44):
The implementation uses External PSK throughout:
let psk = Psk::External(ExternalPsk::new(join_psk_id_bytes)); // ← WRONG TYPE
let psk = Psk::External(ExternalPsk::new(join_psk_id_bytes)); // ← WRONG TYPEThese are entirely different MLS structures. In RFC 9420, a Resumption PSK ( The spec also calls this value The implementation even documents the gap itself in // full PSK ID inspection is blocked by PreSharedKeyProposal::into_psk_id()
// being pub(crate) in OpenMLS.This means the validator cannot even verify the PSK type of the received proposal — it only checks the count. This leaves condition 5 of the External Commit authorization incompletely validated. 2. Fragile Raw-Byte Offset Hack for GroupContext Extraction
// MlsMessage header: version (2 bytes) + wire_format (2 bytes) = 4 bytes.
// GroupInfoTBS starts at offset 4, and its first field is GroupContext.
let (group_context, _) =
GroupContext::tls_deserialize_bytes(&gi_bytes[4..])...And again in let gc_payload = &gi_bytes[4..];
let (_, remainder) =
<GroupContext as tls_codec::DeserializeBytes>::tls_deserialize_bytes(gc_payload)...
let gc_bytes = &gc_payload[..gc_payload.len() - remainder.len()];The spec defines the PSK ID as Additionally, 🟠 High Severity Issues3.
|
| # | File | Severity | Issue |
|---|---|---|---|
| 1 | groups.rs, join_psk.rs, validation.rs |
🔴 Critical | PSK type is External PSK; spec mandates Resumption PSK (usage=application) |
| 2 | groups.rs |
🔴 Critical | GroupContext extracted via raw [4..] byte offset — fragile, not spec-mandated |
| 3 | groups.rs |
🟠 High | enable_multi_device missing admin authorization check |
| 4 | key_packages.rs |
🟠 High | Mixed-case hex test silently gutted — now tests lowercase only |
| 5 | mip06/mod.rs |
🟠 High | External Commit integration test bypasses process_message pipeline — validate_external_commit_* never called |
| 6 | mdk-uniffi/src/lib.rs |
🟠 High | Nostr secret key passed as unzeroized String through FFI |
| 7 | groups.rs |
🟠 High | Old-epoch PSKs never cleaned up — unbounded PSK store growth |
| 8 | groups.rs |
🟡 Medium | own_device_leaves loads MLS group twice |
| 9 | pairing.rs |
🟡 Medium | DevicePairingRequest::key_package_bytes() holds JSON Event, not KeyPackage bytes |
| 10 | mip06/mod.rs |
🟡 Medium | Module doc describes only Add-based flow; External Commit flow undocumented |
| 11 | extension.rs |
🟡 Medium | Condition 3 of signaling gate inferred, not verified per-leaf |
| 12 | groups.rs |
🟡 Medium | enable_multi_device leaves group in pending-commit limbo with no guard |
| 13 | pairing.rs |
🔵 Low | join_psk<1..255> doc vs. 4-byte length-prefix wire encoding |
| 14 | mip06/mod.rs |
🔵 Low | Integration test merge ordering comment unclear |
| 15 | pairing_crypto.rs |
🔵 Low | MIN_SIZE boundary test misleading |
The most important fix is Issue #1 — the External PSK vs. Resumption PSK type mismatch is a protocol-level incompatibility with the spec that will cause interop failures with any other spec-compliant MIP-06 client. Issue #2 (raw offset GroupContext extraction) is the most likely to cause silent failures in future OpenMLS upgrades. Issue #5 (test not covering the real validation path) means the spec-compliance of the new authorization code is effectively untested.
🧠 Learnings used
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T13:56:23.563Z
Learning: Ensure all code follows the Marmot Protocol specification exactly. Ask clarifying questions if any part of the spec is unclear
Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 205
File: crates/mdk-cbindings/src/welcomes.rs:64-70
Timestamp: 2026-03-04T20:41:29.623Z
Learning: In marmot-protocol/mdk, the security restriction on `mls_group_id` and `nostr_group_id` (and other sensitive identifiers) applies ONLY to side-channel leakage contexts: log messages (tracing macros), error strings, debug output (Debug trait impls), and panic messages. It does NOT apply to primary FFI/API return values (e.g., the Welcome struct returned via C bindings or UniFFI). These fields are essential application data that callers need to accept/decline welcomes and identify groups. Flagging their presence in API return types is a false positive.
Learnt from: jgmontoya
Repo: marmot-protocol/mdk PR: 223
File: crates/mdk-core/src/groups.rs:1640-1665
Timestamp: 2026-03-09T21:20:16.704Z
Learning: In marmot-protocol/mdk (crates/mdk-core/src/groups.rs), `add_members` and `remove_members` always produce immediate commits (not queued proposals), so the proposal store is effectively empty before any `update_group_data`/`update_group_context_extensions` call made via the public API. The concern that `prune_and_validate_admin_update` validates against stale (pre-proposal) membership is acknowledged as theoretically possible (only via a `leave_group` proposal received through `process_message`), but the MDK maintainers have decided the fix adds disproportionate complexity for a scenario that cannot arise through normal API usage. Do not re-flag this pattern.
Learnt from: jgmontoya
Repo: marmot-protocol/mdk PR: 250
File: crates/mdk-memory-storage/src/lib.rs:1051-1054
Timestamp: 2026-04-05T13:26:09.683Z
Learning: In marmot-protocol/mdk (crates/mdk-memory-storage/src/lib.rs), `delete_group` prunes `group_snapshots` after releasing the `inner` write lock, creating a theoretical race with `create_group_snapshot` and `rollback_group_to_snapshot`. The maintainers intentionally accept this: the race requires simultaneous `delete_group` and `process_message` on the same group, which cannot occur through normal API usage (user must leave the group before deleting it), and the worst-case outcome is a stale snapshot entry, not data corruption. Introducing a shared higher-level lock would require changing create_group_snapshot, rollback_group_to_snapshot, and release_group_snapshot — disproportionate complexity. Do not re-flag this pattern.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-core/src/groups.rs (1)
1356-1446:⚠️ Potential issue | 🟠 MajorRegister the initial join PSK during multi-device group creation.
register_join_psk()is only reached frommerge_pending_commit(), but this path either merges the initial add-members commit directly withmls_group.merge_pending_commit(&self.provider)or creates a single-member group with no pending commit at all. In both cases, a newly created MIP-06 group is left without the current epoch PSK registered locally, so the creator can't validate the first incoming External Commit for that group.💡 Proposed fix
self.storage() .replace_group_relays(&group.mls_group_id, config.relays.into_iter().collect()) .map_err(|e| Error::Group(e.to_string()))?; + #[cfg(feature = "mip06")] + if crate::mip06::is_multi_device_enabled(&mls_group) { + self.register_join_psk(&group.mls_group_id)?; + } + Ok(GroupResult { group, welcome_rumors, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 1356 - 1446, create_group_inner fails to register the initial join PSK: after you finish applying the initial add_members commit (i.e. after mls_group.merge_pending_commit(&self.provider)? when key_packages_vec was non-empty) and also in the single-member creation path (when no pending commit is created), call the existing register_join_psk(...) helper with the newly-created mls_group (or its group id/epoch as required by register_join_psk) before persisting the group and returning GroupResult so the creator has the current epoch PSK locally for validating the first incoming External Commit.
🧹 Nitpick comments (9)
crates/mdk-core/src/lib.rs (1)
432-446: Move the inneruseto module scope.
MULTI_DEVICE_EXTENSION_TYPEis imported inside the method, which goes against the repo’s Rust style rules. Pull it into the existing top-leveluse self::constant::{...}block instead.As per coding guidelines, "All
usestatements must be placed at the TOP of their containing scope, never inside functions, methods, blocks, or individual test functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/lib.rs` around lines 432 - 446, The inner use of MULTI_DEVICE_EXTENSION_TYPE inside the method required_capabilities_extension_with_multi_device violates the module-scope use rule; move the import into the module-level use block that already imports from self::constant (e.g., add MULTI_DEVICE_EXTENSION_TYPE to the existing top-level use self::constant::{...} list) and remove the in-method use so the method body only references MULTI_DEVICE_EXTENSION_TYPE directly.crates/mdk-core/src/mip06/join_psk.rs (1)
23-27: Avoid dual source-of-truth for the join-PSK label.
JOIN_PSK_LABELandJOIN_PSK_EXPORTER_LABELencode the same value in different forms; divergence would silently break interop. Add an invariant test to lock them together.🧪 Minimal safeguard test
#[cfg(test)] mod tests { use super::*; + #[test] + fn test_join_psk_label_constants_match() { + assert_eq!(JOIN_PSK_LABEL.as_slice(), JOIN_PSK_EXPORTER_LABEL.as_bytes()); + } #[test] fn test_join_psk_id_roundtrip() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/join_psk.rs` around lines 23 - 27, Add a unit test that enforces the invariant between JOIN_PSK_LABEL and JOIN_PSK_EXPORTER_LABEL so they cannot drift: write a test (e.g., join_psk_labels_invariant) that converts JOIN_PSK_EXPORTER_LABEL to bytes (or JOIN_PSK_LABEL to &str) and asserts equality with JOIN_PSK_LABEL, failing the build if they differ; place the test alongside other tests in the same module (crates::mdk_core::mip06::join_psk) so CI will catch any future divergence.crates/mdk-core/src/mip06/extension.rs (2)
54-56: Moveusestatement to the top of the file scope.The
use tls_codec::Serialize;insideas_extension()violates coding guidelines. Move it to the imports at the top of the file.♻️ Suggested refactor
Add to the imports at line 9:
use tls_codec::{DeserializeBytes, TlsDeserialize, TlsDeserializeBytes, TlsSerialize, TlsSize}; +use tls_codec::Serialize;Then remove line 55:
pub fn as_extension(&self) -> Result<Extension, Error> { - use tls_codec::Serialize; let mut bytes = Vec::new();Note: You may need to verify
Serializeisn't already covered byTlsSerializefor this use case.As per coding guidelines: "All
usestatements must be placed at the TOP of their containing scope."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/extension.rs` around lines 54 - 56, Move the inline use inside as_extension() to the top-level imports: remove the local `use tls_codec::Serialize;` from the body of the `as_extension` method and add `use tls_codec::Serialize;` alongside the other imports at the top of the file, ensuring you don't duplicate an existing import or conflict with `TlsSerialize`; then run a quick build to verify `Serialize` is the correct trait for the `as_extension` implementation.
140-151: Moveusestatement outside the test function.Per coding guidelines,
usestatements should be at the top of their containing scope. For tests, this means at the top of themod testsblock, not inside individual test functions.♻️ Suggested refactor
#[cfg(test)] mod tests { use super::*; + use tls_codec::Serialize; #[test] fn test_marmot_multi_device_roundtrip() { - use tls_codec::Serialize; - let ext = MarmotMultiDevice::new();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/extension.rs` around lines 140 - 151, The test function test_marmot_multi_device_roundtrip contains a local use of tls_codec::Serialize; move that use statement to the top of the tests module (or file-level scope that contains the tests) and remove it from inside the function so the trait import is available for all tests; update any other tests in the same module to rely on the module-level use and leave test_marmot_multi_device_roundtrip using MarmotMultiDevice and its tls_serialize/tls_deserialize_bytes as before.crates/mdk-core/src/messages/validation.rs (3)
440-440: Moveusestatement to the top of the file scope.Per coding guidelines, all
usestatements must be placed at the TOP of their containing scope, not inside functions. TheBasicCredentialimport should be added to the existing imports at the top of the file.♻️ Suggested refactor
Add to the imports at the top of the file (around line 7):
-use openmls::prelude::{BasicCredential, LeafNodeIndex, MlsGroup, Proposal, Sender, StagedCommit}; +use openmls::prelude::{BasicCredential, LeafNodeIndex, MlsGroup, Proposal, Sender, StagedCommit};Note:
BasicCredentialis already imported at line 7, so simply remove line 440:#[cfg(feature = "mip06")] fn validate_external_commit_authorization( &self, mls_group: &MlsGroup, staged_commit: &StagedCommit, ) -> Result<()> { - use openmls::prelude::BasicCredential; - // 1-3. MIP-06 signaling gate: extension present, required, at least one memberAs per coding guidelines: "All
usestatements must be placed at the TOP of their containing scope, never inside functions, methods, blocks, or individual test functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/validation.rs` at line 440, The file contains a local `use openmls::prelude::BasicCredential` inside a function; remove that inner `use` and ensure `BasicCredential` is imported with the other top-level imports (it’s already present near the top), so simply delete the `use openmls::prelude::BasicCredential;` line at its current location in messages/validation.rs and rely on the existing top-scope import to provide `BasicCredential`.
519-519: Remove redundantusestatement inside function.The
BasicCredentialimport on line 519 is redundant since it's already imported at the top of the file (line 7). This also violates the coding guideline againstusestatements inside functions.♻️ Suggested fix
#[cfg(feature = "mip06")] fn extract_joining_pubkey(&self, staged_commit: &StagedCommit) -> Result<nostr::PublicKey> { - use openmls::prelude::BasicCredential; - let joining_leaf = staged_commit.update_path_leaf_node().ok_or_else(|| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/validation.rs` at line 519, Remove the redundant inner import by deleting the `use openmls::prelude::BasicCredential;` statement found inside the function body and rely on the existing top-level `BasicCredential` import; ensure no other local `use` of the same symbol remains and run cargo check/rustfmt to confirm there are no unused-import or scoping issues.
542-543: Moveusestatements to the top of the file scope.The
usestatements insidevalidate_external_commit_identity_proofviolate coding guidelines. TheBasicCredentialis already imported at line 7. Themip06module imports should be added to the top-level imports (potentially with the#[cfg(feature = "mip06")]attribute).♻️ Suggested refactor
Add at the top of the file with feature gate:
#[cfg(feature = "mip06")] use crate::mip06::{NostrIdentityProof, verify_identity_proof};Then remove lines 542-543:
#[cfg(feature = "mip06")] pub(super) fn validate_external_commit_identity_proof( &self, mls_group: &MlsGroup, staged_commit: &StagedCommit, authenticated_data: &[u8], ) -> Result<()> { - use crate::mip06::{NostrIdentityProof, verify_identity_proof}; - use openmls::prelude::BasicCredential; - let proof = NostrIdentityProof::from_authenticated_data(authenticated_data)?;As per coding guidelines: "All
usestatements must be placed at the TOP of their containing scope, never inside functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/validation.rs` around lines 542 - 543, Move the function-local imports out to the module top: add a top-level, feature-gated import `#[cfg(feature = "mip06")] use crate::mip06::{NostrIdentityProof, verify_identity_proof};` (BasicCredential is already imported) and delete the `use` lines inside validate_external_commit_identity_proof; ensure the feature gate matches other mip06 imports in this file so compilation and visibility remain consistent.crates/mdk-core/src/mip06/pairing_crypto.rs (2)
207-215: Consider documenting the ephemeral nature more prominently.The function correctly generates a fresh X25519 keypair. The doc comment mentions the caller must securely delete the private key, which is good. Consider adding
#[must_use]to ensure the return value isn't accidentally ignored.♻️ Suggested enhancement
/// Generate a fresh X25519 keypair for the new device's pairing session. /// /// Returns `(private_key, public_key_bytes)`. /// The caller MUST securely delete the private key after the pairing session. +#[must_use] pub fn generate_new_device_keypair() -> (StaticSecret, [u8; 32]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing_crypto.rs` around lines 207 - 215, The function generate_new_device_keypair returns an ephemeral private key that must not be ignored; add the #[must_use] attribute to the generate_new_device_keypair function declaration so callers get a compiler warning if they discard the returned (StaticSecret, [u8; 32]) tuple, and keep the existing doc comment about securely deleting the private key to emphasize its ephemeral nature.
24-35: Consider whetherDebugderive onPairingMessageis appropriate.The
PairingMessagederivesDebugwhich will exposeexisting_ephemeral_pubkey,nonce, andciphertextin debug output. While these are not secret (ephemeral pubkey is public, nonce is transmitted in cleartext, ciphertext is encrypted), you may want to implement a customDebugthat redacts or truncates these fields to avoid accidentally verbose logging in production.This is a minor consideration since none of these fields are key material.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing_crypto.rs` around lines 24 - 35, PairingMessage currently derives Debug which will print full existing_ephemeral_pubkey, nonce, and ciphertext; replace the derive with a custom impl fmt::Debug for PairingMessage that redacts or truncates sensitive/verbose data (e.g., print hex of first N bytes or lengths like existing_ephemeral_pubkey: "[redacted]" or "len=32", nonce: "len=12", ciphertext: "len={}", or hex-prefix) to avoid verbose/logging of raw bytes; remove #[derive(Debug)] and implement std::fmt::Debug for the PairingMessage struct accordingly so other fields remain readable but raw byte arrays are not fully exposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 54: The x25519-dalek dependency was built with default-features = false
which removed the zeroize feature so StaticSecret instances in pairing_crypto.rs
(created around where StaticSecret is instantiated at lines ~125 and ~212) won't
be zeroed on drop; fix this by adding "zeroize" to the features list for
x25519-dalek in Cargo.toml (i.e., change features = ["static_secrets"] to
features = ["static_secrets", "zeroize"]) so StaticSecret will securely zero its
memory on drop.
In `@crates/mdk-core/Cargo.toml`:
- Line 19: Replace the current pattern that immediately converts the X25519
result to bytes and calls reject_zero_shared_secret with a contributory check
using x25519_dalek::SharedSecret::was_contributory() before proceeding to HKDF;
specifically, where you call
existing_secret.diffie_hellman(&new_pubkey).to_bytes() (the two call sites in
pairing_crypto.rs) keep the SharedSecret value, call .was_contributory() and
return an error if it is false, then only call .to_bytes() and feed that into
the HKDF/derive steps (and you can remove or keep reject_zero_shared_secret if
you still want the zero-check as redundant). Ensure both locations (the
diffie_hellman call sites around the current reject_zero_shared_secret usage)
are updated to perform this check.
In `@crates/mdk-core/src/groups.rs`:
- Around line 1506-1530: The loop in add_device_to_groups calls add_members and
abandons earlier successful results on later failure, which leaves pending
commits/rumors stranded; modify add_device_to_groups to either (A) perform a
two-phase approach: first validate/dry-run add_members for all group_ids (or
collect results without finalizing) and only after all succeed produce
GroupWelcomeData and return DevicePairingResponse, or (B) on encountering an
error, iterate over prior successful add_members results and call the
appropriate rollback/abort method (or a new helper like
discard_pending_commit/revert_commit) to clear staged commits before returning
Err; update add_device_to_groups to return partial info only if you also expose
a cleanup API, and ensure you reference add_device_to_groups, add_members,
DevicePairingResponse, GroupWelcomeData and any newly added rollback helper in
your changes.
- Around line 1635-1721: The code in join_group_via_external_commit deserializes
GroupInfo, creates a JoinPskId and stores the join_psk, then proceeds to build
an external commit without enforcing the MIP-06 entry validation; add upfront
validation after extracting vgi and gc_bytes (before creating/storing the PSK
and before calling MlsGroup::external_commit_builder) to reject malformed
pairing payloads: verify marmot_multi_device signaling is present and valid,
ensure required_capabilities includes 0xF2F0, ensure the ratchet_tree and
external_pub are present, ensure every ratchet-tree leaf advertises capability
0xF2F0, and verify join_psk.len() == the ciphersuite KDF output length (Nh) — if
any check fails return Error::PairingError and do not call PreSharedKeyId::new,
psk_id.store, or build_group().
- Around line 1562-1567: The tracing debug call in register_join_psk currently
logs a protected identifier by hex-encoding group_id; remove the group_id field
from the tracing::debug invocation in the register_join_psk code path and log
only non-linkable context (e.g., keep epoch = mls_group.epoch().as_u64() and the
message) or replace group_id with a non-identifying token; ensure the
tracing::debug target "mdk_core::groups::register_join_psk" no longer includes
hex::encode(group_id.as_slice()) to comply with the no-sensitive-identifiers
guideline.
- Around line 56-63: Remove the derived Debug on ExternalCommitResult and wrap
the sensitive group_event_key in the Secret<T> wrapper (e.g., Secret<[u8; 32]>)
or a struct that derives ZeroizeOnDrop so the key is zeroized on drop and not
Copy; then implement Debug manually for ExternalCommitResult to redact both
group_id and group_event_key (showing e.g. "<redacted>") while still exposing
commit_message if desired, and ensure the type does not implement Copy so
secrets cannot be accidentally duplicated; update uses of ExternalCommitResult's
group_event_key to call .expose_secret() only where strictly necessary.
- Around line 1459-1485: The enable_multi_device function is missing an admin
check and allows any member to change GroupContext.extensions and
required_capabilities; before proceeding (after loading mls_group and signer)
call the same admin gate used by update_group_data_extension — verify
is_leaf_node_admin(&mls_group, &signer) or equivalent and return an
Error::Unauthorized (or existing admin error) if false; ensure this check occurs
before constructing extensions or calling
required_capabilities_extension_with_multi_device() and before
update_group_context_extensions(&self.provider, ...).
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 846-848: Update the doc comment above the function that "Returns
the extension tag values that include all required extensions" to stop calling
0x0002 and 0x0003 "GREASE-like" and instead describe them as default/reserved
MLS extension IDs that OpenMLS injects; replace the phrase "GREASE-like values
(0x0002, 0x0003)" with wording such as "default MLS extension IDs injected by
OpenMLS (0x0002, 0x0003)" so the purpose and origin of these IDs are clear for
future maintainers.
In `@crates/mdk-core/src/mip06/device_name.rs`:
- Around line 117-119: The test function test_tls_roundtrip currently has
function-local imports (use tls_codec::{DeserializeBytes, Serialize}); move
these use declarations to the module scope (top of the containing file or test
module) so imports are not inside the function, e.g., add the use
tls_codec::{DeserializeBytes, Serialize} alongside other module-level uses and
remove the local uses from inside test_tls_roundtrip.
In `@crates/mdk-core/src/mip06/identity_proof.rs`:
- Around line 164-173: The verify_identity_proof helper currently only checks
that pubkey signed the challenge but does not ensure credential_identity
actually encodes that same signer; update verify_identity_proof to parse/derive
the expected signer public key from credential_identity (the identity encoding
used for BasicCredential) and assert it equals the pubkey parameter before
computing the challenge and calling verify_canonical_proof_event; if they differ
return an Error (binding NostrIdentityProof/Nostr public key to
credential_identity), keeping compute_challenge and verify_canonical_proof_event
usage unchanged otherwise.
- Around line 74-88: from_authenticated_data currently ignores the remainder
returned by tls_deserialize_bytes, allowing "valid_proof || junk"; change
from_authenticated_data to check that the _remainder is empty and return an
Error::IdentityProofError (e.g., "trailing bytes in authenticated_data" or
similar) if any bytes remain after tls_deserialize_bytes; keep the existing
version check (proof.version != Self::CURRENT_VERSION) and ensure the error type
and message match other IdentityProof errors for consistency.
In `@crates/mdk-core/src/mip06/pairing.rs`:
- Around line 86-105: The constructor new() currently only checks join_psk is
non-empty; enforce the fixed PSK length by validating join_psk.len() == 32
(KDF.Nh for SHA-256) and return Err(Error::PairingError("join_psk must be 32
bytes".to_string())) if not; update the validation inside new() alongside the
existing group_info check so malformed PSKs are rejected early (refer to the
join_psk parameter and the Error::PairingError variant when making the change).
- Around line 146-152: PairingPayload::new currently accepts an empty Vec which
violates the spec (groups must be length >=1); change new to enforce non-empty
groups by validating groups.len() > 0 and failing fast instead of constructing
an invalid payload: update PairingPayload::new to return a Result<Self,
PairingPayloadError> (or another existing error type) and return Err when
groups.is_empty(), or alternatively change the API to accept a non-empty
collection type and document the precondition; reference PairingPayload::new and
GroupPairingDataV1 when implementing the validation and error path.
- Around line 41-50: The from_bytes parser currently ignores the unread suffix
returned by Self::tls_deserialize_bytes, allowing trailing junk; modify the
from_bytes implementation (the call sites using Self::tls_deserialize_bytes
inside the from_bytes generated by the macro) to check the second tuple element
(remainder) and return an Error::PairingError (or appropriate parsing error)
when that remainder is not empty, before calling validate_pairing_version and
returning Ok(val), so that any trailing bytes cause a parse failure.
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 2205-2207: The deserialization error for PairingPayload is
currently mapped to MdkUniffiError::Mdk, which hides caller-side malformed
input; change the map_err in the PairingPayload::from_bytes call so it returns
MdkUniffiError::InvalidInput with a descriptive message (e.g. "failed to
deserialize pairing payload: {e}") instead of MdkUniffiError::Mdk, so the FFI
boundary classifies malformed payloads as InvalidInput.
- Around line 2120-2143: The binding add_device_to_groups currently returns
opaque TLS-serialized bytes so callers can't access per-group commit events;
change the UniFFI surface to return a structured Vec of entries (e.g., a
UniFFI-compatible struct) that includes the GroupId (or group id string), the
commit_event bytes (from GroupWelcomeData.commit_event) and the welcome/response
bytes for each group instead of a single opaque blob; update the function in
lib.rs (add_device_to_groups) to map mdk.add_device_to_groups() response into
that Vec of structs and return it (and add the new UniFFI struct type
definitions) so non-Rust clients can publish each commit_event and call
merge_pending_commit per group.
---
Outside diff comments:
In `@crates/mdk-core/src/groups.rs`:
- Around line 1356-1446: create_group_inner fails to register the initial join
PSK: after you finish applying the initial add_members commit (i.e. after
mls_group.merge_pending_commit(&self.provider)? when key_packages_vec was
non-empty) and also in the single-member creation path (when no pending commit
is created), call the existing register_join_psk(...) helper with the
newly-created mls_group (or its group id/epoch as required by register_join_psk)
before persisting the group and returning GroupResult so the creator has the
current epoch PSK locally for validating the first incoming External Commit.
---
Nitpick comments:
In `@crates/mdk-core/src/lib.rs`:
- Around line 432-446: The inner use of MULTI_DEVICE_EXTENSION_TYPE inside the
method required_capabilities_extension_with_multi_device violates the
module-scope use rule; move the import into the module-level use block that
already imports from self::constant (e.g., add MULTI_DEVICE_EXTENSION_TYPE to
the existing top-level use self::constant::{...} list) and remove the in-method
use so the method body only references MULTI_DEVICE_EXTENSION_TYPE directly.
In `@crates/mdk-core/src/messages/validation.rs`:
- Line 440: The file contains a local `use openmls::prelude::BasicCredential`
inside a function; remove that inner `use` and ensure `BasicCredential` is
imported with the other top-level imports (it’s already present near the top),
so simply delete the `use openmls::prelude::BasicCredential;` line at its
current location in messages/validation.rs and rely on the existing top-scope
import to provide `BasicCredential`.
- Line 519: Remove the redundant inner import by deleting the `use
openmls::prelude::BasicCredential;` statement found inside the function body and
rely on the existing top-level `BasicCredential` import; ensure no other local
`use` of the same symbol remains and run cargo check/rustfmt to confirm there
are no unused-import or scoping issues.
- Around line 542-543: Move the function-local imports out to the module top:
add a top-level, feature-gated import `#[cfg(feature = "mip06")] use
crate::mip06::{NostrIdentityProof, verify_identity_proof};` (BasicCredential is
already imported) and delete the `use` lines inside
validate_external_commit_identity_proof; ensure the feature gate matches other
mip06 imports in this file so compilation and visibility remain consistent.
In `@crates/mdk-core/src/mip06/extension.rs`:
- Around line 54-56: Move the inline use inside as_extension() to the top-level
imports: remove the local `use tls_codec::Serialize;` from the body of the
`as_extension` method and add `use tls_codec::Serialize;` alongside the other
imports at the top of the file, ensuring you don't duplicate an existing import
or conflict with `TlsSerialize`; then run a quick build to verify `Serialize` is
the correct trait for the `as_extension` implementation.
- Around line 140-151: The test function test_marmot_multi_device_roundtrip
contains a local use of tls_codec::Serialize; move that use statement to the top
of the tests module (or file-level scope that contains the tests) and remove it
from inside the function so the trait import is available for all tests; update
any other tests in the same module to rely on the module-level use and leave
test_marmot_multi_device_roundtrip using MarmotMultiDevice and its
tls_serialize/tls_deserialize_bytes as before.
In `@crates/mdk-core/src/mip06/join_psk.rs`:
- Around line 23-27: Add a unit test that enforces the invariant between
JOIN_PSK_LABEL and JOIN_PSK_EXPORTER_LABEL so they cannot drift: write a test
(e.g., join_psk_labels_invariant) that converts JOIN_PSK_EXPORTER_LABEL to bytes
(or JOIN_PSK_LABEL to &str) and asserts equality with JOIN_PSK_LABEL, failing
the build if they differ; place the test alongside other tests in the same
module (crates::mdk_core::mip06::join_psk) so CI will catch any future
divergence.
In `@crates/mdk-core/src/mip06/pairing_crypto.rs`:
- Around line 207-215: The function generate_new_device_keypair returns an
ephemeral private key that must not be ignored; add the #[must_use] attribute to
the generate_new_device_keypair function declaration so callers get a compiler
warning if they discard the returned (StaticSecret, [u8; 32]) tuple, and keep
the existing doc comment about securely deleting the private key to emphasize
its ephemeral nature.
- Around line 24-35: PairingMessage currently derives Debug which will print
full existing_ephemeral_pubkey, nonce, and ciphertext; replace the derive with a
custom impl fmt::Debug for PairingMessage that redacts or truncates
sensitive/verbose data (e.g., print hex of first N bytes or lengths like
existing_ephemeral_pubkey: "[redacted]" or "len=32", nonce: "len=12",
ciphertext: "len={}", or hex-prefix) to avoid verbose/logging of raw bytes;
remove #[derive(Debug)] and implement std::fmt::Debug for the PairingMessage
struct accordingly so other fields remain readable but raw byte arrays are not
fully exposed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1eae65f1-2f36-474c-92ea-7cfe2a45fb32
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/mdk-core/CHANGELOG.mdcrates/mdk-core/Cargo.tomlcrates/mdk-core/src/constant.rscrates/mdk-core/src/error.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/lib.rscrates/mdk-core/src/messages/commit.rscrates/mdk-core/src/messages/process.rscrates/mdk-core/src/messages/validation.rscrates/mdk-core/src/mip06/device_name.rscrates/mdk-core/src/mip06/extension.rscrates/mdk-core/src/mip06/identity_proof.rscrates/mdk-core/src/mip06/join_psk.rscrates/mdk-core/src/mip06/mod.rscrates/mdk-core/src/mip06/pairing.rscrates/mdk-core/src/mip06/pairing_crypto.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/Cargo.tomlcrates/mdk-uniffi/src/lib.rs
You have lost your chance at a golden carrot by looking at a wrong commit in marmot#44. This issue WAS raised in that pr and addressed, external PSKs are correct.
RFC 9420 defines MLSMessage as version(u16) + wire_format(u16) + body; the 4-byte offset is the MLS wire format, not an OpenMLS internal. This will not change unless the MLS spec changes
yep real issues, here's five carrots 🥕 🥕 🥕 🥕 🥕 for your contribution
can't zeroize across FFI boundaries. that's not how memory works
not sure what to do about this one, thoughts @erskingardner @jgmontoya @mubarakcoded ? the rest is all minor stuff |
|
Tip For best results, initiate chat on the files or code changes.
On On On On 🥕🥕🥕🥕🥕 Glad the five confirmed issues were useful — I'll keep digging! ✏️ Learnings added
🧠 Learnings used |
❌ Coverage: 93.68% → 93.16% (-0.52%) |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
crates/mdk-core/src/mip06/pairing.rs (3)
41-50:⚠️ Potential issue | 🟠 MajorReject trailing bytes in the versioned pairing parsers.
from_bytes()discards the unread suffix fromtls_deserialize_bytes(), sovalid_payload || junkis still accepted for every generated pairing type. These control messages should fail closed once any trailing bytes remain.Based on learnings, "Ensure all code follows the Marmot Protocol specification exactly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing.rs` around lines 41 - 50, from_bytes currently ignores the remainder from Self::tls_deserialize_bytes allowing trailing junk; update from_bytes (the function named from_bytes) to reject any non-empty remainder returned by tls_deserialize_bytes by returning an Error::PairingError (or another appropriate error) when the second element (remainder) is non-empty, then proceed to call validate_pairing_version on val.version and return Ok(val) only when remainder is empty; ensure the error uses the same formatting style as existing PairingError messages.
86-105:⚠️ Potential issue | 🟠 MajorReject malformed
join_psklengths at construction time.The docs say
join_pskisKDF.Nh, butnew()only checks non-empty. In this repository the ciphersuite is fixed to SHA-256, so malformed pairing data should be rejected here unlessjoin_psk.len() == 32.As per coding guidelines, "Ciphersuite: MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing.rs` around lines 86 - 105, The constructor new() currently only checks that join_psk is non-empty but must reject malformed lengths since the ciphersuite uses SHA-256 (KDF.Nh == 32); update new() to enforce join_psk.len() == 32 and return Err(Error::PairingError("join_psk must be 32 bytes".to_string())) when that check fails (keep existing group_info non-empty check and the group_event_key fixed-size array), referencing the new() function and Error::PairingError when adding this explicit length validation for join_psk according to the MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519 ciphersuite.
146-152:⚠️ Potential issue | 🟠 MajorDo not serialize empty
PairingPayloads.The TLS shape here is
groups<1..2^32-1>, butPairingPayload::new()acceptsVec::new().export_groups_pairing_payload()incrates/mdk-core/src/groups.rs:1606-1653returnsPairingPayload::new(groups)directly, so an empty input list can still produce a spec-invalid payload.Based on learnings, "Ensure all code follows the Marmot Protocol specification exactly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing.rs` around lines 146 - 152, PairingPayload::new currently allows an empty groups Vec which violates the TLS shape groups<1..2^32-1>; change the API to validate and reject empty input: either make PairingPayload::new return a Result<Self, Error> (e.g., Result<PairingPayload, PairingPayloadError>) and return an Err when groups.is_empty(), or keep the signature but add a clear panic/assert (not preferred) and update callers; specifically update export_groups_pairing_payload to handle the new error (do not construct PairingPayload on an empty groups list) so no empty PairingPayload is ever serialized.crates/mdk-core/src/groups.rs (4)
1594-1599:⚠️ Potential issue | 🔴 CriticalDo not log
group_idinregister_join_psk.This log leaks a protected identifier.
As per coding guidelines "No sensitive identifiers (mls_group_id, nostr_group_id, GroupId, encryption keys, exporter secrets) in tracing macro calls (info!, warn!, error!, debug!, trace!)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 1594 - 1599, In register_join_psk, the tracing::debug call is logging the sensitive group_id; remove the group_id = %hex::encode(...) field (or replace it with a non-identifying placeholder such as "<redacted>") so the tracing::debug invocation no longer emits the raw GroupId; keep the epoch logging and message unchanged to preserve context.
1705-1710:⚠️ Potential issue | 🟠 MajorValidate
join_psklength before storing/using it.This path accepts
pairing_data.join_psk()without enforcing the required KDF output length for the selected ciphersuite. Reject malformed payloads before PSK registration.✅ Minimal guard
+ let expected_nh = ciphersuite.hash_length(); + if pairing_data.join_psk().len() != expected_nh { + return Err(Error::PairingError( + "invalid join_psk length for ciphersuite".to_string(), + )); + } + let psk = Psk::External(ExternalPsk::new(join_psk_id_bytes));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 1705 - 1710, Validate the length of pairing_data.join_psk() against the required KDF output size for the selected ciphersuite before creating the ExternalPsk or calling PreSharedKeyId::new; if the length is incorrect, return an appropriate Error::PairingError. Specifically, check the expected PSK length using the ciphersuite/KDF parameters, then reject malformed inputs prior to calling ExternalPsk::new(join_psk_id_bytes) and psk_id.store(&self.provider, pairing_data.join_psk()) to avoid registering invalid PSKs.
54-64:⚠️ Potential issue | 🔴 CriticalRedact
ExternalCommitResultand protectgroup_event_keyas secret material.
#[derive(Debug)]here can exposegroup_idand raw key bytes. Alsogroup_event_keyshould not be stored as plain[u8; 32]without secret handling/zeroization.🔐 Proposed fix
-#[derive(Debug)] pub struct ExternalCommitResult { @@ - pub group_id: GroupId, + pub group_id: GroupId, @@ - pub group_event_key: [u8; 32], + pub group_event_key: mdk_storage_traits::Secret<[u8; 32]>, } + +impl std::fmt::Debug for ExternalCommitResult { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ExternalCommitResult") + .field("commit_message", &self.commit_message) + .field("group_id", &"[REDACTED]") + .field("group_event_key", &"[REDACTED]") + .finish() + } +}As per coding guidelines "Never log, expose, or include in Debug implementations: encryption keys, MLS exporter secrets,
mls_group_id, ornostr_group_idin log messages, error messages, debug output, panic messages, or user-facing error descriptions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 54 - 64, The ExternalCommitResult struct currently derives Debug and stores group_event_key as a plain [u8; 32], which can leak secrets and group identifiers; remove #[derive(Debug)] from ExternalCommitResult, replace the raw group_event_key field with a secret wrapper type that zeroizes on drop (e.g., a secrecy::Secret or a custom type implementing zeroize::Zeroize), ensure group_id is not printed in any Debug implementation, and implement a custom Debug for ExternalCommitResult that redacts group_event_key and group_id (e.g., show "<redacted>") so no raw key bytes or sensitive IDs are exposed.
1547-1560:⚠️ Potential issue | 🟠 MajorPartial failure can strand earlier groups with pending commits.
If one later group fails, earlier successful
add_members()calls already staged pending commits but this method returnsErrand drops recovery context.🧯 Suggested rollback pattern
let mut group_welcomes = Vec::with_capacity(group_ids.len()); + let mut staged_groups: Vec<GroupId> = Vec::new(); for group_id in group_ids { - let result = self.add_members(group_id, std::slice::from_ref(key_package_event))?; + let result = match self.add_members(group_id, std::slice::from_ref(key_package_event)) { + Ok(r) => r, + Err(e) => { + for staged_group_id in staged_groups.iter().rev() { + let _ = self.clear_pending_commit(staged_group_id); + } + return Err(e); + } + }; + staged_groups.push(group_id.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 1547 - 1560, The loop calls self.add_members(...) for multiple group_ids and if a later iteration returns Err it leaves earlier groups with staged pending commits; modify the code to track successful group_ids/results as you loop and on any error run a rollback/cleanup for each previously-successful group (e.g., call the group-level API that cancels or discards the staged commit such as a revert_pending_commit/cancel_pending_commit method on the group or on self), ensure rollbacks/errors are best-effort (log/collect but do not mask the original add_members error) and then return the original Err; reference self.add_members, group_ids, group_welcomes and GroupWelcomeData when locating the change.
🧹 Nitpick comments (3)
crates/mdk-core/src/messages/validation.rs (2)
435-543: Move theseusestatements to module scope.The local imports here violate the repo’s Rust style rule, and the
BasicCredentialones are redundant because it is already imported on Line 7. As per coding guidelines "Allusestatements must be placed at the TOP of their containing scope, never inside functions, methods, blocks, or individual test functions".Also applies to: 2315-2315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/validation.rs` around lines 435 - 543, The local `use` declarations inside validate_external_commit_authorization, extract_joining_pubkey, and validate_external_commit_identity_proof should be moved to module scope: remove the in-function `use openmls::prelude::BasicCredential;` lines (they are redundant since BasicCredential is already imported at the top) and add the MIP-06 imports `use crate::mip06::{NostrIdentityProof, verify_identity_proof};` and any remaining needed `use` items at the top of the file so all `use` statements live at module scope rather than inside those functions.
2306-2382: Add one true end-to-end assertion for the production path.This test proves the helper validators accept a legitimate External Commit, but it bypasses the new
process_message/process_commitwiring by callingmls_group.process_message(...)and the helpers directly. A companion test that feeds the actual emitted message through the MDK message-processing entrypoint would catch regressions inauthenticated_dataforwarding andSender::NewMemberCommitdispatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/validation.rs` around lines 2306 - 2382, Add a companion end-to-end test that sends the serialized External Commit through the MDK message-processing entrypoint instead of calling mls_group.process_message(...) and helper validators directly: build the pairing payload and create the External Commit as before, then pass commit_result.commit_message into the device's normal incoming-message handler (the MDK entrypoint method on the device struct that receives/dispatches messages, e.g., the Device::handle_incoming_message / process_incoming_message equivalent) so the MDK wiring runs (authenticated_data forwarding and Sender::NewMemberCommit dispatch), and finally assert the production outcome (e.g., that the device merged the staged commit or that load_mls_group reflects the new member / merge_pending_commit succeeded) to confirm the full path works. Ensure the test references alice_device1, alice_device2, commit_result.commit_message and uses the device entrypoint method to process the message and then checks merge_staged_commit/merge_pending_commit or group membership as the final assertion.crates/mdk-core/src/mip06/pairing_crypto.rs (1)
29-30: Prefer role-neutral naming for the wire field and helper docs.
existing_ephemeral_pubkeyand the direction-specific doc comments imply this helper is only “existing device → new device”, but the primary caller incrates/mdk-core/src/mip06/mod.rs:85-127reuses the same primitive in both pairing phases. Renaming towardsender_ephemeral_pubkey/recipient_pairing_pubkeywould make the protocol flow much easier to audit.Also applies to: 81-82, 109-117, 171-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing_crypto.rs` around lines 29 - 30, Rename the direction-specific field and docs to be role-neutral: change the struct field existing_ephemeral_pubkey to sender_ephemeral_pubkey (or recipient_pairing_pubkey if you prefer) and update all occurrences and helper doc comments in pairing_crypto.rs (including the places referenced around the other blocks) to describe the value as "the sender's ephemeral X25519 public key" (or "the pairing peer's ephemeral public key") without implying a fixed device direction; ensure all uses in functions such as the pairing helper that read/write existing_ephemeral_pubkey are updated to the new identifier and that the docstrings explain the field is reused in both pairing phases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/mdk-core/src/groups.rs`:
- Line 470: There are function-local `use` statements (e.g., `use
tls_codec::{DeserializeBytes as _, Serialize as _};` at line ~470 and similar
occurrences at lines ~1334, 1484, 1543, 1575, 1618-1619, 1672) that violate the
style rule; move each of these `use` imports out of their function/block into
the top of the module scope (the top of crates::mdk-core::groups.rs) and remove
the local `use` lines so the referenced traits/types remain available to the
functions (ensure names such as DeserializeBytes and Serialize remain imported
at module scope).
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 846-856: The test fixture valid_extensions_tag_values() is
incorrectly including GREASE/default extension IDs (e.g., "0x0002","0x0003")
that create_key_package_for_event() and crates/mdk-core/src/constant.rs
explicitly exclude from emitted TAG_EXTENSIONS; update
valid_extensions_tag_values() to remove those GREASE IDs (and any other
default/GREASE IDs referenced in constant.rs) and ensure both the non-mip06 and
mip06 variants only list the actual extension tag hex strings that
create_key_package_for_event() advertises so the fixture matches the producer's
emitted mls_extensions precisely.
In `@crates/mdk-core/src/messages/validation.rs`:
- Line 706: Remove the unused import of tls_codec::Deserialize (the line `use
tls_codec::Deserialize as _`) from the test module in validation.rs to satisfy
Clippy; locate the `use tls_codec::Deserialize as _` statement in the tests
(search for the symbol `Deserialize`) and delete it so the unused-import warning
is cleared and CI can pass.
In `@crates/mdk-core/src/mip06/pairing_crypto.rs`:
- Around line 128-142: Replace OsRng usage with getrandom-based byte fills:
instead of StaticSecret::random_from_rng(OsRng) create a 32-byte array, call
getrandom::getrandom/getrandom::fill to populate it, then construct the X25519
StaticSecret from those bytes (so existing_secret originates from the filled
byte array); similarly, replace OsRng.fill_bytes(&mut nonce_bytes) by
getrandom::fill(&mut nonce_bytes) to generate the 12-byte nonce. Apply the same
pattern in generate_new_device_keypair() where a secret/nonce is generated so
all key and nonce material is produced via getrandom before constructing
StaticSecret/X25519PublicKey or using the nonce_bytes; keep existing calls to
reject_zero_shared_secret and derive_key unchanged.
- Around line 176-180: The decrypt_pairing_message API and key-generation should
use Secret<T> for private keys and derive the public key inside decryption:
change generate_new_device_keypair() to return Secret<StaticSecret> (so the
private key zeroizes on drop), update decrypt_pairing_message to accept a
Secret<StaticSecret> new_ephemeral_privkey (remove the new_ephemeral_pubkey
parameter) and compute the corresponding public key internally (e.g., via
x25519_dalek::PublicKey::from(&priv)). Update all callers to pass the
Secret-wrapped private key and remove manual public-key handling; ensure types
match Secret<T> wrappers used elsewhere in the repo.
In `@crates/mdk-core/src/mip06/pairing.rs`:
- Around line 69-81: GroupPairingDataV1 currently stores sensitive bytes in
plain fields and derives Debug, which can leak secrets; change group_event_key
and join_psk to be wrapped in a Secret<T>-style wrapper (e.g., Secret<[u8;32]>
and Secret<Vec<u8>>), add ZeroizeOnDrop/Zeroize to the struct (or derive
ZeroizeOnDrop) so secrets are zeroed when dropped, remove the derived Debug and
implement a custom Debug that redacts those fields (showing only placeholders),
and update any serializers/deserializers or accessors (e.g., constructors,
getters) for GroupPairingDataV1 to unwrap/wrap the Secret types safely when
needed.
---
Duplicate comments:
In `@crates/mdk-core/src/groups.rs`:
- Around line 1594-1599: In register_join_psk, the tracing::debug call is
logging the sensitive group_id; remove the group_id = %hex::encode(...) field
(or replace it with a non-identifying placeholder such as "<redacted>") so the
tracing::debug invocation no longer emits the raw GroupId; keep the epoch
logging and message unchanged to preserve context.
- Around line 1705-1710: Validate the length of pairing_data.join_psk() against
the required KDF output size for the selected ciphersuite before creating the
ExternalPsk or calling PreSharedKeyId::new; if the length is incorrect, return
an appropriate Error::PairingError. Specifically, check the expected PSK length
using the ciphersuite/KDF parameters, then reject malformed inputs prior to
calling ExternalPsk::new(join_psk_id_bytes) and psk_id.store(&self.provider,
pairing_data.join_psk()) to avoid registering invalid PSKs.
- Around line 54-64: The ExternalCommitResult struct currently derives Debug and
stores group_event_key as a plain [u8; 32], which can leak secrets and group
identifiers; remove #[derive(Debug)] from ExternalCommitResult, replace the raw
group_event_key field with a secret wrapper type that zeroizes on drop (e.g., a
secrecy::Secret or a custom type implementing zeroize::Zeroize), ensure group_id
is not printed in any Debug implementation, and implement a custom Debug for
ExternalCommitResult that redacts group_event_key and group_id (e.g., show
"<redacted>") so no raw key bytes or sensitive IDs are exposed.
- Around line 1547-1560: The loop calls self.add_members(...) for multiple
group_ids and if a later iteration returns Err it leaves earlier groups with
staged pending commits; modify the code to track successful group_ids/results as
you loop and on any error run a rollback/cleanup for each previously-successful
group (e.g., call the group-level API that cancels or discards the staged commit
such as a revert_pending_commit/cancel_pending_commit method on the group or on
self), ensure rollbacks/errors are best-effort (log/collect but do not mask the
original add_members error) and then return the original Err; reference
self.add_members, group_ids, group_welcomes and GroupWelcomeData when locating
the change.
In `@crates/mdk-core/src/mip06/pairing.rs`:
- Around line 41-50: from_bytes currently ignores the remainder from
Self::tls_deserialize_bytes allowing trailing junk; update from_bytes (the
function named from_bytes) to reject any non-empty remainder returned by
tls_deserialize_bytes by returning an Error::PairingError (or another
appropriate error) when the second element (remainder) is non-empty, then
proceed to call validate_pairing_version on val.version and return Ok(val) only
when remainder is empty; ensure the error uses the same formatting style as
existing PairingError messages.
- Around line 86-105: The constructor new() currently only checks that join_psk
is non-empty but must reject malformed lengths since the ciphersuite uses
SHA-256 (KDF.Nh == 32); update new() to enforce join_psk.len() == 32 and return
Err(Error::PairingError("join_psk must be 32 bytes".to_string())) when that
check fails (keep existing group_info non-empty check and the group_event_key
fixed-size array), referencing the new() function and Error::PairingError when
adding this explicit length validation for join_psk according to the
MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519 ciphersuite.
- Around line 146-152: PairingPayload::new currently allows an empty groups Vec
which violates the TLS shape groups<1..2^32-1>; change the API to validate and
reject empty input: either make PairingPayload::new return a Result<Self, Error>
(e.g., Result<PairingPayload, PairingPayloadError>) and return an Err when
groups.is_empty(), or keep the signature but add a clear panic/assert (not
preferred) and update callers; specifically update export_groups_pairing_payload
to handle the new error (do not construct PairingPayload on an empty groups
list) so no empty PairingPayload is ever serialized.
---
Nitpick comments:
In `@crates/mdk-core/src/messages/validation.rs`:
- Around line 435-543: The local `use` declarations inside
validate_external_commit_authorization, extract_joining_pubkey, and
validate_external_commit_identity_proof should be moved to module scope: remove
the in-function `use openmls::prelude::BasicCredential;` lines (they are
redundant since BasicCredential is already imported at the top) and add the
MIP-06 imports `use crate::mip06::{NostrIdentityProof, verify_identity_proof};`
and any remaining needed `use` items at the top of the file so all `use`
statements live at module scope rather than inside those functions.
- Around line 2306-2382: Add a companion end-to-end test that sends the
serialized External Commit through the MDK message-processing entrypoint instead
of calling mls_group.process_message(...) and helper validators directly: build
the pairing payload and create the External Commit as before, then pass
commit_result.commit_message into the device's normal incoming-message handler
(the MDK entrypoint method on the device struct that receives/dispatches
messages, e.g., the Device::handle_incoming_message / process_incoming_message
equivalent) so the MDK wiring runs (authenticated_data forwarding and
Sender::NewMemberCommit dispatch), and finally assert the production outcome
(e.g., that the device merged the staged commit or that load_mls_group reflects
the new member / merge_pending_commit succeeded) to confirm the full path works.
Ensure the test references alice_device1, alice_device2,
commit_result.commit_message and uses the device entrypoint method to process
the message and then checks merge_staged_commit/merge_pending_commit or group
membership as the final assertion.
In `@crates/mdk-core/src/mip06/pairing_crypto.rs`:
- Around line 29-30: Rename the direction-specific field and docs to be
role-neutral: change the struct field existing_ephemeral_pubkey to
sender_ephemeral_pubkey (or recipient_pairing_pubkey if you prefer) and update
all occurrences and helper doc comments in pairing_crypto.rs (including the
places referenced around the other blocks) to describe the value as "the
sender's ephemeral X25519 public key" (or "the pairing peer's ephemeral public
key") without implying a fixed device direction; ensure all uses in functions
such as the pairing helper that read/write existing_ephemeral_pubkey are updated
to the new identifier and that the docstrings explain the field is reused in
both pairing phases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2b87b7b-0cd5-4e7f-8464-7994b1cd6c76
📒 Files selected for processing (7)
crates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/messages/validation.rscrates/mdk-core/src/mip06/extension.rscrates/mdk-core/src/mip06/mod.rscrates/mdk-core/src/mip06/pairing.rscrates/mdk-core/src/mip06/pairing_crypto.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/src/mip06/mod.rs
❌ Coverage: 93.68% → 93.16% (-0.52%) |
|
Tip For best results, initiate chat on the files or code changes. The PR description image shows a charming pixel-art marmot in a cozy underground burrow, sitting at a wooden desk and working on three devices — a phone (left), a desktop monitor (center), and a tablet (right). Each device shows a chat interface with messages like "Alice: Hey! Check this key!", "Bob: Secure chat!", and "Alice: Secure chat!". Golden padlock icons are connected by glowing cyan dotted lines between the devices, illustrating the multi-device encrypted key exchange concept. The burrow features warm orange cave walls with glowing veins, a hanging lantern, wooden shelves with scrolls, a world map, and a mug — a fitting thematic backdrop for MIP-06.
Issue The core challenge is: after
This ensures at most one stale PSK entry ever exists per group (the one from the epoch just committed), and it's cleaned up deterministically on the next successful merge. No timer, no background task, no unbounded growth. Pseudo-sketch: // In register_join_psk, before storing new PSK:
if let Some(prev_psk_id) = self.load_prev_join_psk_id(group_id)? {
prev_psk_id.delete(&self.provider); // best-effort; log on failure
}
// Store new PSK, then save current psk_id as the new "prev" for the next epoch
self.save_prev_join_psk_id(group_id, &psk_id)?;
psk_id.store(&self.provider, &join_psk_bytes)?;The tradeoff: requires one extra field in the persisted group record (32 + 24 = 56 bytes per group) and a storage migration. If that's too invasive for now, a simpler fallback is to key the PSK store by Happy to dig into whichever direction the team prefers. 🐾 🧠 Learnings used |
❌ Coverage: 93.68% → 93.11% (-0.57%) |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
crates/mdk-core/src/mip06/pairing.rs (1)
72-84:⚠️ Potential issue | 🟠 MajorRedact pairing secrets instead of deriving
Debug.
GroupPairingDataV1storesgroup_event_keyandjoin_pskas plain bytes and derivesDebug;PairingPayloadthen inherits that leak surface through its own derivedDebug. Any accidental debug print of either type will expose live pairing secrets. These fields should follow the repo’sSecret<T>+ redactedDebugpattern.As per coding guidelines, "Use
Secret<T>wrapper for sensitive values, deriveZeroizeOnDropfor types containing key material" and "Never log, expose, or include in Debug implementations: encryption keys, MLS exporter secrets,mls_group_id, ornostr_group_idin log messages, error messages, debug output, panic messages, or user-facing error descriptions."Also applies to: 133-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing.rs` around lines 72 - 84, Replace plaintext secret fields in GroupPairingDataV1 with the repo's Secret<T> wrapper and add zeroization: change group_event_key: [u8; 32] to group_event_key: Secret<[u8; 32]> and join_psk: Vec<u8> to join_psk: Secret<Vec<u8>>; add/derive ZeroizeOnDrop (and keep TlsSerialize/TlsDeserialize as needed) so key material is zeroed on drop, and rely on Secret<T>'s redacted Debug so derived Debug on GroupPairingDataV1 and any container types like PairingPayload no longer leak secrets; apply the same conversion to the other pairing struct referenced in the comment (the block around lines 133-140) to ensure all pairing secrets use Secret<T> + ZeroizeOnDrop and are redacted from Debug output.crates/mdk-core/src/mip06/pairing_crypto.rs (1)
8-11:⚠️ Potential issue | 🟠 MajorUse
getrandomfor the X25519 secret and nonce paths.This file still generates the X25519 secret and AEAD nonces with
OsRngat Lines 128, 148, and 225. The repo’s crypto rules requiregetrandomfor key generation/nonces, so MIP-06 is still off-pattern in its most security-sensitive path.Run this to verify no
OsRng-based generation remains after the fix:#!/bin/bash rg -n 'OsRng|random_from_rng|fill_bytes' crates/mdk-core/src/mip06/pairing_crypto.rsAs per coding guidelines, "For key generation, MUST use
getrandomfor all key generation (CSPRNG), never use weak RNGs."Also applies to: 128-148, 224-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/pairing_crypto.rs` around lines 8 - 11, Replace all uses of OsRng-based generation for X25519 secrets and AEAD nonces with getrandom-based filling: where StaticSecret is currently constructed from OsRng (X25519 secret creation) and where nonce bytes are produced with OsRng (AEAD nonce generation), allocate the appropriate fixed-size byte arrays, call getrandom::getrandom(...) to fill them, then create the X25519 StaticSecret from the secret byte array (and the AEAD Nonce from the nonce byte array). Ensure you zeroize the secret byte buffer after constructing the StaticSecret and remove any remaining references to OsRng, random_from_rng, or fill_bytes in pairing_crypto.rs so key/nonce generation uses getrandom only.
🧹 Nitpick comments (1)
crates/mdk-core/src/mip06/mod.rs (1)
90-123: This integration test doesn’t exercise the public MIP-06 path end-to-end.Phase 1/2 pairing messages are round-tripped locally with the same ephemeral keypair, and Lines 490-496 feed the External Commit straight into OpenMLS instead of
MDK::process_message(). That leaves both the actual device-to-device transport contract and the public validation path outside the “full flow” scenario this test name suggests. Either drive the exchange through the public MDK API or split this into narrower tests with more precise names.As per coding guidelines, "For this PR’s cryptographic code changes, ensure any new code paths have unit/integration tests for both success and error cases (including feature-gated
mip06paths)."Also applies to: 438-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/mod.rs` around lines 90 - 123, The test is bypassing the public MDK API and using local round-trips (encrypt_pairing_message / decrypt_pairing_message and directly feeding an OpenMLS External Commit) so it does not exercise the public MIP-06 path; update the test to drive the pairing exchange through the MDK public methods (e.g., create DevicePairingRequest via DevicePairingRequest::new, send it into MDK::process_message on the recipient side instead of calling decrypt_pairing_message directly, and likewise send DevicePairingResponse back through MDK::process_message) or split the current test into two focused tests (one unit test for encrypt_pairing_message/decrypt_pairing_message and one integration test that exercises add_device_to_groups and MDK::process_message end-to-end), ensuring both success and error cases for the mip06 feature are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/mdk-core/src/groups.rs`:
- Around line 1901-1926: The finalized MLS group is never persisted into MDK
storage, causing later sync_group_metadata_from_mls()->get_group(...) to fail
with GroupNotFound; after finalize(...) and before returning
ExternalCommitResult in the external-commit path (the block that creates
mls_group, commit_bundle, commit_msg and group_id), construct and persist the
corresponding group_types::Group row and any required relay metadata using the
provider.storage() (use the same mdk_storage_traits::GroupId derived from
mls_group.group_id() and the group_event_key from pairing_data), and ensure the
stored entry includes the necessary fields so
merge_pending_commit()/sync_group_metadata_from_mls() can find the group.
- Around line 1629-1647: register_join_psk stores a new persistent join PSK
every epoch without deleting older or failed-attempt PSKs, causing leaks; update
register_join_psk (and the similar code paths that call Psk::External /
PreSharedKeyId::new and .store, e.g., join_group_via_external_commit and the
other noted blocks) to: (1) before storing a new PSK, attempt to find and delete
any existing/superseded PSK records for this group/id (use the same
PreSharedKeyId/identifier mechanism used to store them), and (2) if storing
succeeds but subsequent operations fail (or if you store preemptively in
join_group_via_external_commit), ensure you delete the newly stored PSK in the
error path/rollback so failed pairing attempts don’t leave records behind; use
the same provider-backed delete/remove API that complements
PreSharedKeyId::store to perform these deletions and propagate sensible
PairingError messages on failures.
- Around line 1557-1563: enable_multi_device() currently builds a commit_event
but never records it, so call the same tracking used elsewhere before returning:
after creating serialized_commit/commit_event, invoke
self.track_processed_message(...) to record the enablement commit (use the same
group identifier and the commit message/serialized bytes as other
commit-producing paths) so the local client treats its own relay echo as
already-processed, then return Ok(commit_event).
In `@crates/mdk-core/src/mip06/pairing.rs`:
- Around line 41-53: from_bytes currently only checks TLS deserialization,
version, and trailing bytes but skips semantic validation; after deserializing
(the result bound to val) call its validate() method (e.g.,
PairingPayload::validate or GroupPairingDataV1::validate) and propagate
validation errors as Error::PairingError to ensure empty groups, malformed
join_psk lengths, or empty group_info are rejected; update the from_bytes
implementations (the functions named from_bytes around the shown diff and the
similar ones at the other affected blocks) to invoke validate() after
validate_pairing_version(val.version)? and before Ok(val), mapping any
validation failure into the same PairingError path.
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 2227-2237: The loop currently calls
mdk.join_group_via_external_commit for each group and returns early on the first
error, discarding prior successes; change the function to emit per-group
outcomes instead of failing fast: iterate payload.groups(), call
mdk.join_group_via_external_commit for each, and push either an
ExternalCommitJoinResult on success or the corresponding error into a
Vec<Result<ExternalCommitJoinResult, /* appropriate error type, e.g. MdkError
*/>> (reference mdk.join_group_via_external_commit, ExternalCommitJoinResult,
payload.groups(), results); update the function’s return type on the FFI
boundary to return this per-group Result vector (or alternatively change the API
to accept a single-group payload) so callers can see which groups succeeded and
which failed without losing prior successes.
---
Duplicate comments:
In `@crates/mdk-core/src/mip06/pairing_crypto.rs`:
- Around line 8-11: Replace all uses of OsRng-based generation for X25519
secrets and AEAD nonces with getrandom-based filling: where StaticSecret is
currently constructed from OsRng (X25519 secret creation) and where nonce bytes
are produced with OsRng (AEAD nonce generation), allocate the appropriate
fixed-size byte arrays, call getrandom::getrandom(...) to fill them, then create
the X25519 StaticSecret from the secret byte array (and the AEAD Nonce from the
nonce byte array). Ensure you zeroize the secret byte buffer after constructing
the StaticSecret and remove any remaining references to OsRng, random_from_rng,
or fill_bytes in pairing_crypto.rs so key/nonce generation uses getrandom only.
In `@crates/mdk-core/src/mip06/pairing.rs`:
- Around line 72-84: Replace plaintext secret fields in GroupPairingDataV1 with
the repo's Secret<T> wrapper and add zeroization: change group_event_key: [u8;
32] to group_event_key: Secret<[u8; 32]> and join_psk: Vec<u8> to join_psk:
Secret<Vec<u8>>; add/derive ZeroizeOnDrop (and keep TlsSerialize/TlsDeserialize
as needed) so key material is zeroed on drop, and rely on Secret<T>'s redacted
Debug so derived Debug on GroupPairingDataV1 and any container types like
PairingPayload no longer leak secrets; apply the same conversion to the other
pairing struct referenced in the comment (the block around lines 133-140) to
ensure all pairing secrets use Secret<T> + ZeroizeOnDrop and are redacted from
Debug output.
---
Nitpick comments:
In `@crates/mdk-core/src/mip06/mod.rs`:
- Around line 90-123: The test is bypassing the public MDK API and using local
round-trips (encrypt_pairing_message / decrypt_pairing_message and directly
feeding an OpenMLS External Commit) so it does not exercise the public MIP-06
path; update the test to drive the pairing exchange through the MDK public
methods (e.g., create DevicePairingRequest via DevicePairingRequest::new, send
it into MDK::process_message on the recipient side instead of calling
decrypt_pairing_message directly, and likewise send DevicePairingResponse back
through MDK::process_message) or split the current test into two focused tests
(one unit test for encrypt_pairing_message/decrypt_pairing_message and one
integration test that exercises add_device_to_groups and MDK::process_message
end-to-end), ensuring both success and error cases for the mip06 feature are
covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acbe4a2d-c57f-4f1b-b6dc-af70284c4da8
📒 Files selected for processing (9)
Cargo.tomlcrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/mip06/device_name.rscrates/mdk-core/src/mip06/identity_proof.rscrates/mdk-core/src/mip06/mod.rscrates/mdk-core/src/mip06/pairing.rscrates/mdk-core/src/mip06/pairing_crypto.rscrates/mdk-uniffi/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- crates/mdk-core/src/key_packages.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/src/mip06/identity_proof.rs
❌ Coverage: 93.68% → 93.07% (-0.61%) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/mdk-core/src/groups.rs (1)
1639-1654:⚠️ Potential issue | 🟠 MajorJoin-PSK entries can still accumulate across epochs.
Current cleanup deletes only the current deterministic ID before re-store. Because the ID changes with
GroupContext, older epoch PSKs remain unless separately tracked/deleted, which can lead to unbounded PSK-store growth in long-lived groups.Also applies to: 1867-1880
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 1639 - 1654, The code deletes only the freshly-derived join PSK ID (join_psk_id_bytes) so PSKs from earlier epochs still accumulate; modify the flow so you persist and rotate the group's previous join-PSK IDs and delete them before storing the new one: update derive_join_psk_with_id/where you create Psk::External and call PreSharedKeyId::new to read any stored prior PSK id(s) for this group (e.g., a last_join_psk_id entry keyed by the group identity), call provider.storage().delete_psk(...) for each prior id, then overwrite the stored last_join_psk_id with the new join_psk_id_bytes and proceed to call psk_id.store(...); apply the same change to the other occurrence at the referenced block (lines ~1867-1880) so old epoch PSKs are removed on re-registration.
🧹 Nitpick comments (5)
crates/mdk-core/src/mip06/mod.rs (5)
97-123: Add a wrong-recipient / tamper case for the pairing channel.This only proves the happy path. It would not catch a regression where
encrypt_pairing_message/decrypt_pairing_messagestops binding ciphertext to the intended device or stops authenticating it. A different private key and a modified ciphertext should both fail here.As per coding guidelines, "Use only authenticated encryption modes: ChaCha20-Poly1305 or AES-GCM. Never use unauthenticated encryption (AES-CBC alone, etc.)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/mod.rs` around lines 97 - 123, Add negative tests to ensure pairing messages are authenticated and bound to the intended recipient: after generating (new_priv, new_pub) via generate_new_device_keypair and producing encrypted ciphertext with encrypt_pairing_message, create a different keypair and assert that decrypt_pairing_message with the wrong private key returns an error (do not unwrap), and separately mutate a byte in the encrypted payload and assert decrypt_pairing_message also errors; do the same for both DevicePairingRequest and DevicePairingResponse paths to catch both wrong-recipient and tampering regressions.
205-243: Add a mismatchedcredential_identityfailure case.This test shows the proof breaks with the wrong signer key, but it would still pass if
verify_identity_proofstopped binding the proof toBasicCredential.identity, which is the impersonation boundary here.Test sketch
// Wrong key fails let other = Keys::generate(); assert!( super::identity_proof::verify_identity_proof( &decoded, &other.public_key(), &credential_identity, b"signature-key", b"group-context", ) .is_err() ); + + // Wrong credential identity also fails + let wrong_identity = Keys::generate().public_key().to_bytes(); + assert!( + super::identity_proof::verify_identity_proof( + &decoded, + &keys.public_key(), + &wrong_identity, + b"signature-key", + b"group-context", + ) + .is_err() + );As per coding guidelines, "Credential identity MUST match event signer (prevents impersonation)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/mod.rs` around lines 205 - 243, The test currently checks that a proof fails with the wrong signer key but doesn't assert the proof is bound to BasicCredential.identity; update test_identity_proof_standalone to also generate a different credential_identity (e.g., from other.public_key().to_bytes()), then call super::identity_proof::verify_identity_proof(&decoded, &keys.public_key(), &other_credential_identity, b"signature-key", b"group-context") and assert it returns Err; this ensures construct_identity_proof/verify_identity_proof bind the proof to the provided credential_identity (use existing variables Keys::generate(), credential_identity, other, decoded, and the same signature/group context symbols).
295-320: Please cover the unauthorized path forenable_multi_device.This is the only regression coverage here for a security-sensitive state transition, and it exercises only the admin happy path. Please add a non-admin case that asserts both rejection and that no pending commit is left behind after the failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/mod.rs` around lines 295 - 320, Add a new test that covers the unauthorized path for enable_multi_device: create a group with Alice as admin, then attempt to call enable_multi_device using a non-admin identity (e.g., Bob's Keys/MDK instance or a second user), assert that enable_multi_device returns an Err indicating lack of authorization, assert that merge_pending_commit either is not created or returns an appropriate error (or calling merge_pending_commit after the failed enable returns Ok but does not change state), and finally reload the MLS group via load_mls_group and assert super::is_multi_device_enabled(&mls_group) is still false and that no pending commit remains; reference the existing test flow in test_enable_multi_device_on_existing_group and reuse functions create_group, enable_multi_device, merge_pending_commit, load_mls_group, and is_multi_device_enabled to implement the checks.
463-473: Add a tampered join-PSK rejection case.The happy-path External Commit test proves a valid payload works, but it will not catch a regression where
join_group_via_external_commitignores or mis-bindsgroup_data.join_psk(). Flipping one byte in the PSK should make the join fail before merge.Test sketch
let group_data = &payload.groups()[0]; + let mut bad_join_psk = group_data.join_psk().to_vec(); + bad_join_psk[0] ^= 0x01; + let tampered = super::GroupPairingDataV1::new( + *group_data.group_event_key(), + bad_join_psk, + group_data.group_info().to_vec(), + ) + .unwrap(); + assert!(alice_device2 + .join_group_via_external_commit(&tampered, &alice_keys) + .is_err()); + let commit_result = alice_device2 .join_group_via_external_commit(group_data, &alice_keys) .expect("External Commit should succeed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/mod.rs` around lines 463 - 473, Add a negative test that flips a byte in the PSK from the pairing payload and asserts join_group_via_external_commit fails instead of succeeding; locate the existing happy-path sequence using build_pairing_payload, payload.groups()[0], and join_group_via_external_commit, clone group_data, mutate group_data.join_psk() (e.g., to_vec and flip one byte) so the PSK is tampered, then call alice_device2.join_group_via_external_commit with the tampered group data and assert it returns an Err (or fails before performing merge) rather than producing a commit_result with the original group_id.
281-284: Prefer structured assertions in these rejection tests.Line 377 only checks
is_err(), and Lines 283 and 392 pin the tests toDisplaytext. Matching the concretecrate::Errorvariant/cause will make these failures much harder to satisfy for the wrong reason.Also applies to: 377-378, 390-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/mip06/mod.rs` around lines 281 - 284, Replace fragile string-based and is_err checks in the tests with structured pattern matching against the concrete crate::Error variants: change assertions that test is_err() to assert!(matches!(result, Err(crate::Error::SomeVariant(_)))) and replace assertions that inspect Display text (like matching msg.contains("ratchet_tree")) with a match on Err(crate::Error::PairingError(_)) or the exact variant/inner type expected; if you need to inspect fields, match the inner value (e.g., Err(crate::Error::PairingError(ref detail))) and assert on those fields explicitly rather than on Display output. This applies to the current assertions that use is_err() and the ones referencing PairingError message text—use assert!(matches!(...)) or if let Err(crate::Error::PairingError(detail)) = result { assert_eq!(detail.some_field, expected) } to make the checks structural and robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/mdk-core/src/groups.rs`:
- Line 1922: Run rustfmt (cargo fmt --all) to fix formatting around the call
that currently ends with "&signer,"; the CI error indicates formatting mismatch
in the invocation containing the &signer argument, so reformat the file (e.g.,
adjust trailing commas/spacing in the function call where &signer is passed) and
commit the resulting changes so cargo fmt --all -- --check passes.
---
Duplicate comments:
In `@crates/mdk-core/src/groups.rs`:
- Around line 1639-1654: The code deletes only the freshly-derived join PSK ID
(join_psk_id_bytes) so PSKs from earlier epochs still accumulate; modify the
flow so you persist and rotate the group's previous join-PSK IDs and delete them
before storing the new one: update derive_join_psk_with_id/where you create
Psk::External and call PreSharedKeyId::new to read any stored prior PSK id(s)
for this group (e.g., a last_join_psk_id entry keyed by the group identity),
call provider.storage().delete_psk(...) for each prior id, then overwrite the
stored last_join_psk_id with the new join_psk_id_bytes and proceed to call
psk_id.store(...); apply the same change to the other occurrence at the
referenced block (lines ~1867-1880) so old epoch PSKs are removed on
re-registration.
---
Nitpick comments:
In `@crates/mdk-core/src/mip06/mod.rs`:
- Around line 97-123: Add negative tests to ensure pairing messages are
authenticated and bound to the intended recipient: after generating (new_priv,
new_pub) via generate_new_device_keypair and producing encrypted ciphertext with
encrypt_pairing_message, create a different keypair and assert that
decrypt_pairing_message with the wrong private key returns an error (do not
unwrap), and separately mutate a byte in the encrypted payload and assert
decrypt_pairing_message also errors; do the same for both DevicePairingRequest
and DevicePairingResponse paths to catch both wrong-recipient and tampering
regressions.
- Around line 205-243: The test currently checks that a proof fails with the
wrong signer key but doesn't assert the proof is bound to
BasicCredential.identity; update test_identity_proof_standalone to also generate
a different credential_identity (e.g., from other.public_key().to_bytes()), then
call super::identity_proof::verify_identity_proof(&decoded, &keys.public_key(),
&other_credential_identity, b"signature-key", b"group-context") and assert it
returns Err; this ensures construct_identity_proof/verify_identity_proof bind
the proof to the provided credential_identity (use existing variables
Keys::generate(), credential_identity, other, decoded, and the same
signature/group context symbols).
- Around line 295-320: Add a new test that covers the unauthorized path for
enable_multi_device: create a group with Alice as admin, then attempt to call
enable_multi_device using a non-admin identity (e.g., Bob's Keys/MDK instance or
a second user), assert that enable_multi_device returns an Err indicating lack
of authorization, assert that merge_pending_commit either is not created or
returns an appropriate error (or calling merge_pending_commit after the failed
enable returns Ok but does not change state), and finally reload the MLS group
via load_mls_group and assert super::is_multi_device_enabled(&mls_group) is
still false and that no pending commit remains; reference the existing test flow
in test_enable_multi_device_on_existing_group and reuse functions create_group,
enable_multi_device, merge_pending_commit, load_mls_group, and
is_multi_device_enabled to implement the checks.
- Around line 463-473: Add a negative test that flips a byte in the PSK from the
pairing payload and asserts join_group_via_external_commit fails instead of
succeeding; locate the existing happy-path sequence using build_pairing_payload,
payload.groups()[0], and join_group_via_external_commit, clone group_data,
mutate group_data.join_psk() (e.g., to_vec and flip one byte) so the PSK is
tampered, then call alice_device2.join_group_via_external_commit with the
tampered group data and assert it returns an Err (or fails before performing
merge) rather than producing a commit_result with the original group_id.
- Around line 281-284: Replace fragile string-based and is_err checks in the
tests with structured pattern matching against the concrete crate::Error
variants: change assertions that test is_err() to assert!(matches!(result,
Err(crate::Error::SomeVariant(_)))) and replace assertions that inspect Display
text (like matching msg.contains("ratchet_tree")) with a match on
Err(crate::Error::PairingError(_)) or the exact variant/inner type expected; if
you need to inspect fields, match the inner value (e.g.,
Err(crate::Error::PairingError(ref detail))) and assert on those fields
explicitly rather than on Display output. This applies to the current assertions
that use is_err() and the ones referencing PairingError message text—use
assert!(matches!(...)) or if let Err(crate::Error::PairingError(detail)) =
result { assert_eq!(detail.some_field, expected) } to make the checks structural
and robust.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5756d6e0-2a36-46c5-8c81-dfa6658bca75
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/mdk-core/Cargo.tomlcrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/mip06/mod.rscrates/mdk-core/src/mip06/pairing.rscrates/mdk-core/src/mip06/pairing_crypto.rscrates/mdk-uniffi/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- crates/mdk-core/src/key_packages.rs
- crates/mdk-core/src/mip06/pairing.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/mdk-core/Cargo.toml
- crates/mdk-core/src/mip06/pairing_crypto.rs
- crates/mdk-uniffi/src/lib.rs
❌ Coverage: 93.68% → 93.06% (-0.62%) |
❌ Coverage: 93.68% → 93.05% (-0.63%) |
❌ Coverage: 93.68% → 93.28% (-0.40%) |
❌ Coverage: 93.68% → 93.22% (-0.46%) |
✅ Coverage: 93.68% → 93.79% (+0.11%) |
1 participant
WIP implementation of MIP-06: multi-device support for Marmot groups.
One marmot, many burrow entrances. This adds the protocol machinery for a single Nostr identity to participate in MLS groups from multiple devices at once.
What's here
mip06feature flag gating all multi-device code behindx25519-dalekmarmot_multi_device(0xF2F0) for GroupContext anddevice_name(0xF2EF) for per-leaf metadatacreate_group_with_multi_device,enable_multi_device,add_device_to_groupscoalesced_membersandown_device_leavesviews that group leaf indices by Nostr pubkeycreate_messageandbuild_message_eventsignatures (removed unusedEventTagparameter)remove_groupfromEpochSnapshotManager(dead code)This WIP implements MIP-06 multi-device support so a single Nostr identity can participate in MLS groups from multiple independent devices, adding X25519-based ephemeral pairing, encrypted pairing messages, deterministic join-PSK derivation, and Nostr identity proofs that cryptographically bind device MLS credentials to an owner’s Nostr keypair. It is feature-gated behind a new
mip06Cargo feature and exposes core APIs and UniFFI bindings to create/enable multi-device groups, perform device pairing (add- or External-Commit-based), and manage per-device leaves and PSKs.What changed:
mip06, and wiredmip06through mdk-core and mdk-uniffi (crates affected: mdk-core, mdk-uniffi, tests/helpers touched; storage-traits, mdk-memory-storage, and mdk-sqlite-storage had minor cleanup noted).mip06feature that exports multi-group and per-group outcome types.Security impact:
Protocol changes:
API surface:
mip06feature and exports the multi-device APIs with input validation/conversion at the FFI boundary.Testing:
Notable observations: