diff --git a/crates/bsk-cli/src/daemon/local_auth/handshake.rs b/crates/bsk-cli/src/daemon/local_auth/handshake.rs index 5a7930bc..b5a361c2 100644 --- a/crates/bsk-cli/src/daemon/local_auth/handshake.rs +++ b/crates/bsk-cli/src/daemon/local_auth/handshake.rs @@ -26,11 +26,8 @@ pub(crate) async fn accept( .as_ref() .and_then(|p| p.get("version")) .and_then(|v| v.as_str()); - let capable = version - .and_then(|v| semver::Version::parse(v).ok()) - .is_some_and(|v| v >= semver::Version::new(0, 4, 0)); - state.record_unpaired(version, capable); if !local.mode().accepts_legacy() { + state.record_unpaired(version); reply( ws, &request.id, @@ -71,7 +68,9 @@ pub(crate) async fn accept( let probe = Probe::parse(&first)?; let hello = probe.params.local_auth; let Some(initial) = local.store.select(&hello.credentials) else { - state.record_unpaired(None, true); + if !local.mode().accepts_legacy() { + state.record_unpaired(None); + } challenge( ws, &probe.id, @@ -306,6 +305,15 @@ async fn durable_commit( txn: bsk_protocol::local_auth::Bytes<16>, instance: String, ) -> Result<()> { + // Publish the paired identity before another unverified registration can + // claim it. Reuse the gate already shared by registration and revocation. + let _registry = state + .local_auth + .lifecycle + .registry_gate + .clone() + .lock_owned() + .await; let local = state.local_auth.clone(); let replaced = tokio::task::spawn_blocking(move || local.store.commit(&id, txn, &instance)).await??; diff --git a/crates/bsk-cli/src/daemon/local_auth/store.rs b/crates/bsk-cli/src/daemon/local_auth/store.rs index 46d79f30..67be3d7f 100644 --- a/crates/bsk-cli/src/daemon/local_auth/store.rs +++ b/crates/bsk-cli/src/daemon/local_auth/store.rs @@ -224,6 +224,14 @@ impl PairingStore { pub fn resolve(&self, id: &str) -> Result { resolve(&self.state.lock().unwrap(), id) } + pub fn has_paired_browser(&self, instance: &str) -> bool { + self.state + .lock() + .unwrap() + .devices + .values() + .any(|device| device.browser_id.as_deref() == Some(instance)) + } pub fn revoke(&self, id: Option<&str>) -> Result> { self.update(|state| { let ids = match id { diff --git a/crates/bsk-cli/src/daemon/peer_authorization.rs b/crates/bsk-cli/src/daemon/peer_authorization.rs index 87d74676..5dfe1413 100644 --- a/crates/bsk-cli/src/daemon/peer_authorization.rs +++ b/crates/bsk-cli/src/daemon/peer_authorization.rs @@ -1,5 +1,5 @@ //! Local HMAC and remote TLS/Bearer authorization share the business driver. -use super::local_auth::{LocalAuth, store::DeviceStatus}; +use super::local_auth::{LocalAuth, LocalAuthMode, store::DeviceStatus}; use std::sync::Arc; pub(crate) struct LocalPeer { @@ -66,22 +66,19 @@ impl PeerAuthorization { pub async fn registration_guard( &self, ) -> Option<( - tokio::sync::OwnedMutexGuard<()>, + Option>, tokio::sync::OwnedMutexGuard<()>, )> { - if let Self::Local(peer) = self { - let device = peer.local.device_gate(&peer.device_id).lock_owned().await; - let registry = peer - .local - .lifecycle - .registry_gate - .clone() - .lock_owned() - .await; - Some((device, registry)) - } else { - None - } + let (device, local) = match self { + Self::Local(peer) => ( + Some(peer.local.device_gate(&peer.device_id).lock_owned().await), + &peer.local, + ), + Self::Legacy(local) => (None, local), + Self::Remote(_) => return None, + }; + let registry = local.lifecycle.registry_gate.clone().lock_owned().await; + Some((device, registry)) } pub fn register(&self, client: &Arc) { if let Self::Local(peer) = self { @@ -91,7 +88,15 @@ impl PeerAuthorization { } } pub fn matches_instance(&self, instance: &str) -> bool { - !matches!(self, Self::Local(peer) if peer.browser_id != instance) + match self { + Self::Local(peer) => peer.browser_id == instance, + // Revoking a key does not turn its identity into an unpaired one. + Self::Legacy(local) => { + local.mode() == LocalAuthMode::TrustLocal + || !local.store.has_paired_browser(instance) + } + Self::Remote(_) => true, + } } } diff --git a/crates/bsk-cli/src/daemon/state.rs b/crates/bsk-cli/src/daemon/state.rs index b14291d9..9621ebcf 100644 --- a/crates/bsk-cli/src/daemon/state.rs +++ b/crates/bsk-cli/src/daemon/state.rs @@ -90,10 +90,7 @@ impl DaemonState { } impl DaemonState { - pub fn record_unpaired(&self, version: Option<&str>, pairing_capable: bool) { - if !pairing_capable && self.local_auth.mode() == super::local_auth::LocalAuthMode::Warn { - return; - } + pub fn record_unpaired(&self, version: Option<&str>) { let extension_version = version .and_then(|v| semver::Version::parse(v).ok()) .map(|v| v.to_string()); @@ -109,6 +106,9 @@ impl DaemonState { )); } pub fn recent_unpaired(&self) -> Option { + if self.local_auth.mode().accepts_legacy() { + return None; + } self.last_unpaired_attempt .lock() .unwrap() diff --git a/crates/bsk-cli/tests/auto_update_handover.rs b/crates/bsk-cli/tests/auto_update_handover.rs index 3accf2c9..2e5429dd 100644 --- a/crates/bsk-cli/tests/auto_update_handover.rs +++ b/crates/bsk-cli/tests/auto_update_handover.rs @@ -31,6 +31,7 @@ const MARKER: &[u8] = b"auto-update-handover-fixture"; struct ReleaseServer { url: String, downloads: Arc, + hold_archive: Arc, stop: Arc, worker: Option>, } @@ -58,9 +59,11 @@ impl ReleaseServer { .unwrap(); let stop = Arc::new(AtomicBool::new(false)); let downloads = Arc::new(AtomicUsize::new(0)); + let hold_archive = Arc::new(AtomicBool::new(false)); let worker = { let stop = Arc::clone(&stop); let downloads = Arc::clone(&downloads); + let hold_archive = Arc::clone(&hold_archive); let archive_request = format!("GET /bsk{suffix} "); thread::spawn(move || { while !stop.load(Ordering::SeqCst) { @@ -86,6 +89,9 @@ impl ReleaseServer { } let body = if request.starts_with(archive_request.as_bytes()) { downloads.fetch_add(1, Ordering::SeqCst); + while hold_archive.load(Ordering::SeqCst) && !stop.load(Ordering::SeqCst) { + thread::sleep(Duration::from_millis(10)); + } thread::sleep(archive_delay); &archive } else { @@ -103,6 +109,7 @@ impl ReleaseServer { Self { url: format!("{base}/version.json"), downloads, + hold_archive, stop, worker: Some(worker), } @@ -496,6 +503,141 @@ fn auto_update_exits_only_after_the_new_daemon_serves() { ); } +#[test] +fn l11b_revocation_during_auto_update_survives_process_handover() { + use bsk::daemon::local_auth::store::{PairingStore, Pending, now, random}; + use bsk_protocol::local_auth::*; + use bsk_protocol::{ResponseBody, ResponseFrame}; + use futures_util::{SinkExt, StreamExt}; + use tokio_tungstenite::tungstenite::{Message, client::IntoClientRequest}; + + let fixture = Fixture::new(newer_bsk); + fixture.server.hold_archive.store(true, Ordering::SeqCst); + let lock = bsk::daemon::lockfile::acquire_at(&fixture.home).unwrap(); + let store = PairingStore::open(&lock).unwrap(); + store.pair(52800, None).unwrap(); + let id = store.devices()[0].device_id.clone(); + let pending = Pending { + txn: random(), + r#gen: 1, + key: random(), + created_at: now(), + }; + store + .stage_rotation(&id, 0, None, pending.clone(), "a1b2c3d4") + .unwrap(); + store.commit(&id, pending.txn, "a1b2c3d4").unwrap(); + drop(store); + drop(lock); + + let port = unused_port(); + let old_pid = fixture.start_daemon(port); + fixture.wait_for("an update download in flight", || { + fixture.server.downloads.load(Ordering::SeqCst) == 1 + }); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let probe = || { + runtime.block_on(async { + let mut request = format!("ws://127.0.0.1:{port}/") + .into_client_request() + .unwrap(); + request.headers_mut().insert( + "Origin", + "chrome-extension://abcdefghijklmnopabcdefghijklmnop" + .parse() + .unwrap(), + ); + let (mut socket, _) = tokio_tungstenite::connect_async(request).await.unwrap(); + let hello = Probe { + id: "handover".into(), + method: ProbeMethod::Ping, + params: ProbeParams { + local_auth: Hello { + v: V1, + client_nonce: random(), + instance_id: "a1b2c3d4".into(), + credentials: vec![Credential { + role: CredentialRole::Active, + device_id: Bytes::decode(&id).unwrap(), + r#gen: 1, + pending: None, + }], + }, + }, + }; + socket + .send(Message::Text(serde_json::to_string(&hello).unwrap())) + .await + .unwrap(); + let reply = tokio::time::timeout(Duration::from_secs(5), socket.next()) + .await + .unwrap() + .unwrap() + .unwrap(); + let frame: ResponseFrame = serde_json::from_str(&reply.into_text().unwrap()).unwrap(); + let ResponseBody::Ok(value) = frame.body else { + panic!("expected authentication reply") + }; + let response: ChallengeResponse = + serde_json::from_value(value["local_auth"].clone()).unwrap(); + let _ = socket.close(None).await; + response + }) + }; + assert!(matches!(probe(), ChallengeResponse::Challenge { .. })); + + let revoked = fixture + .command() + .args(["daemon", "revoke", &id]) + .output() + .unwrap(); + assert!( + revoked.status.success(), + "{}", + String::from_utf8_lossy(&revoked.stderr) + ); + assert_eq!(fixture.info().unwrap()["pid"], old_pid); + fixture.server.hold_archive.store(false, Ordering::SeqCst); + fixture.wait_for("the replacement daemon to load the revocation", || { + fixture + .info() + .is_some_and(|info| info["pid"] != old_pid && info["ws_port"] == port) + }); + fixture.wait_for("the revoked device's previous daemon to exit", || { + fixture.daemon_exited() + }); + assert_eq!(fixture.record().unwrap()["result"], "succeeded"); + assert!(matches!( + probe(), + ChallengeResponse::PairingRequired { + legacy_accepted: true, + .. + } + )); + let devices = fixture + .command() + .args(["daemon", "devices"]) + .output() + .unwrap(); + assert!(devices.status.success()); + let devices: serde_json::Value = serde_json::from_slice(&devices.stdout).unwrap(); + assert_eq!(devices[0]["device_id"], id); + assert_eq!(devices[0]["status"], "revoked"); + let browsers = fixture + .command() + .args(["--json", "browsers"]) + .output() + .unwrap(); + assert!(browsers.status.success()); + assert_eq!( + serde_json::from_slice::(&browsers.stdout).unwrap(), + serde_json::json!([]) + ); +} + #[test] fn a_failed_handover_restores_the_previous_executable_and_keeps_serving() { let fixture = Fixture::new(release_whose_daemon_fails); diff --git a/crates/bsk-cli/tests/local_auth_ws.rs b/crates/bsk-cli/tests/local_auth_ws.rs index 106eca77..82c73992 100644 --- a/crates/bsk-cli/tests/local_auth_ws.rs +++ b/crates/bsk-cli/tests/local_auth_ws.rs @@ -241,11 +241,76 @@ async fn authenticate(socket: &mut Socket, pair: &mut Pairing) { } pair.role = CredentialRole::Active; } +fn handshake_request(instance: &str) -> Value { + json!({"id":"handshake","method":"system.handshake","params":{ + "client":"browser-skill-extension","version":env!("CARGO_PKG_VERSION"), + "protocol_version":bsk::daemon::state::PROTOCOL_VERSION,"instance_id":instance, + "browser":{"name":"Chrome","version":"130"},"label":"test"}}) +} async fn handshake(socket: &mut Socket, instance: &str) -> Value { - send(socket,json!({"id":"handshake","method":"system.handshake","params":{"client":"browser-skill-extension","version":env!("CARGO_PKG_VERSION"),"protocol_version":bsk::daemon::state::PROTOCOL_VERSION,"instance_id":instance,"browser":{"name":"Chrome","version":"130"},"label":"test"}})).await; + send(socket, handshake_request(instance)).await; serde_json::from_str(&receive(socket).await).unwrap() } +#[tokio::test] +async fn optional_pairing_preserves_unpaired_use_but_protects_paired_and_revoked_identities() { + for revoked in [false, true] { + let handle = daemon::run(DaemonConfig::new(0), None).await.unwrap(); + assert_eq!(handle.state().local_auth.mode(), LocalAuthMode::Warn); + let mut unpaired = connect(&handle).await; + assert!( + handshake(&mut unpaired, INSTANCE) + .await + .get("result") + .is_some() + ); + unpaired.close(None).await.unwrap(); + + let mut pairing = Pairing::create(&handle, None); + let mut paired = connect(&handle).await; + authenticate(&mut paired, &mut pairing).await; + assert!( + handshake(&mut paired, INSTANCE) + .await + .get("result") + .is_some() + ); + let state = handle.state(); + let browser = state.browsers.select(Some(INSTANCE)).unwrap(); + let session = state + .sessions + .reserve_id(browser.id.clone(), 1, || 0) + .unwrap(); + if revoked { + daemon::local_auth::lifecycle::revoke(state.clone(), Some(pairing.device.encode())) + .await + .unwrap(); + closed(&mut paired).await; + } + let mut impostor = connect(&handle).await; + send(&mut impostor, handshake_request(INSTANCE)).await; + closed(&mut impostor).await; + if revoked { + assert!(state.browsers.is_empty()); + } else { + assert_eq!( + state.browsers.select(Some(INSTANCE)).unwrap().generation, + browser.generation + ); + assert!(state.sessions.get(&session).is_some()); + } + // Pairing one browser must not force an unrelated old extension to pair. + let mut other = connect(&handle).await; + assert!( + handshake(&mut other, "12345678") + .await + .get("result") + .is_some() + ); + handle.shutdown().await; + } +} + #[tokio::test] async fn l01_l02_no_registration_or_session_purge_without_a_valid_proof() { let handle = daemon().await; @@ -608,9 +673,11 @@ async fn l17_legacy_handshake_obeys_daemon_policy() { let result = handshake(&mut socket, INSTANCE).await; if mode == LocalAuthMode::Required { assert_eq!(result["error"]["code"], "pairing_required"); + assert!(handle.state().recent_unpaired().is_some()); closed(&mut socket).await; } else { assert!(result.get("result").is_some()); + assert!(handle.state().recent_unpaired().is_none()); socket.close(None).await.unwrap(); } handle.shutdown().await; @@ -691,20 +758,19 @@ async fn l10_revocation_after_authentication_prevents_late_registration() { } #[tokio::test] -async fn warn_records_only_pairing_capable_unpaired_attempts() { - let state = daemon::DaemonState::new(DaemonConfig::new(0).with_local_auth(LocalAuthMode::Warn)); - state.record_unpaired(Some("0.3.0"), false); - assert!(state.recent_unpaired().is_none()); - state.record_unpaired(Some("0.4.0"), true); - let status = state.local_auth_status().unwrap(); - assert_eq!(status.mode, "warn"); - assert_eq!(status.paired_devices, 0); - assert_eq!( - status - .last_unpaired_attempt - .unwrap() - .extension_version - .as_deref(), - Some("0.4.0") - ); +async fn optional_mode_does_not_report_required_pairing_from_stale_attempts() { + for mode in [ + LocalAuthMode::Warn, + LocalAuthMode::Required, + LocalAuthMode::TrustLocal, + ] { + let state = daemon::DaemonState::new(DaemonConfig::new(0).with_local_auth(mode)); + state.record_unpaired(Some("0.4.1")); + let status = state.local_auth_status().unwrap(); + assert_eq!(status.paired_devices, 0); + assert_eq!( + status.last_unpaired_attempt.is_some(), + mode == LocalAuthMode::Required + ); + } }