diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index db578d6..4221df8 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -34,6 +34,9 @@ const MAX_PENDING_APPROVALS: usize = 20; /// How long an outbound NIP-46 request (e.g. our own `sign_event`) may wait /// for the remote signer's response before it is abandoned. const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); +/// The connect handshake can involve a human approving the app on the +/// signer's screen, so it gets a far longer leash than ordinary RPCs. +const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(120); /// Internal state for a pending approval. struct PendingApprovalInner { @@ -74,6 +77,12 @@ struct Nip46Inner { remote_pending: HashMap, keys: Option, active_npub: Option, + /// The remote signer's REAL identity key, learned from the + /// `get_public_key` RPC after the connect handshake completes. The + /// pubkey in the connect URI may be a per-connection communication key + /// (Amber's `bunker://` flow mints one per app) and must never be used + /// as an identity. `None` until the handshake resolves it. + identity: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -99,6 +108,7 @@ impl Nip46ClientSigner { remote_pending: HashMap::new(), keys: None, active_npub: None, + identity: None, })), app, } @@ -132,11 +142,23 @@ impl Nip46ClientSigner { } } - /// Parse a nostrconnect:// URI. + /// Parse a `nostrconnect://` or `bunker://` URI. + /// + /// Both carry the same wire shape (authority = the signer's public key, + /// `relay=` params, optional `secret=`). `nostrconnect://` is the + /// client-initiated flow (we generated the URI); `bunker://` is the + /// signer-initiated flow (Amber and self-hosted bunkers show one of + /// these). NOTE: with `bunker://` the authority key may be a + /// per-connection communication key (Amber mints one per app), NOT the + /// user's identity — identity is learned later via `get_public_key`. fn parse_connect_uri(raw: &str) -> Result { - let rest = raw.trim().strip_prefix("nostrconnect://").ok_or_else(|| { - AppError::config("Paste the nostrconnect:// link from your Nostr app.") - })?; + let rest = raw + .trim() + .strip_prefix("nostrconnect://") + .or_else(|| raw.trim().strip_prefix("bunker://")) + .ok_or_else(|| { + AppError::config("Paste the bunker:// or nostrconnect:// link from your Nostr app.") + })?; let (authority, query) = match rest.split_once('?') { Some((a, q)) => (a, Some(q)), @@ -262,8 +284,13 @@ impl Nip46ClientSigner { // Persist the connection AND its secret in the vault. The secret is // encrypted under the vault key (when a password is set) and keyed by // the connection's opaque VaultRef — never stored inline on the - // connection, never logged. Evaluates to the remote identity's npub. - let remote_npub = { + // connection, never logged. + // + // No profile is created yet: the URI key may be a per-connection + // communication key (Amber's bunker:// flow mints one per app), NOT + // the user's identity. The profile row is created by the handshake + // once `get_public_key` reveals the real identity. + { let mut app = self.app.lock().await; // Remove any existing connection for the same signer from the // same profile (reconnect replaces the old connection). @@ -287,19 +314,8 @@ impl Nip46ClientSigner { crate::vault::delete_connection_secret(&mut app.vault, &vault_ref); } app.vault.nip46_connections.push(connection.clone()); - // The remote identity gets its own profile row so publishing has - // a selection: marked Nip46Client with no local secret, so every - // signing path for it routes through this connection and key - // export refuses it. If the peer key collides with an existing - // local (embedded) profile the connect fails closed. - let remote_npub = PublicKey::from_hex(&connection.signer_pubkey) - .map_err(|e| AppError::internal(format!("Invalid signer public key: {e}")))? - .to_bech32() - .map_err(|e| AppError::internal(format!("Could not encode npub: {e}")))?; - profiles::store_remote_profile(&mut app.vault, &remote_npub, connection.label.clone())?; app.save_vault()?; - remote_npub - }; + } // Update state to connecting { @@ -313,9 +329,10 @@ impl Nip46ClientSigner { inner.connection = Some(connection.clone()); inner.conversation_key = Some(conversation); inner.keys = Some(keys.clone()); - // The signing identity is the remote signer's key, and the vault - // now holds a matching Nip46Client profile row (active). - inner.active_npub = Some(remote_npub); + // Identity is unknown until the handshake's `get_public_key` + // resolves it; nothing may sign before then. + inner.identity = None; + inner.active_npub = None; inner.pending.clear(); } @@ -504,6 +521,21 @@ impl Nip46ClientSigner { &self, method: &str, params: Vec, + ) -> Result { + self.send_rpc(method, params, REQUEST_TIMEOUT, true).await + } + + /// Send an encrypted NIP-46 RPC and await its response. + /// + /// `require_connected` gates on the session phase: ordinary RPCs need a + /// fully established session, while the handshake itself runs while the + /// phase is still `Connecting`. + async fn send_rpc( + &self, + method: &str, + params: Vec, + timeout: Duration, + require_connected: bool, ) -> Result { let id = uuid::Uuid::new_v4().to_string(); let (sender, receiver) = oneshot::channel(); @@ -529,7 +561,7 @@ impl Nip46ClientSigner { SigningError::ConnectionExpired }); } - if !matches!(inner.phase, Nip46Phase::Connected) { + if require_connected && !matches!(inner.phase, Nip46Phase::Connected) { return Err(SigningError::NotConnected); } if inner.remote_pending.len() >= MAX_PENDING_APPROVALS { @@ -555,7 +587,7 @@ impl Nip46ClientSigner { return Err(SigningError::Network { detail }); } - match tokio::time::timeout(REQUEST_TIMEOUT, receiver).await { + match tokio::time::timeout(timeout, receiver).await { Ok(Ok(result)) => result.map_err(|detail| SigningError::Internal { detail }), Ok(Err(_)) => { // The waiter was dropped (disconnect/fail) while awaiting. @@ -625,26 +657,47 @@ impl Nip46ClientSigner { .await .map_err(|e| format!("Could not subscribe: {e}"))?; - // Send connect request - self.send_connect(&client, &keys, &conversation, &uri) - .await?; - - // Mark as connected - self.inner.lock().await.phase = Nip46Phase::Connected; + // The connect handshake runs as its own task: it publishes `connect` + // and then must AWAIT the signer's ack — which can wait on a human + // approving us in Amber — followed by `get_public_key` to learn the + // REAL signing identity. Responses only arrive through the demux + // loop below, so the handshake must not block it. The session stays + // `Connecting` (and every signing path fails closed) until the + // handshake resolves the identity. + { + let handshake = self.clone(); + let peer = uri.peer; + let expected_secret = uri.secret.clone(); + tokio::spawn(async move { + if let Err(e) = handshake.clone().run_handshake(peer, expected_secret).await { + handshake.fail(e); + } + }); + } // Handle incoming requests loop { - let incoming = match notifications.next().await { - Some(nostr_sdk::client::ClientNotification::Event { - subscription_id, - event, - .. - }) if subscription_id == *subscription.id() => event, - Some(nostr_sdk::client::ClientNotification::Shutdown) | None => { - return Err("Connection closed".to_string()); - } - Some(_) => continue, - }; + let incoming = + match tokio::time::timeout(Duration::from_secs(2), notifications.next()).await { + Ok(Some(nostr_sdk::client::ClientNotification::Event { + subscription_id, + event, + .. + })) if subscription_id == *subscription.id() => event, + Ok(Some(nostr_sdk::client::ClientNotification::Shutdown)) | Ok(None) => { + return Err("Connection closed".to_string()); + } + Ok(Some(_)) => continue, + Err(_elapsed) => { + // Idle tick: if the handshake task failed, the session is + // dead — exit so teardown runs instead of listening on a + // connection that can never sign. + if matches!(self.inner.lock().await.phase, Nip46Phase::Error(_)) { + return Err("Connect handshake failed".to_string()); + } + continue; + } + }; let event = *incoming; if event.kind != Kind::NostrConnect || event.pubkey != uri.peer { @@ -932,31 +985,44 @@ impl Nip46ClientSigner { } } - async fn send_connect( - &self, - client: &Client, - keys: &Keys, - conversation: &ConversationKey, - uri: &ConnectUri, + /// The connect handshake, run as its own task alongside the demux loop. + /// + /// 1. Send `connect` (our client pubkey + the URI secret, resolved + /// on-demand from the vault — never from memory). + /// 2. Await the signer's ack. This is where a human approving us in + /// Amber happens, so the wait is long (HANDSHAKE_TIMEOUT). + /// 3. Call `get_public_key` to learn the REAL signing identity. The + /// pubkey in the connect URI may be a per-connection communication + /// key (Amber mints one per app); trusting it would publish under a + /// throwaway key. + /// 4. Persist the identity: create/refresh the secretless + /// `Nip46Client` profile row, re-key the vault secret store under + /// it, and only then flip the phase to `Connected`. + /// + /// Until step 4 completes the session is `Connecting`, so every signing + /// path fails closed — nothing can publish under an unverified key. + async fn run_handshake( + self, + peer: PublicKey, + expected_secret: Option, ) -> Result<(), String> { - // Resolve the nostrconnect secret ON-DEMAND from the vault — the single - // source of truth — rather than reading an in-memory copy. The - // connection carries no secret; it is keyed by its VaultRef and fetched - // fresh here. Fail-closed: if the vault cannot produce the secret - // (e.g. it is encrypted and currently locked) the connect is refused - // instead of being sent without it. - let secret = { + // Resolve the connect secret ON-DEMAND from the vault — the single + // source of truth. Fail-closed: if the vault cannot produce the + // secret the connect is refused rather than sent incomplete. + // (Also snapshot the connection label for the profile row below.) + let (secret, label, connect_ref) = { let connection = { let inner = self.inner.lock().await; inner.connection.clone() } .ok_or("No active NIP-46 connection to resolve the secret for")?; + let label = connection.label.clone(); let vault_ref = crate::signer::VaultRef::from_connection(&connection); let app = self.app.lock().await; // Copy the key out before the immutable borrow of the vault so the // two are never borrowed at once. let vault_key = app.vault_key().copied(); - match crate::vault::resolve_connection_secret( + let secret = match crate::vault::resolve_connection_secret( &app.vault, vault_key.as_ref(), &vault_ref, @@ -968,21 +1034,89 @@ impl Nip46ClientSigner { "Could not resolve the connection secret from the vault: {e}" )) } - } + }; + (secret, label, vault_ref) }; - let mut params = vec![keys.public_key().to_hex()]; + let mut params = vec![{ + let inner = self.inner.lock().await; + inner + .keys + .as_ref() + .ok_or("No session keys")? + .public_key() + .to_hex() + }]; if let Some(secret) = &secret { params.push(secret.clone()); } - let payload = json!({ - "id": uuid::Uuid::new_v4().to_string(), - "method": "connect", - "params": params, - }) - .to_string(); - self.publish_payload(client, keys, conversation, &uri.peer, &payload) + + // Await the ack (may wait on a human approving in Amber). + let ack = self + .send_rpc("connect", params, HANDSHAKE_TIMEOUT, false) .await + .map_err(|e| format!("The signer refused the connection: {e}"))?; + + // Anti-spoofing: per NIP-46, a signer answering a nostrconnect:// + // (secret-carrying) handshake must echo the secret back. A mismatch + // means something other than the intended signer answered. + if let Some(expected) = &expected_secret { + let trimmed = ack.trim(); + if trimmed != expected.as_str() { + return Err( + "The signer did not echo the connection secret; refusing the connection." + .to_string(), + ); + } + } + + // Learn the REAL signing identity from the signer itself. + let identity = self + .send_rpc("get_public_key", vec![], REQUEST_TIMEOUT, false) + .await + .map_err(|e| format!("The signer would not reveal its public key: {e}"))?; + let identity = PublicKey::from_hex(identity.trim()) + .map_err(|e| format!("The signer returned an unreadable public key: {e}"))?; + let identity_npub = identity + .to_bech32() + .map_err(|e| format!("Could not encode identity npub: {e}"))?; + + // Persist: profile row for the identity, connection + secret store + // re-keyed under it. Refuses (fails the handshake) if the identity + // collides with a local profile. + { + let mut app = self.app.lock().await; + profiles::store_remote_profile(&mut app.vault, &identity_npub, label) + .map_err(|e| e.message().to_string())?; + // Re-key the stored secret under the identity npub so the + // connection row, VaultRef, and secret store all agree. + if let Some(s) = &secret { + let new_ref = + crate::signer::VaultRef::new(Some(identity_npub.clone()), peer.to_hex()); + let vault_key = app.vault_key().copied(); + crate::vault::store_connection_secret( + &mut app.vault, + vault_key.as_ref(), + &new_ref, + s, + ) + .map_err(|e| e.message().to_string())?; + crate::vault::delete_connection_secret(&mut app.vault, &connect_ref); + } + if let Some(conn) = app.vault.nip46_connections.iter_mut().find(|c| { + c.signer_pubkey == peer.to_hex() && c.profile_npub != Some(identity_npub.clone()) + }) { + conn.profile_npub = Some(identity_npub.clone()); + } + app.save_vault().map_err(|e| e.message().to_string())?; + } + + // Identity verified: adopt it and open the session for signing. + let mut inner = self.inner.lock().await; + inner.identity = Some(identity); + inner.active_npub = Some(identity_npub); + inner.phase = Nip46Phase::Connected; + Ok(()) } async fn publish_payload( @@ -1021,13 +1155,10 @@ impl Clone for Nip46ClientSigner { impl Signer for Nip46ClientSigner { async fn get_public_key(&self) -> Result { let inner = self.inner.lock().await; - let connection = inner - .connection - .as_ref() - .ok_or(SigningError::NotConnected)?; - PublicKey::from_hex(&connection.signer_pubkey).map_err(|e| SigningError::Internal { - detail: format!("Invalid signer public key: {e}"), - }) + // The identity learned from the signer during the handshake — NOT + // the URI key, which for bunker:// flows is a per-connection comms + // key. Until the handshake resolves it the session is not usable. + inner.identity.clone().ok_or(SigningError::NotConnected) } async fn sign_event(&self, event: UnsignedEvent) -> Result { @@ -1120,31 +1251,40 @@ impl Signer for Nip46ClientSigner { .and_then(|c| c.permissions.clone()) } + // NOTE on the `None` arms below: when the connect URI declared no + // `perms=`, there is no local grant list to enforce — enforcement lives + // on the signer itself (Amber shows an approval screen per request). + // Sending the request and letting the signer decide is the NIP-46 flow; + // refusing locally would make bunker:// connections (which carry no + // perms) unusable. When perms WERE declared, the local list is enforced + // as an additional guard. fn can_sign_event(&self, kind: u16) -> bool { match self.permissions() { Some(ref perms) => perms.is_sign_event_kind_allowed(kind), - None => false, + None => true, } } fn can_encrypt(&self) -> bool { match self.permissions() { Some(ref perms) => perms.is_encrypt_allowed(), - None => false, + None => true, } } fn can_decrypt(&self) -> bool { match self.permissions() { Some(ref perms) => perms.is_decrypt_allowed(), - None => false, + None => true, } } fn can_get_public_key(&self) -> bool { + // `get_public_key` is part of the connect handshake for every + // signer; with no declared perms the signer still answers it. match self.permissions() { Some(ref perms) => perms.is_get_public_key_allowed(), - None => false, + None => true, } }