From cf6553fa6dd7496d8b7038cea6db13340199154d Mon Sep 17 00:00:00 2001 From: cid Date: Fri, 14 Aug 2026 11:37:54 -0700 Subject: [PATCH 1/7] fix(berdctl): authenticate broker requests Require a fresh per-process bearer capability on every loopback route and secure its discovery transport and restart behavior. Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- docs/berdctl-architecture.md | 32 +- src-tauri/Cargo.lock | 5 + src-tauri/README.md | 8 +- src-tauri/crates/berdctl/Cargo.toml | 3 + .../crates/berdctl/api-surface-feedback.json | 2 +- src-tauri/crates/berdctl/api-surface.json | 2 +- src-tauri/crates/berdctl/src/client.rs | 439 +++++++++++++++++- src-tauri/crates/berdctl/src/discovery.rs | 256 +++++++++- src-tauri/crates/berdctl/src/validate.rs | 4 +- src-tauri/plugins/berdctl/Cargo.toml | 8 +- src-tauri/plugins/berdctl/src/discovery.rs | 292 ++++++++++-- src-tauri/plugins/berdctl/src/lib.rs | 14 +- src-tauri/plugins/berdctl/src/server.rs | 138 +++++- src-tauri/src/services/berdctl_discovery.rs | 8 +- src/features/berdctl/commands/contract.ts | 2 +- 15 files changed, 1100 insertions(+), 113 deletions(-) diff --git a/docs/berdctl-architecture.md b/docs/berdctl-architecture.md index 8e7480172..110262ee6 100644 --- a/docs/berdctl-architecture.md +++ b/docs/berdctl-architecture.md @@ -10,16 +10,18 @@ berdctl project create --name demo The implementation has three layers: 1. CLI: `src-tauri/crates/berdctl/` - Parses flags with clap, prints help, reads the app discovery file, and sends - JSON calls. CLI validation is convenience only. + Parses flags with clap, prints help, reads the private app discovery file, + authenticates each loopback request with its per-broker capability, and + sends JSON calls. CLI validation is convenience only. 2. Broker: `src-tauri/plugins/berdctl/` - Runs a localhost server inside the app, rejects browser-origin requests, - enforces in-flight and timeout limits, and forwards calls to the renderer - without command-specific logic. + Runs a localhost server inside the app, requires the current discovery-file + capability, rejects browser-origin requests, enforces in-flight and timeout + limits, and forwards calls to the renderer without command-specific logic. 3. Renderer registry: `src/features/berdctl/commands/` Strict-parses args with zod, runs guards, executes through app state, and - returns JSON results. This is the trust boundary because any same-user - process can bypass the CLI and POST to the broker directly. + returns JSON results. This remains the command-policy trust boundary; the + broker capability limits access to processes that can read the owning + user's private discovery file. ## Layer rules @@ -87,9 +89,13 @@ belongs in error messages, not generic help text. ## Safety model -v1 has no auth tokens and no confirmation dialogs. That remains acceptable only -while mutations are visible in the UI and either reversible or direct -user-requested product actions, such as creating a session or sending a prompt. +v1 requires a fresh 256-bit bearer capability for every broker start. The +plugin writes it beside the port and generation in the discovery file, with +owner-only directory/file permissions on Unix, and the CLI presents it on +both `/v1/ping` and `/v1/call`. Missing, malformed, wrong, stale, symlinked, +or non-private capability records fail closed. This authenticates possession +of the app-issued session endpoint; it does not replace renderer command +policy or add interactive confirmation dialogs. Required command properties: @@ -108,8 +114,10 @@ piecemeal auth in a command PR. ## Versioning -The broker writes a discovery file with `protocolVersion`, generation, and port. -The CLI verifies it via `/v1/ping` before calls. +The broker writes a private discovery file with `protocolVersion`, generation, +port, and a per-start capability. The CLI authenticates and verifies it via +`/v1/ping` before calls. Requiring that capability is a breaking wire reshape, +so the authenticated surface starts at protocol version 5. Breaking wire reshapes must bump all three constants: diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 4d2ee9298..1e2af39b5 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -583,6 +583,7 @@ version = "0.6.0" dependencies = [ "clap", "indexmap 2.13.1", + "libc", "serde", "serde_json", "ureq 3.4.0", @@ -6583,10 +6584,14 @@ name = "tauri-plugin-berdctl" version = "0.6.0" dependencies = [ "axum", + "getrandom 0.4.3", + "hex", + "libc", "log", "reqwest 0.13.4", "serde", "serde_json", + "subtle", "tauri", "tauri-plugin", "tokio", diff --git a/src-tauri/README.md b/src-tauri/README.md index c95a93255..627653cbd 100644 --- a/src-tauri/README.md +++ b/src-tauri/README.md @@ -15,8 +15,9 @@ The Tauri 2 shell: the app crate (`src/`), the berdctl workspace crates The CLI embeds the contract artifacts (`crates/berdctl/api-surface.json` + `cli-surface.json`) and builds its clap tree at startup. It locates the -broker through the `BERDCTL_LOCK` discovery file, verifies -`protocolVersion`/generation via `GET /v1/ping`, and sends +broker through the `BERDCTL_LOCK` discovery file, reads its per-start +capability, verifies `protocolVersion`/generation through an authenticated +`GET /v1/ping`, and sends authenticated `POST /v1/call {"command", "args"}`. The broker forwards to the renderer over Tauri IPC (`berdctl:request` event out, `submit_result` back). Command dispatch, zod validation, guards, and execution live in the @@ -41,7 +42,8 @@ capability grants a permission allowing that command. window. This ACL gates webview → Rust IPC only; the localhost HTTP side is governed -separately (discovery file, header rejection, global caps). +separately by the owner-private discovery capability, browser/DNS-rebinding +header rejection, and global caps. Stock Tauri 2 plugin layout. Docs: [Plugin Development](https://v2.tauri.app/develop/plugins/), diff --git a/src-tauri/crates/berdctl/Cargo.toml b/src-tauri/crates/berdctl/Cargo.toml index b30b8d2a1..8a553d81e 100644 --- a/src-tauri/crates/berdctl/Cargo.toml +++ b/src-tauri/crates/berdctl/Cargo.toml @@ -18,6 +18,9 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" ureq = { version = "3", features = ["json"] } +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [features] default = [] block-feedback = [] diff --git a/src-tauri/crates/berdctl/api-surface-feedback.json b/src-tauri/crates/berdctl/api-surface-feedback.json index e3e404ac5..9238d72b8 100644 --- a/src-tauri/crates/berdctl/api-surface-feedback.json +++ b/src-tauri/crates/berdctl/api-surface-feedback.json @@ -1,6 +1,6 @@ { "$comment": "GENERATED FILE — do not hand-edit; run `pnpm generate:berdctl-contract`. Client-neutral wire surface of the Berd desktop app's control API: POST /v1/call {\"command\": \"\", \"args\": {\"action\": \"\", ...fields}} against the loopback endpoint in the berdctl discovery file. protocolVersion mirrors PROTOCOL_VERSION in both discovery.rs copies (berdctl and plugin crate tests pin them equal). Per action: description, fields (flat wire model: name, required, kind, values, description, bounds), and schema (JSON Schema 2020-12 of the args object, minus the action discriminator). Derived from the authoritative zod schemas in the colocated command modules (src/features/berdctl/commands/impl/*.ts); the renderer registry's strict zod parse — not this file — is the trust boundary. vitest asserts freshness (src/features/berdctl/__tests__/apiSurface.test.ts).", - "protocolVersion": 4, + "protocolVersion": 5, "groups": { "sessions": { "description": "Manage the user's chat sessions: create (fire-and-forget, on any installed agent harness), send, open, list, get, rename, move, move to group, clear project, fork, archive.", diff --git a/src-tauri/crates/berdctl/api-surface.json b/src-tauri/crates/berdctl/api-surface.json index 18c5163b0..7272470a2 100644 --- a/src-tauri/crates/berdctl/api-surface.json +++ b/src-tauri/crates/berdctl/api-surface.json @@ -1,6 +1,6 @@ { "$comment": "GENERATED FILE — do not hand-edit; run `pnpm generate:berdctl-contract`. Client-neutral wire surface of the Berd desktop app's control API: POST /v1/call {\"command\": \"\", \"args\": {\"action\": \"\", ...fields}} against the loopback endpoint in the berdctl discovery file. protocolVersion mirrors PROTOCOL_VERSION in both discovery.rs copies (berdctl and plugin crate tests pin them equal). Per action: description, fields (flat wire model: name, required, kind, values, description, bounds), and schema (JSON Schema 2020-12 of the args object, minus the action discriminator). Derived from the authoritative zod schemas in the colocated command modules (src/features/berdctl/commands/impl/*.ts); the renderer registry's strict zod parse — not this file — is the trust boundary. vitest asserts freshness (src/features/berdctl/__tests__/apiSurface.test.ts).", - "protocolVersion": 4, + "protocolVersion": 5, "groups": { "sessions": { "description": "Manage the user's chat sessions: create (fire-and-forget, on any installed agent harness), send, open, list, get, rename, move, move to group, clear project, fork, archive.", diff --git a/src-tauri/crates/berdctl/src/client.rs b/src-tauri/crates/berdctl/src/client.rs index 6383d2986..796cc1791 100644 --- a/src-tauri/crates/berdctl/src/client.rs +++ b/src-tauri/crates/berdctl/src/client.rs @@ -62,11 +62,26 @@ pub struct PingResponse { pub struct Endpoint { pub port: u16, + capability: String, +} + +impl std::fmt::Debug for Endpoint { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("Endpoint") + .field("port", &self.port) + .field("capability", &"[redacted]") + .finish() + } } fn agent(timeout: Duration) -> ureq::Agent { ureq::Agent::config_builder() .timeout_global(Some(timeout)) + // The broker is a literal loopback service. Never hand its bearer + // capability to a user-configured proxy or redirect target. + .proxy(None) + .max_redirects(0) // Non-2xx responses carry the broker's structured error body; read it // instead of treating the status as a transport error. .http_status_as_error(false) @@ -74,53 +89,112 @@ fn agent(timeout: Duration) -> ureq::Agent { .new_agent() } +#[derive(Debug)] +struct PingFailure { + detail: String, + status: Option, +} + +impl PingFailure { + fn transport(detail: String) -> Self { + Self { + detail, + status: None, + } + } + + fn status(detail: String, status: u16) -> Self { + Self { + detail, + status: Some(status), + } + } +} + /// Probe the listener before sending any payload (command args can contain /// prompt text, which must not be sprayed at an unknown local service). /// Returns the failure detail only; callers decide the exit class. -pub fn ping(port: u16) -> Result { +fn ping(port: u16, capability: &str) -> Result { let url = format!("http://127.0.0.1:{port}/v1/ping"); let mut response = agent(PING_TIMEOUT) .get(&url) + .header("Authorization", format!("Bearer {capability}")) .call() - .map_err(|err| format!("nothing answered on 127.0.0.1:{port} ({err})"))?; + .map_err(|err| { + PingFailure::transport(format!("nothing answered on 127.0.0.1:{port} ({err})")) + })?; let status = response.status().as_u16(); if status != 200 { - return Err(format!( - "the listener on 127.0.0.1:{port} does not look like the Berd app \ - control endpoint (ping returned status {status})" + return Err(PingFailure::status( + format!( + "the listener on 127.0.0.1:{port} does not look like the Berd app \ + control endpoint (ping returned status {status})" + ), + status, )); } response .body_mut() .read_json::() .map_err(|err| { - format!( - "the listener on 127.0.0.1:{port} does not look like the Berd app \ - control endpoint (unrecognized ping response: {err})" + PingFailure::status( + format!( + "the listener on 127.0.0.1:{port} does not look like the Berd app \ + control endpoint (unrecognized ping response: {err})" + ), + status, ) }) } /// Read the discovery file and verify the broker behind it echoes the file's -/// generation and this binary's protocol version. A generation mismatch means -/// the file was read across a broker restart: re-read once and retry once. +/// generation and this binary's protocol version. A generation mismatch or +/// authentication failure can mean the file was read across a broker restart: +/// re-read once and retry once. pub fn handshake(lock_path: &Path) -> Result { let mut file = discovery::load_with_retry(lock_path)?; for attempt in 0..2 { if file.protocol_version != PROTOCOL_VERSION { return Err(Failure::env(APP_UPDATED)); } - let ping = ping(file.port).map_err(|detail| { - Failure::env(format!( - "the Berd desktop app is not reachable: {detail}. The app may have \ - quit; {CONTROL_REMEDIATION}" - )) - })?; + let ping = match ping(file.port, &file.capability) { + Ok(ping) => ping, + Err(failure) if failure.status == Some(403) => { + if attempt == 0 { + // Authentication failure can be the observable edge of a + // broker restart: the process has rotated the capability but + // this command opened the previous discovery inode. Re-read + // once, just as for the existing generation-mismatch path. + file = discovery::load(lock_path).map_err(|err| { + Failure::env(format!( + "the Berd desktop app restarted its control endpoint and the new \ + one could not be read ({err}); {CONTROL_REMEDIATION}" + )) + })?; + continue; + } + return Err(Failure::env(format!( + "the Berd desktop app is not reachable: {}. The app may have \ + quit; {CONTROL_REMEDIATION}", + failure.detail + ))); + } + Err(failure) => { + return Err(Failure::env(format!( + "the Berd desktop app is not reachable: {}. The app may have \ + quit; {CONTROL_REMEDIATION}", + failure.detail + ))); + } + }; if ping.protocol_version != PROTOCOL_VERSION { return Err(Failure::env(APP_UPDATED)); } if ping.generation == file.generation { - return Ok(Endpoint { port: file.port }); + return Ok(Endpoint { + port: file.port, + capability: file.capability, + }); } if attempt == 0 { file = discovery::load(lock_path).map_err(|err| { @@ -155,6 +229,7 @@ pub fn call( } let mut response = agent(CALL_TIMEOUT) .post(&url) + .header("Authorization", format!("Bearer {}", endpoint.capability)) .send_json(Value::Object(payload)) .map_err(|err| transport_error_failure(endpoint.port, &err))?; let status = response.status().as_u16(); @@ -247,6 +322,336 @@ fn error_parts(value: &Value) -> Option<(String, String)> { #[cfg(test)] mod tests { use super::*; + use std::io::{BufRead, BufReader, Read, Write}; + use std::net::{TcpListener, TcpStream}; + use std::path::PathBuf; + use std::sync::{mpsc, Arc}; + use std::thread; + + const CURRENT_CAPABILITY: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + const STALE_CAPABILITY: &str = + "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + + struct TempDiscoveryFile(PathBuf); + + impl TempDiscoveryFile { + fn new(label: &str, port: u16, capability: &str) -> Self { + let base = std::env::temp_dir().join(format!( + "berdctl-client-auth-{}-{label}-{port}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).expect("create discovery directory"); + let path = base.join("control.json"); + std::fs::write( + &path, + format!( + r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{capability}"}}"# + ), + ) + .expect("write discovery file"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions( + path.parent().expect("test discovery has a parent"), + std::fs::Permissions::from_mode(0o700), + ) + .expect("make discovery directory private"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("make discovery file private"); + } + Self(path) + } + } + + impl Drop for TempDiscoveryFile { + fn drop(&mut self) { + if let Some(parent) = self.0.parent() { + std::fs::remove_dir_all(parent).ok(); + } + } + } + + struct RecordedRequest { + request_line: String, + authorization: Option, + body: String, + } + + fn read_request(stream: &mut TcpStream) -> RecordedRequest { + let mut reader = BufReader::new(stream.try_clone().expect("clone request stream")); + let mut request_line = String::new(); + reader + .read_line(&mut request_line) + .expect("read request line"); + let mut authorization = None; + let mut content_length = 0; + loop { + let mut line = String::new(); + reader.read_line(&mut line).expect("read request header"); + if line == "\r\n" { + break; + } + let Some((name, value)) = line.trim_end().split_once(':') else { + continue; + }; + if name.eq_ignore_ascii_case("authorization") { + authorization = Some(value.trim().to_string()); + } + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().expect("valid content length"); + } + } + let mut body = vec![0; content_length]; + reader.read_exact(&mut body).expect("read request body"); + RecordedRequest { + request_line: request_line.trim_end().to_string(), + authorization, + body: String::from_utf8(body).expect("request body is UTF-8"), + } + } + + fn write_response(stream: &mut TcpStream, status: &str, body: &str) { + write!( + stream, + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ) + .expect("write test response"); + } + + fn spawn_broker_sequence( + expected_requests: Vec<(&'static str, BrokerResponse)>, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + spawn_broker_responses(expected_requests) + } + + #[derive(Clone, Copy)] + enum BrokerResponse { + Ping { generation: u64 }, + Call, + } + + fn spawn_broker_responses( + expected_requests: Vec<(&'static str, BrokerResponse)>, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + spawn_broker_responses_with_sync(expected_requests, None) + } + + fn spawn_broker_responses_with_sync( + expected_requests: Vec<(&'static str, BrokerResponse)>, + first_response_sync: Option<(Arc, Arc)>, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind test broker"); + let port = listener.local_addr().expect("test broker address").port(); + let (requests_tx, requests_rx) = mpsc::channel(); + let handle = thread::spawn(move || { + for (request_number, (expected_capability, response)) in + expected_requests.into_iter().enumerate() + { + let (mut stream, _) = listener.accept().expect("accept client request"); + let request = read_request(&mut stream); + let authorized = request.authorization.as_deref() + == Some(&format!("Bearer {expected_capability}")); + requests_tx.send(request).expect("record client request"); + if request_number == 0 { + if let Some((request_seen, response_ready)) = &first_response_sync { + request_seen.wait(); + response_ready.wait(); + } + } + match (authorized, response) { + (true, BrokerResponse::Ping { generation }) => write_response( + &mut stream, + "200 OK", + &format!( + r#"{{"generation":{generation},"protocolVersion":{PROTOCOL_VERSION}}}"# + ), + ), + (true, BrokerResponse::Call) => { + write_response(&mut stream, "200 OK", r#"{"ok":true,"result":"ok"}"#) + } + (false, _) => write_response( + &mut stream, + "403 Forbidden", + r#"{"ok":false,"error":{"code":"forbidden","message":"valid bearer capability required"}}"#, + ), + } + } + }); + (port, requests_rx, handle) + } + + fn spawn_broker( + expected_capability: &'static str, + request_count: usize, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + spawn_broker_responses( + (0..request_count) + .map(|request_number| { + let response = if request_number == 0 { + BrokerResponse::Ping { generation: 7 } + } else { + BrokerResponse::Call + }; + (expected_capability, response) + }) + .collect(), + ) + } + + #[test] + fn handshake_and_call_send_current_capability() { + let (port, requests, broker) = spawn_broker(CURRENT_CAPABILITY, 2); + let lock_file = TempDiscoveryFile::new("current", port, CURRENT_CAPABILITY); + + let endpoint = handshake(&lock_file.0).expect("current capability handshakes"); + assert_eq!( + format!("{endpoint:?}"), + format!("Endpoint {{ port: {port}, capability: \"[redacted]\" }}"), + "debug output must not disclose the bearer capability" + ); + let result = call( + &endpoint, + "sessions", + Map::from_iter([("action".to_string(), Value::String("list".to_string()))]), + None, + ) + .expect("current capability calls"); + assert_eq!(result, Value::String("ok".to_string())); + + let ping = requests.recv().expect("record ping"); + assert_eq!(ping.request_line, "GET /v1/ping HTTP/1.1"); + assert_eq!( + ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + assert!(ping.body.is_empty()); + + let call = requests.recv().expect("record call"); + assert_eq!(call.request_line, "POST /v1/call HTTP/1.1"); + assert_eq!( + call.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + let call_body: Value = serde_json::from_str(&call.body).expect("call body is JSON"); + assert_eq!(call_body["command"], "sessions"); + assert_eq!(call_body["args"]["action"], "list"); + + broker.join().expect("test broker exits"); + } + + #[test] + fn handshake_recovers_when_capability_rotates_after_discovery_read() { + let first_request = Arc::new(std::sync::Barrier::new(2)); + let response_ready = Arc::new(std::sync::Barrier::new(2)); + let (port, requests, broker) = spawn_broker_responses_with_sync( + vec![ + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + ], + Some((first_request.clone(), response_ready.clone())), + ); + let lock_file = TempDiscoveryFile::new("rotating", port, STALE_CAPABILITY); + let path = lock_file.0.clone(); + let rewrite_first_request = first_request.clone(); + let rewrite_response_ready = response_ready.clone(); + let rewrite = thread::spawn(move || { + rewrite_first_request.wait(); + std::fs::write( + &path, + format!( + r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{CURRENT_CAPABILITY}"}}"# + ), + ) + .expect("publish rotated discovery capability"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("keep rewritten discovery private"); + } + rewrite_response_ready.wait(); + let stale_ping = requests.recv().expect("record stale ping"); + assert_eq!( + stale_ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + requests.recv().expect("record retried current ping") + }); + + let endpoint = handshake(&lock_file.0).expect("rotated capability retries successfully"); + assert_eq!(endpoint.port, port); + let current_ping = rewrite.join().expect("discovery rewrite exits"); + assert_eq!( + current_ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + broker.join().expect("test broker exits"); + } + + #[test] + fn handshake_retries_generation_mismatch_with_rotated_capability() { + let (port, requests, broker) = spawn_broker_sequence(vec![ + (STALE_CAPABILITY, BrokerResponse::Ping { generation: 6 }), + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + ]); + let lock_file = TempDiscoveryFile::new("generation-race", port, STALE_CAPABILITY); + let path = lock_file.0.clone(); + let rewrite = thread::spawn(move || { + let stale_ping = requests.recv().expect("record old-generation ping"); + assert_eq!( + stale_ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + std::fs::write( + &path, + format!( + r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{CURRENT_CAPABILITY}"}}"# + ), + ) + .expect("publish new generation and capability"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("keep rewritten discovery private"); + } + requests.recv().expect("record new-generation ping") + }); + + let endpoint = handshake(&lock_file.0).expect("generation mismatch retries successfully"); + assert_eq!(endpoint.port, port); + let current_ping = rewrite.join().expect("discovery rewrite exits"); + assert_eq!( + current_ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + broker.join().expect("test broker exits"); + } + + #[test] + fn handshake_rejects_stale_capability() { + // The first 403 triggers the one permitted discovery re-read; an + // unchanged stale record must still fail closed on the second probe. + let (port, requests, broker) = spawn_broker(CURRENT_CAPABILITY, 2); + let lock_file = TempDiscoveryFile::new("stale", port, STALE_CAPABILITY); + + let failure = handshake(&lock_file.0).expect_err("stale capability fails closed"); + assert_eq!(failure.exit, EXIT_ENV); + assert!(failure.message.contains("ping returned status 403")); + for _ in 0..2 { + let ping = requests.recv().expect("record stale ping"); + assert_eq!( + ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + } + + broker.join().expect("test broker exits"); + } #[test] fn ok_true_yields_the_result_verbatim() { diff --git a/src-tauri/crates/berdctl/src/discovery.rs b/src-tauri/crates/berdctl/src/discovery.rs index 078d63e1f..c7b9a6c79 100644 --- a/src-tauri/crates/berdctl/src/discovery.rs +++ b/src-tauri/crates/berdctl/src/discovery.rs @@ -1,4 +1,5 @@ -//! Discovery-file resolution: how berdctl finds the app's control endpoint. +//! Discovery-file resolution: how berdctl finds and authenticates to the +//! app's control endpoint. use std::path::{Path, PathBuf}; use std::time::Duration; @@ -11,7 +12,7 @@ use crate::client::Failure; /// `PROTOCOL_VERSION` in the `tauri-plugin-berdctl` crate /// (src-tauri/plugins/berdctl) — the CLI does not depend on the plugin /// crate; bump both copies together. -pub const PROTOCOL_VERSION: u32 = 4; +pub const PROTOCOL_VERSION: u32 = 5; /// Exact wording pinned by the implementation spec: the missing env var is the /// provenance signal that we are not running under the app. @@ -19,17 +20,51 @@ pub const NOT_UNDER_APP: &str = "berdctl must run inside a Berd desktop app session (the app sets this up automatically)"; const REREAD_DELAY: Duration = Duration::from_millis(200); +const CAPABILITY_HEX_LEN: usize = 64; /// Shape of the discovery file the berdctl broker writes on start /// (`/berdctl/control-.json`). Duplicated by hand from /// the writer's struct in `tauri-plugin-berdctl`; keep in sync. #[derive(Debug, Deserialize, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", try_from = "RawDiscoveryFile")] pub struct DiscoveryFile { pub port: u16, pub pid: u32, pub generation: u64, pub protocol_version: u32, + pub capability: String, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawDiscoveryFile { + port: u16, + pid: u32, + generation: u64, + protocol_version: u32, + capability: String, +} + +impl TryFrom for DiscoveryFile { + type Error = String; + + fn try_from(raw: RawDiscoveryFile) -> Result { + if raw.capability.len() != CAPABILITY_HEX_LEN + || !raw + .capability + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) + { + return Err("capability must be a 256-bit hexadecimal value".to_string()); + } + Ok(Self { + port: raw.port, + pid: raw.pid, + generation: raw.generation, + protocol_version: raw.protocol_version, + capability: raw.capability, + }) + } } /// The lock path comes from `--lock-path` or `BERDCTL_LOCK` (clap merges @@ -46,11 +81,83 @@ pub fn parse(contents: &str) -> Result { } pub fn load(path: &Path) -> Result { - let contents = std::fs::read_to_string(path) - .map_err(|err| format!("cannot read {}: {err}", path.display()))?; + let contents = read_private_discovery_file(path)?; parse(&contents) } +#[cfg(unix)] +fn read_private_discovery_file(path: &Path) -> Result { + use std::io::Read; + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + + const MAX_DISCOVERY_BYTES: u64 = 4096; + + // Check the containing directory first. Once it is owner-private, another + // user cannot replace the final path while it is opened below. + let parent = path + .parent() + .ok_or_else(|| format!("{} has no parent directory", path.display()))?; + let parent_metadata = std::fs::symlink_metadata(parent) + .map_err(|err| format!("cannot inspect {}: {err}", parent.display()))?; + // SAFETY: `geteuid` takes no arguments and has no preconditions. + let current_uid = unsafe { libc::geteuid() }; + if !parent_metadata.file_type().is_dir() + || parent_metadata.uid() != current_uid + || parent_metadata.mode() & 0o077 != 0 + { + return Err(format!( + "{} is not an owner-private directory (expected mode 0700)", + parent.display() + )); + } + + // O_NOFOLLOW makes the final symlink check atomic with opening the file. + // O_NONBLOCK keeps a malicious FIFO from blocking before metadata reveals + // that it is not a regular file. + let file = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(path) + .map_err(|err| format!("cannot open {}: {err}", path.display()))?; + let metadata = file + .metadata() + .map_err(|err| format!("cannot inspect {}: {err}", path.display()))?; + if !metadata.file_type().is_file() { + return Err(format!("{} is not a regular file", path.display())); + } + if metadata.uid() != current_uid { + return Err(format!( + "{} is not owned by the current user", + path.display() + )); + } + if metadata.mode() & 0o077 != 0 { + return Err(format!( + "{} is accessible by other users (expected mode 0600)", + path.display() + )); + } + if metadata.len() > MAX_DISCOVERY_BYTES { + return Err(format!("{} is unexpectedly large", path.display())); + } + + // Limit the read too: the handle may grow after the metadata check, but it + // must never make berdctl allocate an unbounded discovery record. + let mut contents = String::new(); + file.take(MAX_DISCOVERY_BYTES + 1) + .read_to_string(&mut contents) + .map_err(|err| format!("cannot read {}: {err}", path.display()))?; + if contents.len() as u64 > MAX_DISCOVERY_BYTES { + return Err(format!("{} is unexpectedly large", path.display())); + } + Ok(contents) +} + +#[cfg(not(unix))] +fn read_private_discovery_file(path: &Path) -> Result { + std::fs::read_to_string(path).map_err(|err| format!("cannot read {}: {err}", path.display())) +} + /// The broker writes the file atomically, so a read/parse failure is either /// transient (broker restarting) or means the app is gone; one short retry /// distinguishes the two. @@ -74,7 +181,8 @@ pub fn load_with_retry(path: &Path) -> Result { mod tests { use super::*; - const VALID: &str = r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1}"#; + const CAPABILITY: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + const VALID: &str = r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}"#; #[test] fn parses_a_valid_discovery_file() { @@ -86,14 +194,17 @@ mod tests { pid: 4242, generation: 3, protocol_version: 1, + capability: CAPABILITY.to_string(), } ); } #[test] fn tolerates_unknown_fields_for_forward_compat() { - let file = parse(r#"{"port":1,"pid":2,"generation":3,"protocolVersion":1,"token":"x"}"#) - .expect("unknown fields are ignored"); + let file = parse( + r#"{"port":1,"pid":2,"generation":3,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef","future":"x"}"#, + ) + .expect("unknown fields are ignored"); assert_eq!(file.port, 1); } @@ -106,14 +217,37 @@ mod tests { #[test] fn rejects_missing_fields() { assert!(parse(r#"{"port":52341,"pid":4242}"#).is_err()); + assert!( + parse(r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1}"#).is_err(), + "legacy discovery without a capability must fail closed" + ); assert!(parse(r#"{}"#).is_err()); } #[test] - fn rejects_wrongly_typed_fields() { + fn rejects_wrongly_typed_or_malformed_fields() { + assert!(parse( + r#"{"port":"not-a-port","pid":1,"generation":1,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}"# + ) + .is_err()); assert!( - parse(r#"{"port":"not-a-port","pid":1,"generation":1,"protocolVersion":1}"#).is_err() + parse(r#"{"port":1,"pid":1,"generation":1,"protocolVersion":1,"capability":123}"#) + .is_err() ); + for capability in [ + "", + "short", + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF", + ] { + let contents = format!( + r#"{{"port":1,"pid":1,"generation":1,"protocolVersion":1,"capability":"{capability}"}}"# + ); + assert!( + parse(&contents).is_err(), + "malformed capability {capability:?} must fail closed" + ); + } } #[test] @@ -135,4 +269,106 @@ mod tests { .expect("present path resolves"); assert_eq!(path, PathBuf::from("/tmp/control-1.json")); } + + #[cfg(unix)] + #[test] + fn load_accepts_private_discovery_file_from_shared_working_directory() { + use std::os::unix::fs::PermissionsExt; + + let base = + std::env::temp_dir().join(format!("berdctl-discovery-private-{}", std::process::id())); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let path = base.join("control.json"); + std::fs::write(&path, VALID).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + assert_eq!(load(&path).expect("private discovery loads").port, 52341); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_permissive_discovery_file() { + use std::os::unix::fs::PermissionsExt; + + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-permissions-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let path = base.join("control.json"); + std::fs::write(&path, VALID).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + let error = load(&path).expect_err("world-readable capability must fail closed"); + assert!(error.contains("accessible by other users")); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_permissive_discovery_directory() { + use std::os::unix::fs::PermissionsExt; + + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-directory-permissions-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let path = base.join("control.json"); + std::fs::write(&path, VALID).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o755)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + let error = load(&path).expect_err("shared discovery directory must fail closed"); + assert!(error.contains("not an owner-private directory")); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_symlinked_discovery_file() { + use std::os::unix::fs::{symlink, PermissionsExt}; + + let base = + std::env::temp_dir().join(format!("berdctl-discovery-symlink-{}", std::process::id())); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + let target = base.join("target.json"); + let link = base.join("control.json"); + std::fs::write(&target, VALID).unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o600)).unwrap(); + symlink(&target, &link).unwrap(); + assert!(load(&link).is_err(), "symlink must fail closed"); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_non_regular_and_oversized_discovery_files() { + use std::os::unix::fs::PermissionsExt; + + let base = + std::env::temp_dir().join(format!("berdctl-discovery-shape-{}", std::process::id())); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + + let directory_path = base.join("control-dir"); + std::fs::create_dir(&directory_path).unwrap(); + assert!(load(&directory_path).is_err(), "directory must fail closed"); + + let oversized_path = base.join("control-large.json"); + std::fs::write(&oversized_path, vec![b'x'; 4097]).unwrap(); + std::fs::set_permissions(&oversized_path, std::fs::Permissions::from_mode(0o600)).unwrap(); + let error = load(&oversized_path).expect_err("oversized discovery must fail closed"); + assert!(error.contains("unexpectedly large")); + + std::fs::remove_dir_all(base).ok(); + } } diff --git a/src-tauri/crates/berdctl/src/validate.rs b/src-tauri/crates/berdctl/src/validate.rs index 9b4ae3cde..80bf464d0 100644 --- a/src-tauri/crates/berdctl/src/validate.rs +++ b/src-tauri/crates/berdctl/src/validate.rs @@ -192,7 +192,7 @@ mod tests { use crate::contract::Contract; const MINIMAL_API: &str = r#"{ - "protocolVersion": 4, + "protocolVersion": 5, "groups": { "sessions": { "description": "Manage the user's chat sessions.", @@ -374,7 +374,7 @@ mod tests { #[test] fn mismatched_protocol_version_is_reported() { - let api = MINIMAL_API.replace("\"protocolVersion\": 4", "\"protocolVersion\": 999"); + let api = MINIMAL_API.replace("\"protocolVersion\": 5", "\"protocolVersion\": 999"); let errors = errors_for(&api, MINIMAL_SURFACE); assert_one_error_containing(&errors, "protocolVersion 999 does not match"); } diff --git a/src-tauri/plugins/berdctl/Cargo.toml b/src-tauri/plugins/berdctl/Cargo.toml index e6fc4980c..299da4bca 100644 --- a/src-tauri/plugins/berdctl/Cargo.toml +++ b/src-tauri/plugins/berdctl/Cargo.toml @@ -10,18 +10,24 @@ name = "tauri_plugin_berdctl" path = "src/lib.rs" [dependencies] +getrandom = { version = "0.4", optional = true } +hex = { version = "0.4", optional = true } log = "0.4" serde = { version = "1", features = ["derive"] } serde_json = "1" +subtle = { version = "2", optional = true } tauri = { version = "2", default-features = false } tokio = { version = "1", features = ["sync", "time", "rt", "net"] } uuid = { version = "1", features = ["v4"] } axum = { version = "0.8", default-features = false, features = ["http1", "json", "tokio"], optional = true } +[target.'cfg(unix)'.dependencies] +libc = { version = "0.2", optional = true } + [features] # Without `server` the crate compiles to an inert stub: permissions are still # generated by build.rs, but no runtime code (including `init`) exists. -server = ["dep:axum"] +server = ["dep:axum", "dep:getrandom", "dep:hex", "dep:libc", "dep:subtle"] [dev-dependencies] reqwest = { version = "0.13", default-features = false, features = ["json"] } diff --git a/src-tauri/plugins/berdctl/src/discovery.rs b/src-tauri/plugins/berdctl/src/discovery.rs index e1ea06495..a64a7be2a 100644 --- a/src-tauri/plugins/berdctl/src/discovery.rs +++ b/src-tauri/plugins/berdctl/src/discovery.rs @@ -1,5 +1,6 @@ -//! Per-instance discovery ("lock") file the berdctl CLI reads to find the -//! running broker: `{port, pid, generation, protocolVersion}`. +//! Per-instance discovery ("lock") file the berdctl CLI reads to find and +//! authenticate to the running broker: `{port, pid, generation, +//! protocolVersion, capability}`. //! //! The path formula and protocol version are exported unconditionally (not //! behind the `server` feature) so the app crate can compute the path for the @@ -12,17 +13,21 @@ use std::path::{Path, PathBuf}; /// (src-tauri/crates/berdctl); the CLI does not depend on this crate — /// bump both together. #[cfg_attr(not(feature = "server"), allow(dead_code))] -pub const PROTOCOL_VERSION: u32 = 4; +pub const PROTOCOL_VERSION: u32 = 5; /// Directory under the app data dir holding the per-instance discovery files. pub const DISCOVERY_DIR_NAME: &str = "berdctl"; const DISCOVERY_FILE_PREFIX: &str = "control-"; const DISCOVERY_FILE_SUFFIX: &str = ".json"; -/// A crash between the temp-file write and the atomic rename below leaves -/// `control-.json.tmp` behind; the app crate's stale-file sweep owns -/// those orphans too. -const DISCOVERY_TEMP_SUFFIX: &str = ".json.tmp"; +/// A crash between a temp-file write and its atomic rename can leave either +/// the legacy fixed-name `control-.json.tmp` orphan or the current +/// `control-.json..tmp` orphan. The app crate's stale-file sweep +/// owns both forms. +const LEGACY_DISCOVERY_TEMP_SUFFIX: &str = ".json.tmp"; +const DISCOVERY_TEMP_MARKER: &str = ".json."; +const DISCOVERY_TEMP_SUFFIX: &str = ".tmp"; +const DISCOVERY_TEMP_NONCE_HEX_LEN: usize = 32; /// `/berdctl/control-.json`. Per-instance (pid /// suffix): dev worktrees share a bundle identifier, so a well-known filename @@ -33,24 +38,82 @@ pub fn discovery_file_path(app_data_dir: &Path, pid: u32) -> PathBuf { )) } -/// Owning app pid encoded in a discovery file name: `control-.json` or -/// its orphaned temp form `control-.json.tmp`. `None` for anything else. +/// Owning app pid encoded in a discovery file name. Recognized forms are the +/// final `control-.json`, legacy `control-.json.tmp`, and current +/// `control-.json.<32 lowercase hex chars>.tmp` orphan names. `None` for +/// anything else, so the stale-file sweep cannot delete unrelated files. pub fn owner_pid_from_discovery_file_name(name: &str) -> Option { let stem = name.strip_prefix(DISCOVERY_FILE_PREFIX)?; - stem.strip_suffix(DISCOVERY_TEMP_SUFFIX) - .or_else(|| stem.strip_suffix(DISCOVERY_FILE_SUFFIX))? - .parse() - .ok() + let pid = if let Some(pid) = stem.strip_suffix(DISCOVERY_FILE_SUFFIX) { + pid + } else if let Some(pid) = stem.strip_suffix(LEGACY_DISCOVERY_TEMP_SUFFIX) { + pid + } else { + let (pid, nonce_with_suffix) = stem.split_once(DISCOVERY_TEMP_MARKER)?; + let nonce = nonce_with_suffix.strip_suffix(DISCOVERY_TEMP_SUFFIX)?; + if nonce.len() != DISCOVERY_TEMP_NONCE_HEX_LEN + || !nonce + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) + { + return None; + } + pid + }; + pid.parse().ok() +} + +#[cfg(feature = "server")] +fn private_discovery_directory(dir: &Path) -> std::io::Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + + // Refuse to follow a symlink or repair a directory after it has been + // swapped out from under the checked path. + let handle = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_DIRECTORY) + .open(dir)?; + let metadata = handle.metadata()?; + // SAFETY: `geteuid` takes no arguments and has no preconditions. + let current_uid = unsafe { libc::geteuid() }; + if !metadata.file_type().is_dir() || metadata.uid() != current_uid { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!( + "discovery directory {} is not owned by the current user", + dir.display() + ), + )); + } + handle.set_permissions(unix_permissions(0o700))?; + Ok(()) + } + #[cfg(not(unix))] + { + let metadata = std::fs::symlink_metadata(dir)?; + if !metadata.file_type().is_dir() { + return Err(std::io::Error::other(format!( + "discovery directory {} is not a directory", + dir.display() + ))); + } + Ok(()) + } } /// Atomically write the discovery file: private dir + temp file + fsync + -/// rename, so a CLI reading mid-write never sees partial JSON. +/// rename, so a CLI reading mid-write never sees partial JSON. The capability +/// is sensitive to other users on the host, so Unix paths are tightened to +/// owner-only access even when they predate this write. #[cfg(feature = "server")] pub(crate) fn write_discovery_file( path: &Path, port: u16, pid: u32, generation: u64, + capability: &str, ) -> std::io::Result<()> { use std::io::Write; @@ -65,33 +128,87 @@ pub(crate) fn write_discovery_file( dir_builder.mode(0o700); } dir_builder.create(dir)?; + private_discovery_directory(dir)?; let payload = serde_json::json!({ "port": port, "pid": pid, "generation": generation, "protocolVersion": PROTOCOL_VERSION, + "capability": capability, }); - let mut tmp_name = path - .file_name() - .map(std::ffi::OsStr::to_os_string) - .unwrap_or_default(); - tmp_name.push(".tmp"); - let tmp = path.with_file_name(tmp_name); + // Use a unique adjacent path for each write. A stale fixed-name temp file + // must never block broker startup, and `create_new` prevents following or + // truncating a same-user symlink planted at the candidate path. + let tmp = (0_u8..16) + .find_map(|_| { + let mut suffix = [0_u8; 16]; + if let Err(err) = getrandom::fill(&mut suffix) { + return Some(Err(std::io::Error::other(err))); + } + let mut tmp_name = path + .file_name() + .map(std::ffi::OsStr::to_os_string) + .unwrap_or_default(); + tmp_name.push(format!(".{}.tmp", hex::encode(suffix))); + let candidate = path.with_file_name(tmp_name); + + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + match options.open(&candidate) { + Ok(file) => Some(Ok((candidate, file))), + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => None, + Err(err) => Some(Err(err)), + } + }) + .transpose()? + .ok_or_else(|| std::io::Error::other("could not allocate discovery temp file"))?; + let (tmp, mut file) = tmp; + #[cfg(unix)] + file.set_permissions(unix_permissions(0o600))?; - let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); + let mut renamed = false; + let result = (|| { + file.write_all(payload.to_string().as_bytes())?; + file.sync_all()?; + drop(file); + std::fs::rename(&tmp, path)?; + renamed = true; + sync_directory(dir) + })(); + if result.is_err() { + let cleanup_path = if renamed { path } else { &tmp }; + let _ = std::fs::remove_file(cleanup_path); + if renamed { + let _ = sync_directory(dir); + } + } + result +} + +#[cfg(feature = "server")] +fn sync_directory(dir: &Path) -> std::io::Result<()> { #[cfg(unix)] { - use std::os::unix::fs::OpenOptionsExt; - options.mode(0o600); + std::fs::File::open(dir)?.sync_all() + } + #[cfg(not(unix))] + { + let _ = dir; + Ok(()) } - let mut file = options.open(&tmp)?; - file.write_all(payload.to_string().as_bytes())?; - file.sync_all()?; - drop(file); - std::fs::rename(&tmp, path) +} + +#[cfg(all(feature = "server", unix))] +fn unix_permissions(mode: u32) -> std::fs::Permissions { + use std::os::unix::fs::PermissionsExt; + std::fs::Permissions::from_mode(mode) } /// Best-effort removal (stop / app exit); missing files are expected. @@ -124,16 +241,34 @@ mod tests { #[test] fn parses_owner_pid_from_file_name() { - assert_eq!( - owner_pid_from_discovery_file_name("control-1234.json"), - Some(1234) - ); - assert_eq!( - owner_pid_from_discovery_file_name("control-1234.json.tmp"), - Some(1234) - ); - assert_eq!(owner_pid_from_discovery_file_name("other.json"), None); - assert_eq!(owner_pid_from_discovery_file_name("control-1234.tmp"), None); + const NONCE: &str = "0123456789abcdef0123456789abcdef"; + + for name in [ + "control-1234.json".to_string(), + "control-1234.json.tmp".to_string(), + format!("control-1234.json.{NONCE}.tmp"), + ] { + assert_eq!( + owner_pid_from_discovery_file_name(&name), + Some(1234), + "expected to recognize {name}" + ); + } + + for name in [ + "other.json", + "control-1234.tmp", + "control-1234.json.short.tmp", + "control-1234.json.0123456789abcdef0123456789abcdeg.tmp", + "control-1234.json.0123456789ABCDEF0123456789ABCDEF.tmp", + "control-1234.json.0123456789abcdef0123456789abcdef.tmp.extra", + ] { + assert_eq!( + owner_pid_from_discovery_file_name(name), + None, + "must not recognize unrelated name {name}" + ); + } // The parser round-trips the name `discovery_file_path` writes. let path = discovery_file_path(Path::new("/data"), 4242); @@ -155,23 +290,75 @@ mod tests { ); } + #[cfg(all(feature = "server", unix))] + #[test] + fn write_rejects_symlinked_discovery_directory() { + use std::os::unix::fs::symlink; + + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-dir-symlink-test-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let target = base.join("target"); + let link = base.join("berdctl"); + std::fs::create_dir(&target).unwrap(); + std::fs::set_permissions(&target, unix_permissions(0o700)).unwrap(); + symlink(&target, &link).unwrap(); + let path = link.join("control-4242.json"); + + let error = write_discovery_file( + &path, + 8080, + 4242, + 7, + "1111111111111111111111111111111111111111111111111111111111111111", + ) + .expect_err("symlinked discovery directory must fail closed"); + assert!(!target.join("control-4242.json").exists()); + assert_ne!(error.kind(), std::io::ErrorKind::NotFound); + + std::fs::remove_dir_all(base).ok(); + } + #[cfg(feature = "server")] #[test] fn write_and_remove_lifecycle() { + const FIRST_CAPABILITY: &str = + "1111111111111111111111111111111111111111111111111111111111111111"; + const ROTATED_CAPABILITY: &str = + "2222222222222222222222222222222222222222222222222222222222222222"; let base = std::env::temp_dir().join(format!("berdctl-discovery-test-{}", std::process::id())); std::fs::remove_dir_all(&base).ok(); let path = discovery_file_path(&base, 4242); - write_discovery_file(&path, 8080, 4242, 7).unwrap(); + write_discovery_file(&path, 8080, 4242, 7, FIRST_CAPABILITY).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(parsed["port"], 8080); assert_eq!(parsed["pid"], 4242); assert_eq!(parsed["generation"], 7); assert_eq!(parsed["protocolVersion"], PROTOCOL_VERSION); + assert_eq!(parsed["capability"], FIRST_CAPABILITY); // The temp file is renamed away, never left behind. - assert!(!path.with_file_name("control-4242.json.tmp").exists()); + let leftovers: Vec<_> = std::fs::read_dir(path.parent().unwrap()) + .unwrap() + .flatten() + .filter(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with("control-4242.json.") + }) + .collect(); + assert!(leftovers.is_empty(), "leftover temp files: {leftovers:?}"); + + // A crash orphan at the legacy fixed temp name cannot block a future + // broker start or be overwritten with the new capability. + let legacy_tmp = path.with_file_name("control-4242.json.tmp"); + std::fs::write(&legacy_tmp, "stale").unwrap(); #[cfg(unix)] { @@ -183,14 +370,33 @@ mod tests { assert_eq!(dir_mode & 0o777, 0o700); let file_mode = std::fs::metadata(&path).unwrap().permissions().mode(); assert_eq!(file_mode & 0o777, 0o600); + + // Pre-existing permissive paths are tightened too; creation modes + // alone do not repair them. + std::fs::set_permissions(path.parent().unwrap(), unix_permissions(0o755)).unwrap(); + std::fs::set_permissions(&path, unix_permissions(0o644)).unwrap(); } - // Restart case: a rewrite replaces the content atomically. - write_discovery_file(&path, 9090, 4242, 8).unwrap(); + // Restart case: an atomic rewrite rotates both generation and secret. + write_discovery_file(&path, 9090, 4242, 8, ROTATED_CAPABILITY).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(parsed["port"], 9090); assert_eq!(parsed["generation"], 8); + assert_eq!(parsed["capability"], ROTATED_CAPABILITY); + assert_eq!(std::fs::read_to_string(&legacy_tmp).unwrap(), "stale"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let dir_mode = std::fs::metadata(path.parent().unwrap()) + .unwrap() + .permissions() + .mode(); + assert_eq!(dir_mode & 0o777, 0o700); + let file_mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(file_mode & 0o777, 0o600); + } remove_discovery_file(&path); assert!(!path.exists()); diff --git a/src-tauri/plugins/berdctl/src/lib.rs b/src-tauri/plugins/berdctl/src/lib.rs index 86d5b6f8a..a3a8f8227 100644 --- a/src-tauri/plugins/berdctl/src/lib.rs +++ b/src-tauri/plugins/berdctl/src/lib.rs @@ -2,8 +2,9 @@ //! //! A lazily started, loopback-only HTTP server (`GET /v1/ping`, `POST //! /v1/call`) that forwards commands over a request/response bridge into the -//! main-window renderer. The CLI finds it through a per-instance discovery -//! file written on start and removed on stop/exit. +//! main-window renderer. The CLI finds it through a per-instance, owner-private +//! discovery file written on start and removed on stop/exit, and presents the +//! file's fresh bearer capability on every broker request. //! //! Without the `server` feature this crate is an inert stub: build.rs still //! generates the command permissions (so capability validation passes in @@ -24,7 +25,8 @@ mod plugin { use crate::bridge::{Bridge, BridgeError, BridgeRequest, BridgeResult}; use crate::discovery; use crate::server::{ - self, BridgeDispatcher, ServerContext, ServerHandle, TimeoutStore, IN_FLIGHT_LIMIT, + self, generate_capability, BridgeDispatcher, ServerContext, ServerHandle, TimeoutStore, + IN_FLIGHT_LIMIT, }; use serde::Serialize; use std::collections::HashMap; @@ -150,6 +152,8 @@ mod plugin { return Ok(StartedEndpoint { port: handle.port }); } let generation = state.generation.fetch_add(1, Ordering::Relaxed) + 1; + let capability = generate_capability() + .map_err(|err| format!("failed to generate berdctl capability: {err}"))?; // Each server gets its own semaphore: graceful shutdown lets the // previous server's in-flight handlers outlive `stop`, and their // permits must release slots on that dead instance, not free up (and @@ -162,6 +166,7 @@ mod plugin { state.timeouts.clone(), Arc::new(tokio::sync::Semaphore::new(IN_FLIGHT_LIMIT)), generation, + capability.clone(), )); let handle = server::start_server(ctx) .await @@ -176,7 +181,8 @@ mod plugin { .map_err(|err| format!("failed to resolve app data dir: {err}"))?; let pid = std::process::id(); let path = discovery::discovery_file_path(&app_data_dir, pid); - if let Err(err) = discovery::write_discovery_file(&path, port, pid, generation) { + if let Err(err) = discovery::write_discovery_file(&path, port, pid, generation, &capability) + { handle.shutdown(); return Err(format!( "failed to write berdctl discovery file {}: {err}", diff --git a/src-tauri/plugins/berdctl/src/server.rs b/src-tauri/plugins/berdctl/src/server.rs index 5b62bc395..8f7fb53a5 100644 --- a/src-tauri/plugins/berdctl/src/server.rs +++ b/src-tauri/plugins/berdctl/src/server.rs @@ -1,16 +1,16 @@ //! Loopback-only HTTP broker for the berdctl CLI. //! //! Serves `GET /v1/ping` (generation/protocol handshake) and `POST /v1/call` -//! (command dispatch over the renderer bridge). There is no application auth -//! in v1; the header rejection below (any `Origin`, any `Sec-Fetch-*`, `Host` -//! mismatch) is the sole defense against browser-JS-to-localhost and DNS -//! rebinding, so it applies to every route. +//! (command dispatch over the renderer bridge). Every route requires the +//! per-server bearer capability published in the private discovery file. The +//! existing Origin, Sec-Fetch, and literal Host checks remain a separate +//! defense against browser-JS-to-localhost and DNS rebinding. use crate::bridge::{Bridge, BridgeError, BridgeRequest, BridgeResult}; use crate::discovery::PROTOCOL_VERSION; use axum::body::Bytes; use axum::extract::State; -use axum::http::header::{HOST, ORIGIN}; +use axum::http::header::{AUTHORIZATION, HOST, ORIGIN}; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing::{get, post}; @@ -21,6 +21,7 @@ use std::collections::HashMap; use std::future::Future; use std::sync::{Arc, OnceLock, RwLock}; use std::time::{Duration, Instant}; +use subtle::ConstantTimeEq; use tauri::{AppHandle, Runtime}; use tokio::sync::{oneshot, Semaphore}; @@ -29,6 +30,13 @@ pub const IN_FLIGHT_LIMIT: usize = 4; const DEFAULT_COMMAND_TIMEOUT: Duration = Duration::from_secs(30); const MIN_REQUEST_TIMEOUT: Duration = Duration::from_secs(1); const MAX_COMMAND_TIMEOUT: Duration = Duration::from_secs(900); +const CAPABILITY_BYTES: usize = 32; + +pub fn generate_capability() -> std::io::Result { + let mut bytes = [0_u8; CAPABILITY_BYTES]; + getrandom::fill(&mut bytes).map_err(std::io::Error::other)?; + Ok(hex::encode(bytes)) +} /// Resolve the bridge timeout for a call: a request `timeout_ms` wins /// (clamped to [`MIN_REQUEST_TIMEOUT`]..=[`MAX_COMMAND_TIMEOUT`]); otherwise @@ -118,6 +126,7 @@ pub struct ServerContext { // against their own instance, never the next server's. inflight: Arc, generation: u64, + capability: String, // Set by `start_server` once the listener is bound, before any request. port: OnceLock, } @@ -128,12 +137,14 @@ impl ServerContext { timeouts: Arc, inflight: Arc, generation: u64, + capability: String, ) -> Self { Self { dispatcher, timeouts, inflight, generation, + capability, port: OnceLock::new(), } } @@ -183,8 +194,9 @@ pub fn build_router(ctx: Arc>) -> Router } /// Reject requests that look like they came from a browser (any `Origin` or -/// `Sec-Fetch-*` header) or through DNS rebinding (`Host` other than our -/// loopback bind). Applied by every handler before anything else. +/// `Sec-Fetch-*` header), through DNS rebinding (`Host` other than our +/// loopback bind), or without this server instance's bearer capability. +/// Applied by every handler before reading or dispatching a body. fn forbidden_header_response(ctx: &ServerContext, headers: &HeaderMap) -> Option { let violation = if headers.contains_key(ORIGIN) { Some("Origin header not allowed".to_string()) @@ -199,13 +211,30 @@ fn forbidden_header_response(ctx: &ServerContext, headers: &HeaderMap) -> Some(host) if host == expected => None, _ => Some(format!("Host must be {expected}")), } - }; + } + .or_else(|| { + let authorized = headers + .get(AUTHORIZATION) + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.strip_prefix("Bearer ")) + .is_some_and(|provided| capability_matches(&ctx.capability, provided)); + (!authorized).then(|| "valid bearer capability required".to_string()) + }); violation.map(|message| { log::warn!("[berdctl] rejected request: {message}"); error_response(StatusCode::FORBIDDEN, "forbidden", &message) }) } +fn capability_matches(expected: &str, provided: &str) -> bool { + let expected = expected.as_bytes(); + let provided = provided.as_bytes(); + if expected.len() != provided.len() { + return false; + } + bool::from(expected.ct_eq(provided)) +} + async fn handle_ping( State(ctx): State>>, headers: HeaderMap, @@ -359,6 +388,10 @@ mod tests { use tokio::sync::{mpsc, Notify}; const TEST_GENERATION: u64 = 3; + const TEST_CAPABILITY: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + const STALE_CAPABILITY: &str = + "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; #[derive(Clone)] enum StubBehavior { @@ -451,6 +484,7 @@ mod tests { timeouts, Arc::new(Semaphore::new(limits.permits)), TEST_GENERATION, + TEST_CAPABILITY.to_string(), )); let handle = start_server(ctx).await.unwrap(); TestServer { @@ -459,13 +493,32 @@ mod tests { } } - async fn post_call(base: &str, body: &Value) -> reqwest::Response { - reqwest::Client::new() + async fn get_ping(base: &str, capability: Option<&str>) -> reqwest::Response { + let request = reqwest::Client::new().get(format!("{base}/v1/ping")); + let request = match capability { + Some(capability) => request.bearer_auth(capability), + None => request, + }; + request.send().await.unwrap() + } + + async fn post_call_with_capability( + base: &str, + body: &Value, + capability: Option<&str>, + ) -> reqwest::Response { + let request = reqwest::Client::new() .post(format!("{base}/v1/call")) - .json(body) - .send() - .await - .unwrap() + .json(body); + let request = match capability { + Some(capability) => request.bearer_auth(capability), + None => request, + }; + request.send().await.unwrap() + } + + async fn post_call(base: &str, body: &Value) -> reqwest::Response { + post_call_with_capability(base, body, Some(TEST_CAPABILITY)).await } fn call_body(command: &str, args: Value) -> Value { @@ -475,15 +528,61 @@ mod tests { #[tokio::test] async fn ping_echoes_generation_and_protocol_version() { let server = spawn_server(StubBehavior::Echo, Limits::default()).await; - let response = reqwest::get(format!("{}/v1/ping", server.base)) - .await - .unwrap(); + let response = get_ping(&server.base, Some(TEST_CAPABILITY)).await; assert_eq!(response.status(), 200); let body: Value = response.json().await.unwrap(); assert_eq!(body["generation"], TEST_GENERATION); assert_eq!(body["protocolVersion"], PROTOCOL_VERSION); } + #[tokio::test] + async fn missing_wrong_and_stale_capabilities_are_rejected_on_all_routes() { + let server = spawn_server(StubBehavior::Echo, Limits::default()).await; + let body = call_body("sessions", json!({ "action": "list" })); + + for capability in [None, Some("wrong"), Some(STALE_CAPABILITY)] { + let ping = get_ping(&server.base, capability).await; + assert_eq!(ping.status(), 403, "ping capability {capability:?}"); + let ping_body: Value = ping.json().await.unwrap(); + assert_eq!(ping_body["error"]["code"], "forbidden"); + + let call = post_call_with_capability(&server.base, &body, capability).await; + assert_eq!(call.status(), 403, "call capability {capability:?}"); + let call_body: Value = call.json().await.unwrap(); + assert_eq!(call_body["error"]["code"], "forbidden"); + } + + assert_eq!( + get_ping(&server.base, Some(TEST_CAPABILITY)).await.status(), + 200 + ); + assert_eq!( + post_call_with_capability(&server.base, &body, Some(TEST_CAPABILITY)) + .await + .status(), + 200 + ); + } + + #[test] + fn generated_capabilities_are_random_256_bit_hex() { + let first = generate_capability().unwrap(); + let second = generate_capability().unwrap(); + assert_eq!(first.len(), CAPABILITY_BYTES * 2); + assert!(first + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte))); + assert_ne!(first, second); + } + + #[test] + fn capability_match_checks_content_and_length() { + assert!(capability_matches(TEST_CAPABILITY, TEST_CAPABILITY)); + assert!(!capability_matches(TEST_CAPABILITY, STALE_CAPABILITY)); + assert!(!capability_matches(TEST_CAPABILITY, "short")); + assert!(!capability_matches(TEST_CAPABILITY, &"0".repeat(128))); + } + #[tokio::test] async fn origin_header_is_rejected_on_all_routes() { let server = spawn_server(StubBehavior::Echo, Limits::default()).await; @@ -491,6 +590,7 @@ mod tests { let ping = client .get(format!("{}/v1/ping", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Origin", "https://evil.example") .send() .await @@ -502,6 +602,7 @@ mod tests { let call = client .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Origin", "http://localhost:3000") .json(&call_body("sessions", json!({ "action": "list" }))) .send() @@ -519,6 +620,7 @@ mod tests { for header in ["Sec-Fetch-Site", "Sec-Fetch-Mode", "Sec-Fetch-Dest"] { let response = client .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) .header(header, "cross-site") .json(&call_body("sessions", json!({ "action": "list" }))) .send() @@ -539,6 +641,7 @@ mod tests { for host in ["evil.example:1234", "localhost:80"] { let response = client .get(format!("{}/v1/ping", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Host", host) .send() .await @@ -626,6 +729,7 @@ mod tests { // Not JSON at all. let response = client .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Content-Type", "application/json") .body("{not json") .send() diff --git a/src-tauri/src/services/berdctl_discovery.rs b/src-tauri/src/services/berdctl_discovery.rs index 1659caf36..28e143aaa 100644 --- a/src-tauri/src/services/berdctl_discovery.rs +++ b/src-tauri/src/services/berdctl_discovery.rs @@ -3,7 +3,8 @@ //! Each app instance's berdctl broker writes a discovery file at //! `/berdctl/control-.json` and deletes it on //! stop/exit. A crashed instance leaves its file behind (possibly as a -//! `control-.json.tmp` orphan from a crash mid-write); this sweep +//! legacy `control-.json.tmp` or current +//! `control-.json..tmp` orphan from a crash mid-write); this sweep //! removes files whose owning app process is no longer alive. The directory //! and filename formats are owned by the plugin's discovery module. Compiled //! unconditionally — stale files must be cleaned even by builds where the @@ -96,6 +97,10 @@ mod tests { let dead = write_discovery_file(app_data_dir.path(), &format!("control-{gone}.json")); let dead_tmp = write_discovery_file(app_data_dir.path(), &format!("control-{gone}.json.tmp")); + let dead_random_tmp = write_discovery_file( + app_data_dir.path(), + &format!("control-{gone}.json.0123456789abcdef0123456789abcdef.tmp"), + ); let own = write_discovery_file( app_data_dir.path(), &format!("control-{}.json", std::process::id()), @@ -108,6 +113,7 @@ mod tests { assert!(!dead.exists()); assert!(!dead_tmp.exists()); + assert!(!dead_random_tmp.exists()); assert!(own.exists()); assert!(live.exists()); assert!(unrelated.exists()); diff --git a/src/features/berdctl/commands/contract.ts b/src/features/berdctl/commands/contract.ts index 92dad983b..85aefc243 100644 --- a/src/features/berdctl/commands/contract.ts +++ b/src/features/berdctl/commands/contract.ts @@ -29,7 +29,7 @@ import type { AppCommand, ToolGroup } from "./types"; * Mirror of `PROTOCOL_VERSION` in both discovery.rs copies (a berdctl * crate test pins the CLI copy, and a plugin crate test pins the broker * copy); bump all copies together. */ -const WIRE_PROTOCOL_VERSION = 4; +const WIRE_PROTOCOL_VERSION = 5; type FieldSpec = { /** snake_case wire field name. */ From 55519983ac039ad429029b080d20734ec68c70c3 Mon Sep 17 00:00:00 2001 From: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Date: Sun, 16 Aug 2026 11:55:17 -0700 Subject: [PATCH 2/7] fix(berdctl): authorize app-owned goosed descendants Replace discovery-file bearer disclosure with kernel peer-process admission over local IPC. Unix validates stable process ancestry; Windows retains a no-breakaway Job Object for the spawned goosed tree. Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- docs/berdctl-architecture.md | 43 +- src-tauri/Cargo.lock | 36 ++ src-tauri/README.md | 11 +- src-tauri/crates/berdctl/Cargo.toml | 1 + src-tauri/crates/berdctl/src/client.rs | 427 +++++++----------- src-tauri/crates/berdctl/src/discovery.rs | 56 +-- src-tauri/plugins/berdctl/Cargo.toml | 12 +- .../plugins/berdctl/src/authorization.rs | 366 +++++++++++++++ src-tauri/plugins/berdctl/src/bootstrap.rs | 231 ++++++++++ src-tauri/plugins/berdctl/src/discovery.rs | 79 +++- src-tauri/plugins/berdctl/src/lib.rs | 41 +- src-tauri/plugins/berdctl/src/server.rs | 40 +- src-tauri/src/services/acp/goose_serve.rs | 8 + 13 files changed, 980 insertions(+), 371 deletions(-) create mode 100644 src-tauri/plugins/berdctl/src/authorization.rs create mode 100644 src-tauri/plugins/berdctl/src/bootstrap.rs diff --git a/docs/berdctl-architecture.md b/docs/berdctl-architecture.md index 110262ee6..309d030e8 100644 --- a/docs/berdctl-architecture.md +++ b/docs/berdctl-architecture.md @@ -10,18 +10,19 @@ berdctl project create --name demo The implementation has three layers: 1. CLI: `src-tauri/crates/berdctl/` - Parses flags with clap, prints help, reads the private app discovery file, - authenticates each loopback request with its per-broker capability, and - sends JSON calls. CLI validation is convenience only. + Parses flags with clap, prints help, reads non-secret endpoint discovery, + obtains the broker-generation capability over authenticated local IPC, and sends + JSON calls. CLI validation is convenience only. 2. Broker: `src-tauri/plugins/berdctl/` - Runs a localhost server inside the app, requires the current discovery-file - capability, rejects browser-origin requests, enforces in-flight and timeout - limits, and forwards calls to the renderer without command-specific logic. + Admits only kernel-identified descendants of this Berd instance's owned + `goosed` tree, then requires the issued capability on the loopback server, + rejects browser-origin requests, enforces in-flight and timeout limits, and + forwards calls without command-specific logic. 3. Renderer registry: `src/features/berdctl/commands/` Strict-parses args with zod, runs guards, executes through app state, and returns JSON results. This remains the command-policy trust boundary; the - broker capability limits access to processes that can read the owning - user's private discovery file. + broker admission boundary prevents unrelated same-user processes from + directly reaching it. ## Layer rules @@ -89,13 +90,18 @@ belongs in error messages, not generic help text. ## Safety model -v1 requires a fresh 256-bit bearer capability for every broker start. The -plugin writes it beside the port and generation in the discovery file, with -owner-only directory/file permissions on Unix, and the CLI presents it on -both `/v1/ping` and `/v1/call`. Missing, malformed, wrong, stale, symlinked, -or non-private capability records fail closed. This authenticates possession -of the app-issued session endpoint; it does not replace renderer command -policy or add interactive confirmation dialogs. +v1 publishes no bearer in the discovery file. Discovery contains only the +loopback port, generation, protocol version, and local bootstrap address. The +CLI connects to that local IPC endpoint; the broker obtains the peer PID from +the kernel and admits it only when it is a descendant of the exact app-owned +`goosed` process on Unix or a member of the exact retained no-breakaway Job +Object on Windows. Only then does it return the per-broker 256-bit capability, +which the CLI presents on `/v1/ping` and `/v1/call`. + +This blocks direct broker use by unrelated same-user processes. It deliberately +does not claim protection from same-user malware that can inspect or inject +into an admitted descendant. Closing that stronger boundary requires OS +isolation or interactive user authorization, not another ambient bearer. Required command properties: @@ -115,9 +121,10 @@ piecemeal auth in a command PR. ## Versioning The broker writes a private discovery file with `protocolVersion`, generation, -port, and a per-start capability. The CLI authenticates and verifies it via -`/v1/ping` before calls. Requiring that capability is a breaking wire reshape, -so the authenticated surface starts at protocol version 5. +port, and a non-secret local bootstrap address. The CLI obtains the bearer only +after peer-process admission, then authenticates and verifies `/v1/ping` before +calls. This authenticated bootstrap is a breaking wire reshape, so the surface +starts at protocol version 5. Breaking wire reshapes must bump all three constants: diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 1e2af39b5..6bbb12720 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -583,6 +583,7 @@ version = "0.6.0" dependencies = [ "clap", "indexmap 2.13.1", + "interprocess", "libc", "serde", "serde_json", @@ -1685,6 +1686,12 @@ dependencies = [ "const-random", ] +[[package]] +name = "doctest-file" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c2db04e74f0a9a93103b50e90b96024c9b2bdca8bce6a632ec71b88736d3d359" + [[package]] name = "doctor" version = "0.1.0" @@ -2992,6 +2999,21 @@ dependencies = [ "cfb", ] +[[package]] +name = "interprocess" +version = "2.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "798de1433ba514cc6c04c4144c2469af81396e4906195218737c776d47769572" +dependencies = [ + "doctest-file", + "futures-core", + "libc", + "recvmsg", + "tokio", + "widestring", + "windows-sys 0.61.2", +] + [[package]] name = "ipnet" version = "2.12.1" @@ -5006,6 +5028,12 @@ dependencies = [ "rustfft", ] +[[package]] +name = "recvmsg" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3edd4d5d42c92f0a659926464d4cce56b562761267ecf0f469d85b7de384175" + [[package]] name = "redox_syscall" version = "0.5.18" @@ -6586,6 +6614,7 @@ dependencies = [ "axum", "getrandom 0.4.3", "hex", + "interprocess", "libc", "log", "reqwest 0.13.4", @@ -6596,6 +6625,7 @@ dependencies = [ "tauri-plugin", "tokio", "uuid", + "windows-sys 0.59.0", ] [[package]] @@ -8076,6 +8106,12 @@ dependencies = [ "wasite", ] +[[package]] +name = "widestring" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72069c3113ab32ab29e5584db3c6ec55d416895e60715417b5b883a357c3e471" + [[package]] name = "winapi" version = "0.3.9" diff --git a/src-tauri/README.md b/src-tauri/README.md index 627653cbd..bd45d6526 100644 --- a/src-tauri/README.md +++ b/src-tauri/README.md @@ -15,10 +15,11 @@ The Tauri 2 shell: the app crate (`src/`), the berdctl workspace crates The CLI embeds the contract artifacts (`crates/berdctl/api-surface.json` + `cli-surface.json`) and builds its clap tree at startup. It locates the -broker through the `BERDCTL_LOCK` discovery file, reads its per-start -capability, verifies `protocolVersion`/generation through an authenticated -`GET /v1/ping`, and sends authenticated -`POST /v1/call {"command", "args"}`. The broker forwards to the renderer +broker through the `BERDCTL_LOCK` discovery file, connects to its non-secret +local bootstrap address, and receives a capability only after the broker admits +the kernel-reported process as a descendant of this app's owned `goosed` tree. +It then verifies `protocolVersion`/generation through authenticated +`GET /v1/ping` and sends authenticated `POST /v1/call {"command", "args"}`. The broker forwards to the renderer over Tauri IPC (`berdctl:request` event out, `submit_result` back). Command dispatch, zod validation, guards, and execution live in the renderer registry (`src/features/berdctl/`). The two crates share no code; @@ -42,7 +43,7 @@ capability grants a permission allowing that command. window. This ACL gates webview → Rust IPC only; the localhost HTTP side is governed -separately by the owner-private discovery capability, browser/DNS-rebinding +separately by process-authenticated capability bootstrap, browser/DNS-rebinding header rejection, and global caps. Stock Tauri 2 plugin layout. Docs: diff --git a/src-tauri/crates/berdctl/Cargo.toml b/src-tauri/crates/berdctl/Cargo.toml index 8a553d81e..a5515141c 100644 --- a/src-tauri/crates/berdctl/Cargo.toml +++ b/src-tauri/crates/berdctl/Cargo.toml @@ -16,6 +16,7 @@ clap = { version = "4", features = ["env", "string", "wrap_help"] } indexmap = { version = "2", features = ["serde"] } serde = { version = "1", features = ["derive"] } serde_json = "1" +interprocess = { version = "2.4.3", features = ["tokio"] } ureq = { version = "3", features = ["json"] } [target.'cfg(unix)'.dependencies] diff --git a/src-tauri/crates/berdctl/src/client.rs b/src-tauri/crates/berdctl/src/client.rs index 796cc1791..4d0c6d905 100644 --- a/src-tauri/crates/berdctl/src/client.rs +++ b/src-tauri/crates/berdctl/src/client.rs @@ -2,6 +2,9 @@ //! mapping from HTTP outcomes to the CLI's exit-code contract: //! 0 ok, 1 command error, 2 transport, 3 environment/reachability/version. +#[cfg(windows)] +use interprocess::local_socket::{prelude::*, GenericNamespaced, Stream, ToNsName}; +use std::io::{BufRead, BufReader, Read}; use std::path::Path; use std::time::Duration; @@ -15,6 +18,7 @@ pub const EXIT_TRANSPORT: u8 = 2; pub const EXIT_ENV: u8 = 3; const PING_TIMEOUT: Duration = Duration::from_secs(2); +const MAX_BOOTSTRAP_RESPONSE_BYTES: u64 = 4096; /// Above the broker's 900s command-timeout ceiling, so the broker's /// structured 504 always arrives before this client-side timeout fires. const CALL_TIMEOUT: Duration = Duration::from_secs(910); @@ -147,6 +151,58 @@ fn ping(port: u16, capability: &str) -> Result { }) } +fn bootstrap(file: &discovery::DiscoveryFile) -> Result { + #[cfg(unix)] + let stream = std::os::unix::net::UnixStream::connect(&file.bootstrap_endpoint).map_err(|error| Failure::env(format!("the Berd desktop app's authenticated control bootstrap is unavailable ({error}); {CONTROL_REMEDIATION}")))?; + #[cfg(windows)] + let stream = { + let name = file + .bootstrap_endpoint + .to_string_lossy() + .to_string() + .to_ns_name::() + .map_err(|error| { + Failure::env(format!("invalid Berd control bootstrap endpoint: {error}")) + })?; + Stream::connect(name).map_err(|error| Failure::env(format!("the Berd desktop app's authenticated control bootstrap is unavailable ({error}); {CONTROL_REMEDIATION}")))? + }; + let mut response = String::new(); + BufReader::new(stream) + .take(MAX_BOOTSTRAP_RESPONSE_BYTES + 1) + .read_line(&mut response) + .map_err(|error| Failure::env(format!("the Berd desktop app's authenticated control bootstrap failed ({error}); {CONTROL_REMEDIATION}")))?; + if response.len() as u64 > MAX_BOOTSTRAP_RESPONSE_BYTES { + return Err(Failure::env( + "the Berd control bootstrap returned an unexpectedly large response", + )); + } + #[derive(Deserialize)] + #[serde(rename_all = "camelCase")] + struct BootstrapResponse { + port: u16, + generation: u64, + protocol_version: u32, + capability: String, + } + let response: BootstrapResponse = serde_json::from_str(&response).map_err(|error| { + Failure::env(format!( + "the Berd control bootstrap returned invalid data ({error})" + )) + })?; + if response.port != file.port + || response.generation != file.generation + || response.protocol_version != PROTOCOL_VERSION + { + return Err(Failure::env( + "the Berd desktop app restarted its control endpoint; retry the command", + )); + } + Ok(Endpoint { + port: response.port, + capability: response.capability, + }) +} + /// Read the discovery file and verify the broker behind it echoes the file's /// generation and this binary's protocol version. A generation mismatch or /// authentication failure can mean the file was read across a broker restart: @@ -157,7 +213,8 @@ pub fn handshake(lock_path: &Path) -> Result { if file.protocol_version != PROTOCOL_VERSION { return Err(Failure::env(APP_UPDATED)); } - let ping = match ping(file.port, &file.capability) { + let endpoint = bootstrap(&file)?; + let ping = match ping(endpoint.port, &endpoint.capability) { Ok(ping) => ping, Err(failure) if failure.status == Some(403) => { if attempt == 0 { @@ -191,10 +248,7 @@ pub fn handshake(lock_path: &Path) -> Result { return Err(Failure::env(APP_UPDATED)); } if ping.generation == file.generation { - return Ok(Endpoint { - port: file.port, - capability: file.capability, - }); + return Ok(endpoint); } if attempt == 0 { file = discovery::load(lock_path).map_err(|err| { @@ -322,335 +376,176 @@ fn error_parts(value: &Value) -> Option<(String, String)> { #[cfg(test)] mod tests { use super::*; - use std::io::{BufRead, BufReader, Read, Write}; + #[cfg(unix)] + use std::io::Write; + #[cfg(unix)] use std::net::{TcpListener, TcpStream}; + #[cfg(unix)] use std::path::PathBuf; - use std::sync::{mpsc, Arc}; + #[cfg(unix)] + use std::sync::mpsc; + #[cfg(unix)] use std::thread; - const CURRENT_CAPABILITY: &str = + #[cfg(unix)] + const TEST_CAPABILITY: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; - const STALE_CAPABILITY: &str = - "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; - struct TempDiscoveryFile(PathBuf); + #[cfg(unix)] + struct TempDiscoveryFile { + path: PathBuf, + bootstrap_endpoint: PathBuf, + } + #[cfg(unix)] impl TempDiscoveryFile { - fn new(label: &str, port: u16, capability: &str) -> Self { + fn new(port: u16, generation: u64) -> Self { + use std::os::unix::fs::PermissionsExt; let base = std::env::temp_dir().join(format!( - "berdctl-client-auth-{}-{label}-{port}", + "berdctl-client-bootstrap-{}-{port}", std::process::id() )); std::fs::remove_dir_all(&base).ok(); - std::fs::create_dir(&base).expect("create discovery directory"); + std::fs::create_dir(&base).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); let path = base.join("control.json"); + let bootstrap_endpoint = base.join("bootstrap.sock"); std::fs::write( &path, format!( - r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{capability}"}}"# + r#"{{"port":{port},"pid":4242,"generation":{generation},"protocolVersion":{PROTOCOL_VERSION},"bootstrapEndpoint":"{}"}}"#, + bootstrap_endpoint.display() ), ) - .expect("write discovery file"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions( - path.parent().expect("test discovery has a parent"), - std::fs::Permissions::from_mode(0o700), - ) - .expect("make discovery directory private"); - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .expect("make discovery file private"); + .unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + Self { + path, + bootstrap_endpoint, } - Self(path) } } + #[cfg(unix)] impl Drop for TempDiscoveryFile { fn drop(&mut self) { - if let Some(parent) = self.0.parent() { + if let Some(parent) = self.path.parent() { std::fs::remove_dir_all(parent).ok(); } } } - struct RecordedRequest { - request_line: String, - authorization: Option, - body: String, - } - - fn read_request(stream: &mut TcpStream) -> RecordedRequest { - let mut reader = BufReader::new(stream.try_clone().expect("clone request stream")); + #[cfg(unix)] + fn read_request(stream: &mut TcpStream) -> (String, Option, String) { + let mut reader = BufReader::new(stream.try_clone().unwrap()); let mut request_line = String::new(); - reader - .read_line(&mut request_line) - .expect("read request line"); + reader.read_line(&mut request_line).unwrap(); let mut authorization = None; let mut content_length = 0; loop { let mut line = String::new(); - reader.read_line(&mut line).expect("read request header"); + reader.read_line(&mut line).unwrap(); if line == "\r\n" { break; } - let Some((name, value)) = line.trim_end().split_once(':') else { - continue; - }; - if name.eq_ignore_ascii_case("authorization") { - authorization = Some(value.trim().to_string()); - } - if name.eq_ignore_ascii_case("content-length") { - content_length = value.trim().parse().expect("valid content length"); + if let Some((name, value)) = line.trim_end().split_once(':') { + if name.eq_ignore_ascii_case("authorization") { + authorization = Some(value.trim().to_string()); + } + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().unwrap(); + } } } let mut body = vec![0; content_length]; - reader.read_exact(&mut body).expect("read request body"); - RecordedRequest { - request_line: request_line.trim_end().to_string(), + reader.read_exact(&mut body).unwrap(); + ( + request_line.trim_end().to_string(), authorization, - body: String::from_utf8(body).expect("request body is UTF-8"), - } + String::from_utf8(body).unwrap(), + ) } - fn write_response(stream: &mut TcpStream, status: &str, body: &str) { + #[cfg(unix)] + fn write_response(stream: &mut TcpStream, body: &str) { write!( stream, - "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", body.len() - ) - .expect("write test response"); - } - - fn spawn_broker_sequence( - expected_requests: Vec<(&'static str, BrokerResponse)>, - ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { - spawn_broker_responses(expected_requests) - } - - #[derive(Clone, Copy)] - enum BrokerResponse { - Ping { generation: u64 }, - Call, - } - - fn spawn_broker_responses( - expected_requests: Vec<(&'static str, BrokerResponse)>, - ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { - spawn_broker_responses_with_sync(expected_requests, None) + ).unwrap(); } - fn spawn_broker_responses_with_sync( - expected_requests: Vec<(&'static str, BrokerResponse)>, - first_response_sync: Option<(Arc, Arc)>, - ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { - let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind test broker"); - let port = listener.local_addr().expect("test broker address").port(); + #[cfg(unix)] + #[test] + fn handshake_bootstraps_capability_and_call_reuses_it() { + let broker = TcpListener::bind(("127.0.0.1", 0)).unwrap(); + let port = broker.local_addr().unwrap().port(); + let discovery = TempDiscoveryFile::new(port, 7); + let bootstrap = + std::os::unix::net::UnixListener::bind(&discovery.bootstrap_endpoint).unwrap(); let (requests_tx, requests_rx) = mpsc::channel(); - let handle = thread::spawn(move || { - for (request_number, (expected_capability, response)) in - expected_requests.into_iter().enumerate() - { - let (mut stream, _) = listener.accept().expect("accept client request"); - let request = read_request(&mut stream); - let authorized = request.authorization.as_deref() - == Some(&format!("Bearer {expected_capability}")); - requests_tx.send(request).expect("record client request"); - if request_number == 0 { - if let Some((request_seen, response_ready)) = &first_response_sync { - request_seen.wait(); - response_ready.wait(); - } - } - match (authorized, response) { - (true, BrokerResponse::Ping { generation }) => write_response( - &mut stream, - "200 OK", - &format!( - r#"{{"generation":{generation},"protocolVersion":{PROTOCOL_VERSION}}}"# - ), - ), - (true, BrokerResponse::Call) => { - write_response(&mut stream, "200 OK", r#"{"ok":true,"result":"ok"}"#) - } - (false, _) => write_response( - &mut stream, - "403 Forbidden", - r#"{"ok":false,"error":{"code":"forbidden","message":"valid bearer capability required"}}"#, - ), - } + let worker = thread::spawn(move || { + let (mut stream, _) = bootstrap.accept().unwrap(); + writeln!(stream, r#"{{"port":{port},"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{TEST_CAPABILITY}"}}"#).unwrap(); + for response in [ + format!(r#"{{"generation":7,"protocolVersion":{PROTOCOL_VERSION}}}"#), + r#"{"ok":true,"result":"ok"}"#.to_string(), + ] { + let (mut stream, _) = broker.accept().unwrap(); + requests_tx.send(read_request(&mut stream)).unwrap(); + write_response(&mut stream, &response); } }); - (port, requests_rx, handle) - } - - fn spawn_broker( - expected_capability: &'static str, - request_count: usize, - ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { - spawn_broker_responses( - (0..request_count) - .map(|request_number| { - let response = if request_number == 0 { - BrokerResponse::Ping { generation: 7 } - } else { - BrokerResponse::Call - }; - (expected_capability, response) - }) - .collect(), - ) - } - - #[test] - fn handshake_and_call_send_current_capability() { - let (port, requests, broker) = spawn_broker(CURRENT_CAPABILITY, 2); - let lock_file = TempDiscoveryFile::new("current", port, CURRENT_CAPABILITY); - let endpoint = handshake(&lock_file.0).expect("current capability handshakes"); + let endpoint = handshake(&discovery.path).unwrap(); assert_eq!( format!("{endpoint:?}"), - format!("Endpoint {{ port: {port}, capability: \"[redacted]\" }}"), - "debug output must not disclose the bearer capability" + format!("Endpoint {{ port: {port}, capability: \"[redacted]\" }}") ); - let result = call( - &endpoint, - "sessions", - Map::from_iter([("action".to_string(), Value::String("list".to_string()))]), - None, - ) - .expect("current capability calls"); - assert_eq!(result, Value::String("ok".to_string())); - - let ping = requests.recv().expect("record ping"); - assert_eq!(ping.request_line, "GET /v1/ping HTTP/1.1"); assert_eq!( - ping.authorization.as_deref(), - Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + call( + &endpoint, + "sessions", + Map::from_iter([("action".into(), Value::String("list".into()))]), + None + ) + .unwrap(), + Value::String("ok".into()) ); - assert!(ping.body.is_empty()); - let call = requests.recv().expect("record call"); - assert_eq!(call.request_line, "POST /v1/call HTTP/1.1"); + let ping = requests_rx.recv().unwrap(); + assert_eq!(ping.0, "GET /v1/ping HTTP/1.1"); assert_eq!( - call.authorization.as_deref(), - Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ping.1.as_deref(), + Some(format!("Bearer {TEST_CAPABILITY}").as_str()) ); - let call_body: Value = serde_json::from_str(&call.body).expect("call body is JSON"); - assert_eq!(call_body["command"], "sessions"); - assert_eq!(call_body["args"]["action"], "list"); - - broker.join().expect("test broker exits"); - } - - #[test] - fn handshake_recovers_when_capability_rotates_after_discovery_read() { - let first_request = Arc::new(std::sync::Barrier::new(2)); - let response_ready = Arc::new(std::sync::Barrier::new(2)); - let (port, requests, broker) = spawn_broker_responses_with_sync( - vec![ - (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), - (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), - ], - Some((first_request.clone(), response_ready.clone())), - ); - let lock_file = TempDiscoveryFile::new("rotating", port, STALE_CAPABILITY); - let path = lock_file.0.clone(); - let rewrite_first_request = first_request.clone(); - let rewrite_response_ready = response_ready.clone(); - let rewrite = thread::spawn(move || { - rewrite_first_request.wait(); - std::fs::write( - &path, - format!( - r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{CURRENT_CAPABILITY}"}}"# - ), - ) - .expect("publish rotated discovery capability"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .expect("keep rewritten discovery private"); - } - rewrite_response_ready.wait(); - let stale_ping = requests.recv().expect("record stale ping"); - assert_eq!( - stale_ping.authorization.as_deref(), - Some(format!("Bearer {STALE_CAPABILITY}").as_str()) - ); - requests.recv().expect("record retried current ping") - }); - - let endpoint = handshake(&lock_file.0).expect("rotated capability retries successfully"); - assert_eq!(endpoint.port, port); - let current_ping = rewrite.join().expect("discovery rewrite exits"); + let call = requests_rx.recv().unwrap(); + assert_eq!(call.0, "POST /v1/call HTTP/1.1"); assert_eq!( - current_ping.authorization.as_deref(), - Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + call.1.as_deref(), + Some(format!("Bearer {TEST_CAPABILITY}").as_str()) ); - broker.join().expect("test broker exits"); - } - - #[test] - fn handshake_retries_generation_mismatch_with_rotated_capability() { - let (port, requests, broker) = spawn_broker_sequence(vec![ - (STALE_CAPABILITY, BrokerResponse::Ping { generation: 6 }), - (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), - ]); - let lock_file = TempDiscoveryFile::new("generation-race", port, STALE_CAPABILITY); - let path = lock_file.0.clone(); - let rewrite = thread::spawn(move || { - let stale_ping = requests.recv().expect("record old-generation ping"); - assert_eq!( - stale_ping.authorization.as_deref(), - Some(format!("Bearer {STALE_CAPABILITY}").as_str()) - ); - std::fs::write( - &path, - format!( - r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{CURRENT_CAPABILITY}"}}"# - ), - ) - .expect("publish new generation and capability"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .expect("keep rewritten discovery private"); - } - requests.recv().expect("record new-generation ping") - }); - - let endpoint = handshake(&lock_file.0).expect("generation mismatch retries successfully"); - assert_eq!(endpoint.port, port); - let current_ping = rewrite.join().expect("discovery rewrite exits"); assert_eq!( - current_ping.authorization.as_deref(), - Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + serde_json::from_str::(&call.2).unwrap()["command"], + "sessions" ); - broker.join().expect("test broker exits"); + worker.join().unwrap(); } + #[cfg(unix)] #[test] - fn handshake_rejects_stale_capability() { - // The first 403 triggers the one permitted discovery re-read; an - // unchanged stale record must still fail closed on the second probe. - let (port, requests, broker) = spawn_broker(CURRENT_CAPABILITY, 2); - let lock_file = TempDiscoveryFile::new("stale", port, STALE_CAPABILITY); - - let failure = handshake(&lock_file.0).expect_err("stale capability fails closed"); + fn bootstrap_rejects_mismatched_discovery_generation() { + let discovery = TempDiscoveryFile::new(43123, 7); + let bootstrap = + std::os::unix::net::UnixListener::bind(&discovery.bootstrap_endpoint).unwrap(); + let worker = thread::spawn(move || { + let (mut stream, _) = bootstrap.accept().unwrap(); + writeln!(stream, r#"{{"port":43123,"generation":6,"protocolVersion":{PROTOCOL_VERSION},"capability":"{TEST_CAPABILITY}"}}"#).unwrap(); + }); + let failure = handshake(&discovery.path).expect_err("mismatched generation fails closed"); assert_eq!(failure.exit, EXIT_ENV); - assert!(failure.message.contains("ping returned status 403")); - for _ in 0..2 { - let ping = requests.recv().expect("record stale ping"); - assert_eq!( - ping.authorization.as_deref(), - Some(format!("Bearer {STALE_CAPABILITY}").as_str()) - ); - } - - broker.join().expect("test broker exits"); + assert!(failure.message.contains("restarted its control endpoint")); + worker.join().unwrap(); } #[test] diff --git a/src-tauri/crates/berdctl/src/discovery.rs b/src-tauri/crates/berdctl/src/discovery.rs index c7b9a6c79..7693d096f 100644 --- a/src-tauri/crates/berdctl/src/discovery.rs +++ b/src-tauri/crates/berdctl/src/discovery.rs @@ -20,7 +20,6 @@ pub const NOT_UNDER_APP: &str = "berdctl must run inside a Berd desktop app session (the app sets this up automatically)"; const REREAD_DELAY: Duration = Duration::from_millis(200); -const CAPABILITY_HEX_LEN: usize = 64; /// Shape of the discovery file the berdctl broker writes on start /// (`/berdctl/control-.json`). Duplicated by hand from @@ -32,7 +31,7 @@ pub struct DiscoveryFile { pub pid: u32, pub generation: u64, pub protocol_version: u32, - pub capability: String, + pub bootstrap_endpoint: PathBuf, } #[derive(Deserialize)] @@ -42,27 +41,21 @@ struct RawDiscoveryFile { pid: u32, generation: u64, protocol_version: u32, - capability: String, + bootstrap_endpoint: PathBuf, } impl TryFrom for DiscoveryFile { type Error = String; - fn try_from(raw: RawDiscoveryFile) -> Result { - if raw.capability.len() != CAPABILITY_HEX_LEN - || !raw - .capability - .bytes() - .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) - { - return Err("capability must be a 256-bit hexadecimal value".to_string()); + if raw.bootstrap_endpoint.as_os_str().is_empty() { + return Err("bootstrap endpoint must not be empty".to_string()); } Ok(Self { port: raw.port, pid: raw.pid, generation: raw.generation, protocol_version: raw.protocol_version, - capability: raw.capability, + bootstrap_endpoint: raw.bootstrap_endpoint, }) } } @@ -181,8 +174,7 @@ pub fn load_with_retry(path: &Path) -> Result { mod tests { use super::*; - const CAPABILITY: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; - const VALID: &str = r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}"#; + const VALID: &str = r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1,"bootstrapEndpoint":"/tmp/berdctl.sock"}"#; #[test] fn parses_a_valid_discovery_file() { @@ -194,7 +186,7 @@ mod tests { pid: 4242, generation: 3, protocol_version: 1, - capability: CAPABILITY.to_string(), + bootstrap_endpoint: PathBuf::from("/tmp/berdctl.sock"), } ); } @@ -202,7 +194,7 @@ mod tests { #[test] fn tolerates_unknown_fields_for_forward_compat() { let file = parse( - r#"{"port":1,"pid":2,"generation":3,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef","future":"x"}"#, + r#"{"port":1,"pid":2,"generation":3,"protocolVersion":1,"bootstrapEndpoint":"/tmp/berdctl.sock","future":"x"}"#, ) .expect("unknown fields are ignored"); assert_eq!(file.port, 1); @@ -219,7 +211,7 @@ mod tests { assert!(parse(r#"{"port":52341,"pid":4242}"#).is_err()); assert!( parse(r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1}"#).is_err(), - "legacy discovery without a capability must fail closed" + "legacy discovery without a bootstrap endpoint must fail closed" ); assert!(parse(r#"{}"#).is_err()); } @@ -227,27 +219,17 @@ mod tests { #[test] fn rejects_wrongly_typed_or_malformed_fields() { assert!(parse( - r#"{"port":"not-a-port","pid":1,"generation":1,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}"# + r#"{"port":"not-a-port","pid":1,"generation":1,"protocolVersion":1,"bootstrapEndpoint":"/tmp/berdctl.sock"}"# + ) + .is_err()); + assert!(parse( + r#"{"port":1,"pid":1,"generation":1,"protocolVersion":1,"bootstrapEndpoint":123}"# + ) + .is_err()); + assert!(parse( + r#"{"port":1,"pid":1,"generation":1,"protocolVersion":1,"bootstrapEndpoint":""}"# ) .is_err()); - assert!( - parse(r#"{"port":1,"pid":1,"generation":1,"protocolVersion":1,"capability":123}"#) - .is_err() - ); - for capability in [ - "", - "short", - "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", - "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF", - ] { - let contents = format!( - r#"{{"port":1,"pid":1,"generation":1,"protocolVersion":1,"capability":"{capability}"}}"# - ); - assert!( - parse(&contents).is_err(), - "malformed capability {capability:?} must fail closed" - ); - } } #[test] @@ -303,7 +285,7 @@ mod tests { std::fs::write(&path, VALID).unwrap(); std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); - let error = load(&path).expect_err("world-readable capability must fail closed"); + let error = load(&path).expect_err("permissive discovery file must fail closed"); assert!(error.contains("accessible by other users")); std::fs::remove_dir_all(base).ok(); } diff --git a/src-tauri/plugins/berdctl/Cargo.toml b/src-tauri/plugins/berdctl/Cargo.toml index 299da4bca..70620caa7 100644 --- a/src-tauri/plugins/berdctl/Cargo.toml +++ b/src-tauri/plugins/berdctl/Cargo.toml @@ -15,15 +15,25 @@ hex = { version = "0.4", optional = true } log = "0.4" serde = { version = "1", features = ["derive"] } serde_json = "1" +interprocess = { version = "2.4.3", features = ["tokio"] } subtle = { version = "2", optional = true } tauri = { version = "2", default-features = false } -tokio = { version = "1", features = ["sync", "time", "rt", "net"] } +tokio = { version = "1", features = ["sync", "time", "rt", "net", "io-util", "process"] } uuid = { version = "1", features = ["v4"] } axum = { version = "0.8", default-features = false, features = ["http1", "json", "tokio"], optional = true } [target.'cfg(unix)'.dependencies] libc = { version = "0.2", optional = true } +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.59", features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_System_Diagnostics_ToolHelp", + "Win32_System_JobObjects", + "Win32_System_Threading", +] } + [features] # Without `server` the crate compiles to an inert stub: permissions are still # generated by build.rs, but no runtime code (including `init`) exists. diff --git a/src-tauri/plugins/berdctl/src/authorization.rs b/src-tauri/plugins/berdctl/src/authorization.rs new file mode 100644 index 000000000..3b7c3a5ad --- /dev/null +++ b/src-tauri/plugins/berdctl/src/authorization.rs @@ -0,0 +1,366 @@ +//! Kernel-backed admission for berdctl bootstrap connections. +//! +//! A caller is admitted only when its process belongs to the current Berd-owned +//! `goosed` tree. Unix proves that by walking stable `(pid, start time)` process +//! snapshots to the retained root. Windows uses an exact, retained Job Object; +//! logical parent PIDs are not an authorization primitive there. + +use std::io; +use std::sync::{Arc, OnceLock, RwLock}; + +static AUTHORIZER: OnceLock = OnceLock::new(); + +pub(crate) fn authorizer() -> ProcessAuthorizer { + AUTHORIZER.get_or_init(ProcessAuthorizer::default).clone() +} + +pub fn prepare_goosed(command: &mut tokio::process::Command) -> io::Result { + prepare_goosed_authorization(authorizer(), command) +} + +#[derive(Clone, Default)] +pub(crate) struct ProcessAuthorizer { + root: Arc>>, +} + +impl ProcessAuthorizer { + pub(crate) fn authorize(&self, pid: u32) -> io::Result { + let root = self.root.read().unwrap(); + let Some(root) = root.as_ref() else { + return Ok(false); + }; + root.authorize(pid) + } + + #[cfg(unix)] + pub(crate) fn install_root(&self, pid: u32) -> io::Result<()> { + let root = PlatformRoot::capture(pid)?; + *self.root.write().unwrap() = Some(root); + Ok(()) + } + + #[cfg(windows)] + fn install_job(&self, job: Arc) { + *self.root.write().unwrap() = Some(PlatformRoot { job }); + } +} + +#[cfg(unix)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +struct ProcessSnapshot { + pid: u32, + parent_pid: u32, + started_at: u64, +} + +#[cfg(unix)] +#[derive(Clone, Copy)] +struct PlatformRoot(ProcessSnapshot); + +#[cfg(unix)] +impl PlatformRoot { + fn capture(pid: u32) -> io::Result { + Ok(Self(process_snapshot(pid)?)) + } + + fn authorize(&self, peer_pid: u32) -> io::Result { + const MAX_DEPTH: usize = 128; + let mut pid = peer_pid; + let mut chain = Vec::new(); + for _ in 0..MAX_DEPTH { + if pid == 0 + || chain + .iter() + .any(|snapshot: &ProcessSnapshot| snapshot.pid == pid) + { + return Ok(false); + } + let snapshot = process_snapshot(pid)?; + chain.push(snapshot); + if snapshot == self.0 { + // Re-read every hop after reaching the root. Any PID reuse or + // parent mutation observed during the walk fails closed. + for expected in &chain { + if process_snapshot(expected.pid)? != *expected { + return Ok(false); + } + } + return Ok(true); + } + pid = snapshot.parent_pid; + } + Ok(false) + } +} + +#[cfg(target_os = "linux")] +fn process_snapshot(pid: u32) -> io::Result { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat"))?; + let close = stat.rfind(')').ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "missing process comm terminator", + ) + })?; + let fields: Vec<&str> = stat[close + 1..].split_whitespace().collect(); + // After `comm`, fields[0] is state (field 3), fields[1] is ppid (4), and + // fields[19] is starttime (22). + if fields.len() <= 19 { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "truncated process stat", + )); + } + let parent_pid = fields[1] + .parse() + .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "invalid parent pid"))?; + let started_at = fields[19] + .parse() + .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "invalid process start time"))?; + Ok(ProcessSnapshot { + pid, + parent_pid, + started_at, + }) +} + +#[cfg(target_os = "macos")] +fn process_snapshot(pid: u32) -> io::Result { + let pid_i32 = i32::try_from(pid) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "pid outside platform range"))?; + let mut info = std::mem::MaybeUninit::::zeroed(); + let expected = std::mem::size_of::(); + // SAFETY: `info` points to writable storage of exactly the supplied size. + let read = unsafe { + libc::proc_pidinfo( + pid_i32, + libc::PROC_PIDTBSDINFO, + 0, + info.as_mut_ptr().cast(), + expected as i32, + ) + }; + if read != expected as i32 { + return Err(io::Error::last_os_error()); + } + // SAFETY: proc_pidinfo initialized the full structure, verified above. + let info = unsafe { info.assume_init() }; + Ok(ProcessSnapshot { + pid: info.pbi_pid, + parent_pid: info.pbi_ppid, + started_at: info + .pbi_start_tvsec + .saturating_mul(1_000_000) + .saturating_add(info.pbi_start_tvusec), + }) +} + +#[cfg(all(unix, not(any(target_os = "linux", target_os = "macos"))))] +fn process_snapshot(_pid: u32) -> io::Result { + Err(io::Error::new( + io::ErrorKind::Unsupported, + "berdctl process admission is unsupported on this Unix platform", + )) +} + +#[cfg(windows)] +struct PlatformRoot { + job: Arc, +} + +#[cfg(windows)] +impl PlatformRoot { + fn authorize(&self, pid: u32) -> io::Result { + self.job.contains_pid(pid) + } +} + +/// Spawn authorization guard. On Windows it owns the exact no-breakaway Job +/// Object and keeps the child suspended until admission is established. +pub struct GoosedAuthorization { + authorizer: ProcessAuthorizer, + #[cfg(windows)] + job: Arc, +} + +pub(crate) fn prepare_goosed_authorization( + authorizer: ProcessAuthorizer, + command: &mut tokio::process::Command, +) -> io::Result { + #[cfg(windows)] + { + let job = Arc::new(WindowsJob::new()?); + command.creation_flags( + windows_sys::Win32::System::Threading::CREATE_NO_WINDOW + | windows_sys::Win32::System::Threading::CREATE_SUSPENDED, + ); + Ok(GoosedAuthorization { authorizer, job }) + } + #[cfg(not(windows))] + { + let _ = command; + Ok(GoosedAuthorization { authorizer }) + } +} + +impl GoosedAuthorization { + pub fn admit(self, child: &tokio::process::Child) -> io::Result<()> { + let pid = child + .id() + .ok_or_else(|| io::Error::other("goosed child has no process id"))?; + #[cfg(unix)] + { + self.authorizer.install_root(pid) + } + #[cfg(windows)] + { + let result = self + .job + .assign_pid(pid) + .and_then(|()| resume_process_main_thread(pid)); + if result.is_ok() { + self.authorizer.install_job(self.job); + } + result + } + } +} + +#[cfg(windows)] +struct WindowsJob { + handle: windows_sys::Win32::Foundation::HANDLE, +} +#[cfg(windows)] +unsafe impl Send for WindowsJob {} +#[cfg(windows)] +unsafe impl Sync for WindowsJob {} + +#[cfg(windows)] +impl WindowsJob { + fn new() -> io::Result { + use windows_sys::Win32::System::JobObjects::*; + let handle = unsafe { CreateJobObjectW(std::ptr::null(), std::ptr::null()) }; + if handle.is_null() { + return Err(io::Error::last_os_error()); + } + let job = Self { handle }; + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = unsafe { std::mem::zeroed() }; + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + let ok = unsafe { + SetInformationJobObject( + handle, + JobObjectExtendedLimitInformation, + (&info as *const JOBOBJECT_EXTENDED_LIMIT_INFORMATION).cast(), + std::mem::size_of_val(&info) as u32, + ) + }; + if ok == 0 { + return Err(io::Error::last_os_error()); + } + Ok(job) + } + + fn assign_pid(&self, pid: u32) -> io::Result<()> { + use windows_sys::Win32::Foundation::CloseHandle; + use windows_sys::Win32::System::JobObjects::AssignProcessToJobObject; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SET_QUOTA, PROCESS_TERMINATE, + }; + let process = unsafe { OpenProcess(PROCESS_SET_QUOTA | PROCESS_TERMINATE, 0, pid) }; + if process.is_null() { + return Err(io::Error::last_os_error()); + } + let ok = unsafe { AssignProcessToJobObject(self.handle, process) }; + let error = (ok == 0).then(io::Error::last_os_error); + unsafe { CloseHandle(process) }; + error.map_or(Ok(()), Err) + } + + fn contains_pid(&self, pid: u32) -> io::Result { + use windows_sys::Win32::Foundation::CloseHandle; + use windows_sys::Win32::System::JobObjects::IsProcessInJob; + use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, + }; + let process = unsafe { OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid) }; + if process.is_null() { + return Err(io::Error::last_os_error()); + } + let mut result = 0; + let ok = unsafe { IsProcessInJob(process, self.handle, &mut result) }; + let error = (ok == 0).then(io::Error::last_os_error); + unsafe { CloseHandle(process) }; + error.map_or(Ok(result != 0), Err) + } +} + +#[cfg(windows)] +impl Drop for WindowsJob { + fn drop(&mut self) { + unsafe { windows_sys::Win32::Foundation::CloseHandle(self.handle) }; + } +} + +#[cfg(windows)] +fn resume_process_main_thread(pid: u32) -> io::Result<()> { + use windows_sys::Win32::Foundation::{CloseHandle, INVALID_HANDLE_VALUE}; + use windows_sys::Win32::System::Diagnostics::ToolHelp::{ + CreateToolhelp32Snapshot, Thread32First, Thread32Next, TH32CS_SNAPTHREAD, THREADENTRY32, + }; + use windows_sys::Win32::System::Threading::{OpenThread, ResumeThread, THREAD_SUSPEND_RESUME}; + let snapshot = unsafe { CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0) }; + if snapshot == INVALID_HANDLE_VALUE { + return Err(io::Error::last_os_error()); + } + let mut entry: THREADENTRY32 = unsafe { std::mem::zeroed() }; + entry.dwSize = std::mem::size_of::() as u32; + let mut found = false; + let mut current = unsafe { Thread32First(snapshot, &mut entry) }; + while current != 0 { + if entry.th32OwnerProcessID == pid { + let thread = unsafe { OpenThread(THREAD_SUSPEND_RESUME, 0, entry.th32ThreadID) }; + if !thread.is_null() { + found = unsafe { ResumeThread(thread) } != u32::MAX; + unsafe { CloseHandle(thread) }; + if found { + break; + } + } + } + current = unsafe { Thread32Next(snapshot, &mut entry) }; + } + unsafe { CloseHandle(snapshot) }; + if found { + Ok(()) + } else { + Err(io::Error::other("could not resume goosed main thread")) + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + + #[test] + fn admits_descendant_and_rejects_sibling_of_root() { + let authorizer = ProcessAuthorizer::default(); + authorizer.install_root(std::process::id()).unwrap(); + let mut child = std::process::Command::new("sleep") + .arg("5") + .spawn() + .unwrap(); + assert!(authorizer.authorize(child.id()).unwrap()); + let _ = child.kill(); + let _ = child.wait(); + + let mut fake_root = std::process::Command::new("sleep") + .arg("5") + .spawn() + .unwrap(); + let isolated = ProcessAuthorizer::default(); + isolated.install_root(fake_root.id()).unwrap(); + assert!(!matches!(isolated.authorize(std::process::id()), Ok(true))); + let _ = fake_root.kill(); + let _ = fake_root.wait(); + } +} diff --git a/src-tauri/plugins/berdctl/src/bootstrap.rs b/src-tauri/plugins/berdctl/src/bootstrap.rs new file mode 100644 index 000000000..53e2f27c3 --- /dev/null +++ b/src-tauri/plugins/berdctl/src/bootstrap.rs @@ -0,0 +1,231 @@ +//! Authenticated local bootstrap transport. The endpoint is discoverable; the +//! kernel-reported peer process is the credential. + +use crate::authorization::ProcessAuthorizer; +use serde::Serialize; +use std::io; +use std::path::Path; +use tokio::io::AsyncWriteExt; +use tokio::sync::oneshot; + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct LeaseResponse<'a> { + port: u16, + generation: u64, + protocol_version: u32, + capability: &'a str, +} + +pub(crate) struct BootstrapHandle { + shutdown: oneshot::Sender<()>, + #[cfg(unix)] + endpoint: std::path::PathBuf, +} +impl BootstrapHandle { + pub(crate) fn shutdown(self) { + let _ = self.shutdown.send(()); + #[cfg(unix)] + if let Err(error) = std::fs::remove_file(&self.endpoint) { + if error.kind() != io::ErrorKind::NotFound { + log::warn!("[berdctl] failed to remove bootstrap endpoint: {error}"); + } + } + } +} + +pub(crate) fn start( + endpoint: &Path, + port: u16, + generation: u64, + capability: String, + authorizer: ProcessAuthorizer, +) -> io::Result { + #[cfg(unix)] + let listener = { + use std::os::unix::fs::PermissionsExt; + let listener = tokio::net::UnixListener::bind(endpoint)?; + std::fs::set_permissions(endpoint, std::fs::Permissions::from_mode(0o600))?; + listener + }; + #[cfg(windows)] + use interprocess::local_socket::tokio::prelude::*; + #[cfg(windows)] + let listener = { + use interprocess::local_socket::{GenericNamespaced, ListenerOptions, ToNsName}; + let name = endpoint + .to_string_lossy() + .to_string() + .to_ns_name::()?; + ListenerOptions::new() + .name(name) + .reclaim_name(false) + .try_overwrite(false) + .create_tokio()? + }; + + let (shutdown_tx, mut shutdown_rx) = oneshot::channel(); + #[cfg(unix)] + let endpoint_for_cleanup = endpoint.to_path_buf(); + tokio::spawn(async move { + loop { + #[cfg(unix)] + let mut stream = tokio::select! { + _ = &mut shutdown_rx => break, + accepted = listener.accept() => match accepted { + Ok((stream, _address)) => stream, + Err(error) => { log::warn!("[berdctl] bootstrap accept failed: {error}"); continue; } + } + }; + #[cfg(windows)] + let mut stream = tokio::select! { + _ = &mut shutdown_rx => break, + accepted = listener.accept() => match accepted { + Ok(stream) => stream, + Err(error) => { log::warn!("[berdctl] bootstrap accept failed: {error}"); continue; } + } + }; + let authorizer = authorizer.clone(); + let capability = capability.clone(); + tokio::spawn(async move { + #[cfg(unix)] + let peer_pid = stream + .peer_cred() + .ok() + .and_then(|credentials| credentials.pid()) + .and_then(|pid| u32::try_from(pid).ok()); + #[cfg(windows)] + let peer_pid = { + stream + .peer_creds() + .ok() + .and_then(|credentials| credentials.pid()) + }; + let admitted = + peer_pid.and_then(|pid| authorizer.authorize(pid).ok()) == Some(true); + if !admitted { + log::warn!( + "[berdctl] rejected bootstrap peer outside the app-owned goosed tree" + ); + return; + } + let response = LeaseResponse { + port, + generation, + protocol_version: crate::discovery::PROTOCOL_VERSION, + capability: &capability, + }; + if let Ok(mut payload) = serde_json::to_vec(&response) { + payload.push(b'\n'); + let _ = stream.write_all(&payload).await; + } + }); + } + #[cfg(unix)] + if let Err(error) = std::fs::remove_file(&endpoint_for_cleanup) { + if error.kind() != io::ErrorKind::NotFound { + log::warn!("[berdctl] failed to remove bootstrap endpoint: {error}"); + } + } + }); + Ok(BootstrapHandle { + shutdown: shutdown_tx, + #[cfg(unix)] + endpoint: endpoint.to_path_buf(), + }) +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::io::{BufRead, BufReader}; + + fn test_endpoint(label: &str) -> std::path::PathBuf { + std::env::temp_dir().join(format!( + "bctl-{label}-{}-{}.sock", + std::process::id(), + &uuid::Uuid::new_v4().simple().to_string()[..8] + )) + } + + fn connect(endpoint: &Path) -> String { + let stream = std::os::unix::net::UnixStream::connect(endpoint).unwrap(); + let mut response = String::new(); + BufReader::new(stream).read_line(&mut response).unwrap(); + response + } + + #[tokio::test] + async fn clean_app_data_starts_and_admitted_process_receives_capability() { + let app_data = std::env::temp_dir().join(format!( + "bctl-clean-{}-{}", + std::process::id(), + &uuid::Uuid::new_v4().simple().to_string()[..8] + )); + std::fs::remove_dir_all(&app_data).ok(); + crate::discovery::prepare_discovery_directory(&app_data).unwrap(); + let endpoint = crate::discovery::bootstrap_endpoint(&app_data, std::process::id()); + let authorizer = ProcessAuthorizer::default(); + authorizer.install_root(std::process::id()).unwrap(); + let handle = start(&endpoint, 43123, 7, "test-capability".into(), authorizer).unwrap(); + + let response = tokio::task::spawn_blocking({ + let endpoint = endpoint.clone(); + move || connect(&endpoint) + }) + .await + .unwrap(); + let response: serde_json::Value = serde_json::from_str(&response).unwrap(); + assert_eq!(response["port"], 43123); + assert_eq!(response["generation"], 7); + assert_eq!(response["capability"], "test-capability"); + + handle.shutdown(); + assert!(!endpoint.exists()); + std::fs::remove_dir_all(app_data).ok(); + } + + #[tokio::test] + async fn stale_socket_from_crash_does_not_block_restart() { + let app_data = std::env::temp_dir().join(format!( + "bctl-restart-{}-{}", + std::process::id(), + &uuid::Uuid::new_v4().simple().to_string()[..8] + )); + crate::discovery::prepare_discovery_directory(&app_data).unwrap(); + let stale_endpoint = crate::discovery::bootstrap_endpoint(&app_data, std::process::id()); + let stale_listener = std::os::unix::net::UnixListener::bind(&stale_endpoint).unwrap(); + drop(stale_listener); + + let replacement = crate::discovery::bootstrap_endpoint(&app_data, std::process::id()); + assert_ne!(replacement, stale_endpoint); + let authorizer = ProcessAuthorizer::default(); + let handle = start(&replacement, 43123, 7, "test-capability".into(), authorizer).unwrap(); + handle.shutdown(); + std::fs::remove_dir_all(app_data).ok(); + } + + #[tokio::test] + async fn unrelated_process_receives_no_capability() { + let endpoint = test_endpoint("rejected"); + let mut unrelated_root = std::process::Command::new("sleep") + .arg("5") + .spawn() + .unwrap(); + let authorizer = ProcessAuthorizer::default(); + authorizer.install_root(unrelated_root.id()).unwrap(); + let handle = start(&endpoint, 43123, 7, "test-capability".into(), authorizer).unwrap(); + + let response = tokio::task::spawn_blocking({ + let endpoint = endpoint.clone(); + move || connect(&endpoint) + }) + .await + .unwrap(); + assert!(response.is_empty()); + + handle.shutdown(); + let _ = unrelated_root.kill(); + let _ = unrelated_root.wait(); + } +} diff --git a/src-tauri/plugins/berdctl/src/discovery.rs b/src-tauri/plugins/berdctl/src/discovery.rs index a64a7be2a..57f643580 100644 --- a/src-tauri/plugins/berdctl/src/discovery.rs +++ b/src-tauri/plugins/berdctl/src/discovery.rs @@ -1,6 +1,6 @@ -//! Per-instance discovery ("lock") file the berdctl CLI reads to find and -//! authenticate to the running broker: `{port, pid, generation, -//! protocolVersion, capability}`. +//! Per-instance discovery ("lock") file the berdctl CLI reads to find the +//! running broker and its authenticated-bootstrap endpoint. The record contains +//! no bearer credential. //! //! The path formula and protocol version are exported unconditionally (not //! behind the `server` feature) so the app crate can compute the path for the @@ -38,6 +38,23 @@ pub fn discovery_file_path(app_data_dir: &Path, pid: u32) -> PathBuf { )) } +/// Per-instance authenticated bootstrap endpoint. This value is an address, +/// not a credential; the listener authorizes the kernel-reported peer process. +#[cfg(feature = "server")] +pub(crate) fn bootstrap_endpoint(app_data_dir: &Path, pid: u32) -> PathBuf { + let nonce = uuid::Uuid::new_v4().simple(); + #[cfg(unix)] + { + let _ = app_data_dir; + std::env::temp_dir().join(format!("bctl-{pid}-{nonce}.sock")) + } + #[cfg(windows)] + { + let _ = app_data_dir; + PathBuf::from(format!("berdctl-bootstrap-{pid}-{nonce}")) + } +} + /// Owning app pid encoded in a discovery file name. Recognized forms are the /// final `control-.json`, legacy `control-.json.tmp`, and current /// `control-.json.<32 lowercase hex chars>.tmp` orphan names. `None` for @@ -103,17 +120,31 @@ fn private_discovery_directory(dir: &Path) -> std::io::Result<()> { } } +#[cfg(feature = "server")] +pub(crate) fn prepare_discovery_directory(app_data_dir: &Path) -> std::io::Result { + let dir = app_data_dir.join(DISCOVERY_DIR_NAME); + let mut builder = std::fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + builder.create(&dir)?; + private_discovery_directory(&dir)?; + Ok(dir) +} + /// Atomically write the discovery file: private dir + temp file + fsync + -/// rename, so a CLI reading mid-write never sees partial JSON. The capability -/// is sensitive to other users on the host, so Unix paths are tightened to -/// owner-only access even when they predate this write. +/// rename, so a CLI reading mid-write never sees partial JSON. Unix paths stay +/// owner-only to prevent other users from redirecting the bootstrap address. #[cfg(feature = "server")] pub(crate) fn write_discovery_file( path: &Path, port: u16, pid: u32, generation: u64, - capability: &str, + bootstrap_endpoint: &Path, ) -> std::io::Result<()> { use std::io::Write; @@ -135,7 +166,7 @@ pub(crate) fn write_discovery_file( "pid": pid, "generation": generation, "protocolVersion": PROTOCOL_VERSION, - "capability": capability, + "bootstrapEndpoint": bootstrap_endpoint.to_string_lossy(), }); // Use a unique adjacent path for each write. A stale fixed-name temp file @@ -308,14 +339,8 @@ mod tests { symlink(&target, &link).unwrap(); let path = link.join("control-4242.json"); - let error = write_discovery_file( - &path, - 8080, - 4242, - 7, - "1111111111111111111111111111111111111111111111111111111111111111", - ) - .expect_err("symlinked discovery directory must fail closed"); + let error = write_discovery_file(&path, 8080, 4242, 7, Path::new("/tmp/bootstrap.sock")) + .expect_err("symlinked discovery directory must fail closed"); assert!(!target.join("control-4242.json").exists()); assert_ne!(error.kind(), std::io::ErrorKind::NotFound); @@ -325,23 +350,24 @@ mod tests { #[cfg(feature = "server")] #[test] fn write_and_remove_lifecycle() { - const FIRST_CAPABILITY: &str = - "1111111111111111111111111111111111111111111111111111111111111111"; - const ROTATED_CAPABILITY: &str = - "2222222222222222222222222222222222222222222222222222222222222222"; + let first_endpoint = Path::new("/tmp/bootstrap-first.sock"); + let rotated_endpoint = Path::new("/tmp/bootstrap-rotated.sock"); let base = std::env::temp_dir().join(format!("berdctl-discovery-test-{}", std::process::id())); std::fs::remove_dir_all(&base).ok(); let path = discovery_file_path(&base, 4242); - write_discovery_file(&path, 8080, 4242, 7, FIRST_CAPABILITY).unwrap(); + write_discovery_file(&path, 8080, 4242, 7, first_endpoint).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(parsed["port"], 8080); assert_eq!(parsed["pid"], 4242); assert_eq!(parsed["generation"], 7); assert_eq!(parsed["protocolVersion"], PROTOCOL_VERSION); - assert_eq!(parsed["capability"], FIRST_CAPABILITY); + assert_eq!( + parsed["bootstrapEndpoint"], + first_endpoint.to_string_lossy().as_ref() + ); // The temp file is renamed away, never left behind. let leftovers: Vec<_> = std::fs::read_dir(path.parent().unwrap()) .unwrap() @@ -377,13 +403,16 @@ mod tests { std::fs::set_permissions(&path, unix_permissions(0o644)).unwrap(); } - // Restart case: an atomic rewrite rotates both generation and secret. - write_discovery_file(&path, 9090, 4242, 8, ROTATED_CAPABILITY).unwrap(); + // Restart case: an atomic rewrite rotates both generation and endpoint. + write_discovery_file(&path, 9090, 4242, 8, rotated_endpoint).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(parsed["port"], 9090); assert_eq!(parsed["generation"], 8); - assert_eq!(parsed["capability"], ROTATED_CAPABILITY); + assert_eq!( + parsed["bootstrapEndpoint"], + rotated_endpoint.to_string_lossy().as_ref() + ); assert_eq!(std::fs::read_to_string(&legacy_tmp).unwrap(), "stale"); #[cfg(unix)] diff --git a/src-tauri/plugins/berdctl/src/lib.rs b/src-tauri/plugins/berdctl/src/lib.rs index a3a8f8227..29d8681bc 100644 --- a/src-tauri/plugins/berdctl/src/lib.rs +++ b/src-tauri/plugins/berdctl/src/lib.rs @@ -3,8 +3,9 @@ //! A lazily started, loopback-only HTTP server (`GET /v1/ping`, `POST //! /v1/call`) that forwards commands over a request/response bridge into the //! main-window renderer. The CLI finds it through a per-instance, owner-private -//! discovery file written on start and removed on stop/exit, and presents the -//! file's fresh bearer capability on every broker request. +//! discovery file written on start and removed on stop/exit. The file carries +//! only a local bootstrap address; kernel peer-process admission releases the +//! bearer capability to descendants of this instance's owned goosed tree. //! //! Without the `server` feature this crate is an inert stub: build.rs still //! generates the command permissions (so capability validation passes in @@ -12,12 +13,18 @@ //! exists. Only the discovery path helpers below stay unconditional so the //! app crate can compute paths without enabling the broker. +#[cfg(feature = "server")] +mod authorization; +#[cfg(feature = "server")] +mod bootstrap; #[cfg(feature = "server")] mod bridge; mod discovery; #[cfg(feature = "server")] mod server; +#[cfg(feature = "server")] +pub use authorization::{prepare_goosed, GoosedAuthorization}; pub use discovery::{discovery_file_path, owner_pid_from_discovery_file_name, DISCOVERY_DIR_NAME}; #[cfg(feature = "server")] @@ -25,8 +32,7 @@ mod plugin { use crate::bridge::{Bridge, BridgeError, BridgeRequest, BridgeResult}; use crate::discovery; use crate::server::{ - self, generate_capability, BridgeDispatcher, ServerContext, ServerHandle, TimeoutStore, - IN_FLIGHT_LIMIT, + self, BridgeDispatcher, ServerContext, ServerHandle, TimeoutStore, IN_FLIGHT_LIMIT, }; use serde::Serialize; use std::collections::HashMap; @@ -152,8 +158,6 @@ mod plugin { return Ok(StartedEndpoint { port: handle.port }); } let generation = state.generation.fetch_add(1, Ordering::Relaxed) + 1; - let capability = generate_capability() - .map_err(|err| format!("failed to generate berdctl capability: {err}"))?; // Each server gets its own semaphore: graceful shutdown lets the // previous server's in-flight handlers outlive `stop`, and their // permits must release slots on that dead instance, not free up (and @@ -166,22 +170,29 @@ mod plugin { state.timeouts.clone(), Arc::new(tokio::sync::Semaphore::new(IN_FLIGHT_LIMIT)), generation, - capability.clone(), )); - let handle = server::start_server(ctx) - .await - .map_err(|err| format!("failed to start berdctl server: {err}"))?; - let port = handle.port; - - // The CLI can only find the broker through the discovery file, so a - // failed write means a failed start. let app_data_dir = app .path() .app_data_dir() .map_err(|err| format!("failed to resolve app data dir: {err}"))?; + discovery::prepare_discovery_directory(&app_data_dir) + .map_err(|err| format!("failed to prepare berdctl discovery directory: {err}"))?; + let bootstrap_endpoint = discovery::bootstrap_endpoint(&app_data_dir, std::process::id()); + let handle = server::start_server_with_bootstrap( + ctx, + &bootstrap_endpoint, + crate::authorization::authorizer(), + ) + .await + .map_err(|err| format!("failed to start berdctl server: {err}"))?; + let port = handle.port; + + // The CLI can only find the broker through the discovery file, so a + // failed write means a failed start. let pid = std::process::id(); let path = discovery::discovery_file_path(&app_data_dir, pid); - if let Err(err) = discovery::write_discovery_file(&path, port, pid, generation, &capability) + if let Err(err) = + discovery::write_discovery_file(&path, port, pid, generation, &bootstrap_endpoint) { handle.shutdown(); return Err(format!( diff --git a/src-tauri/plugins/berdctl/src/server.rs b/src-tauri/plugins/berdctl/src/server.rs index 8f7fb53a5..0dedfaee3 100644 --- a/src-tauri/plugins/berdctl/src/server.rs +++ b/src-tauri/plugins/berdctl/src/server.rs @@ -2,7 +2,7 @@ //! //! Serves `GET /v1/ping` (generation/protocol handshake) and `POST /v1/call` //! (command dispatch over the renderer bridge). Every route requires the -//! per-server bearer capability published in the private discovery file. The +//! per-server bearer capability released by authenticated local bootstrap. The //! existing Origin, Sec-Fetch, and literal Host checks remain a separate //! defense against browser-JS-to-localhost and DNS rebinding. @@ -137,14 +137,14 @@ impl ServerContext { timeouts: Arc, inflight: Arc, generation: u64, - capability: String, ) -> Self { Self { dispatcher, timeouts, inflight, generation, - capability, + capability: generate_capability() + .expect("operating-system randomness is required for berdctl"), port: OnceLock::new(), } } @@ -155,11 +155,15 @@ impl ServerContext { pub struct ServerHandle { pub port: u16, shutdown: oneshot::Sender<()>, + bootstrap: Option, } impl ServerHandle { pub fn shutdown(self) { let _ = self.shutdown.send(()); + if let Some(bootstrap) = self.bootstrap { + bootstrap.shutdown(); + } } } @@ -183,9 +187,34 @@ pub async fn start_server( Ok(ServerHandle { port, shutdown: shutdown_tx, + bootstrap: None, }) } +pub async fn start_server_with_bootstrap( + ctx: Arc>, + endpoint: &std::path::Path, + authorizer: crate::authorization::ProcessAuthorizer, +) -> std::io::Result { + let mut handle = start_server(ctx.clone()).await?; + match crate::bootstrap::start( + endpoint, + handle.port, + ctx.generation, + ctx.capability.clone(), + authorizer, + ) { + Ok(bootstrap) => { + handle.bootstrap = Some(bootstrap); + Ok(handle) + } + Err(error) => { + handle.shutdown(); + Err(error) + } + } +} + pub fn build_router(ctx: Arc>) -> Router { Router::new() .route("/v1/ping", get(handle_ping::)) @@ -484,8 +513,11 @@ mod tests { timeouts, Arc::new(Semaphore::new(limits.permits)), TEST_GENERATION, - TEST_CAPABILITY.to_string(), )); + // Tests pin a known capability while production generates one. + let mut ctx = Arc::try_unwrap(ctx).ok().unwrap(); + ctx.capability = TEST_CAPABILITY.to_string(); + let ctx = Arc::new(ctx); let handle = start_server(ctx).await.unwrap(); TestServer { base: format!("http://127.0.0.1:{}", handle.port), diff --git a/src-tauri/src/services/acp/goose_serve.rs b/src-tauri/src/services/acp/goose_serve.rs index fb34f649a..4460d7305 100644 --- a/src-tauri/src/services/acp/goose_serve.rs +++ b/src-tauri/src/services/acp/goose_serve.rs @@ -262,6 +262,9 @@ impl GooseServeProcess { ); crate::services::process::apply_no_window_async(&mut command); + #[cfg(feature = "berdctl")] + let berdctl_authorization = tauri_plugin_berdctl::prepare_goosed(&mut command) + .map_err(|error| format!("Failed to prepare berdctl process authorization: {error}"))?; let mut child = command.spawn().map_err(|error| { diagnostic_log::record_event( DiagnosticLevel::Error, @@ -282,6 +285,11 @@ impl GooseServeProcess { ) })?; let pid = child.id(); + #[cfg(feature = "berdctl")] + if let Err(error) = berdctl_authorization.admit(&child) { + let _ = child.kill().await; + return Err(format!("Failed to authorize goosed for berdctl: {error}")); + } diagnostic_log::record_event( DiagnosticLevel::Info, DiagnosticCategory::GooseServe, From 691b79d421e6c83b7cc118d62def769214a67e45 Mon Sep 17 00:00:00 2001 From: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Date: Sun, 16 Aug 2026 12:16:43 -0700 Subject: [PATCH 3/7] fix(berdctl): bound bootstrap admission work Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/crates/berdctl/src/client.rs | 74 +++++++++++++++++- .../plugins/berdctl/src/authorization.rs | 76 ++++++++++++++++++- src-tauri/plugins/berdctl/src/bootstrap.rs | 10 ++- 3 files changed, 156 insertions(+), 4 deletions(-) diff --git a/src-tauri/crates/berdctl/src/client.rs b/src-tauri/crates/berdctl/src/client.rs index 4d0c6d905..39cc2aa97 100644 --- a/src-tauri/crates/berdctl/src/client.rs +++ b/src-tauri/crates/berdctl/src/client.rs @@ -18,6 +18,7 @@ pub const EXIT_TRANSPORT: u8 = 2; pub const EXIT_ENV: u8 = 3; const PING_TIMEOUT: Duration = Duration::from_secs(2); +const BOOTSTRAP_TIMEOUT: Duration = Duration::from_secs(2); const MAX_BOOTSTRAP_RESPONSE_BYTES: u64 = 4096; /// Above the broker's 900s command-timeout ceiling, so the broker's /// structured 504 always arrives before this client-side timeout fires. @@ -166,11 +167,28 @@ fn bootstrap(file: &discovery::DiscoveryFile) -> Result { })?; Stream::connect(name).map_err(|error| Failure::env(format!("the Berd desktop app's authenticated control bootstrap is unavailable ({error}); {CONTROL_REMEDIATION}")))? }; + #[cfg(unix)] + stream + .set_read_timeout(Some(BOOTSTRAP_TIMEOUT)) + .map_err(|error| { + Failure::env(format!( + "the Berd desktop app's authenticated control bootstrap could not set a read timeout ({error}); {CONTROL_REMEDIATION}" + )) + })?; + #[cfg(windows)] + stream.set_nonblocking(true).map_err(|error| { + Failure::env(format!( + "the Berd desktop app's authenticated control bootstrap could not set nonblocking mode ({error}); {CONTROL_REMEDIATION}" + )) + })?; let mut response = String::new(); + #[cfg(unix)] BufReader::new(stream) .take(MAX_BOOTSTRAP_RESPONSE_BYTES + 1) .read_line(&mut response) .map_err(|error| Failure::env(format!("the Berd desktop app's authenticated control bootstrap failed ({error}); {CONTROL_REMEDIATION}")))?; + #[cfg(windows)] + read_bootstrap_response_with_deadline(&mut BufReader::new(stream), &mut response)?; if response.len() as u64 > MAX_BOOTSTRAP_RESPONSE_BYTES { return Err(Failure::env( "the Berd control bootstrap returned an unexpectedly large response", @@ -203,6 +221,36 @@ fn bootstrap(file: &discovery::DiscoveryFile) -> Result { }) } +#[cfg(windows)] +fn read_bootstrap_response_with_deadline( + reader: &mut R, + response: &mut String, +) -> Result<(), Failure> { + let deadline = std::time::Instant::now() + BOOTSTRAP_TIMEOUT; + loop { + let remaining = (MAX_BOOTSTRAP_RESPONSE_BYTES + 1).saturating_sub(response.len() as u64); + if remaining == 0 { + return Ok(()); + } + match (&mut *reader).take(remaining).read_line(response) { + Ok(_) => return Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { + if std::time::Instant::now() >= deadline { + return Err(Failure::env(format!( + "the Berd desktop app's authenticated control bootstrap timed out; {CONTROL_REMEDIATION}" + ))); + } + std::thread::sleep(Duration::from_millis(10)); + } + Err(error) => { + return Err(Failure::env(format!( + "the Berd desktop app's authenticated control bootstrap failed ({error}); {CONTROL_REMEDIATION}" + ))); + } + } + } +} + /// Read the discovery file and verify the broker behind it echoes the file's /// generation and this binary's protocol version. A generation mismatch or /// authentication failure can mean the file was read across a broker restart: @@ -401,8 +449,11 @@ mod tests { impl TempDiscoveryFile { fn new(port: u16, generation: u64) -> Self { use std::os::unix::fs::PermissionsExt; + static NEXT_DISCOVERY: std::sync::atomic::AtomicU64 = + std::sync::atomic::AtomicU64::new(0); + let nonce = NEXT_DISCOVERY.fetch_add(1, std::sync::atomic::Ordering::Relaxed); let base = std::env::temp_dir().join(format!( - "berdctl-client-bootstrap-{}-{port}", + "berdctl-client-bootstrap-{}-{port}-{nonce}", std::process::id() )); std::fs::remove_dir_all(&base).ok(); @@ -548,6 +599,27 @@ mod tests { worker.join().unwrap(); } + #[cfg(unix)] + #[test] + fn bootstrap_stall_times_out_as_environment_failure() { + let discovery = TempDiscoveryFile::new(43123, 7); + let bootstrap = + std::os::unix::net::UnixListener::bind(&discovery.bootstrap_endpoint).unwrap(); + let worker = thread::spawn(move || { + let (_stream, _) = bootstrap.accept().unwrap(); + thread::sleep(BOOTSTRAP_TIMEOUT + Duration::from_secs(1)); + }); + + let started = std::time::Instant::now(); + let failure = handshake(&discovery.path).expect_err("stalled bootstrap must time out"); + assert_eq!(failure.exit, EXIT_ENV); + assert!(failure + .message + .contains("authenticated control bootstrap failed")); + assert!(started.elapsed() < BOOTSTRAP_TIMEOUT + Duration::from_secs(1)); + worker.join().unwrap(); + } + #[test] fn ok_true_yields_the_result_verbatim() { let result = classify_response(200, r#"{"ok":true,"result":{"session_id":"s1"}}"#) diff --git a/src-tauri/plugins/berdctl/src/authorization.rs b/src-tauri/plugins/berdctl/src/authorization.rs index 3b7c3a5ad..1a34b3b6d 100644 --- a/src-tauri/plugins/berdctl/src/authorization.rs +++ b/src-tauri/plugins/berdctl/src/authorization.rs @@ -64,6 +64,14 @@ impl PlatformRoot { } fn authorize(&self, peer_pid: u32) -> io::Result { + self.authorize_with(peer_pid, process_snapshot) + } + + fn authorize_with( + &self, + peer_pid: u32, + mut snapshot_for: impl FnMut(u32) -> io::Result, + ) -> io::Result { const MAX_DEPTH: usize = 128; let mut pid = peer_pid; let mut chain = Vec::new(); @@ -75,13 +83,13 @@ impl PlatformRoot { { return Ok(false); } - let snapshot = process_snapshot(pid)?; + let snapshot = snapshot_for(pid)?; chain.push(snapshot); if snapshot == self.0 { // Re-read every hop after reaching the root. Any PID reuse or // parent mutation observed during the walk fails closed. for expected in &chain { - if process_snapshot(expected.pid)? != *expected { + if snapshot_for(expected.pid)? != *expected { return Ok(false); } } @@ -340,6 +348,15 @@ fn resume_process_main_thread(pid: u32) -> io::Result<()> { #[cfg(all(test, unix))] mod tests { use super::*; + use std::collections::HashMap; + + fn snapshot(pid: u32, parent_pid: u32, started_at: u64) -> ProcessSnapshot { + ProcessSnapshot { + pid, + parent_pid, + started_at, + } + } #[test] fn admits_descendant_and_rejects_sibling_of_root() { @@ -363,4 +380,59 @@ mod tests { let _ = fake_root.kill(); let _ = fake_root.wait(); } + + #[test] + fn admits_multi_hop_descendant() { + let root = snapshot(10, 1, 100); + let snapshots = HashMap::from([ + (10, root), + (20, snapshot(20, 10, 200)), + (30, snapshot(30, 20, 300)), + ]); + + assert!(PlatformRoot(root) + .authorize_with(30, |pid| Ok(snapshots[&pid])) + .unwrap()); + } + + #[test] + fn rejects_when_mid_chain_snapshot_changes_during_revalidation() { + let root = snapshot(10, 1, 100); + let original_mid = snapshot(20, 10, 200); + let reused_mid = snapshot(20, 10, 201); + let leaf = snapshot(30, 20, 300); + let mut mid_reads = 0; + + let admitted = PlatformRoot(root) + .authorize_with(30, |pid| match pid { + 10 => Ok(root), + 20 => { + mid_reads += 1; + Ok(if mid_reads == 1 { + original_mid + } else { + reused_mid + }) + } + 30 => Ok(leaf), + _ => Err(io::Error::new(io::ErrorKind::NotFound, "unknown pid")), + }) + .unwrap(); + + assert!(!admitted); + assert_eq!(mid_reads, 2); + } + + #[test] + fn rejects_cycles_and_chains_over_depth_limit() { + let root = snapshot(10, 1, 100); + let cycle = HashMap::from([(20, snapshot(20, 30, 200)), (30, snapshot(30, 20, 300))]); + assert!(!PlatformRoot(root) + .authorize_with(30, |pid| Ok(cycle[&pid])) + .unwrap()); + + assert!(!PlatformRoot(root) + .authorize_with(1_000, |pid| Ok(snapshot(pid, pid + 1, u64::from(pid)))) + .unwrap()); + } } diff --git a/src-tauri/plugins/berdctl/src/bootstrap.rs b/src-tauri/plugins/berdctl/src/bootstrap.rs index 53e2f27c3..b5d32ef80 100644 --- a/src-tauri/plugins/berdctl/src/bootstrap.rs +++ b/src-tauri/plugins/berdctl/src/bootstrap.rs @@ -6,7 +6,9 @@ use serde::Serialize; use std::io; use std::path::Path; use tokio::io::AsyncWriteExt; -use tokio::sync::oneshot; +use tokio::sync::{oneshot, Semaphore}; + +const ADMISSION_IN_FLIGHT_LIMIT: usize = 4; #[derive(Serialize)] #[serde(rename_all = "camelCase")] @@ -65,6 +67,7 @@ pub(crate) fn start( }; let (shutdown_tx, mut shutdown_rx) = oneshot::channel(); + let admission_slots = std::sync::Arc::new(Semaphore::new(ADMISSION_IN_FLIGHT_LIMIT)); #[cfg(unix)] let endpoint_for_cleanup = endpoint.to_path_buf(); tokio::spawn(async move { @@ -85,9 +88,14 @@ pub(crate) fn start( Err(error) => { log::warn!("[berdctl] bootstrap accept failed: {error}"); continue; } } }; + let Ok(admission_slot) = admission_slots.clone().try_acquire_owned() else { + log::warn!("[berdctl] rejected bootstrap peer: admission limit reached"); + continue; + }; let authorizer = authorizer.clone(); let capability = capability.clone(); tokio::spawn(async move { + let _admission_slot = admission_slot; #[cfg(unix)] let peer_pid = stream .peer_cred() From 822b9de8a1d1b7444aa6a12b53d4e054c92c9487 Mon Sep 17 00:00:00 2001 From: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Date: Sun, 16 Aug 2026 13:16:01 -0700 Subject: [PATCH 4/7] fix(berdctl): own and revoke windows authorization jobs Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> --- scripts/windows/CI-Windows.ps1 | 15 +- .../plugins/berdctl/src/authorization.rs | 237 +++++++++++++++--- src-tauri/plugins/berdctl/src/bootstrap.rs | 117 +++++++++ src-tauri/plugins/berdctl/src/lib.rs | 2 +- src-tauri/src/services/acp/goose_serve.rs | 123 +++++++-- 5 files changed, 428 insertions(+), 66 deletions(-) diff --git a/scripts/windows/CI-Windows.ps1 b/scripts/windows/CI-Windows.ps1 index 7b706e150..0e9c357fe 100644 --- a/scripts/windows/CI-Windows.ps1 +++ b/scripts/windows/CI-Windows.ps1 @@ -1,10 +1,11 @@ # Native x64 MSVC CI gate for the managed Node runtime + npm ACP bridge. # -# Runs the Rust checks that only a real Windows host can exercise: the -# `managed_node` / `managed_acp_tools` module tests (including the native gate -# that downloads and executes the real pinned Node ZIP), plus Windows clippy in -# the default and app-feature configurations. Invoked through `just ci-windows` -# for local and release validation. +# Runs the Rust checks that only a real Windows host can exercise: berdctl's +# Job Object / named-pipe authorization tests, the `managed_node` / +# `managed_acp_tools` module tests (including the native gate that downloads and +# executes the real pinned Node ZIP), plus Windows clippy in the default and app +# feature configurations. Invoked through `just ci-windows` for local and release +# validation. $ErrorActionPreference = "Stop" trap { Write-Host $_.Exception.Message -ForegroundColor Red @@ -45,6 +46,10 @@ Invoke-CargoCheck -ArgumentList @("fmt", "--check") -Label "cargo fmt --check" # Both managed-Node modules share this test-name prefix. Run them in one process # so the Windows test binary is linked once. The live ACP bridge install has no # equivalent macOS/Linux CI coverage, so leave it for targeted manual runs. +Invoke-CargoCheck -ArgumentList @( + "test", "-p", "tauri-plugin-berdctl", "--features", "server" +) -Label "cargo test berdctl plugin" + Invoke-CargoCheck -ArgumentList @( "test", "--lib", "services::managed_", "--", "--skip", "native_gate_installs_and_launches_a_bridge_by_bare_name" diff --git a/src-tauri/plugins/berdctl/src/authorization.rs b/src-tauri/plugins/berdctl/src/authorization.rs index 1a34b3b6d..92797c040 100644 --- a/src-tauri/plugins/berdctl/src/authorization.rs +++ b/src-tauri/plugins/berdctl/src/authorization.rs @@ -43,6 +43,17 @@ impl ProcessAuthorizer { fn install_job(&self, job: Arc) { *self.root.write().unwrap() = Some(PlatformRoot { job }); } + + #[cfg(windows)] + fn revoke_job(&self, job: &Arc) { + let mut root = self.root.write().unwrap(); + if root + .as_ref() + .is_some_and(|root| Arc::ptr_eq(&root.job, job)) + { + *root = None; + } + } } #[cfg(unix)] @@ -212,28 +223,65 @@ pub(crate) fn prepare_goosed_authorization( } impl GoosedAuthorization { - pub fn admit(self, child: &tokio::process::Child) -> io::Result<()> { + pub fn admit(self, child: &tokio::process::Child) -> io::Result { let pid = child .id() .ok_or_else(|| io::Error::other("goosed child has no process id"))?; #[cfg(unix)] { - self.authorizer.install_root(pid) + self.authorizer.install_root(pid)?; + Ok(GoosedAdmission {}) } #[cfg(windows)] { - let result = self - .job - .assign_pid(pid) - .and_then(|()| resume_process_main_thread(pid)); - if result.is_ok() { - self.authorizer.install_job(self.job); - } - result + let process = child + .raw_handle() + .ok_or_else(|| io::Error::other("goosed child has no process handle"))?; + self.job.assign_handle(process.cast(), pid)?; + resume_process_main_thread(process.cast(), pid)?; + self.authorizer.install_job(Arc::clone(&self.job)); + Ok(GoosedAdmission { + authorizer: self.authorizer, + job: self.job, + }) } } } +/// Revocable ownership of the process tree admitted for berdctl bootstrap. +/// +/// On Windows this lease retains the exact Job Object installed as the +/// authorization root. Dropping it revokes admission and closes the Job; call +/// `terminate` when shutdown must synchronously confirm the tree is gone. +pub struct GoosedAdmission { + #[cfg(windows)] + authorizer: ProcessAuthorizer, + #[cfg(windows)] + job: Arc, +} + +impl GoosedAdmission { + #[cfg(windows)] + pub fn terminate( + &self, + child: &tokio::process::Child, + timeout: std::time::Duration, + ) -> io::Result<()> { + let process = child + .raw_handle() + .ok_or_else(|| io::Error::other("goosed child has no process handle"))?; + self.authorizer.revoke_job(&self.job); + self.job.terminate_and_wait(process.cast(), timeout) + } +} + +#[cfg(windows)] +impl Drop for GoosedAdmission { + fn drop(&mut self) { + self.authorizer.revoke_job(&self.job); + } +} + #[cfg(windows)] struct WindowsJob { handle: windows_sys::Win32::Foundation::HANDLE, @@ -268,20 +316,77 @@ impl WindowsJob { Ok(job) } - fn assign_pid(&self, pid: u32) -> io::Result<()> { - use windows_sys::Win32::Foundation::CloseHandle; + fn assign_handle( + &self, + process: windows_sys::Win32::Foundation::HANDLE, + expected_pid: u32, + ) -> io::Result<()> { + use windows_sys::Win32::Foundation::WAIT_TIMEOUT; use windows_sys::Win32::System::JobObjects::AssignProcessToJobObject; - use windows_sys::Win32::System::Threading::{ - OpenProcess, PROCESS_SET_QUOTA, PROCESS_TERMINATE, - }; - let process = unsafe { OpenProcess(PROCESS_SET_QUOTA | PROCESS_TERMINATE, 0, pid) }; - if process.is_null() { + use windows_sys::Win32::System::Threading::{GetProcessId, WaitForSingleObject}; + let actual_pid = unsafe { GetProcessId(process) }; + if actual_pid == 0 { return Err(io::Error::last_os_error()); } + if actual_pid != expected_pid { + return Err(io::Error::other("goosed process handle PID changed")); + } + if unsafe { WaitForSingleObject(process, 0) } != WAIT_TIMEOUT { + return Err(io::Error::other("goosed exited before Job assignment")); + } let ok = unsafe { AssignProcessToJobObject(self.handle, process) }; - let error = (ok == 0).then(io::Error::last_os_error); - unsafe { CloseHandle(process) }; - error.map_or(Ok(()), Err) + if ok == 0 { + Err(io::Error::last_os_error()) + } else { + Ok(()) + } + } + + fn terminate_and_wait( + &self, + process: windows_sys::Win32::Foundation::HANDLE, + timeout: std::time::Duration, + ) -> io::Result<()> { + use windows_sys::Win32::Foundation::{WAIT_OBJECT_0, WAIT_TIMEOUT}; + use windows_sys::Win32::System::JobObjects::{ + JobObjectBasicAccountingInformation, QueryInformationJobObject, TerminateJobObject, + JOBOBJECT_BASIC_ACCOUNTING_INFORMATION, + }; + use windows_sys::Win32::System::Threading::WaitForSingleObject; + + if unsafe { TerminateJobObject(self.handle, 1) } == 0 { + return Err(io::Error::last_os_error()); + } + let deadline = std::time::Instant::now() + timeout; + loop { + let mut info: JOBOBJECT_BASIC_ACCOUNTING_INFORMATION = unsafe { std::mem::zeroed() }; + let queried = unsafe { + QueryInformationJobObject( + self.handle, + JobObjectBasicAccountingInformation, + (&mut info as *mut JOBOBJECT_BASIC_ACCOUNTING_INFORMATION).cast(), + std::mem::size_of_val(&info) as u32, + std::ptr::null_mut(), + ) + }; + if queried == 0 { + return Err(io::Error::last_os_error()); + } + let child_wait = unsafe { WaitForSingleObject(process, 0) }; + if info.ActiveProcesses == 0 && child_wait == WAIT_OBJECT_0 { + return Ok(()); + } + if child_wait != WAIT_OBJECT_0 && child_wait != WAIT_TIMEOUT { + return Err(io::Error::last_os_error()); + } + if std::time::Instant::now() >= deadline { + return Err(io::Error::new( + io::ErrorKind::TimedOut, + "timed out waiting for goosed Job to become empty", + )); + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } } fn contains_pid(&self, pid: u32) -> io::Result { @@ -310,38 +415,98 @@ impl Drop for WindowsJob { } #[cfg(windows)] -fn resume_process_main_thread(pid: u32) -> io::Result<()> { - use windows_sys::Win32::Foundation::{CloseHandle, INVALID_HANDLE_VALUE}; +fn resume_process_main_thread( + process: windows_sys::Win32::Foundation::HANDLE, + expected_pid: u32, +) -> io::Result<()> { + use windows_sys::Win32::Foundation::{CloseHandle, INVALID_HANDLE_VALUE, WAIT_TIMEOUT}; use windows_sys::Win32::System::Diagnostics::ToolHelp::{ CreateToolhelp32Snapshot, Thread32First, Thread32Next, TH32CS_SNAPTHREAD, THREADENTRY32, }; - use windows_sys::Win32::System::Threading::{OpenThread, ResumeThread, THREAD_SUSPEND_RESUME}; + use windows_sys::Win32::System::Threading::{ + GetProcessId, GetProcessIdOfThread, OpenThread, ResumeThread, WaitForSingleObject, + THREAD_QUERY_LIMITED_INFORMATION, THREAD_SUSPEND_RESUME, + }; + if unsafe { GetProcessId(process) } != expected_pid + || unsafe { WaitForSingleObject(process, 0) } != WAIT_TIMEOUT + { + return Err(io::Error::other( + "goosed process identity or liveness changed before resume", + )); + } let snapshot = unsafe { CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0) }; if snapshot == INVALID_HANDLE_VALUE { return Err(io::Error::last_os_error()); } let mut entry: THREADENTRY32 = unsafe { std::mem::zeroed() }; entry.dwSize = std::mem::size_of::() as u32; - let mut found = false; + let mut thread_id = None; let mut current = unsafe { Thread32First(snapshot, &mut entry) }; while current != 0 { - if entry.th32OwnerProcessID == pid { - let thread = unsafe { OpenThread(THREAD_SUSPEND_RESUME, 0, entry.th32ThreadID) }; - if !thread.is_null() { - found = unsafe { ResumeThread(thread) } != u32::MAX; - unsafe { CloseHandle(thread) }; - if found { - break; - } + if entry.th32OwnerProcessID == expected_pid { + if thread_id.replace(entry.th32ThreadID).is_some() { + unsafe { CloseHandle(snapshot) }; + return Err(io::Error::other( + "suspended goosed unexpectedly has multiple threads", + )); } } current = unsafe { Thread32Next(snapshot, &mut entry) }; } unsafe { CloseHandle(snapshot) }; - if found { - Ok(()) - } else { - Err(io::Error::other("could not resume goosed main thread")) + let thread_id = thread_id.ok_or_else(|| io::Error::other("goosed main thread not found"))?; + let thread = unsafe { + OpenThread( + THREAD_SUSPEND_RESUME | THREAD_QUERY_LIMITED_INFORMATION, + 0, + thread_id, + ) + }; + if thread.is_null() { + return Err(io::Error::last_os_error()); + } + let owner_pid = unsafe { GetProcessIdOfThread(thread) }; + if owner_pid != expected_pid { + unsafe { CloseHandle(thread) }; + return Err(io::Error::other("goosed main thread identity changed")); + } + let previous_suspend_count = unsafe { ResumeThread(thread) }; + unsafe { CloseHandle(thread) }; + if previous_suspend_count != 1 { + return Err(io::Error::other(format!( + "goosed main thread had unexpected suspend count {previous_suspend_count}" + ))); + } + if unsafe { GetProcessId(process) } != expected_pid { + return Err(io::Error::other( + "goosed process identity changed after resume", + )); + } + Ok(()) +} + +#[cfg(all(test, windows))] +mod windows_tests { + use super::*; + + #[tokio::test] + async fn suspended_child_is_assigned_resumed_revoked_and_terminated() { + let authorizer = ProcessAuthorizer::default(); + let mut command = tokio::process::Command::new("cmd.exe"); + command.args(["/d", "/c", "ping -t 127.0.0.1 > nul"]); + let authorization = prepare_goosed_authorization(authorizer.clone(), &mut command).unwrap(); + let mut child = command.spawn().unwrap(); + let pid = child.id().unwrap(); + + let admission = authorization.admit(&child).unwrap(); + assert!(authorizer.authorize(pid).unwrap()); + assert!(!authorizer.authorize(std::process::id()).unwrap()); + + admission + .terminate(&child, std::time::Duration::from_secs(5)) + .unwrap(); + child.wait().await.unwrap(); + assert!(!authorizer.authorize(pid).unwrap()); } } diff --git a/src-tauri/plugins/berdctl/src/bootstrap.rs b/src-tauri/plugins/berdctl/src/bootstrap.rs index b5d32ef80..93d327ba1 100644 --- a/src-tauri/plugins/berdctl/src/bootstrap.rs +++ b/src-tauri/plugins/berdctl/src/bootstrap.rs @@ -143,6 +143,123 @@ pub(crate) fn start( }) } +#[cfg(all(test, windows))] +mod windows_tests { + use super::*; + use interprocess::local_socket::{prelude::*, GenericNamespaced, Stream, ToNsName}; + use std::io::{BufRead, BufReader}; + + const HELPER_ENDPOINT_ENV: &str = "BERDCTL_BOOTSTRAP_TEST_ENDPOINT"; + const HELPER_LEAF_ENV: &str = "BERDCTL_BOOTSTRAP_TEST_LEAF"; + const HELPER_READY_ENV: &str = "BERDCTL_BOOTSTRAP_TEST_READY"; + + fn connect(endpoint: &str) -> String { + let name = endpoint + .to_ns_name::() + .expect("valid test pipe name"); + let stream = Stream::connect(name).expect("connect to test bootstrap"); + let mut response = String::new(); + BufReader::new(stream) + .read_line(&mut response) + .expect("read test bootstrap response"); + response + } + + #[test] + fn bootstrap_helper_connects_from_admitted_child() { + let Ok(endpoint) = std::env::var(HELPER_ENDPOINT_ENV) else { + return; + }; + if std::env::var_os(HELPER_LEAF_ENV).is_none() { + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "bootstrap::windows_tests::bootstrap_helper_connects_from_admitted_child", + "--nocapture", + ]) + .env(HELPER_LEAF_ENV, "1") + .spawn() + .unwrap(); + let ready = std::path::PathBuf::from(std::env::var_os(HELPER_READY_ENV).unwrap()); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !ready.exists() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + assert!(ready.exists()); + assert!(child.wait().unwrap().success()); + return; + } + let ready = std::path::PathBuf::from(std::env::var_os(HELPER_READY_ENV).unwrap()); + let response = connect(&endpoint); + let response: serde_json::Value = serde_json::from_str(&response).unwrap(); + assert_eq!(response["capability"], "test-capability"); + std::fs::write(&ready, b"ready").unwrap(); + std::thread::sleep(std::time::Duration::from_secs(30)); + } + + #[tokio::test] + async fn exact_job_child_receives_capability_and_unrelated_process_does_not() { + let endpoint = format!( + "berdctl-bootstrap-test-{}-{}", + std::process::id(), + uuid::Uuid::new_v4().simple() + ); + let endpoint_path = Path::new(&endpoint); + let authorizer = ProcessAuthorizer::default(); + let handle = start( + endpoint_path, + 43123, + 7, + "test-capability".into(), + authorizer.clone(), + ) + .unwrap(); + + let unrelated_response = tokio::task::spawn_blocking({ + let endpoint = endpoint.clone(); + move || connect(&endpoint) + }) + .await + .unwrap(); + assert!(unrelated_response.is_empty()); + + let ready_path = std::env::temp_dir().join(format!( + "berdctl-bootstrap-ready-{}-{}", + std::process::id(), + uuid::Uuid::new_v4().simple() + )); + let mut command = tokio::process::Command::new(std::env::current_exe().unwrap()); + command + .args([ + "--exact", + "bootstrap::windows_tests::bootstrap_helper_connects_from_admitted_child", + "--nocapture", + ]) + .env(HELPER_ENDPOINT_ENV, &endpoint) + .env(HELPER_READY_ENV, &ready_path) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + let authorization = + crate::authorization::prepare_goosed_authorization(authorizer.clone(), &mut command) + .unwrap(); + let mut child = command.spawn().unwrap(); + let admission = authorization.admit(&child).unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !ready_path.exists() && std::time::Instant::now() < deadline { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert!(ready_path.exists()); + let child_pid = child.id().unwrap(); + admission + .terminate(&child, std::time::Duration::from_secs(5)) + .unwrap(); + child.wait().await.unwrap(); + assert!(!authorizer.authorize(child_pid).unwrap()); + assert!(!ready_path.exists() || std::fs::remove_file(&ready_path).is_ok()); + handle.shutdown(); + } +} + #[cfg(all(test, unix))] mod tests { use super::*; diff --git a/src-tauri/plugins/berdctl/src/lib.rs b/src-tauri/plugins/berdctl/src/lib.rs index 29d8681bc..2dc17b694 100644 --- a/src-tauri/plugins/berdctl/src/lib.rs +++ b/src-tauri/plugins/berdctl/src/lib.rs @@ -24,7 +24,7 @@ mod discovery; mod server; #[cfg(feature = "server")] -pub use authorization::{prepare_goosed, GoosedAuthorization}; +pub use authorization::{prepare_goosed, GoosedAdmission, GoosedAuthorization}; pub use discovery::{discovery_file_path, owner_pid_from_discovery_file_name, DISCOVERY_DIR_NAME}; #[cfg(feature = "server")] diff --git a/src-tauri/src/services/acp/goose_serve.rs b/src-tauri/src/services/acp/goose_serve.rs index 4460d7305..429f8e633 100644 --- a/src-tauri/src/services/acp/goose_serve.rs +++ b/src-tauri/src/services/acp/goose_serve.rs @@ -58,6 +58,8 @@ pub struct GooseServeProcess { secret_key: String, process_record_dir: PathBuf, _child: Child, + #[cfg(all(windows, feature = "berdctl"))] + berdctl_admission: tauri_plugin_berdctl::GoosedAdmission, } /// Global singleton — initialised once at app startup. @@ -106,25 +108,47 @@ impl GooseServeProcess { } #[cfg(windows)] - let remove_process_record = if let Some(handle) = self._child.raw_handle() { - log::info!("Killing goose serve child through its retained process handle"); - // SAFETY: Tokio owns this process handle for the lifetime of `_child`. - match unsafe { - crate::services::process::terminate_process_handle(handle, Duration::from_secs(5)) - } { - Ok(()) => true, - Err(error) => { - log::warn!( - "Failed to stop goose serve child: {error}; keeping process record for recovery" - ); - false + let remove_process_record = { + #[cfg(feature = "berdctl")] + { + log::info!("Killing goose serve process tree through its retained Job"); + match self + .berdctl_admission + .terminate(&self._child, Duration::from_secs(5)) + { + Ok(()) => true, + Err(error) => { + log::warn!( + "Failed to stop goose serve process tree: {error}; keeping process record for recovery" + ); + false + } } } - } else { - log::warn!( - "Cannot stop goose serve child through its retained handle; keeping process record for recovery" - ); - false + #[cfg(not(feature = "berdctl"))] + if let Some(handle) = self._child.raw_handle() { + log::info!("Killing goose serve child through its retained process handle"); + // SAFETY: Tokio owns this process handle for the lifetime of `_child`. + match unsafe { + crate::services::process::terminate_process_handle( + handle, + Duration::from_secs(5), + ) + } { + Ok(()) => true, + Err(error) => { + log::warn!( + "Failed to stop goose serve child: {error}; keeping process record for recovery" + ); + false + } + } + } else { + log::warn!( + "Cannot stop goose serve child through its retained handle; keeping process record for recovery" + ); + false + } }; #[cfg(unix)] @@ -286,10 +310,16 @@ impl GooseServeProcess { })?; let pid = child.id(); #[cfg(feature = "berdctl")] - if let Err(error) = berdctl_authorization.admit(&child) { - let _ = child.kill().await; - return Err(format!("Failed to authorize goosed for berdctl: {error}")); - } + let berdctl_admission = match berdctl_authorization.admit(&child) { + Ok(admission) => admission, + Err(error) => { + let _ = child.kill().await; + let _ = child.wait().await; + return Err(format!("Failed to authorize goosed for berdctl: {error}")); + } + }; + #[cfg(all(feature = "berdctl", not(windows)))] + let _ = &berdctl_admission; diagnostic_log::record_event( DiagnosticLevel::Info, DiagnosticCategory::GooseServe, @@ -303,16 +333,40 @@ impl GooseServeProcess { log::warn!( "Failed to publish goose serve recovery record: {error}; stopping child and failing startup" ); - if let Some(handle) = child.raw_handle() { + #[cfg(all(feature = "berdctl", windows))] + let stopped = match berdctl_admission.terminate(&child, Duration::from_secs(5)) { + Ok(()) => match child.wait().await { + Ok(_) => true, + Err(stop_error) => { + log::warn!("Failed to reap recordless goose serve child: {stop_error}"); + false + } + }, + Err(stop_error) => { + log::warn!("Failed to stop recordless goose serve Job: {stop_error}"); + false + } + }; + #[cfg(all(not(feature = "berdctl"), windows))] + let stopped = if let Some(handle) = child.raw_handle() { // SAFETY: Tokio owns this process handle for the lifetime of `child`. - if let Err(stop_error) = unsafe { + match unsafe { crate::services::process::terminate_process_handle( handle, Duration::from_secs(5), ) } { - log::warn!("Failed to stop recordless goose serve child: {stop_error}"); + Ok(()) => true, + Err(stop_error) => { + log::warn!("Failed to stop recordless goose serve child: {stop_error}"); + false + } } + } else { + false + }; + if stopped { + let _ = std::fs::remove_file(process_record_path(&process_record_dir)); } return Err(format!( "Failed to publish goose serve recovery record: {error}" @@ -348,6 +402,25 @@ impl GooseServeProcess { ("port", port.into()), ]), ); + #[cfg(all(feature = "berdctl", windows))] + let rollback_result = + match berdctl_admission.terminate(&child, Duration::from_secs(5)) { + Ok(()) => child + .wait() + .await + .map(|_| ()) + .map_err(std::io::Error::other), + Err(error) => Err(error), + }; + #[cfg(all(feature = "berdctl", windows))] + if let Err(stop_error) = rollback_result { + log::warn!( + "Failed to roll back unready goose serve Job: {stop_error}; keeping process record for recovery" + ); + } else { + #[cfg(all(feature = "berdctl", windows))] + let _ = std::fs::remove_file(process_record_path(&process_record_dir)); + } return Err(error); } } @@ -364,6 +437,8 @@ impl GooseServeProcess { secret_key, process_record_dir, _child: child, + #[cfg(all(windows, feature = "berdctl"))] + berdctl_admission, }) } } From 1e64bf11b55077029f9c5e6f5982aa8d84dd64ed Mon Sep 17 00:00:00 2001 From: cid Date: Sun, 16 Aug 2026 13:35:25 -0700 Subject: [PATCH 5/7] test(berdctl): support crlf source checkout Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/plugins/berdctl/src/server.rs | 42 ++++++++++++++++++++----- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src-tauri/plugins/berdctl/src/server.rs b/src-tauri/plugins/berdctl/src/server.rs index 0dedfaee3..d0ba9dda4 100644 --- a/src-tauri/plugins/berdctl/src/server.rs +++ b/src-tauri/plugins/berdctl/src/server.rs @@ -941,16 +941,27 @@ mod tests { /// The non-test portion of a plugin source file: everything before its /// `mod tests` module, which must be unique and must run to end-of-file - /// so no scannable code can hide after it. The brace walk is naive about - /// braces inside test string literals, but that confusion fails CLOSED - /// (the gate then scans test code too and trips loudly). + /// so no scannable code can hide after it. Both LF and CRLF are accepted + /// because `include_str!` preserves the checkout's line endings. The brace + /// walk is naive about braces inside test string literals, but that confusion + /// fails CLOSED (the gate then scans test code too and trips loudly). fn non_test_source<'a>(name: &str, source: &'a str) -> &'a str { - const MARKER: &str = "#[cfg(test)]\nmod tests {"; - match source.matches(MARKER).count() { + const MARKERS: &[&str] = &["#[cfg(test)]\nmod tests {", "#[cfg(test)]\r\nmod tests {"]; + let mut matches = Vec::new(); + for marker in MARKERS { + matches.extend( + source + .match_indices(marker) + .map(|(start, _)| (start, *marker)), + ); + } + + match matches.len() { 0 => source, 1 => { - let head = source.split(MARKER).next().unwrap(); - let tail = &source[head.len() + MARKER.len()..]; + let (start, marker) = matches[0]; + let head = &source[..start]; + let tail = &source[start + marker.len()..]; let mut depth: i64 = 1; let mut after = ""; for (i, c) in tail.char_indices() { @@ -977,6 +988,23 @@ mod tests { } } + #[test] + fn non_test_source_accepts_crlf_checkouts() { + let source = [ + "const LIVE: &str = \"transport\";", + "#[cfg(test)]", + "mod tests {", + " const TEST_ONLY: &str = \"create\";", + "}", + "", + ] + .join("\r\n"); + let non_test = non_test_source("fixture.rs", &source); + + assert_eq!(non_test, "const LIVE: &str = \"transport\";\r\n"); + assert!(!non_test.contains("\"create\"")); + } + /// Invariant #1 of the berdctl architecture /// (docs/berdctl-architecture.md): no command-specific knowledge below /// the renderer registry. Fails when the non-test source of any plugin From 53ba7977a5ed0f80c0729baee3b2e4c6c86f8a5b Mon Sep 17 00:00:00 2001 From: cid Date: Sun, 16 Aug 2026 13:45:07 -0700 Subject: [PATCH 6/7] fix(berdctl): satisfy windows clippy gate Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/plugins/berdctl/src/authorization.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src-tauri/plugins/berdctl/src/authorization.rs b/src-tauri/plugins/berdctl/src/authorization.rs index 92797c040..a3f987374 100644 --- a/src-tauri/plugins/berdctl/src/authorization.rs +++ b/src-tauri/plugins/berdctl/src/authorization.rs @@ -443,13 +443,13 @@ fn resume_process_main_thread( let mut thread_id = None; let mut current = unsafe { Thread32First(snapshot, &mut entry) }; while current != 0 { - if entry.th32OwnerProcessID == expected_pid { - if thread_id.replace(entry.th32ThreadID).is_some() { - unsafe { CloseHandle(snapshot) }; - return Err(io::Error::other( - "suspended goosed unexpectedly has multiple threads", - )); - } + if entry.th32OwnerProcessID == expected_pid + && thread_id.replace(entry.th32ThreadID).is_some() + { + unsafe { CloseHandle(snapshot) }; + return Err(io::Error::other( + "suspended goosed unexpectedly has multiple threads", + )); } current = unsafe { Thread32Next(snapshot, &mut entry) }; } From 97190f9bf1c2bdd4d97ff32b1c92a058f3b94338 Mon Sep 17 00:00:00 2001 From: cid Date: Sun, 16 Aug 2026 13:53:20 -0700 Subject: [PATCH 7/7] fix(windows): scope legacy process termination helper Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/src/services/process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src-tauri/src/services/process.rs b/src-tauri/src/services/process.rs index 2b9f14bf8..e06670cc5 100644 --- a/src-tauri/src/services/process.rs +++ b/src-tauri/src/services/process.rs @@ -155,7 +155,7 @@ pub(crate) unsafe fn process_identity_from_handle( /// # Safety /// `handle` must remain a valid process handle with terminate and synchronize access. -#[cfg(windows)] +#[cfg(all(windows, not(feature = "berdctl")))] pub(crate) unsafe fn terminate_process_handle( handle: *mut std::ffi::c_void, wait: std::time::Duration,