diff --git a/keep-mobile/src/nip46.rs b/keep-mobile/src/nip46.rs index 235baf96..e6721b56 100644 --- a/keep-mobile/src/nip46.rs +++ b/keep-mobile/src/nip46.rs @@ -11,10 +11,45 @@ use keep_nip46::{ ServerConfig, SignerHandler, }; use nostr_sdk::{Kind, PublicKey}; -use std::sync::atomic::{AtomicU8, Ordering}; +use std::sync::atomic::{AtomicU8, AtomicUsize, Ordering}; use std::sync::Arc; +use std::time::Duration; use zeroize::{Zeroize, Zeroizing}; +/// How long this side waits for the foreign prompt before denying. +/// +/// The same 60 seconds keep-cli, keep-web and keep-desktop each apply to their +/// own approval channel, so no answer means a denial on every surface rather +/// than on three of four. +/// +/// [`BunkerCallbacks::request_approval`] still requires the implementation to +/// bound its own wait, and keep-android does. This is the backstop for one that +/// does not: without it the contract is unenforceable from here, and an +/// implementation that waits forever stops every later request from every +/// client, because the handler reads one at a time. +/// +/// Worth knowing when reading a denial: this and the platform's own 60 seconds +/// are two timers started at nearly the same moment, so an answer that arrives +/// within a hair of the deadline can be denied here after the operator +/// approved it. Both orderings end in a denial when nobody answers, which is +/// the direction that matters, and the window is a few milliseconds wide. +const APPROVAL_TIMEOUT: Duration = Duration::from_secs(60); + +/// How many prompts may be waiting on the platform at once. +/// +/// The handler reads one request at a time, so more than one is outstanding +/// only when earlier prompts timed out and their threads are still parked in a +/// platform call that never returned. Against such an implementation the +/// threads accumulate at roughly one per [`APPROVAL_TIMEOUT`], each holding a +/// stack, a JVM attachment and a copy of the request, which trades a stalled +/// bunker for memory pressure on the whole app. Past this many, the answer is +/// a refusal without asking. +/// +/// keep-android bounds its own wait and caps its own concurrent prompts, so +/// this is not reached in practice; it is the ceiling for an implementation +/// that does neither. +const MAX_OUTSTANDING_APPROVALS: usize = 4; + const STATUS_STOPPED: u8 = 0; const STATUS_STARTING: u8 = 1; const STATUS_RUNNING: u8 = 2; @@ -167,6 +202,11 @@ pub trait BunkerCallbacks: Send + Sync { /// (keep-android waits 60 seconds). The NIP-46 handler reads one request at /// a time, so a wait that never ends stops every later request from every /// client, not just this one. + /// + /// That requirement is now backed rather than merely stated: an answer that + /// does not arrive within [`APPROVAL_TIMEOUT`] is treated as a refusal. An + /// implementation that bounds its own wait, as keep-android does, decides + /// its own outcome; one that does not gets denied for it. fn request_approval(&self, request: BunkerApprovalRequest) -> BunkerApprovalResult; /// Fired when an app completes the NIP-46 connect handshake. `pubkey` and /// `name` are untrusted, remote-derived values: render them as inert text, @@ -175,9 +215,47 @@ pub trait BunkerCallbacks: Send + Sync { fn on_connect(&self, pubkey: String, name: String, authorization: ConnectAuthorization); } +/// One outstanding prompt, released when the platform answers. +struct OutstandingApproval(Arc); + +impl OutstandingApproval { + fn take(count: &Arc) -> Option { + count + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |in_flight| { + (in_flight < MAX_OUTSTANDING_APPROVALS).then_some(in_flight + 1) + }) + .ok() + .map(|_| Self(Arc::clone(count))) + } +} + +impl Drop for OutstandingApproval { + fn drop(&mut self) { + self.0.fetch_sub(1, Ordering::AcqRel); + } +} + struct CallbackBridge { callbacks: Arc, mobile: Arc, + /// How long to wait for the foreign prompt. A field rather than the + /// constant directly so a test can reach the timeout without taking a + /// minute to do it. + approval_timeout: Duration, + /// Prompts handed to the platform that have not come back, including the + /// ones this side has already stopped waiting for. + outstanding: Arc, +} + +impl CallbackBridge { + /// A refusal that grants nothing. `JustThisTime` carries no duration, so + /// the handler cannot turn it into a stored denial or a grant. + fn refused() -> ApprovalResult { + ApprovalResult { + approved: false, + remember: RememberDuration::JustThisTime, + } + } } impl ServerCallbacks for CallbackBridge { @@ -203,16 +281,68 @@ impl ServerCallbacks for CallbackBridge { method: d.method, }), }; - // The foreign implementation blocks until the operator answers or its - // own timeout fires, so hand this worker's core off for the wait, as - // the FROST approval path does in `lib.rs`. Queued tasks on the worker - // are stealable either way; what `block_in_place` adds is moving the - // one task in the LIFO slot back to the run queue and sending the core - // to another thread, so the worker's work keeps draining instead of - // waiting out the prompt. Later NIP-46 requests stall regardless, since - // the handler's notification loop reads one at a time. + // A synchronous foreign call cannot be interrupted, so the wait is + // bounded by making it somebody else's thread and timing out the + // answer. The thread outlives the timeout: it stays parked in the + // platform prompt until that returns, and its answer is dropped. That + // is the cost of bounding a call that offers no cancellation, and it is + // preferable to the alternative, where one unanswered prompt holds the + // handler forever. + // + // Kept for the log, because the request itself moves onto the thread + // and a refusal that does not say which app it refused is not much use + // when reading one back. The name is left out deliberately: it is + // remote-derived and unbounded, where the pubkey and method are + // neither. + let asked_by = request.app_pubkey.clone(); + let asked_for = request.method.clone(); + + // A slot is taken before the thread exists, and released by the guard + // when the platform finally answers, so the count tracks threads still + // parked rather than requests still being waited on. + let slot = match OutstandingApproval::take(&self.outstanding) { + Some(slot) => slot, + None => { + tracing::warn!( + "Refusing {asked_for} from {asked_by} without asking: \ + {MAX_OUTSTANDING_APPROVALS} earlier prompts have not come back" + ); + return Self::refused(); + } + }; + + let (tx, rx) = std::sync::mpsc::channel(); + let callbacks = Arc::clone(&self.callbacks); + // Named, and a failure to spawn is a refusal rather than a panic. + // `std::thread::spawn` panics when the OS refuses the thread, and that + // panic unwinds through the handler and out of the notification loop, + // which stops the bunker while its status still reads running. + if let Err(e) = std::thread::Builder::new() + .name("nip46-approval".into()) + .spawn(move || { + // Dropped when the platform returns, however it returns, so a + // callback that panics releases its slot too. + let _slot = slot; + // Ignored deliberately: a send fails only once the timeout + // below has already answered for this request and dropped the + // receiver. + let _ = tx.send(callbacks.request_approval(request)); + }) + { + tracing::error!("Refusing {asked_for} from {asked_by}, no thread to ask on: {e}"); + return Self::refused(); + } + + // Hand this worker's core off for the wait, as the FROST approval path + // does in `lib.rs`. Queued tasks on the worker are stealable either + // way; what `block_in_place` adds is moving the one task in the LIFO + // slot back to the run queue and sending the core to another thread, so + // the worker's work keeps draining instead of waiting out the prompt. + // Later NIP-46 requests stall regardless, since the handler's + // notification loop reads one at a time; what the timeout changes is + // that the stall is now bounded. // - // It also leaves the runtime for the duration of the call, so a foreign + // It also leaves the runtime for the duration of the wait, so a foreign // callback that re-enters an exported method built on // `runtime.block_on` stops panicking and can deadlock instead if it // waits on this runtime. Nothing in keep-android does that today. @@ -226,12 +356,31 @@ impl ServerCallbacks for CallbackBridge { tokio::runtime::Handle::try_current().map(|h| h.runtime_flavor()), Ok(tokio::runtime::RuntimeFlavor::MultiThread) ); - if blocking { - tokio::task::block_in_place(|| self.callbacks.request_approval(request)) + let answered = if blocking { + tokio::task::block_in_place(|| rx.recv_timeout(self.approval_timeout)) } else { - self.callbacks.request_approval(request) + rx.recv_timeout(self.approval_timeout) + }; + + match answered { + Ok(result) => result.into(), + // Denied, not retried. The operator did not answer, and the + // request's own rate limiting decides whether the client gets + // another prompt. The two errors are reported apart because they + // are different events: one is nobody answering, the other is the + // platform callback having panicked, which fails closed at once. + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + tracing::warn!( + "Refusing {asked_for} from {asked_by}: no answer within {}s", + self.approval_timeout.as_secs() + ); + Self::refused() + } + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + tracing::error!("Refusing {asked_for} from {asked_by}: the prompt did not return"); + Self::refused() + } } - .into() } fn on_connect( @@ -682,6 +831,8 @@ impl BunkerHandler { }; let server_callbacks = Some(Arc::new(CallbackBridge { + approval_timeout: APPROVAL_TIMEOUT, + outstanding: Arc::new(AtomicUsize::new(0)), callbacks, mobile: Arc::clone(&self.mobile), }) as Arc); @@ -925,6 +1076,174 @@ mod tests { assert_eq!(*secret, *secret2); } + /// A prompt that is never answered is a denial, not a wait without end. + /// + /// The implementation is required to bound its own wait and keep-android + /// does, so this is the backstop for one that does not. Before it, the + /// handler read the foreign answer straight through and a callback that + /// never returned stopped every later request from every client. + #[test] + fn an_unanswered_prompt_is_denied_rather_than_waited_on() { + struct NeverAnswers(Mutex>); + impl BunkerCallbacks for NeverAnswers { + fn on_log(&self, _event: BunkerLogEvent) {} + fn request_approval(&self, _request: BunkerApprovalRequest) -> BunkerApprovalResult { + // Parked until the test ends, which is what an implementation + // with no timeout of its own does. + let _ = self.0.lock().unwrap().recv(); + BunkerApprovalResult { + approved: true, + remember: BunkerRememberDuration::Forever, + } + } + fn on_connect(&self, _pubkey: String, _name: String, _a: ConnectAuthorization) {} + } + + let storage: Arc = Arc::new(MemStorage::default()); + let mobile = Arc::new(KeepMobile::new(Arc::clone(&storage)).unwrap()); + // Held so the sender outlives the call and the callback stays parked + // rather than waking on a disconnect. + let (_hold, rx) = std::sync::mpsc::channel(); + // `mobile` stays owned here: it holds a runtime of its own, and + // letting the bridge carry the last reference into the async block + // below drops that runtime from inside an async context, which panics. + let bridge = CallbackBridge { + callbacks: Arc::new(NeverAnswers(Mutex::new(rx))), + mobile: Arc::clone(&mobile), + approval_timeout: Duration::from_millis(200), + outstanding: Arc::new(AtomicUsize::new(0)), + }; + + let started = std::time::Instant::now(); + let rt = tokio::runtime::Builder::new_multi_thread() + .worker_threads(1) + .enable_all() + .build() + .unwrap(); + let result = + rt.block_on(async { ServerCallbacks::request_approval(&bridge, an_approval()) }); + + assert!( + !result.approved, + "an unanswered prompt must not be approved" + ); + assert!( + started.elapsed() < Duration::from_secs(5), + "the wait must be bounded by the timeout, not by the callback" + ); + } + + /// Prompts that never came back do not accumulate without limit. Past the + /// ceiling the answer is a refusal without asking, so a platform that + /// never returns costs a bounded number of parked threads rather than + /// steady memory and thread pressure on the whole app. + #[test] + fn prompts_that_never_return_stop_being_asked() { + struct NeverAnswers(Mutex>); + impl BunkerCallbacks for NeverAnswers { + fn on_log(&self, _event: BunkerLogEvent) {} + fn request_approval(&self, _request: BunkerApprovalRequest) -> BunkerApprovalResult { + let _ = self.0.lock().unwrap().recv(); + BunkerApprovalResult { + approved: true, + remember: BunkerRememberDuration::Forever, + } + } + fn on_connect(&self, _pubkey: String, _name: String, _a: ConnectAuthorization) {} + } + + let storage: Arc = Arc::new(MemStorage::default()); + let mobile = Arc::new(KeepMobile::new(Arc::clone(&storage)).unwrap()); + let (_hold, rx) = std::sync::mpsc::channel(); + let outstanding = Arc::new(AtomicUsize::new(0)); + let bridge = CallbackBridge { + callbacks: Arc::new(NeverAnswers(Mutex::new(rx))), + mobile: Arc::clone(&mobile), + approval_timeout: Duration::from_millis(50), + outstanding: Arc::clone(&outstanding), + }; + + let rt = tokio::runtime::Builder::new_multi_thread() + .worker_threads(1) + .enable_all() + .build() + .unwrap(); + rt.block_on(async { + for _ in 0..MAX_OUTSTANDING_APPROVALS + 3 { + assert!(!ServerCallbacks::request_approval(&bridge, an_approval()).approved); + } + }); + + // Every slot is held by a thread still parked in the platform call, and + // nothing above the ceiling was handed out. + assert_eq!( + outstanding.load(Ordering::Acquire), + MAX_OUTSTANDING_APPROVALS, + "outstanding prompts must stop at the ceiling" + ); + } + + /// A slot is released however the platform call ends, including when it + /// panics, so a panicking callback does not retire a slot for good. + #[test] + fn a_panicking_prompt_releases_its_slot() { + struct Panics; + impl BunkerCallbacks for Panics { + fn on_log(&self, _event: BunkerLogEvent) {} + fn request_approval(&self, _request: BunkerApprovalRequest) -> BunkerApprovalResult { + panic!("the platform callback failed"); + } + fn on_connect(&self, _pubkey: String, _name: String, _a: ConnectAuthorization) {} + } + + let storage: Arc = Arc::new(MemStorage::default()); + let mobile = Arc::new(KeepMobile::new(Arc::clone(&storage)).unwrap()); + let outstanding = Arc::new(AtomicUsize::new(0)); + let bridge = CallbackBridge { + callbacks: Arc::new(Panics), + mobile: Arc::clone(&mobile), + approval_timeout: Duration::from_secs(5), + outstanding: Arc::clone(&outstanding), + }; + + let rt = tokio::runtime::Builder::new_multi_thread() + .worker_threads(1) + .enable_all() + .build() + .unwrap(); + rt.block_on(async { + for _ in 0..MAX_OUTSTANDING_APPROVALS + 2 { + assert!( + !ServerCallbacks::request_approval(&bridge, an_approval()).approved, + "a panicking prompt is a refusal" + ); + } + }); + + assert_eq!( + outstanding.load(Ordering::Acquire), + 0, + "a panicking callback must not retire its slot" + ); + } + + fn an_approval() -> ApprovalRequest { + ApprovalRequest { + app_pubkey: PublicKey::from_slice( + keep_core::keys::NostrKeypair::generate() + .unwrap() + .public_bytes(), + ) + .unwrap(), + app_name: "an app".into(), + method: "sign_event".into(), + event_kind: None, + event_content: None, + requested_permissions: None, + http_auth: None, + } + } + // Two properties of the approval bridge, in the two runtime shapes that // can reach it. // @@ -957,6 +1276,8 @@ mod tests { let bridge = |rx| CallbackBridge { callbacks: Arc::new(Waiter(Mutex::new(Some(rx)))), mobile: Arc::clone(&mobile), + approval_timeout: APPROVAL_TIMEOUT, + outstanding: Arc::new(AtomicUsize::new(0)), }; let ask = |bridge: &CallbackBridge| { ServerCallbacks::request_approval(