spec(chat): replace gift wrap with shared-key signed kind 14 events - #52
Conversation
The P2P chat used a simplified NIP-59 gift wrap whose outer event was signed with a random ephemeral key. That construction is vulnerable and buys no privacy. It buys nothing because the p tag already points at a per-order shared key, not at anyone's identity: an observer saw 'ephemeral -> shared' and now sees 'shared -> shared', with no trade key exposed either way. It is vulnerable because the shared pubkey travels in clear text in the p tag, so anyone scraping relays harvests the address of every active conversation, and the per-message ephemeral author makes attacker events indistinguishable from genuine ones until after the recipient has decrypted them. There is nothing cheap to filter on and no author to rate-limit, which allows an anonymous third party to exhaust a client until a trade deadline passes. Signing the outer event with a key derived from the shared secret makes producing a valid event require the shared private scalar, so a third party cannot publish into the conversation at all and clients filter it out at the relay. - Outer: kind 14, signed by K_sign, p-tagged to pub(K_conv), real timestamp (no NIP-59 tweaking, which would break since-based sync). - Inner: kind 1 signed by the sender's trade key — now the only sender authentication, since both parties hold K_sign. - HKDF domain separation into K_conv (encrypt) and K_sign (sign), so a dispute disclosure of K_conv alone is read-only: the solver reads the conversation but cannot write into it, and only the two parties' trade keys are accepted as inner signers. - Client security requirements: mandatory authors=[pub(K_sign)] filter, bounded backlog, cheapest-check-first validation order, replay protection via inner event id plus inner/outer timestamp binding, rate limiting, and the invariant that chat can never block the order state machine or a dispute. - Document the kind 14 disambiguation against protocol v2 traffic, and that relay storage for a signed kind 14 must be verified empirically. - Rewrite the Rust example accordingly; it compiles and runs against nostr-sdk 0.44, and the test vector carries the derived pubkeys it produces.
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe chat protocol replaces gift-wrapped transport with signed kind 14 events containing NIP-44-encrypted kind 1 events. It adds ECDH/HKDF key derivation, validation and replay protections, migration guidance, and updated Rust wrapping, unwrapping, and test-vector examples. ChangesChat Protocol v2
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Buyer
participant mostro_wrap
participant NIP44
participant Seller
Buyer->>mostro_wrap: Build inner kind 1 and derive keys
mostro_wrap->>NIP44: Encrypt inner event with K_conv
NIP44-->>mostro_wrap: Return encrypted content
mostro_wrap->>Seller: Send signed kind 14 event
Seller->>mostro_unwrap: Validate outer event
mostro_unwrap->>NIP44: Decrypt encrypted content
NIP44-->>mostro_unwrap: Return inner kind 1 event
mostro_unwrap-->>Seller: Return verified message
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat.md`:
- Around line 338-339: Enforce exactly one conversation `p` tag: at src/chat.md
lines 338-339, reject or filter caller-supplied `p` tags in the tag construction
before appending extras; at src/chat.md lines 369-377, make `mostro_unwrap`
validate that exactly one `p` tag exists and matches `pub(K_conv)` before
signature or decryption processing, rejecting missing, duplicate, or conflicting
tags.
- Around line 160-167: Revise the replay-protection claims in src/chat.md lines
160-167 to state that inner-event-ID deduplication, rather than timestamp skew
alone, rejects rewrapped events, and remove the claim that the checks close
replay protection completely unless the stated retention window supports it.
Update src/chat.md lines 390-395 to define durable deduplication-state retention
covering the relay replay window for offline delivery, or explicitly document
the limited protection window if bounded caches remain.
- Around line 360-377: Update mostro_unwrap’s ingress path to validate the
mandatory raw-event size limit before outer.verify() or nip44::decrypt(). Prefer
enforcing this in the raw-event parser boundary; otherwise make the
checked-boundary precondition explicit in mostro_unwrap and demonstrate callers
performing the validation first, while preserving the existing author and kind
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2bfeb651b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 8. **Inner pubkey** is the buyer's or the seller's trade key for this order — otherwise discard. No other signer is accepted, including a dispute solver. | ||
| 9. **Inner kind** is 1 — otherwise discard. | ||
| 10. **Inner event id** has not been seen before — otherwise discard. | ||
| 11. **Timestamps**: `|inner.created_at − outer.created_at|` is within tolerance (60 seconds is a reasonable default) — otherwise discard. |
There was a problem hiding this comment.
A malicious counterparty controls both signatures and can therefore set both timestamps to the same far-future value, satisfying this relative-skew check. Once the client persists that value as the last processed timestamp as required above, subsequent honest messages with real timestamps are excluded by its since filter, effectively disabling the conversation until that future time. Require an absolute wall-clock bound before accepting the event or advancing the subscription cursor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and the most serious of the round — fixed in 25d0296. The relative check compared the two timestamps against each other only, and a counterparty signs both, so dating them to the same far-future value satisfied it. Persisting that as the cursor then made the client's own subscription exclude every honest message, permanently and across restarts. Two fixes: reject any event dated further into the future than the clock-skew tolerance measured against the recipient's own clock (the past stays unbounded, since offline catch-up is legitimate), and clamp the persisted cursor to min(accepted_timestamp, local_now). Both are normative and implemented in the example.
| To communicate directly, both the buyer and the seller do not use the current `Message` scheme explained [here](https://mostro.network/protocol/overview.html), as this communication excludes the Mostro daemon. To preserve user privacy, we use a simplified version of NIP-59 that allows us to hide the metadata of both parties from outside observers. However, this variant only contains a single event inside the wrapper. The inner event includes the sender’s trade pubkey and the corresponding signature to maintain the authenticity of the sender. | ||
| To communicate directly, the buyer and the seller do not use the `Message` scheme explained [here](https://mostro.network/protocol/overview.html), because this communication excludes the Mostro daemon. | ||
|
|
||
| Messages are **not** gift wrapped. Each party publishes a **kind 14** event signed with a key derived from the ECDH secret shared by the two trade keys, carrying a NIP-44 encrypted **kind 1** event signed with the sender's trade key. No ephemeral keys are involved. |
There was a problem hiding this comment.
The repository's dispute-chat specification still says it uses the “same” peer-chat scheme, links to the now-removed chat.md#example anchor, and explicitly prescribes kind 1059 gift wraps in src/dispute_chat.md:44-67. After this line changes peer chat to signed kind 14 events, implementers following that cross-reference receive contradictory envelope and subscription requirements and can produce clients that cannot communicate with dispute solvers. Update the dispute-chat document for the new scheme or explicitly decouple it from this specification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and worse than a stale link — fixed in 25d0296. dispute_chat.md still prescribed kind 1059 gift wraps and subscribed by p tag alone, which is exactly the vulnerability this PR removes, so a client built from that cross-reference would have been both incompatible and exposed. It now uses the same envelope (kind 14 signed with K_sign, p-tagged to pub(K_conv), same HKDF derivation over the party-admin ECDH secret), subscribes by author, and inherits the client security requirements by reference. One substitution: the admin is an accepted inner signer there, since that channel is precisely how a solver talks to each party privately.
…rable dedup Addresses all five review findings. - Absolute timestamp bound (Codex P1). The relative inner/outer check was satisfiable by a counterparty who dates both events to the same far-future value, and the resulting cursor then filtered out every honest message that followed: a permanent, restart-surviving denial of service from one message. Reject events dated beyond the clock-skew tolerance into the future, and never advance the persisted cursor past the local clock. - The p tag is now normative (CodeRabbit). Producers emit exactly one, set to pub(K_conv), and callers may not supply their own; recipients reject anything else, checked before any crypto. Without this a party could send messages the counterparty sees normally but that are invisible to the #p query a dispute solver uses to rebuild the transcript. - Replay protection restated correctly (CodeRabbit). Inner-event-id dedup is what rejects re-wrapping, not the timestamp bound, which only limits how far back that dedup must reach. Dedup state on the inner id must therefore be durable rather than a bounded LRU, since an evicted entry makes its message replayable; the LRU applies only to the outer id as a cheap pre-decryption filter with no security role. - Size bound (CodeRabbit). The normative order requires it before expensive work but the example went straight to verification. The ingress precondition is now explicit, and the payload bound is re-checked before decrypting. - dispute_chat.md reconciled (Codex P2). It still specified kind 1059 gift wraps, subscribed by p tag alone, and linked to a chat.md anchor this PR removed, so implementers following the cross-reference would have built clients that cannot reach a solver. It now uses the same envelope and the same client requirements, with the admin as an accepted inner signer since that channel is how a solver talks to each party privately. The Rust example still compiles and runs against nostr-sdk 0.44 with the new checks in place.
1 participant
Summary
Replaces the simplified NIP-59 gift wrap used by the peer-to-peer chat with a kind 14 event signed by a key derived from the trade-key ECDH secret, carrying the same NIP-44 encrypted, trade-key-signed kind 1 inner event. No ephemeral keys are involved.
Why
The current construction signs the outer event with a random ephemeral key. That is both vulnerable and pointless here.
It buys no privacy. In standard NIP-59 the ephemeral key matters because the
ptag points at the recipient's real identity key. In this protocol theptag already points at a shared key that is anonymous and unique per order. An observer sawephemeral → shared; it now seesshared → shared. Neither form ever exposes a trade key, and both group events identically.It is vulnerable. The shared pubkey travels in clear text in the
ptag of every event, so anyone scraping relays harvests the address of every active conversation — no need to be a party to any trade. Because each event carries a fresh ephemeral author, attacker events are indistinguishable from genuine ones until the recipient has already downloaded and attempted to decrypt them, one ECDH each. There is nothing cheap to filter on and no author to rate-limit.That makes a cheap, anonymous, profitable attack possible: flood the shared pubkey of a trade waiting on a counterparty until its deadline passes. In Lightning mode the hold invoice expires and the funds return to the attacker's counterparty; in Cashu mode the P2P channel carries the settlement signature itself, so the same flood blocks the money directly.
Signing the outer event with the shared key removes it at the root: the shared pubkey stays observable, but producing a valid event requires the shared private scalar, obtainable only from one of the two trade private keys. A third party is cryptographically unable to publish into the conversation, and clients drop everything else at the relay with an
authorsfilter — at zero cost.What changed
K_sign,p-tagged topub(K_conv), with the real timestamp. NIP-59's timestamp tweaking is dropped: it would breaksince-based sync, and with no identity exposed there is nothing left for time analysis to correlate to.K_sign. Verifying it is a MUST.K_conv(encryption, and theptag) andK_sign(outer signature). This is what makes a dispute disclosure ofK_convalone read-only: the solver reads the whole conversation but cannot publish into it. Only the buyer's and seller's trade keys are accepted as inner signers, so the chat holds only the parties' own messages and a solver talks to each side privately instead.authors = [pub(K_sign)]filter, bounded backlog viasince+limit, a cheapest-check-first validation order, replay protection, rate limiting, and the isolation invariant that chat must never be able to block the order state machine or a dispute.K_sign, so either could re-publish the other's old inner event inside a fresh wrapper and it would verify. Binding the innercreated_atto the outer one closes the resent-inner case, and inner-event-id dedup closes the verbatim-resend case.Code example
Rewritten to implement this scheme end to end (
derive_chat_keys,mostro_wrap,mostro_unwrapwith every mandatory check).Verified, not just written: the example was extracted from this document into a crate and built and run against
nostr-sdk0.44. It compiles with no warnings, both sides derive the same key pair, and the round trip recovers the inner event signed by the sender's trade key. Three real bugs surfaced that way and were fixed (an ambiguoushkdfimport against the nostr prelude, an error type that does not implementStdError, and the deprecatedTimestamp::as_u64).That run also confirms the riskiest assumption in the spec — that NIP-44 self-encryption (
K_convon both sides of the exchange) is valid and deterministic. The test vector now carries the derived pubkeys it produced, so implementers can check their HKDF before debugging ciphertexts.Open items for reviewers
mostrod,mostro-cli, MostriX, and the mobile clients.🤖 Generated with Claude Code
https://claude.ai/code/session_01VQUKX88xRBCPsHQTXBEiYa
Summary by CodeRabbit
New Features
Documentation