From 1af79d81cdc5c21540b5707c1c964303886fe93b Mon Sep 17 00:00:00 2001 From: Avi Date: Thu, 10 Sep 2026 21:54:32 -0500 Subject: [PATCH] feat(signer): end-to-end external NIP-46 signing in publish and upload auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 3 sub-step 2 (IPC reroute) — the publish path now actually signs remotely instead of returning 'not yet supported': - src/signer/nip46_client.rs: outbound NIP-46 request half — send sign_event over the encrypted channel, demux responses to waiting callers, 30s timeout, waiters woken on disconnect/fail. Signer::sign_event verifies the returned event matches the requested unsigned event, is signed by the connected identity, and carries a valid signature; no local fallback. Permission-denied audit uses try_lock so a denied in-flight sign cannot deadlock the dispatcher. - src/app.rs: App::signing_for / signing_active — one place that maps a profile's SignerMode to a Signing source. Embedded -> Local(vault key); Nip46Client -> External(live signer) only when connected, otherwise ExternalSignerNotConnected; Nip46Bunker fails closed. - src/publish.rs: publish_with_keys builds the unsigned event from the Signing's own pubkey and signs via Signing::sign; publish_signed entry point for IPC (CLI keeps publish_active local path). - src/ipc.rs: PublishNote and UploadAuth route through signing_active. Nip46Connect/Nip46Disconnect drop the App guard before awaiting connect()/disconnect() (they re-lock internally — latent deadlock). - src/relays.rs: keyless relay pool (open_pool_inner(Option)) so external signing publishes without local keys. - src/uploads.rs: nip98_authorization takes a Signing source, so upload auth signs remotely for external profiles too. - src/profiles.rs: store_remote_profile — connecting a NIP-46 signer creates/refreshes a secretless Nip46Client profile row; refuses to silently convert an existing local profile. Tests: signing selection (local, fail-closed external, no profile), store_remote_profile create + no-clobber. 200 tests pass; clippy clean; fmt clean; release build green. --- src/app.rs | 117 ++++++++++++++++++++ src/ipc.rs | 46 +++++--- src/profiles.rs | 54 ++++++++- src/publish.rs | 56 +++++----- src/relays.rs | 30 ++++- src/signer/nip46_client.rs | 217 ++++++++++++++++++++++++++++++++++--- src/uploads.rs | 108 +++++------------- 7 files changed, 486 insertions(+), 142 deletions(-) diff --git a/src/app.rs b/src/app.rs index f58875e..8ee1134 100644 --- a/src/app.rs +++ b/src/app.rs @@ -9,9 +9,12 @@ use crate::crypto::{self, VaultKey}; use crate::errors::AppError; use crate::profiles::{self, ProfileSummary}; use crate::settings::Settings; +use crate::signer::Signer as SignerTrait; +use crate::signer::Signing; use crate::vault::{ self, KdfParams, SignerMode, StoredProfile, StoredPublishReport, Vault, VaultCrypto, }; +use nostr_sdk::prelude::{Keys, PublicKey}; /// Minimum password length accepted when encrypting the vault. pub const MIN_PASSWORD_LEN: usize = 8; @@ -116,6 +119,56 @@ impl App { self.vault.is_encrypted() && self.unlock_key.is_none() } + /// The [`Signing`] source for user content of a specific profile. + /// + /// The single place the "where does signing happen" decision is made, so + /// no caller branches on signer mode itself: + /// + /// - Profile mode `Embedded` → [`Signing::Local`] with the vault-resolved + /// key (locked vault surfaces as the usual `VaultLocked` error). + /// - Profile mode `Nip46Client` → [`Signing::External`] wrapping the live + /// NIP-46 client signer, **only** when one is present and connected. + /// Never falls back to the local key: an external profile that cannot + /// reach its signer fails with `ExternalSignerNotConnected`. + /// - `Nip46Bunker` (legacy, not wired) → fails closed like a missing + /// connection. + pub async fn signing_for(&self, npub: &str) -> Result { + let profile = profiles::find_stored_profile(&self.vault, npub)?; + match profile.signer_mode { + SignerMode::Embedded => { + let secret_hex = profiles::resolve_secret_key(&self.vault, npub, self.vault_key())?; + let secret_key = profiles::parse_secret_key(&secret_hex)?; + Ok(Signing::Local(Keys::new(secret_key))) + } + SignerMode::Nip46Client => { + let Some(signer) = self.nip46_signer.clone() else { + return Err(AppError::external_signer_not_connected()); + }; + if !signer.is_available().await { + return Err(AppError::external_signer_not_connected()); + } + let profile_pubkey = PublicKey::parse(npub).map_err(|e| { + AppError::internal(format!("Stored profile npub is not valid: {e}")) + })?; + Ok(Signing::External { + signer, + profile_pubkey, + }) + } + SignerMode::Nip46Bunker => Err(AppError::external_signer_not_connected()), + } + } + + /// [`Signing`] for the active profile (see [`App::signing_for`]). + pub async fn signing_active(&self) -> Result { + let npub = self + .vault + .active_profile + .clone() + .ok_or_else(AppError::no_active_profile)?; + self.signing_for(&npub).await + } + /// Verify a password and keep the derived key in memory for the session. pub fn unlock(&mut self, password: &str) -> Result<(), AppError> { let crypto = self @@ -451,6 +504,70 @@ mod tests { } } + #[test] + fn signing_for_embedded_profile_yields_local_signing() { + let app = sample_app(); + let npub = app.vault.profiles[0].public_key.clone(); + let runtime = tokio::runtime::Runtime::new().unwrap(); + let signing = runtime + .block_on(app.signing_for(&npub)) + .expect("embedded profile must select local signing"); + assert!(matches!(signing, crate::signer::Signing::Local(_))); + } + + #[test] + fn signing_for_external_profile_without_connection_fails_closed() { + let mut app = sample_app(); + // Mark the active profile as externally signed; no signer is + // connected (and none can be without a live NIP-46 session). + app.vault.profiles[0].signer_mode = SignerMode::Nip46Client; + let npub = app.vault.profiles[0].public_key.clone(); + let runtime = tokio::runtime::Runtime::new().unwrap(); + match runtime.block_on(app.signing_for(&npub)) { + Err(err) => assert_eq!(err.kind(), ErrorKind::ExternalSignerNotConnected), + Ok(_) => panic!("external profile with no signer must fail closed"), + } + } + + #[test] + fn signing_active_requires_a_profile() { + let mut app = sample_app(); + app.vault.active_profile = None; + let runtime = tokio::runtime::Runtime::new().unwrap(); + match runtime.block_on(app.signing_active()) { + Err(err) => assert_eq!(err.kind(), ErrorKind::NoActiveProfile), + Ok(_) => panic!("no active profile must error"), + } + } + + #[test] + fn store_remote_profile_creates_secretless_external_profile() { + use nostr::nips::nip19::ToBech32; + let mut vault = plaintext_vault(); + let remote = Keys::generate(); + let npub = remote.public_key().to_bech32().unwrap(); + let summary = profiles::store_remote_profile(&mut vault, &npub, "Remote".to_string()) + .expect("remote profile must be created"); + assert_eq!(summary.npub, npub); + assert!(summary.is_active); + let stored = profiles::find_stored_profile(&vault, &npub).unwrap(); + assert_eq!(stored.signer_mode, SignerMode::Nip46Client); + assert!(stored.secret_key.is_empty(), "no local secret for remote"); + } + + #[test] + fn store_remote_profile_refuses_to_clobber_local_profile() { + let mut vault = plaintext_vault(); + let existing = vault.profiles[0].public_key.clone(); + let err = profiles::store_remote_profile(&mut vault, &existing, "Hijack".to_string()) + .expect_err("a local profile must not be converted silently"); + assert!(err.message().contains("local profile")); + // Untouched: still embedded, secret intact, active unchanged. + let stored = profiles::find_stored_profile(&vault, &existing).unwrap(); + assert_eq!(stored.signer_mode, SignerMode::Embedded); + assert!(!stored.secret_key.is_empty()); + } + #[test] fn set_password_encrypts_every_secret() { let mut app = sample_app(); diff --git a/src/ipc.rs b/src/ipc.rs index 3edaa24..14435e3 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -396,15 +396,21 @@ async fn run(app: &Arc>, request: Request) -> Result { - let guard = app.lock().await; - if let Some(signer) = &guard.nip46_signer { - let status = signer.connect(&uri, label).await?; - Ok(json!(status)) - } else { - Err(AppError::config( + // Take the signer handle under the lock, then drop the guard + // before awaiting: connect() re-locks the App internally (to + // persist the connection and resolve its secret), so holding the + // guard across the await would deadlock. + let signer = { + let guard = app.lock().await; + guard.nip46_signer.clone() + }; + let Some(signer) = signer else { + return Err(AppError::config( "NIP-46 signer not initialized. Set signer mode to nip46 first.", - )) - } + )); + }; + let status = signer.connect(&uri, label).await?; + Ok(json!(status)) } Request::Nip46Disconnect => { let guard = app.lock().await; @@ -646,9 +652,17 @@ async fn run_with_app(app: &mut App, request: Request) -> Result { - let report = - publish::publish_active(&app.vault, &app.settings, &content, app.vault_key()) - .await?; + // Signer selection lives in App::signing_for: an embedded profile + // signs with the vault key; an external (NIP-46) profile's note + // round-trips to the connected signer, and an unconnected one + // fails closed — never with a silent fallback to the local key. + // + // As before, the shared App guard is held across the publish. + // The signer's background task needs no App lock to deliver the + // sign response (only audit paths take it, briefly), so the + // round-trip completes with the guard held. + let signing = app.signing_active().await?; + let report = publish::publish_signed(&app.settings, &content, &signing).await?; let stored = crate::vault::StoredPublishReport { event_id: report.event_id.clone(), succeeded: report.succeeded.clone(), @@ -765,13 +779,9 @@ async fn run_with_app(app: &mut App, request: Request) -> Result { - let authorization = crate::uploads::nip98_authorization( - &app.vault, - &url, - &http_method, - app.vault_key(), - ) - .await?; + let signing = app.signing_active().await?; + let authorization = + crate::uploads::nip98_authorization(&url, &http_method, &signing).await?; Ok(json!({ "authorization": authorization })) } diff --git a/src/profiles.rs b/src/profiles.rs index 218f229..7d66d18 100644 --- a/src/profiles.rs +++ b/src/profiles.rs @@ -9,7 +9,7 @@ use crate::errors::AppError; use crate::publish::RelayFailure; use crate::relays; use crate::settings::Settings; -use crate::vault::{unix_timestamp, StoredProfile, Vault}; +use crate::vault::{unix_timestamp, SignerMode, StoredProfile, Vault}; /// A safe view of a profile that contains no secret key material. #[derive(Debug, Clone, Serialize, PartialEq, Eq)] @@ -463,6 +463,58 @@ pub fn find_stored_profile<'a>( .ok_or_else(|| AppError::profile_not_found(npub)) } +/// Create or refresh the vault profile for a remote (NIP-46) identity. +/// +/// When a NIP-46 client connection is established the identity lives on the +/// remote signer, but the user still needs a profile row so publishing has a +/// selection. The row is marked `Nip46Client` and carries **no secret key** +/// (there is none locally): every signing operation for it must go through +/// the connected signer, and key export refuses it. Re-connecting updates the +/// label and re-activates the profile rather than duplicating it. +pub fn store_remote_profile( + vault: &mut Vault, + npub: &str, + label: String, +) -> Result { + // Validate the identity before writing anything. + PublicKey::parse(npub) + .map_err(|e| AppError::internal(format!("Remote signer identity is not valid: {e}")))?; + // An existing local profile must never be silently converted to remote: + // refuse *before* mutating if it carries a local secret. + if let Some(existing) = vault.profiles.iter().find(|p| p.public_key == npub) { + if existing.signer_mode != SignerMode::Nip46Client && !existing.secret_key.trim().is_empty() + { + return Err(AppError::config( + "That identity already exists as a local profile. Delete it first if you want to use an external signer for it.", + )); + } + } + if let Some(existing) = vault.profiles.iter_mut().find(|p| p.public_key == npub) { + existing.signer_mode = SignerMode::Nip46Client; + existing.label = label; + } else { + vault.profiles.push(StoredProfile { + label: label.clone(), + public_key: npub.to_string(), + secret_key: String::new(), // no local key — identity lives on the signer + created_at: unix_timestamp()?, + picture: None, + nip05: None, + signer_mode: SignerMode::Nip46Client, + }); + } + vault.active_profile = Some(npub.to_string()); + let stored = find_profile(vault, npub)?; + Ok(ProfileSummary { + label: stored.label.clone(), + npub: stored.public_key.clone(), + created_at: stored.created_at, + is_active: true, + picture: stored.picture.clone(), + nip05: stored.nip05.clone(), + }) +} + fn find_profile<'a>(vault: &'a Vault, npub: &str) -> Result<&'a StoredProfile, AppError> { vault .profiles diff --git a/src/publish.rs b/src/publish.rs index de3fbad..30b13b9 100644 --- a/src/publish.rs +++ b/src/publish.rs @@ -48,6 +48,9 @@ impl PublishReport { /// Publish a text note with the active profile. /// /// `key` must be the unlocked vault key when the vault is password-protected. +/// Always signs locally from the vault — used by the CLI, which has no signer +/// instances. GUI callers use [`publish_signed`] with an [`App::signing_for`] +/// signing source so external-signer profiles route to their remote signer. pub async fn publish_active( vault: &Vault, settings: &Settings, @@ -61,6 +64,17 @@ pub async fn publish_active( publish_with_keys(settings, content, &signing).await } +/// Publish a text note through an explicit [`Signing`] source (embedded or +/// external). This is what the GUI publish path uses. +pub async fn publish_signed( + settings: &Settings, + content: &str, + signing: &Signing, +) -> Result { + validate_content(content)?; + publish_with_keys(settings, content, signing).await +} + /// Publish a text note as a specific profile (used by the CLI). /// /// `key` must be the unlocked vault key when the vault is password-protected. @@ -164,33 +178,19 @@ async fn publish_with_keys( return Err(AppError::no_enabled_relays()); } - // Extract &Keys from Signing::Local for EventBuilder operations. - // Currently Signing::Local is used from publish_active/publish_as, - // but the pattern supports External signers in the future. - let keys = match signing { - Signing::Local(k) => k, - Signing::External { - signer: _, - profile_pubkey: _, - } => { - return Err(AppError::sign_failed( - "External signer not yet supported in publish_with_keys", - )); - } - }; + // The pubkey comes from the Signing itself. For an external signer this + // performs identity validation first: a signer that does not control the + // active profile's key fails here, before any event is built. + let pubkey = signing.pubkey().await.map_err(AppError::from)?; - // Build the unsigned event. + // Build the unsigned event under the signing identity, then sign it + // through `Signing` (local key or the NIP-46 round-trip). This is the + // core reroute: the IPC layer never calls Keys::sign_event directly, and + // an external profile never needs a local secret. let builder = EventBuilder::new(Kind::TextNote, content.to_string()).tags(image_tags(content)); - let unsigned = builder - .finalize_async(keys) - .await - .map_err(|e| AppError::sign_failed(format!("{e}")))?; - - // Sign the event through the Signing trait (routes to Keys::sign_event or - // Signer::sign_event depending on the variant). This is the core refactor: - // the IPC layer no longer calls Keys::sign_event directly. + let unsigned = builder.finalize_unsigned(pubkey); let signed = signing - .sign(unsigned.into()) + .sign(unsigned) .await .map_err(|e| AppError::sign_failed(format!("{e}")))?; @@ -199,7 +199,13 @@ async fn publish_with_keys( .to_bech32() .map_err(|e| AppError::internal(format!("Could not encode the event id: {e}")))?; - let client = relays::open_pool(keys.clone(), &relay_urls, None).await?; + // Local signing can answer NIP-42 AUTH challenges; with an external + // signer the app holds no key, so the pool opens without an + // authenticator and auth-gated relays report their rejection per-relay. + let client = match signing { + Signing::Local(keys) => relays::open_pool(keys.clone(), &relay_urls, None).await?, + Signing::External { .. } => relays::open_pool_anon(&relay_urls, None).await?, + }; let (succeeded, failed) = send_to_all_relays(&client, relay_urls, &signed, "note").await; if succeeded.is_empty() { diff --git a/src/relays.rs b/src/relays.rs index 303698c..51b26ba 100644 --- a/src/relays.rs +++ b/src/relays.rs @@ -78,12 +78,36 @@ pub(crate) async fn open_pool( keys: Keys, relay_urls: &[String], wait: Option, +) -> Result { + open_pool_inner(Some(keys), relay_urls, wait).await +} + +/// Open a relay pool with no signing identity. +/// +/// Used when user content is signed by an external (NIP-46) signer: the app +/// holds no key to answer NIP-42 AUTH challenges with, so the pool is built +/// without an authenticator. Relays that demand auth will reject reads/ +/// writes at the protocol level, which `send_to_all_relays` already reports +/// per-relay. +pub(crate) async fn open_pool_anon( + relay_urls: &[String], + wait: Option, +) -> Result { + open_pool_inner(None, relay_urls, wait).await +} + +async fn open_pool_inner( + keys: Option, + relay_urls: &[String], + wait: Option, ) -> Result { // The authenticator answers NIP-42 AUTH challenges automatically on every // path that opens a client (nostr-sdk >= 0.45 has no implicit signer). - let client = Client::builder() - .authenticator(SignerAuthenticator::new(keys)) - .build(); + let builder = match keys { + Some(keys) => Client::builder().authenticator(SignerAuthenticator::new(keys)), + None => Client::builder(), + }; + let client = builder.build(); for url in relay_urls { client .add_relay(url.as_str()) diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index 1319443..db578d6 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -31,6 +31,9 @@ const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); const APPROVAL_TIMEOUT: Duration = Duration::from_secs(300); /// Maximum number of requests kept waiting for approval at once. 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); /// Internal state for a pending approval. struct PendingApprovalInner { @@ -39,6 +42,12 @@ struct PendingApprovalInner { sender: oneshot::Sender, } +/// A response awaited from the *remote signer* for a request we sent +/// (the client half of the NIP-46 flow, e.g. our `sign_event` request). +struct PendingRemoteRequest { + sender: oneshot::Sender>, +} + /// Parsed nostrconnect:// URI. struct ConnectUri { peer: PublicKey, @@ -60,6 +69,9 @@ struct Nip46Inner { conversation_key: Option, client: Option, pending: HashMap, + /// Outbound requests we sent to the remote signer (e.g. `sign_event`) + /// waiting for its encrypted response, keyed by request id. + remote_pending: HashMap, keys: Option, active_npub: Option, } @@ -84,6 +96,7 @@ impl Nip46ClientSigner { conversation_key: None, client: None, pending: HashMap::new(), + remote_pending: HashMap::new(), keys: None, active_npub: None, })), @@ -97,10 +110,16 @@ impl Nip46ClientSigner { } /// Emit an audit event for a permission-denied NIP-46 operation. + /// + /// Uses `try_lock`: the IPC dispatcher may hold the App lock while a + /// sign request is in flight (PublishNote), and audit is best-effort — + /// blocking here would deadlock the very request being denied. async fn audit_permission_denied(&self, method: &str) { let npub = self.inner.lock().await.active_npub.clone(); if let Some(npub) = npub { - let mut app = self.app.lock().await; + let Ok(mut app) = self.app.try_lock() else { + return; + }; if let Some(ref mut log) = app.audit_log { let _ = log.record( &npub, @@ -243,8 +262,8 @@ 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. - { + // connection, never logged. Evaluates to the remote identity's npub. + let remote_npub = { let mut app = self.app.lock().await; // Remove any existing connection for the same signer from the // same profile (reconnect replaces the old connection). @@ -268,8 +287,19 @@ 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 { @@ -283,6 +313,9 @@ 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); inner.pending.clear(); } @@ -326,6 +359,13 @@ impl Nip46ClientSigner { inner.conversation_key = None; inner.keys = None; inner.pending.clear(); + // Wake any callers awaiting a remote response; their waiters turn + // into `NotConnected` rather than hanging until the request timeout. + for (_, waiter) in inner.remote_pending.drain() { + let _ = waiter.sender.send(Err( + "Disconnected from the signer before it responded.".to_string() + )); + } Ok(()) } @@ -412,6 +452,11 @@ impl Nip46ClientSigner { inner.conversation_key = None; inner.keys = None; inner.pending.clear(); + for (_, waiter) in inner.remote_pending.drain() { + let _ = waiter.sender.send(Err( + "Lost the connection to the signer before it responded.".to_string(), + )); + } } } @@ -447,6 +492,83 @@ impl Nip46ClientSigner { } } + /// Send a NIP-46 request to the connected remote signer and await its + /// encrypted response (the client half of the protocol — e.g. asking the + /// signer to `sign_event` an event for us). + /// + /// Fails closed: no connection, no live session, or a signer error all + /// return [`SigningError`] rather than falling back to any local key. The + /// waiter is always removed from the pending map, even on timeout, so a + /// late response to an abandoned request finds nothing to wake. + async fn send_remote_request( + &self, + method: &str, + params: Vec, + ) -> Result { + let id = uuid::Uuid::new_v4().to_string(); + let (sender, receiver) = oneshot::channel(); + + // Snapshot the live session, register the waiter, and send the + // encrypted request in one lock pass. Holding the lock across the send + // is safe (the send is a relay hand-off, never a re-entry into this + // signer) and guarantees registration cannot interleave with the send. + let send_result = { + let mut inner = self.inner.lock().await; + let client = inner.client.clone().ok_or(SigningError::NotConnected)?; + let keys = inner.keys.clone().ok_or(SigningError::NotConnected)?; + let conversation = inner + .conversation_key + .as_ref() + .cloned() + .ok_or(SigningError::NotConnected)?; + let connection = inner.connection.clone().ok_or(SigningError::NotConnected)?; + if !connection_valid_now(&connection) { + return Err(if connection.revoked_at.is_some() { + SigningError::ConnectionRevoked + } else { + SigningError::ConnectionExpired + }); + } + if !matches!(inner.phase, Nip46Phase::Connected) { + return Err(SigningError::NotConnected); + } + if inner.remote_pending.len() >= MAX_PENDING_APPROVALS { + return Err(SigningError::Internal { + detail: "Too many requests already awaiting the signer.".to_string(), + }); + } + let peer = PublicKey::from_hex(&connection.signer_pubkey).map_err(|e| { + SigningError::Internal { + detail: format!("Invalid signer public key: {e}"), + } + })?; + inner + .remote_pending + .insert(id.clone(), PendingRemoteRequest { sender }); + + let payload = json!({ "id": id, "method": method, "params": params }).to_string(); + self.publish_payload(&client, &keys, &conversation, &peer, &payload) + .await + }; + if let Err(detail) = send_result { + self.inner.lock().await.remote_pending.remove(&id); + return Err(SigningError::Network { detail }); + } + + match tokio::time::timeout(REQUEST_TIMEOUT, receiver).await { + Ok(Ok(result)) => result.map_err(|detail| SigningError::Internal { detail }), + Ok(Err(_)) => { + // The waiter was dropped (disconnect/fail) while awaiting. + Err(SigningError::NotConnected) + } + Err(_) => { + // Abandon the waiter so a late response finds nothing. + self.inner.lock().await.remote_pending.remove(&id); + Err(SigningError::Timeout) + } + } + } + /// Main background task: connect to relays, subscribe, handle requests. async fn run_sign_task(self, uri: ConnectUri) -> Result<(), String> { let (conversation, keys) = { @@ -534,6 +656,35 @@ impl Nip46ClientSigner { Err(_) => continue, }; + // A payload carrying `result`/`error` is a response to a request + // WE sent (e.g. `sign_event`), not an incoming signer request. + // Deliver it to the waiting caller; unknown ids are ignored — a + // stray response must never earn an error reply back to the + // signer, and must never fall through to the request path. + let shaped: serde_json::Value = + serde_json::from_str(&plaintext).unwrap_or(serde_json::Value::Null); + if shaped.get("result").is_some() || shaped.get("error").is_some() { + let id = shaped.get("id").and_then(|v| v.as_str()).unwrap_or(""); + let outcome = if shaped.get("error").is_some() && !shaped["error"].is_null() { + Err(shaped["error"] + .as_str() + .unwrap_or("The signer reported an error.") + .to_string()) + } else { + Ok(shaped["result"].as_str().unwrap_or("").to_string()) + }; + let waiter = self.inner.lock().await.remote_pending.remove(id); + match waiter { + Some(pending) => { + let _ = pending.sender.send(outcome); + } + None => { + eprintln!("Ignoring NIP-46 response with no waiting request: {id}"); + } + } + continue; + } + let request: RawRequest = match serde_json::from_str(&plaintext) { Ok(r) => r, Err(_) => continue, @@ -879,16 +1030,41 @@ impl Signer for Nip46ClientSigner { }) } - async fn sign_event(&self, _event: UnsignedEvent) -> Result { - // For NIP-46 client, signing happens via the NIP-46 channel with user - // approval. The actual flow uses request_approval + - // respond_to_approval; this method exists to satisfy the object-safe - // trait and fails closed if a caller tries to bypass it. - Err(SigningError::Internal { - detail: - "NIP-46 signing uses the async approval flow; direct sign_event is not supported" - .to_string(), - }) + async fn sign_event(&self, event: UnsignedEvent) -> Result { + // The client half of NIP-46: ask the remote signer to sign, await the + // encrypted response, and verify the returned event is exactly what we + // asked for (identity + id + signature) before handing it back. A + // misbehaving or MITM'd signer cannot swap content or keys. + if !self.can_sign_event(event.kind.as_u16()) { + self.audit_permission_denied("sign_event").await; + return Err(SigningError::PermissionDenied { + method: "sign_event".to_string(), + }); + } + let unsigned_json = event.try_as_json().map_err(|e| SigningError::Internal { + detail: format!("Could not encode the event for signing: {e}"), + })?; + let response = self + .send_remote_request("sign_event", vec![unsigned_json]) + .await?; + let signed = Event::from_json(response.as_bytes()).map_err(|e| SigningError::Internal { + detail: format!("The signer returned an unreadable event: {e}"), + })?; + // The signer must sign WITH the identity it is connected as... + let signer_pubkey = Signer::get_public_key(self).await?; + if signed.pubkey != signer_pubkey { + return Err(SigningError::IdentityMismatch); + } + // ...the exact event we sent (same id covers pubkey, kind, tags, + // content, timestamp)... + if signed.id != event.compute_id() { + return Err(SigningError::InvalidSignature); + } + // ...and with a cryptographically valid signature. + signed + .verify() + .map_err(|_| SigningError::InvalidSignature)?; + Ok(signed) } fn get_signer_type(&self) -> SignerType { @@ -1009,6 +1185,19 @@ fn response_ok(id: &str, result: String) -> String { json!({ "id": id, "result": result, "error": null }).to_string() } +/// Whether a stored connection is usable *right now* (not revoked, not +/// expired), computed without taking the signer's async lock. The trait's +/// `is_connection_valid` cannot be used from contexts that already hold it. +fn connection_valid_now(conn: &Nip46Connection) -> bool { + if conn.revoked_at.is_some() { + return false; + } + match conn.expires_at { + Some(expires_at) => crate::vault::unix_timestamp().unwrap_or(0) < expires_at, + None => true, + } +} + fn response_err(id: &str, error: String) -> String { json!({ "id": id, "result": null, "error": error }).to_string() } diff --git a/src/uploads.rs b/src/uploads.rs index a1ed3ac..e194583 100644 --- a/src/uploads.rs +++ b/src/uploads.rs @@ -1,21 +1,22 @@ +use base64::engine::general_purpose::STANDARD as B64; +use base64::Engine; use nostr::nips::nip98::{HttpData, HttpMethod}; use nostr_sdk::prelude::*; -use crate::crypto::VaultKey; use crate::errors::{AppError, ErrorKind}; -use crate::profiles; +use crate::signer::Signing; -/// Sign a NIP-98 HTTP auth event for `url` with the active profile's key and -/// return the `Authorization` header value (`Nostr `). +/// Sign a NIP-98 HTTP auth event for `url` with the given [`Signing`] source +/// and return the `Authorization` header value (`Nostr `). /// /// This is what image hosts like nostr.build require before accepting an -/// upload. Like publishing, it needs an unlocked vault when the vault is -/// password-protected. +/// upload. It follows the same signer selection as publishing: an embedded +/// profile signs with the vault key, an external profile round-trips the +/// auth event through its connected NIP-46 signer. pub async fn nip98_authorization( - vault: &crate::vault::Vault, url: &str, method: &str, - key: Option<&VaultKey>, + signing: &Signing, ) -> Result { let http_method = match method.to_ascii_uppercase().as_str() { "GET" => HttpMethod::GET, @@ -33,112 +34,57 @@ pub async fn nip98_authorization( let parsed_url = Url::parse(url) .map_err(|e| AppError::config(format!("The upload URL is not valid: {e}")))?; - let secret_hex = profiles::resolve_active_secret_key(vault, key)?; - let secret_key = profiles::parse_secret_key(&secret_hex)?; - let keys = Keys::new(secret_key); - - let header = HttpData::new(parsed_url, http_method) - .to_authorization(&keys) + // Build the same event HttpData::to_authorization would build (kind + // 27235 with the u/method tags), but sign it through `Signing` so an + // external profile never needs a local secret. + let http_data = HttpData::new(parsed_url, http_method); + let pubkey = signing.pubkey().await.map_err(AppError::from)?; + let unsigned = IntoEventBuilder::into_event_builder(http_data).finalize_unsigned(pubkey); + let event = signing + .sign(unsigned) .await .map_err(|e| AppError::sign_failed(format!("Could not sign the upload request: {e}")))?; - Ok(header) + let encoded = B64.encode(event.as_json()); + Ok(format!("Nostr {encoded}")) } #[cfg(test)] mod tests { use super::*; - use crate::settings::Settings; - use crate::vault::Vault; - /// Settings with no relays so tests never touch the network. - fn offline_settings() -> Settings { - Settings { - relays: Vec::new(), - ..Default::default() - } - } - - fn vault_with_profile() -> Vault { - let mut vault = Vault::empty(); - crate::profiles::create_profile(&mut vault, "A".to_string(), None, &offline_settings()) - .unwrap(); - vault - } - - #[test] - fn missing_profile_errors() { - let vault = Vault::empty(); - let runtime = tokio::runtime::Runtime::new().unwrap(); - let err = runtime - .block_on(nip98_authorization( - &vault, - "https://nostr.build/api/v2/upload/files", - "POST", - None, - )) - .expect_err("no active profile must error"); - assert_eq!(err.kind(), ErrorKind::NoActiveProfile); + fn local_signing() -> Signing { + Signing::Local(Keys::generate()) } #[test] fn unsupported_method_errors() { - let vault = vault_with_profile(); + let signing = local_signing(); let runtime = tokio::runtime::Runtime::new().unwrap(); let err = runtime .block_on(nip98_authorization( - &vault, "https://example.com/upload", "DELETE", - None, + &signing, )) .expect_err("unsupported method must error"); assert_eq!(err.kind(), ErrorKind::Config); } #[test] - fn locked_encrypted_vault_errors() { - let mut vault = vault_with_profile(); - vault.crypto = Some(crate::vault::VaultCrypto { - kdf: crate::vault::KdfParams { - algorithm: "argon2id".to_string(), - salt: "c2FsdA==".to_string(), - m_cost: 1, - t_cost: 1, - p_cost: 1, - }, - verifier: "dmVyaWZpZXI=".to_string(), - }); - vault.profiles[0].secret_key = "encrypted-blob".to_string(); - let runtime = tokio::runtime::Runtime::new().unwrap(); - let err = runtime - .block_on(nip98_authorization( - &vault, - "https://nostr.build/api/v2/upload/files", - "POST", - None, - )) - .expect_err("locked vault must error"); - assert_eq!(err.kind(), ErrorKind::VaultLocked); - } - - #[test] - fn signs_a_nip98_auth_header_for_the_active_profile() { - let vault = vault_with_profile(); + fn signs_a_nip98_auth_header() { + let signing = local_signing(); let runtime = tokio::runtime::Runtime::new().unwrap(); let header = runtime .block_on(nip98_authorization( - &vault, "https://nostr.build/api/v2/upload/files", "POST", - None, + &signing, )) - .expect("valid profile must sign"); + .expect("local signing must produce a header"); assert!(header.starts_with("Nostr "), "expected a Nostr auth header"); let encoded = header.trim_start_matches("Nostr ").trim(); - use base64::engine::general_purpose::STANDARD as B64; - use base64::Engine as _; let raw = B64 .decode(encoded) .expect("the header payload must be base64");