From 01ce5de4172ee79b0e4a6720e99abb62532562cc Mon Sep 17 00:00:00 2001 From: Avi Date: Fri, 25 Sep 2026 16:19:19 -0500 Subject: [PATCH] fix(nip46): restored sessions no longer demand a connect secret re-echo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live Amber sign-in (Sep 25) paired fine, but every restart died with 'the signer did not echo the connection secret': the restore re-sends connect with the ORIGINAL pairing secret, and an already-approved signer legitimately answers 'true' without re-echoing — NIP-46 reserves the echo for proving possession during the INITIAL pairing, and Amber proved it once. Requiring it on re-dial made session restore fail 100% against real Amber (the e2e missed it because its fake answered a plain ack with no secret in play). ConnectUri gains a 'restore' flag (set only by reactivate_saved_sessions). Fresh handshakes still fail closed on a wrong echo. The restore path's anti-spoofing is expected_identity in adopt_identity — a peer answering as any other account is refused, and only the real key holder can decrypt traffic on the stored conversation key. Log now says 'secret validation: SKIPPED (restored session)' and proceeds. e2e: run_fake_amber now mirrors Amber's ack shapes (plain ack on first pairing; 'true' when the client re-presents a secret) and the restore test seeds a pairing secret so it exercises exactly the live failure — it fails without the fix and passes with it. cargo test 216 unit + 5 e2e green; clippy --all-targets 0 warnings; fmt clean; release rebuilt. --- src/signer/nip46_client.rs | 43 +++++++++++++++++++++++++++++++++++--- tests/nip46_e2e.rs | 25 ++++++++++++++++++++-- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index 7e2b2ee..94e011f 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -112,6 +112,15 @@ struct ConnectUri { relays: Vec, secret: Option, permissions: Option, + /// True when this session was rebuilt from a saved vault row + /// (`reactivate_saved_sessions`) rather than dialed from a fresh URI. + /// A restored session re-sends `connect` with the ORIGINAL pairing + /// secret, but an already-approved signer answers `true` without + /// re-echoing it (NIP-46 reserves the echo for proving possession + /// during the initial pairing) — so the echo check is skipped and the + /// anti-spoofing is `expected_identity` in `adopt_identity` instead: + /// answering as any other account fails closed. + restore: bool, } /// The NIP-46 client signer. @@ -274,6 +283,7 @@ impl Nip46ClientSigner { relays, secret, permissions, + restore: false, }) } @@ -630,6 +640,11 @@ impl Nip46ClientSigner { relays, secret: connect_secret, permissions: None, + // Restored vault session: already-approved signers answer the + // re-`connect` with `true` instead of re-echoing the pairing + // secret; the handshake skips the echo and binds identity via + // `expected_identity` instead. + restore: true, }; let signer = self.clone(); let task = tokio::spawn(async move { @@ -1082,6 +1097,7 @@ impl Nip46ClientSigner { relays, secret: Some(secret), permissions: connection.permissions.clone(), + restore: false, }; let paired = { let mut inner = self.inner.lock().await; @@ -1565,8 +1581,13 @@ impl Nip46ClientSigner { let handshake = self.clone(); let peer = uri.peer; let expected_secret = uri.secret.clone(); + let restore = uri.restore; tokio::spawn(async move { - if let Err(e) = handshake.clone().run_handshake(peer, expected_secret).await { + if let Err(e) = handshake + .clone() + .run_handshake(peer, expected_secret, restore) + .await + { handshake.fail(e); } }); @@ -1998,6 +2019,7 @@ impl Nip46ClientSigner { self, peer: PublicKey, expected_secret: Option, + restore: bool, ) -> Result<(), String> { // Resolve the connect secret ON-DEMAND from the vault — the single // source of truth. Fail-closed: if the vault cannot produce the @@ -2053,16 +2075,31 @@ impl Nip46ClientSigner { // 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. + // + // RESTORED sessions are exempt from the echo: the secret they resend + // is the ORIGINAL pairing secret, and a signer that already approved + // this client answers `true` instead of re-echoing (an echo would + // re-prove possession that the initial pairing proved once). Real + // Amber does exactly this, so requiring the echo made every restart + // fail with "did not echo the connection secret". The restore path's + // anti-spoofing is `expected_identity`, enforced in adopt_identity: + // anything that answers as another account is refused, and only the + // key holder can decrypt our traffic on the stored conversation key. if let Some(expected) = &expected_secret { let trimmed = ack.trim(); - if trimmed != expected.as_str() { + if trimmed == expected.as_str() { + eprintln!("[NIP46] secret validation: PASS (connect ack echo matched)"); + } else if restore { + eprintln!( + "[NIP46] secret validation: SKIPPED (restored session; ack={trimmed}) — identity will be re-proved" + ); + } else { eprintln!("[NIP46] secret validation: FAIL (connect ack echo did not match)"); return Err( "The signer did not echo the connection secret; refusing the connection." .to_string(), ); } - eprintln!("[NIP46] secret validation: PASS (connect ack echo matched)"); } self.adopt_identity(peer).await diff --git a/tests/nip46_e2e.rs b/tests/nip46_e2e.rs index c818e12..86b281b 100644 --- a/tests/nip46_e2e.rs +++ b/tests/nip46_e2e.rs @@ -284,9 +284,18 @@ async fn run_fake_amber(relay_url: String, comms: Keys, identity: Keys, approval let response: Value = match method { "connect" => { - // Simulate a human tapping "approve" in Amber. + // Simulate a human tapping "approve" in Amber. Amber's + // ack SHAPE depends on the connection state: a first-time + // pairing (no secret in params) gets a plain ack; an + // ALREADY-APPROVED connection re-dialing with the stored + // secret gets `true` WITHOUT a secret echo (live Amber, + // Sep 25 — the client must not demand an echo there). tokio::time::sleep(approval_delay).await; - json!({"id": id, "result": "ack"}) + if req["params"].as_array().is_some_and(|p| p.len() > 1) { + json!({"id": id, "result": true}) + } else { + json!({"id": id, "result": "ack"}) + } } "get_public_key" => json!({"id": id, "result": identity.public_key().to_hex()}), "sign_event" => { @@ -1189,6 +1198,18 @@ async fn nip46_session_restore_redials_and_refuses_wrong_identity() { // (The old instance's listener task is left running on purpose — the // live process exiting is modeled by the new instance, not by // `disconnect()`, which revokes and wipes the stored key.) + // + // Seed a pairing secret at the identity ref first: a QR pairing stores + // one, and the restore re-sends it — real Amber then answers `true` + // WITHOUT echoing (already-approved connection), which the fake models. + // Without the restore-mode skip this handshake fails "did not echo the + // connection secret" (the exact live Sep 25 failure). + { + let mut g = app.lock().await; + keynectr::vault::store_connection_secret(&mut g.vault, None, &ref_id, "restore-secret-123") + .unwrap(); + g.save_vault().unwrap(); + } let signer2 = Nip46ClientSigner::new(app.clone()); let restored = signer2 .reactivate_saved_sessions()