fix(nip46): restored sessions no longer demand a connect secret re-echo
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.
This commit is contained in:
parent
929d791240
commit
01ce5de417
2 changed files with 63 additions and 5 deletions
|
|
@ -112,6 +112,15 @@ struct ConnectUri {
|
||||||
relays: Vec<RelayUrl>,
|
relays: Vec<RelayUrl>,
|
||||||
secret: Option<String>,
|
secret: Option<String>,
|
||||||
permissions: Option<Nip46Permissions>,
|
permissions: Option<Nip46Permissions>,
|
||||||
|
/// 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.
|
/// The NIP-46 client signer.
|
||||||
|
|
@ -274,6 +283,7 @@ impl Nip46ClientSigner {
|
||||||
relays,
|
relays,
|
||||||
secret,
|
secret,
|
||||||
permissions,
|
permissions,
|
||||||
|
restore: false,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -630,6 +640,11 @@ impl Nip46ClientSigner {
|
||||||
relays,
|
relays,
|
||||||
secret: connect_secret,
|
secret: connect_secret,
|
||||||
permissions: None,
|
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 signer = self.clone();
|
||||||
let task = tokio::spawn(async move {
|
let task = tokio::spawn(async move {
|
||||||
|
|
@ -1082,6 +1097,7 @@ impl Nip46ClientSigner {
|
||||||
relays,
|
relays,
|
||||||
secret: Some(secret),
|
secret: Some(secret),
|
||||||
permissions: connection.permissions.clone(),
|
permissions: connection.permissions.clone(),
|
||||||
|
restore: false,
|
||||||
};
|
};
|
||||||
let paired = {
|
let paired = {
|
||||||
let mut inner = self.inner.lock().await;
|
let mut inner = self.inner.lock().await;
|
||||||
|
|
@ -1565,8 +1581,13 @@ impl Nip46ClientSigner {
|
||||||
let handshake = self.clone();
|
let handshake = self.clone();
|
||||||
let peer = uri.peer;
|
let peer = uri.peer;
|
||||||
let expected_secret = uri.secret.clone();
|
let expected_secret = uri.secret.clone();
|
||||||
|
let restore = uri.restore;
|
||||||
tokio::spawn(async move {
|
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);
|
handshake.fail(e);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
@ -1998,6 +2019,7 @@ impl Nip46ClientSigner {
|
||||||
self,
|
self,
|
||||||
peer: PublicKey,
|
peer: PublicKey,
|
||||||
expected_secret: Option<String>,
|
expected_secret: Option<String>,
|
||||||
|
restore: bool,
|
||||||
) -> Result<(), String> {
|
) -> Result<(), String> {
|
||||||
// Resolve the connect secret ON-DEMAND from the vault — the single
|
// Resolve the connect secret ON-DEMAND from the vault — the single
|
||||||
// source of truth. Fail-closed: if the vault cannot produce the
|
// 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://
|
// Anti-spoofing: per NIP-46, a signer answering a nostrconnect://
|
||||||
// (secret-carrying) handshake must echo the secret back. A mismatch
|
// (secret-carrying) handshake must echo the secret back. A mismatch
|
||||||
// means something other than the intended signer answered.
|
// 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 {
|
if let Some(expected) = &expected_secret {
|
||||||
let trimmed = ack.trim();
|
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)");
|
eprintln!("[NIP46] secret validation: FAIL (connect ack echo did not match)");
|
||||||
return Err(
|
return Err(
|
||||||
"The signer did not echo the connection secret; refusing the connection."
|
"The signer did not echo the connection secret; refusing the connection."
|
||||||
.to_string(),
|
.to_string(),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
eprintln!("[NIP46] secret validation: PASS (connect ack echo matched)");
|
|
||||||
}
|
}
|
||||||
|
|
||||||
self.adopt_identity(peer).await
|
self.adopt_identity(peer).await
|
||||||
|
|
|
||||||
|
|
@ -284,10 +284,19 @@ async fn run_fake_amber(relay_url: String, comms: Keys, identity: Keys, approval
|
||||||
|
|
||||||
let response: Value = match method {
|
let response: Value = match method {
|
||||||
"connect" => {
|
"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;
|
tokio::time::sleep(approval_delay).await;
|
||||||
|
if req["params"].as_array().is_some_and(|p| p.len() > 1) {
|
||||||
|
json!({"id": id, "result": true})
|
||||||
|
} else {
|
||||||
json!({"id": id, "result": "ack"})
|
json!({"id": id, "result": "ack"})
|
||||||
}
|
}
|
||||||
|
}
|
||||||
"get_public_key" => json!({"id": id, "result": identity.public_key().to_hex()}),
|
"get_public_key" => json!({"id": id, "result": identity.public_key().to_hex()}),
|
||||||
"sign_event" => {
|
"sign_event" => {
|
||||||
let unsigned_json = req["params"].get(0).and_then(|v| v.as_str());
|
let unsigned_json = req["params"].get(0).and_then(|v| v.as_str());
|
||||||
|
|
@ -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
|
// (The old instance's listener task is left running on purpose — the
|
||||||
// live process exiting is modeled by the new instance, not by
|
// live process exiting is modeled by the new instance, not by
|
||||||
// `disconnect()`, which revokes and wipes the stored key.)
|
// `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 signer2 = Nip46ClientSigner::new(app.clone());
|
||||||
let restored = signer2
|
let restored = signer2
|
||||||
.reactivate_saved_sessions()
|
.reactivate_saved_sessions()
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue