reya/coop
1
0
Fork 1
Code Issues Pull Requests 1 Releases 3 Activity

add plan

Browse Source
This commit is contained in:
reya
2026-10-04 12:43:14 +07:00
parent 045151fe5c
commit e34f325164
1 changed files with 111 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/nip4e-audit.md
+111
View File
@@ -0,0 +1,111 @@
# NIP-4e Audit Report
**Scope**: `crates/nip4e/src/lib.rs` (key announcement/sync), the send path (`crates/chat/src/room.rs`), the receive path (`crates/chat/src/lib.rs`), UI integration (`crates/workspace`), compared against the [NIP-4e draft](https://github.com/nostr-protocol/nips/blob/per-device-keys/4e.md) and [PsstPsst's implementation](https://github.com/CodyTseng/psstpsst) (`docs/protocols/nip4e.md` + `docs/protocols/nip17-extensions.md`, plus rust-nostr's NIP-59 source to verify crypto mechanics).
## Executive summary
The key-sync side (kinds 10044/4454/4455) is a reasonable sketch of the spec, but **the actual message encryption is inverted and non-functional end-to-end**:
- **Sending** seals/signs with the **device encryption key** (should be the identity key).
- **Receiving** unwraps gift wraps with the **identity key** (should be the device key when the wrap is addressed to your encryption key `E`).
Every NIP-4e message between two Coop clients fails at decryption; messages to/from PsstPsst fail in both directions. On top of that there are ~10 sync/UX bugs that make even key distribution unreliable.
## 1. Critical — the message path is broken (P0)
rust-nostr's `GiftWrapBuilder::finalize_async(signer)` (verified against upstream source):
- Seal **content** = `nip44_encrypt(conversation_key(signer_sk, receiver_pk), rumor)`
- Seal is **signed by `signer`** → `seal.pubkey = signer_pk`
- Gift wrap = `nip44_encrypt(conversation_key(ephemeral_sk, receiver_pk), seal)`
- Unwrap requires the receiver's secret matching `receiver_pk`; then rust-nostr **rejects if `rumor.pubkey != seal.pubkey`** ("sender mismatch").
NIP-4e draft says the *sender* keeps using their **identity key** (`ecdh(b, E)` — Bob's `b`) and only the *target* changes to `E`. So:
| # | Finding | Location |
|---|---------|----------|
| **C1** | **Send path signs the seal with the device key.** `select_signer` returns the encryption signer, and `GiftWrapBuilder::finalize_async(device_signer)` signs the seal with `E` while `rumor.pubkey` = identity (`Room::rumor` uses `current_user`). Result: (a) receivers can't attribute the sender, (b) Coop's own `rumor.pubkey != seal.pubkey` check rejects it, (c) PsstPsst verifies the seal against the sender's *identity* key → invalid. | `crates/chat/src/room.rs:482-495, 590-596, 643-646`; rumor pubkey at `room.rs:404-417` |
| **C2** | **Receive path unwraps with the identity key.** `handle_notifications` captures the *user* signer and passes it to `extract_rumor` → `nip44_decrypt(ecdh(a, wrap_pubkey))`. A 4e wrap is encrypted to your `E`, so decryption always fails → "Failed to extract rumor" → trash. The correct unwrap key for 4e messages is the **device key** (`ecdh(e, wrap_pubkey)`), then `ecdh(e, seal.pubkey)` for the rumor — both succeed with the device signer in rust-nostr's unwrapper. | `crates/chat/src/lib.rs:184, 252, 743-801` |
| **C3** | **The `DEVICE_GIFTWRAP` subscription is never created.** The constant exists (`state/src/constants.rs:24`), its ID is used for EOSE matching (`chat/src/lib.rs:190`), but no code ever `subscribe()`s with it (presumably intended: `#p = E` filter on inbox relays). Only `USER_GIFTWRAP` (`#p = A`) exists. | `crates/chat/src/lib.rs:190-191, 410-418` |
**Consequences**:
| Path | Status |
|------|--------|
| Coop → Coop (4e) | ❌ Fails at receiver wrap-decryption (C2); even with a device-key unwrap it fails the sender-mismatch check (C1) |
| Coop → PsstPsst | ❌ PsstPsst unwraps fine (wrap → `E`), but the seal signature doesn't verify against your identity key and there's no seal `n` tag → rejected |
| PsstPsst → Coop | ❌ Wrap routed via identity `p` tag arrives, but Coop unwraps with the identity key → fail (C2) |
| Coop → self backup copy | ❌ Same broken envelope, so cross-device history sync also fails |
| Non-4e (`SignerKind::User`) | ✅ Works — the bug only bites when 4e is enabled, which is why it looks "half-baked" rather than fully broken |
> Note on interop targets: the **draft** wants `seal signed by identity, content = ecdh(identity_sk, receiver_E)` — which is exactly `GiftWrapBuilder::finalize_async(user_signer)` with `receiver = E`. **PsstPsst's profile** goes further: seal signed by identity **plus** an `n` tag with the sender's `E`, content encrypted with `ecdh(sender_e, receiver_E)`. So minimal correctness (Coop↔Coop) = swap the signers; PsstPsst interop additionally requires implementing their split-key envelope (`n` tag, sender-`e` content key).
```mermaid
sequenceDiagram
participant S as Sender (Coop)
participant R as Receiver (Coop)
Note over S: TODAY (broken)<br/>seal signed by device key Es<br/>wrap → Er, unwrap with identity a
S->>R: gift wrap (→ Er)
R->>R: nip44_decrypt(a, wrap) ✗ FAILS
Note over S: CORRECT (draft)<br/>seal signed by identity As<br/>wrap → Er, unwrap with device e
S->>R: gift wrap (→ Er)
R->>R: nip44_decrypt(e, wrap) ✓<br/>nip44_decrypt(e, seal.pubkey) ✓
```
## 2. Key-sync bugs (P1)
| # | Finding | Location |
|---|---------|----------|
| **F1** | **No `p`-tag filter on 4455.** `wait_for_approval` subscribes to *all* your 4455s (`author=me, since=now`). `extract_encryption` then tries to decrypt transfers meant for *other* devices → NIP-44 failure → `DeviceEvent::Error` → error notification. The approving device also picks up its own just-published 4455 and errors on it. Spec/PsstPsst: filter by `p = own client key` and *ignore* undecryptable responses. | `nip4e/src/lib.rs:476-482, 163-165, 489-526` |
| **F2** | **No validation of the received key.** After decrypting a 4455, the key is installed without comparing `derived_pubkey` to the current 10044 `n` value. A stale 4455 (pre-rotation) silently installs a dead key. PsstPsst: "accepts the key only when that public key equals the current kind 10044 `n` value". | `nip4e/src/lib.rs:489-526` |
| **F3** | **`since(Timestamp::now())` on both subscriptions.** The approver misses all 4454s published while it was offline (the draft says Cope "looks for **all** 4454 events" — i.e. backfill). The requester likewise misses 4455s published while it was offline; recovery depends on LMDB having synced them. This is the main "works only when both apps are open at the same time" behavior. | `nip4e/src/lib.rs:388-394, 476-482` |
| **F4** | **`pending_request` is never reset to false.** After the first approval in a session, every subsequent device request is silently dropped (`ask_for_approval` early-returns). | `nip4e/src/lib.rs:597-601` (no `set_pending_request(false)` anywhere) |
| **F5** | **No deletion after success.** The draft explicitly says the requester deletes its 4454 + the received 4455 (NIP-09). Coop leaves them on relays forever, and combined with F6 below, stale events get re-applied on later launches. | absent |
| **F6** | **`request()` short-circuits on *any* old 4455 in the local DB** (`limit(1)`, no recency check, no decrypt-check before committing to it) — stale after rotation. | `nip4e/src/lib.rs:414-422` |
| **F7** | **4454/4455 tag deviations vs PsstPsst profile**: no `n` tag (which key is requested) or `relay` hints on 4454; no identity `p` tag on 4455; `P` tag in 4455 is the device key `E` instead of a freshly generated client key (`c` per the draft). The last one is mostly cosmetic (recipients read `P` either way) but links every transfer to your announcement. | `nip4e/src/lib.rs:426-432, 559-565` |
| **F8** | **Self-request race.** `set_encryption` races the 10044 echo against the local `set_keys` write: if the announcement you just published comes back before the DB save completes, `get_keys` fails → the *master* publishes a 4454 asking *itself*, then shows "You've requested an encryption key from another device" about its own profile. | `nip4e/src/lib.rs:342-375` vs `335` |
## 3. Robustness / state management (P2)
- **`announcement_existed` never resets** and the notification loop resolves `current_user` **once** at startup (`nip4e/src/lib.rs:144`) — after an account switch, 10044/4454/4455 filtering uses the stale pubkey, and a new account without an announcement never gets `NotSet`.
- **`NotSet` after a fixed 5s timer**, not EOSE — slow relays produce false "no key" prompts (`nip4e/src/lib.rs:269-283`).
- **`get_announcement` runs on *every* settings change** (including theme changes) — each call pushes 2 more tasks into `self.tasks` (never drained, grows all session) and re-creates the `wait_for_request` subscription on every `Set` (`nip4e/src/lib.rs:100-116, 295`).
- **Publish-before-save**: `create_encryption` publishes 10044 *then* saves locally; if `set_keys` fails, the key exists only in memory while 10044 advertises it → permanent unlockable requests (`nip4e/src/lib.rs:317-338`).
- Unbounded `processed_events` HashSet in the 4e notification loop (chat's version prunes at 10k; this one doesn't).
## 4. Security considerations
- **One-click approval with no out-of-band check.** Anyone who can get a 4454 signed by your identity (i.e., any of your devices — fine) is approved by client-name alone. PsstPsst requires comparing an 8-hex-char authorization code derived from the requester's client key. Given this event transfers your *master encryption key*, an approval-code comparison is worth copying.
- **`backup()` writes the bech32 secret key to a plaintext file** (`nip4e/src/lib.rs:222-240`). Consider at least warning in the UI.
- **`Announcement::from` falls back to `val.pubkey`** when the `n` tag is missing/malformed (`state/src/nip4e.rs:14-23`) → a malformed 10044 makes `deliver()` believe 4e is available and produce hybrid messages signed with the device key but wrapped to the identity — worst of both worlds. PsstPsst treats a missing `n` as "not a 4e peer".
- Plaintext-encrypted device key sits in kind-30078 local DB events — local-only, acceptable.
## 5. UI/UX bugs
| # | Finding | Location |
|---|---------|----------|
| **U1** | **The "Retry" button is a key-rotation footgun.** On `NotSet`, the notification's action calls `set_announcement(Keys::generate())`. If the `NotSet` was a false negative (F-timer) and an announcement *does* exist, clicking "Retry" silently **rotates your encryption key**, breaking decryption on all other devices until each re-requests. The label says retry; the code creates a new key. | `workspace/src/lib.rs:130-140` |
| **U2** | **Inverted empty-check** in restore: `if !content.is_empty() { set_error("cannot be empty") }` — errors when the input is *not* empty, then falls through and reports "invalid" for empty input. | `workspace/src/dialogs/restore.rs:54-56` |
| **U3** | Restore/reset republish a 10044 with a new key without warning that other devices will need to re-request (reset dialog warns about message loss — good — but restore doesn't warn at all). | `workspace/src/lib.rs:466-473` |
| **U4** | Every spurious decrypt failure from F1 surfaces as a persistent error notification. | `workspace/src/lib.rs:152-154` |
## 6. What's already correct
- 10044 construction matches the draft (`n` tag, replaceable kind, extra `client` tag is harmless).
- 4454 tags (`P`, `client`) match the draft; request/response events are correctly signed by the identity key, so requests are authenticated.
- Client keys are generated per install and stored in the OS keyring — reasonable.
- Local key escrow (kind 30078, NIP-44 self-encrypted, `d = "coop:device"`, never published) is a sound design; the `IDENTIFIER` value surviving the crate rename is fine (changing it would orphan stored keys).
- Dedup of relay echoes, opt-in `nip4e: false` default, and `Auto` mode degrading to the identity path when no announcement exists (this fallback is why normal DMs still work).
## 7. Recommended fix roadmap (ordered)
1. **P0 — fix the envelope (C1+C2+C3)**, Coop↔Coop:
- Send: always sign the seal with the **identity** signer (`GiftWrapBuilder::finalize_async(user_signer)` with `receiver = announcement E`). The device key is *not* needed to send.
- Receive: create the `DEVICE_GIFTWRAP` subscription (`#p = E`) on the user's inbox relays; in `extract_rumor`, unwrap with the **device signer** for wraps addressed to `E` (fall back to identity for wraps addressed to `A`); re-process trash when `DeviceEvent::Set` arrives.
- Keep `rumor.pubkey = identity` — the sender-mismatch check then passes by construction.
2. **P0.5 — decide the interop target**: if PsstPsst interop matters, adopt their split-key envelope (seal `n` tag = sender `E`, seal content encrypted with `ecdh(sender_e, receiver_E)`, seal signed by identity). rust-nostr's builder can't express that split today — you'd build the seal manually.
3. **P1 — sync hardening**: add `p`-tag filters, validate decrypted key against the current 10044 `n`, replace `since(now)` with a startup backfill query, reset `pending_request`, delete 4454/4455 after success, ignore (don't error on) transfers for other devices, and save locally *before* publishing.
4. **P2 — UX**: fix the Retry button to actually retry (`get_announcement`) with a separate explicit "Create key" action; fix the inverted restore check; add an approval code comparison before sharing the key.
5. **P3 — state hygiene**: reset `announcement_existed` + re-resolve `current_user` on signer change, drive `NotSet` off EOSE, bound the task vec/dedup set.
**Validation status**: audit only, no code changed; the workspace compiled cleanly at the last `cargo check`. The rust-nostr NIP-59 behavior was verified against upstream master source (the workspace pins `nostr-sdk` to that git repo; the seal-content/signing behavior has been stable across recent versions).