avi/Keynctr
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(signer): Amber-compatible handshake — bunker:// URIs, deferred identity, ack wait

Browse source 
- parse_connect_uri accepts bunker:// as well as nostrconnect://
- URI authority key is no longer treated as identity (Amber mints a
  per-connection comms key); real identity learned via get_public_key
  after the connect ack, then persisted (profile row + secret re-key)
- connect ack awaited in a spawned handshake task with a 120s human
  approval window; session stays Connecting (all signing fails closed)
  until identity is verified
- absent perms= no longer locally denies signing; enforcement is
  delegated to the signer's approval UI
- send_rpc honours its timeout parameter
This commit is contained in:
Avi 2026-09-11 11:08:09 -05:00
commit f917e5ecfd
1 changed files with 214 additions and 74 deletions
Download patch file Download diff file Expand all files Collapse all files

288
src/signer/nip46_client.rs
View file

@ -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<String, PendingRemoteRequest>,
keys: Option<Keys>,
active_npub: Option<String>,
/// 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<PublicKey>,
}
#[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<ConnectUri, AppError> {
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<String>,
) -> Result<String, SigningError> {
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<String>,
timeout: Duration,
require_connected: bool,
) -> Result<String, SigningError> {
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<String>,
) -> 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<PublicKey, SigningError> {
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<Event, SigningError> {
@ -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,
}
}