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

clean up

Browse Source
This commit is contained in:
reya
2026-10-04 13:28:41 +07:00
parent 758ffaf7d9
commit 5cceaa0fc8
1 changed files with 0 additions and 154 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/nip4e-audit.md
-154
View File
@@ -1,154 +0,0 @@
# NIP-4e Audit Report
> **Update**: The P0 findings (C1, C2, C3) are fixed — seals are always signed by the
> identity key, `extract_rumor` falls back to the device key, the `DEVICE_GIFTWRAP`
> subscription is created, and trashed wraps are retried on `DeviceEvent::Set`.
>
> **Update 2**: The P1 sync findings are fixed — F1 (4455 approvals now filtered by
> the requester's `p` tag so the approver no longer processes its own response), F2
> (decrypted keys are validated against the current 10044 announcement via
> `decrypt_approval`), F3 (`since(now)` removed so offline requests/approvals
> backfill), F4 (`pending_request` resets after an approval completes), F5 (a NIP-09
> deletion now removes the 4454 requests and the consumed 4455 after a successful
> sync), F6 (stale stored 4455s no longer short-circuit `request()` — they are
> validated first and a fresh request is published when invalid), F8 (device keys
> are saved locally *before* the 10044 announcement is published, closing the
> echo/self-request race).
>
> **Update 3**: The P2/UX findings are fixed — the notification listener is now
> restarted on account switch and `announcement_existed` is reset (no stale
> `current_user` / false negatives after changing accounts), `NotSet` fires on
> EOSE instead of a fixed 5-second timer, `Announcement::from_event` returns
> `None` on a missing/malformed `n` tag instead of falling back to the identity
> pubkey, the 4454 notification parses the requester key from the `P` tag, the
> NotSet "Retry" button re-checks for an announcement instead of silently
> generating (rotating) a new encryption key, and the restore dialog's
> inverted empty-key check is fixed. Additionally, the `client` tag and
> `client_name` were dropped from announcements/requests, and the PsstPsst
> authorization code was adapted: both sides display the first 8 hex chars of
> the requester client pubkey (`XXXX XXXX`) — the approver in the request
> notification, the requester in the "Wait for approval" notification — so the
> transfer can be verified out of band before approving. Still open: the
> plaintext backup file in section 4.
>
> **Update 4**: Verified against the PsstPsst source (nip17-gift-wrap.ts,
> encryption-key.service.ts) and closed the remaining envelope incompatibility.
> Coop now emits PsstPsst's split-key NIP-59 format when the sender has an
> encryption key: the seal (kind 13) is signed by the identity, the rumor
> content is NIP-44'd under the sender's encryption key, and the seal carries
> the sender's encryption pubkey in an `n` tag; the wrap uses a one-shot
> ephemeral key. Receiving supports both formats: the seal's `n` tag selects
> the content-decryption key, falling back to the seal author for legacy
> unified seals. Interop matrix is now green in all four directions, with one
> caveat: a Coop sender without an encryption key degrades to the legacy
> format, which PsstPsst ignores on its relay intake path.
**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).