diff --git a/Cargo.lock b/Cargo.lock --- a/Cargo.lock +++ b/Cargo.lock @@ -1611,6 +1611,7 @@ checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" dependencies = [ "block-buffer", "crypto-common", + "subtle", ] [[package]] @@ -2817,6 +2818,15 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" +[[package]] +name = "hmac" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" +dependencies = [ + "digest", +] + [[package]] name = "http" version = "1.5.0" @@ -4911,6 +4921,7 @@ dependencies = [ "cap-std", "clap", "getrandom 0.3.4", + "hmac", "photocraft-codecs", "photocraft-compose", "photocraft-doc", @@ -4923,6 +4934,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "sha2", "thiserror 2.0.21", "tokio", ] diff --git a/apps/photocraft/src/control_server.rs b/apps/photocraft/src/control_server.rs --- a/apps/photocraft/src/control_server.rs +++ b/apps/photocraft/src/control_server.rs @@ -9,7 +9,7 @@ use std::time::{Duration, Instant}; use photocraft_automation::budgets::write_reply; use photocraft_automation::security::{ - AUTH_DEADLINE, AuthDeadlineStream, ConnectionLimiter, LineRead, MAX_CONNECTIONS, MAX_REQUEST_BYTES, authentication_reply, configure_stream, + AUTH_DEADLINE, AuthDeadlineStream, AuthStep, ConnectionLimiter, LineRead, MAX_CONNECTIONS, MAX_REQUEST_BYTES, ServerHandshake, configure_stream, read_bounded_line, repeat_auth_reply, }; use photocraft_ui_egui::ControlRequest; @@ -71,6 +71,7 @@ fn serve_with_auth_deadline(stream: TcpStream, token: &str, tx: Sender {} } if !authenticated { - let (reply, ok) = authentication_reply(&line, token); - authenticated = ok; - if ok && reader.get_mut().authenticated().is_err() { + let (reply, step) = handshake.reply(&line, token); + authenticated = step == AuthStep::Authenticated; + if authenticated && reader.get_mut().authenticated().is_err() { break; } - if write_reply(&mut out, &reply).is_err() || out.flush().is_err() || !authenticated { + if write_reply(&mut out, &reply).is_err() || out.flush().is_err() || step == AuthStep::Rejected { break; } continue; @@ -307,4 +308,49 @@ mod tests { handler.join().unwrap(); server.join().unwrap(); } + + /// NV-6: the desktop server answers challenge authentication, proving it holds the token + /// before the client sends its proof, and dispatches only after that proof. + #[test] + fn challenge_authentication_proves_the_server_then_serves() { + use photocraft_automation::security::{CHALLENGE_METHOD, ProofRole, generate_nonce, handshake_proof, proof_matches}; + const TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let (tx, rx) = channel::(); + let server = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + serve(stream, TOKEN, tx, egui::Context::default()); + }); + let handler = std::thread::spawn(move || { + let req = rx.recv_timeout(Duration::from_secs(5)).unwrap(); + assert_eq!(req.method, "test.after", "only the request after the proof is dispatched"); + req.reply.send(json!({"ok": true, "result": "served"})).unwrap(); + }); + let mut stream = TcpStream::connect(addr).unwrap(); + stream.set_read_timeout(Some(Duration::from_secs(5))).unwrap(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut line = String::new(); + let client_nonce = generate_nonce().unwrap(); + writeln!(stream, "{}", json!({"id": 1, "method": CHALLENGE_METHOD, "params": {"clientNonce": client_nonce}})).unwrap(); + reader.read_line(&mut line).unwrap(); + let challenge: Value = serde_json::from_str(&line).unwrap(); + assert!(!line.contains(TOKEN), "{line}"); + let server_nonce = challenge["result"]["serverNonce"].as_str().unwrap().to_owned(); + let server_proof = challenge["result"]["serverProof"].as_str().unwrap(); + assert!(proof_matches(TOKEN, ProofRole::Server, &client_nonce, &server_nonce, server_proof), "{line}"); + let proof = handshake_proof(TOKEN, ProofRole::Client, &client_nonce, &server_nonce).unwrap(); + writeln!(stream, "{}", json!({"id": 2, "method": "auth", "params": {"proof": proof}})).unwrap(); + line.clear(); + reader.read_line(&mut line).unwrap(); + assert_eq!(serde_json::from_str::(&line).unwrap()["result"]["authenticated"], true, "{line}"); + writeln!(stream, "{}", json!({"id": 3, "method": "test.after"})).unwrap(); + line.clear(); + reader.read_line(&mut line).unwrap(); + assert_eq!(serde_json::from_str::(&line).unwrap()["result"], "served"); + drop(reader); + drop(stream); + handler.join().unwrap(); + server.join().unwrap(); + } } diff --git a/book/src/automation/control-protocol.md b/book/src/automation/control-protocol.md --- a/book/src/automation/control-protocol.md +++ b/book/src/automation/control-protocol.md @@ -1,6 +1,6 @@ # Control protocol -The desktop control protocol is newline-delimited JSON over a TCP listener bound to `127.0.0.1`. The first frame on every connection must be `auth` with the configured 256-bit bearer token. After authentication, each request carries `id`, `method`, and optional `params`; each reply includes the matching `id` and either a result or error. +The desktop control protocol is newline-delimited JSON over a TCP listener bound to `127.0.0.1`. Every connection must first authenticate with the configured 256-bit token: either by challenge (`auth.challenge`, then `auth` with an HMAC proof; the token never crosses the wire and the server proves it holds the token first), or with the original single `auth` frame carrying the token. After authentication, each request carries `id`, `method`, and optional `params`; each reply includes the matching `id` and either a result or error. ```json {"id":"auth","method":"auth","params":{"token":"<64-hex>"}} diff --git a/crates/automation/Cargo.toml b/crates/automation/Cargo.toml --- a/crates/automation/Cargo.toml +++ b/crates/automation/Cargo.toml @@ -24,6 +24,9 @@ tokio = { version = "1", features = ["rt-multi-thread", "macros", "io-std", "syn base64 = "0.23" schemars = "1" getrandom = { workspace = true } +# HMAC-SHA256 for the control channel's challenge authentication: the token never crosses the wire (NV-6). +hmac = "0.12" +sha2 = "0.10" cap-std = "4" clap = { workspace = true } diff --git a/crates/automation/src/bridge.rs b/crates/automation/src/bridge.rs --- a/crates/automation/src/bridge.rs +++ b/crates/automation/src/bridge.rs @@ -4,7 +4,13 @@ //! a call, once the connection has been idle long enough that the app //! (which closes idle control connections after //! [`IO_TIMEOUT`](crate::security::IO_TIMEOUT)) may already have closed it. +//! +//! Each connection authenticates with the challenge handshake (NV-6): the client connects to +//! the literal loopback address it was given (never a name lookup that may pick another +//! listener) and the server must prove it holds the control token before the client sends its +//! own proof. The token itself is never sent, so a process that took the port gets nothing. +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant}; @@ -15,7 +21,9 @@ use tokio::sync::Mutex; use crate::AutomationError; use crate::budgets::MAX_RESPONSE_BYTES; -use crate::security::{AUTH_METHOD, IO_TIMEOUT, MAX_REQUEST_BYTES, validate_token}; +use crate::security::{ + AUTH_METHOD, CHALLENGE_METHOD, IO_TIMEOUT, MAX_REQUEST_BYTES, ProofRole, generate_nonce, handshake_proof, is_hex256, proof_matches, validate_token, +}; type Conn = (BufReader, tokio::net::tcp::OwnedWriteHalf); @@ -35,6 +43,8 @@ const NO_REPLY_CAUSES: &str = pub struct BridgeClient { addr: String, + /// Where to connect: `addr` parsed, with `localhost` taken as 127.0.0.1. + socket: SocketAddr, token: String, conn: Mutex>, next_id: AtomicU64, @@ -48,13 +58,11 @@ impl BridgeClient { pub fn new(addr: impl Into, token: impl Into) -> Result { let addr = addr.into(); let token = token.into(); - let host = addr.rsplit_once(':').map(|(h, _)| h).unwrap_or(&addr); - if !matches!(host, "127.0.0.1" | "localhost" | "[::1]" | "::1") { - return Err(AutomationError::BadRequest(format!("bridge address must be loopback, got `{addr}`"))); - } + let socket = loopback_socket(&addr)?; validate_token(&token)?; Ok(BridgeClient { addr, + socket, token: token.to_ascii_lowercase(), conn: Mutex::new(None), next_id: AtomicU64::new(1), @@ -126,7 +134,7 @@ impl BridgeClient { *guard = None; } if guard.is_none() { - let s = tokio::time::timeout(Duration::from_secs(5), TcpStream::connect(&self.addr)) + let s = tokio::time::timeout(Duration::from_secs(5), TcpStream::connect(self.socket)) .await .map_err(|_| Failure::NotSent(format!("timed out connecting to {}", self.addr)))? .map_err(|e| { @@ -137,8 +145,7 @@ impl BridgeClient { })?; let (r, w) = s.into_split(); let mut conn = (BufReader::new(r), w); - let auth_id = self.next_id.fetch_add(1, Ordering::Relaxed); - let auth = tokio::time::timeout(Duration::from_secs(5), exchange(&mut conn, auth_id, AUTH_METHOD, &json!({"token": self.token}))) + let auth = tokio::time::timeout(Duration::from_secs(5), self.authenticate(&mut conn)) .await .map_err(|_| Failure::NotSent("control authentication timed out".into()))? .map_err(|e| Failure::NotSent(e.to_string()))?; @@ -187,6 +194,51 @@ impl BridgeClient { } } +impl BridgeClient { + /// Challenge authentication on a fresh connection. The server must prove it holds the token + /// before the client sends its own proof; the token itself is never sent (NV-6). + /// Outer `Err` = transport failure; inner = refused. + async fn authenticate(&self, conn: &mut Conn) -> Result, AutomationError> { + let client_nonce = generate_nonce()?; + let id = self.next_id.fetch_add(1, Ordering::Relaxed); + let challenge = match exchange(conn, id, CHALLENGE_METHOD, &json!({"clientNonce": client_nonce})).await? { + Ok(challenge) => challenge, + Err(AutomationError::App(e)) => { + return Ok(Err(AutomationError::Bridge(format!( + "{} refused challenge authentication ({e}): it is not PhotoCraft, or a PhotoCraft too old to support it (update the app); the control token was not sent", + self.addr + )))); + } + Err(e) => return Ok(Err(e)), + }; + let field = |key: &str| challenge.get(key).and_then(Value::as_str).filter(|v| is_hex256(v)); + let server_nonce = match (field("serverNonce"), field("serverProof")) { + (Some(nonce), Some(proof)) if proof_matches(&self.token, ProofRole::Server, &client_nonce, nonce, proof) => nonce, + _ => { + return Ok(Err(AutomationError::Bridge(format!( + "{} did not prove it holds this control token: the token does not match the app's, or the listener is not that PhotoCraft app; the control token was not sent", + self.addr + )))); + } + }; + let proof = handshake_proof(&self.token, ProofRole::Client, &client_nonce, server_nonce)?; + let id = self.next_id.fetch_add(1, Ordering::Relaxed); + Ok(exchange(conn, id, AUTH_METHOD, &json!({"proof": proof})).await?.map(|_| ())) + } +} + +/// The literal loopback socket address for `addr`: `127.0.0.1:` or `[::1]:`, with +/// `localhost:` meaning 127.0.0.1, the address the app binds. The name is never looked up, +/// since a lookup may return `[::1]` first and reach another process listening there (NV-6). +fn loopback_socket(addr: &str) -> Result { + let refuse = || AutomationError::BadRequest(format!("bridge address must be loopback `127.0.0.1:` or `[::1]:`, got `{addr}`")); + let socket = match addr.strip_prefix("localhost:") { + Some(port) => SocketAddr::from((Ipv4Addr::LOCALHOST, port.parse::().map_err(|_| refuse())?)), + None => addr.parse::().map_err(|_| refuse())?, + }; + if socket.ip() == IpAddr::V4(Ipv4Addr::LOCALHOST) || socket.ip() == IpAddr::V6(Ipv6Addr::LOCALHOST) { Ok(socket) } else { Err(refuse()) } +} + /// How a single attempt failed, deciding the retry and the wording above. enum Failure { /// The connection could not be established or authenticated: nothing was sent. @@ -245,6 +297,17 @@ mod tests { assert!(DEFAULT_CALL_TIMEOUT < Duration::from_secs(60)); } + /// NV-6: `localhost` is the IPv4 loopback the app binds, never a name lookup. + #[test] + fn bridge_addresses_are_literal_loopback_sockets() { + assert_eq!(loopback_socket("localhost:7878").unwrap(), SocketAddr::from((Ipv4Addr::LOCALHOST, 7878))); + assert_eq!(loopback_socket("127.0.0.1:7878").unwrap(), SocketAddr::from((Ipv4Addr::LOCALHOST, 7878))); + assert_eq!(loopback_socket("[::1]:7878").unwrap(), SocketAddr::from((Ipv6Addr::LOCALHOST, 7878))); + for bad in ["10.0.0.5:7878", "127.0.0.2:7878", "localhost", "localhost:", "localhost:99999", "::1:7878", "example.com:7878", "127.0.0.1"] { + assert!(loopback_socket(bad).is_err(), "{bad}"); + } + } + #[test] fn idle_reconnect_stays_below_the_app_idle_cutoff() { // Reconnecting later than IO_TIMEOUT would send into a socket the app dropped. diff --git a/crates/automation/src/rpc.rs b/crates/automation/src/rpc.rs --- a/crates/automation/src/rpc.rs +++ b/crates/automation/src/rpc.rs @@ -33,7 +33,7 @@ use serde_json::{Value, json}; use crate::budgets::{BatchReplyBudget, write_reply}; use crate::security::{ - AUTH_DEADLINE, AuthDeadlineStream, ConnectionLimiter, LineRead, MAX_BATCH_STEPS, MAX_CONNECTIONS, MAX_REQUEST_BYTES, authentication_reply, + AUTH_DEADLINE, AuthDeadlineStream, AuthStep, ConnectionLimiter, LineRead, MAX_BATCH_STEPS, MAX_CONNECTIONS, MAX_REQUEST_BYTES, ServerHandshake, configure_stream, discard_rest_of_line, read_bounded_line, repeat_auth_reply, }; use crate::{AutomationError, Headless}; @@ -292,6 +292,7 @@ fn serve_tcp_connection(h: &Mutex, stream: std::net::TcpStream, token: let mut reader = std::io::BufReader::new(AuthDeadlineStream::new(read, auth_deadline)); let mut out = stream; let mut line = String::new(); + let mut handshake = ServerHandshake::new(); let mut authenticated = false; loop { match read_bounded_line(&mut reader, &mut line)? { @@ -309,19 +310,19 @@ fn serve_tcp_connection(h: &Mutex, stream: std::net::TcpStream, token: LineRead::Line if line.trim().is_empty() => continue, LineRead::Line => {} } - let reply = if authenticated { - repeat_auth_reply(&line).unwrap_or_else(|| respond(h, &line)) + let (reply, step) = if authenticated { + (repeat_auth_reply(&line).unwrap_or_else(|| respond(h, &line)), AuthStep::Authenticated) } else { - let (reply, ok) = authentication_reply(&line, token); - authenticated = ok; - if ok { + let (reply, step) = handshake.reply(&line, token); + if step == AuthStep::Authenticated { + authenticated = true; reader.get_mut().authenticated()?; } - reply + (reply, step) }; write_reply(&mut out, &reply)?; out.flush()?; - if !authenticated { + if step == AuthStep::Rejected { return Ok(()); } } diff --git a/crates/automation/src/security.rs b/crates/automation/src/security.rs --- a/crates/automation/src/security.rs +++ b/crates/automation/src/security.rs @@ -7,12 +7,17 @@ use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; use std::time::{Duration, Instant}; +use hmac::{Hmac, Mac}; use serde_json::{Value, json}; +use sha2::Sha256; use crate::AutomationError; -/// The first request on every TCP connection must use this method. +/// The request that authenticates a TCP connection: with the token itself (the original form), +/// or with the client's proof after [`CHALLENGE_METHOD`]. pub const AUTH_METHOD: &str = "auth"; +/// The first frame of challenge authentication: it carries a client nonce, never the token (NV-6). +pub const CHALLENGE_METHOD: &str = "auth.challenge"; /// Maximum encoded JSON request line, including its newline. pub const MAX_REQUEST_BYTES: usize = 1 << 20; /// Maximum simultaneously serviced TCP connections per listener. @@ -75,22 +80,42 @@ pub fn discard_rest_of_line(reader: &mut impl BufRead) -> std::io::Result<()> { } } -/// Generate a 256-bit bearer token with the operating system CSPRNG. -pub fn generate_token() -> Result { - let mut bytes = [0u8; TOKEN_BYTES]; - getrandom::fill(&mut bytes).map_err(|e| AutomationError::Other(format!("cannot generate control token: {e}")))?; - let mut token = String::with_capacity(TOKEN_HEX_LEN); +/// Lowercase hexadecimal, two characters per byte. +fn hex(bytes: &[u8]) -> String { const HEX: &[u8; 16] = b"0123456789abcdef"; - for byte in bytes { - token.push(HEX[(byte >> 4) as usize] as char); - token.push(HEX[(byte & 0x0f) as usize] as char); + let mut out = String::with_capacity(bytes.len() * 2); + for &byte in bytes { + out.push(HEX[(byte >> 4) as usize] as char); + out.push(HEX[(byte & 0x0f) as usize] as char); } - Ok(token) + out +} + +/// 256 bits from the operating system CSPRNG, as 64 lowercase hexadecimal characters. +fn random_hex(what: &str) -> Result { + let mut bytes = [0u8; TOKEN_BYTES]; + getrandom::fill(&mut bytes).map_err(|e| AutomationError::Other(format!("cannot generate {what}: {e}")))?; + Ok(hex(&bytes)) +} + +/// Generate a 256-bit bearer token with the operating system CSPRNG. +pub fn generate_token() -> Result { + random_hex("control token") +} + +/// Generate a fresh 256-bit nonce for challenge authentication ([`CHALLENGE_METHOD`]). +pub fn generate_nonce() -> Result { + random_hex("authentication nonce") +} + +/// Whether `s` has the shape of a token or nonce: exactly 64 hexadecimal characters. +pub fn is_hex256(s: &str) -> bool { + s.len() == TOKEN_HEX_LEN && s.bytes().all(|b| b.is_ascii_hexdigit()) } /// Accept only the fixed-width hexadecimal representation emitted by [`generate_token`]. pub fn validate_token(token: &str) -> Result<(), AutomationError> { - if token.len() != TOKEN_HEX_LEN || !token.bytes().all(|b| b.is_ascii_hexdigit()) { + if !is_hex256(token) { return Err(AutomationError::BadRequest("control token must contain exactly 64 hexadecimal characters".into())); } Ok(()) @@ -108,30 +133,106 @@ pub fn token_matches(expected: &str, supplied: &str) -> bool { expected.bytes().zip(supplied.bytes()).fold(0u8, |different, (a, b)| different | (a.to_ascii_lowercase() ^ b.to_ascii_lowercase())) == 0 } -/// Validate the first TCP frame without exposing any control method before authentication. -pub fn authentication_reply(line: &str, expected_token: &str) -> (Value, bool) { - let req: Value = match serde_json::from_str(line) { - Ok(v) => v, - Err(_) => { - return (json!({"id": null, "ok": false, "error": "authentication required"}), false); - } +/// Which side of the challenge handshake a proof comes from. Each side's proof is keyed by the +/// token over both nonces under its own label, so one side's proof never passes for the other's. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ProofRole { + Server, + Client, +} + +/// HMAC-SHA256, keyed by the (lowercased) control token, over the role's label and both nonces, +/// as 64 lowercase hexadecimal characters. `docs/control-protocol.md` gives the exact message. +pub fn handshake_proof(token: &str, role: ProofRole, client_nonce: &str, server_nonce: &str) -> Result { + let label: &[u8] = match role { + ProofRole::Server => b"photocraft-control-v1 server", + ProofRole::Client => b"photocraft-control-v1 client", }; - let id = req.get("id").cloned().unwrap_or(Value::Null); - let supplied = req.get("params").and_then(|p| p.get("token")).and_then(Value::as_str).unwrap_or(""); - let ok = req.get("method").and_then(Value::as_str) == Some(AUTH_METHOD) && token_matches(expected_token, supplied); - if ok { - (json!({"id": id, "ok": true, "result": {"authenticated": true}}), true) - } else { - (json!({"id": id, "ok": false, "error": "authentication required"}), false) + let mut mac = Hmac::::new_from_slice(token.to_ascii_lowercase().as_bytes()) + .map_err(|e| AutomationError::Other(format!("cannot key the authentication proof: {e}")))?; + for part in [label, b"\n", client_nonce.to_ascii_lowercase().as_bytes(), b"\n", server_nonce.to_ascii_lowercase().as_bytes()] { + mac.update(part); } + Ok(hex(&mac.finalize().into_bytes())) } -/// Answer a repeat `auth` frame on an already authenticated connection. Returns `None` for any -/// other frame, which the caller dispatches as usual. +/// Whether `supplied` is `role`'s proof for these nonces, compared without an early exit. +pub fn proof_matches(token: &str, role: ProofRole, client_nonce: &str, server_nonce: &str, supplied: &str) -> bool { + handshake_proof(token, role, client_nonce, server_nonce).is_ok_and(|expected| token_matches(&expected, supplied)) +} + +/// Where a connection's authentication stands after one frame. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AuthStep { + /// Authenticated: dispatch the frames that follow. + Authenticated, + /// A challenge was answered: the next frame must be `auth` with the client's proof. + Challenged, + /// Refused: send the reply, then close the connection. + Rejected, +} + +/// Server side of connection authentication, one per connection. It accepts either +/// +/// - challenge authentication (NV-6): `auth.challenge {clientNonce}` is answered with +/// `{serverNonce, serverProof}`, then `auth {proof}` authenticates. The token never crosses the +/// wire, and the client learns that the server holds the token before revealing anything +/// derived from it; or +/// - the original one-frame `auth {token}`, kept so existing clients and scripts keep working. +/// +/// No control method is answered before authentication. +#[derive(Debug, Default)] +pub struct ServerHandshake { + /// The client and server nonces of the challenge this connection was given, until its `auth`. + challenge: Option<(String, String)>, +} + +impl ServerHandshake { + pub fn new() -> Self { + Self::default() + } + + /// Answer one frame received before authentication. Never dispatches a control method. + pub fn reply(&mut self, line: &str, token: &str) -> (Value, AuthStep) { + let refused = |id: Value| (json!({"id": id, "ok": false, "error": "authentication required"}), AuthStep::Rejected); + let authenticated = |id: Value| (json!({"id": id, "ok": true, "result": {"authenticated": true}}), AuthStep::Authenticated); + let Ok(req) = serde_json::from_str::(line) else { + return refused(Value::Null); + }; + let id = req.get("id").cloned().unwrap_or(Value::Null); + let param = |key: &str| req.get("params").and_then(|p| p.get(key)).and_then(Value::as_str); + match (req.get("method").and_then(Value::as_str), self.challenge.take()) { + (Some(CHALLENGE_METHOD), None) => { + let Some(client_nonce) = param("clientNonce").filter(|n| is_hex256(n)).map(str::to_ascii_lowercase) else { + return refused(id); + }; + let Ok(server_nonce) = generate_nonce() else { + return (json!({"id": id, "ok": false, "error": "cannot generate an authentication challenge"}), AuthStep::Rejected); + }; + let Ok(server_proof) = handshake_proof(token, ProofRole::Server, &client_nonce, &server_nonce) else { + return refused(id); + }; + let reply = json!({"id": id, "ok": true, "result": {"serverNonce": server_nonce, "serverProof": server_proof}}); + self.challenge = Some((client_nonce, server_nonce)); + (reply, AuthStep::Challenged) + } + // After a challenge only the proof authenticates; the token is never needed. + (Some(AUTH_METHOD), Some((client_nonce, server_nonce))) => match param("proof") { + Some(proof) if proof_matches(token, ProofRole::Client, &client_nonce, &server_nonce, proof) => authenticated(id), + _ => refused(id), + }, + (Some(AUTH_METHOD), None) if token_matches(token, param("token").unwrap_or("")) => authenticated(id), + _ => refused(id), + } + } +} + +/// Answer a repeat `auth` or `auth.challenge` frame on an already authenticated connection. +/// Returns `None` for any other frame, which the caller dispatches as usual. /// Without this, a second `auth` fell through to the method dispatcher as an unknown method. pub fn repeat_auth_reply(line: &str) -> Option { let req: Value = serde_json::from_str(line).ok()?; - if req.get("method").and_then(Value::as_str) != Some(AUTH_METHOD) { + if !matches!(req.get("method").and_then(Value::as_str), Some(AUTH_METHOD | CHALLENGE_METHOD)) { return None; } let id = req.get("id").cloned().unwrap_or(Value::Null); @@ -312,6 +413,74 @@ impl Read for AuthDeadlineStream { mod tests { use super::*; + /// One frame through a fresh handshake: the reply and whether it authenticated. + fn authentication_reply(line: &str, token: &str) -> (Value, bool) { + let (reply, step) = ServerHandshake::new().reply(line, token); + (reply, step == AuthStep::Authenticated) + } + + fn challenge_line(client_nonce: &str) -> String { + json!({"id": 1, "method": CHALLENGE_METHOD, "params": {"clientNonce": client_nonce}}).to_string() + } + + fn proof_line(proof: &str) -> String { + json!({"id": 2, "method": AUTH_METHOD, "params": {"proof": proof}}).to_string() + } + + /// NV-6: a client proves it holds the token, and learns that the server does, without the + /// token crossing the wire. + #[test] + fn challenge_authentication_proves_the_token_both_ways_without_sending_it() { + let token = generate_token().unwrap(); + let client_nonce = generate_nonce().unwrap(); + let mut server = ServerHandshake::new(); + let (reply, step) = server.reply(&challenge_line(&client_nonce), &token); + assert_eq!(step, AuthStep::Challenged, "{reply}"); + assert!(!reply.to_string().contains(&token), "the challenge reply must not carry the token"); + let server_nonce = reply["result"]["serverNonce"].as_str().unwrap(); + let server_proof = reply["result"]["serverProof"].as_str().unwrap(); + assert!(is_hex256(server_nonce) && server_nonce != client_nonce); + assert!(proof_matches(&token, ProofRole::Server, &client_nonce, server_nonce, server_proof)); + // A server holding another token cannot produce that proof. + let other = generate_token().unwrap(); + assert!(!proof_matches(&other, ProofRole::Server, &client_nonce, server_nonce, server_proof)); + // The server's proof is not the client's: replaying it back does not authenticate. + assert!(!proof_matches(&token, ProofRole::Client, &client_nonce, server_nonce, server_proof)); + + let proof = handshake_proof(&token, ProofRole::Client, &client_nonce, server_nonce).unwrap(); + assert_ne!(proof, token); + let (reply, step) = server.reply(&proof_line(&proof), &token); + assert_eq!(step, AuthStep::Authenticated, "{reply}"); + assert_eq!(reply["result"]["authenticated"], true); + } + + #[test] + fn challenge_authentication_refuses_a_wrong_or_missing_proof() { + let token = generate_token().unwrap(); + let client_nonce = generate_nonce().unwrap(); + let wrong = handshake_proof(&generate_token().unwrap(), ProofRole::Client, &client_nonce, &client_nonce).unwrap(); + let bad_proofs = [proof_line(&wrong), proof_line(&token), json!({"id": 2, "method": AUTH_METHOD, "params": {}}).to_string()]; + for line in bad_proofs { + let mut server = ServerHandshake::new(); + assert_eq!(server.reply(&challenge_line(&client_nonce), &token).1, AuthStep::Challenged); + let (reply, step) = server.reply(&line, &token); + assert_eq!(step, AuthStep::Rejected, "{line}: {reply}"); + assert_eq!(reply["error"], "authentication required"); + } + // After a challenge the token itself no longer authenticates, nor does a second challenge. + for line in [auth_line(&token), challenge_line(&client_nonce), r#"{"id":3,"method":"methods"}"#.to_owned()] { + let mut server = ServerHandshake::new(); + assert_eq!(server.reply(&challenge_line(&client_nonce), &token).1, AuthStep::Challenged); + assert_eq!(server.reply(&line, &token).1, AuthStep::Rejected, "{line}"); + } + // A malformed nonce is refused before any proof is computed. + for nonce in ["", "short", &"g".repeat(64), &"a".repeat(65)] { + let (reply, step) = ServerHandshake::new().reply(&challenge_line(nonce), &token); + assert_eq!(step, AuthStep::Rejected, "{nonce}: {reply}"); + assert!(reply.get("result").is_none()); + } + } + #[test] fn generated_tokens_are_valid_and_distinct() { let a = generate_token().unwrap(); @@ -411,6 +580,8 @@ mod tests { assert_eq!(reply["id"], 3); assert_eq!(reply["ok"], false); assert_eq!(reply["error"], "already authenticated"); + let challenge = json!({"id": 5, "method": CHALLENGE_METHOD, "params": {"clientNonce": "anything"}}).to_string(); + assert_eq!(repeat_auth_reply(&challenge).unwrap()["error"], "already authenticated"); assert!(repeat_auth_reply(r#"{"id":4,"method":"methods"}"#).is_none()); assert!(repeat_auth_reply("{not json").is_none()); } diff --git a/crates/automation/tests/it/bridge.rs b/crates/automation/tests/it/bridge.rs --- a/crates/automation/tests/it/bridge.rs +++ b/crates/automation/tests/it/bridge.rs @@ -4,10 +4,10 @@ use std::time::Duration; -use photocraft_automation::security::authentication_reply; +use photocraft_automation::security::{AuthStep, ServerHandshake}; use photocraft_automation::{AutomationError, BridgeClient}; use serde_json::{Value, json}; -use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; +use tokio::io::{AsyncBufRead, AsyncBufReadExt, AsyncWrite, AsyncWriteExt, BufReader}; use tokio::net::{TcpListener, TcpStream}; const CONTROL_TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; @@ -68,15 +68,28 @@ async fn fake_server( addr } -/// Authenticate one connection: read the auth line, reply to it. +/// Authenticate one connection. async fn authenticate(socket: &mut TcpStream) { let (read, mut write) = socket.split(); - let mut reader = BufReader::new(read); + serve_handshake(&mut BufReader::new(read), &mut write).await; +} + +/// The server side of one connection's authentication, as the real control servers run it: the +/// bridge's challenge (or a one-frame token), answered frame by frame until it authenticates. +pub(crate) async fn serve_handshake(reader: &mut (impl AsyncBufRead + Unpin), writer: &mut (impl AsyncWrite + Unpin)) { + let mut handshake = ServerHandshake::new(); let mut line = String::new(); - reader.read_line(&mut line).await.unwrap(); - let (auth, ok) = authentication_reply(&line, CONTROL_TOKEN); - assert!(ok, "each connection must authenticate"); - write.write_all(format!("{auth}\n").as_bytes()).await.unwrap(); + loop { + line.clear(); + reader.read_line(&mut line).await.unwrap(); + let (reply, step) = handshake.reply(&line, CONTROL_TOKEN); + writer.write_all(format!("{reply}\n").as_bytes()).await.unwrap(); + match step { + AuthStep::Authenticated => return, + AuthStep::Challenged => {} + AuthStep::Rejected => panic!("each connection must authenticate: {reply}"), + } + } } #[tokio::test(flavor = "multi_thread")] diff --git a/crates/automation/tests/it/bridge_token.rs b/crates/automation/tests/it/bridge_token.rs new file mode 100644 --- /dev/null +++ b/crates/automation/tests/it/bridge_token.rs @@ -0,0 +1,140 @@ +//! NV-6: the bridge never reveals the control token to whatever holds the port. It connects to +//! the literal loopback address the app binds (never a `localhost` lookup that may reach a +//! `[::1]` listener), and the server must prove it holds the token before the client sends its +//! own proof; the token itself never crosses the wire. + +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use photocraft_automation::{BridgeClient, Headless}; +use serde_json::{Value, json}; +use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; +use tokio::net::TcpListener; + +const CONTROL_TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + +/// How a listener that is not the app answers the frames it receives. +#[derive(Clone, Copy)] +enum Impostor { + /// Accepts and reads, never answers. + Silent, + /// Refuses every frame, as a server without challenge authentication does. + Refuses, + /// Answers the challenge with a made-up nonce and proof. + FakesTheChallenge, +} + +/// Accept one connection on `listener` and return every line it received (until the client +/// closes or goes quiet), answering as `kind`. +async fn impostor(listener: TcpListener, kind: Impostor) -> Vec { + let mut lines = Vec::new(); + let Ok(Ok((socket, _))) = tokio::time::timeout(Duration::from_secs(3), listener.accept()).await else { + return lines; + }; + let (read, mut write) = socket.into_split(); + let mut reader = BufReader::new(read); + loop { + let mut line = String::new(); + match tokio::time::timeout(Duration::from_secs(3), reader.read_line(&mut line)).await { + Ok(Ok(n)) if n > 0 => {} + _ => return lines, + } + let id = serde_json::from_str::(&line).map(|v| v["id"].clone()).unwrap_or(Value::Null); + lines.push(line); + let reply = match kind { + Impostor::Silent => continue, + Impostor::Refuses => json!({"id": id, "ok": false, "error": "authentication required"}), + Impostor::FakesTheChallenge => json!({"id": id, "ok": true, "result": {"serverNonce": "ab".repeat(32), "serverProof": "cd".repeat(32)}}), + }; + if write.write_all(format!("{reply}\n").as_bytes()).await.is_err() { + return lines; + } + } +} + +/// Call through a bridge to `addr` while `listener` plays `kind`; the lines it got, and the +/// call's error. +async fn call_impostor(addr: &str, listener: TcpListener, kind: Impostor) -> (Vec, String) { + let bridge = BridgeClient::new(addr, CONTROL_TOKEN).unwrap().with_timeout(Duration::from_secs(2)); + let call = tokio::spawn(async move { bridge.call("methods", json!({})).await }); + let lines = impostor(listener, kind).await; + let error = match tokio::time::timeout(Duration::from_secs(10), call).await { + Ok(Ok(Err(e))) => e.to_string(), + other => format!("no error: {other:?}"), + }; + (lines, error) +} + +fn assert_token_withheld(lines: &[String]) { + for line in lines { + assert!(!line.contains(CONTROL_TOKEN), "the control token reached a listener that is not the app: {line}"); + } +} + +/// `localhost` is the 127.0.0.1 the app binds: a process listening on `[::1]` at the app's port +/// gets no connection, and the app does. +#[tokio::test(flavor = "multi_thread")] +async fn localhost_reaches_the_ipv4_app_not_an_ipv6_listener() { + let app = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + app.set_nonblocking(true).unwrap(); + let port = app.local_addr().unwrap().port(); + let Ok(other) = TcpListener::bind(format!("[::1]:{port}")).await else { + eprintln!("skipped: IPv6 loopback unavailable"); + return; + }; + let bridge = BridgeClient::new(format!("localhost:{port}"), CONTROL_TOKEN).unwrap().with_timeout(Duration::from_secs(1)); + let call = tokio::spawn(async move { bridge.call("methods", json!({})).await }); + let lines = impostor(other, Impostor::Silent).await; + call.abort(); + assert!(lines.is_empty(), "the [::1] listener was reached: {lines:?}"); + assert!(app.accept().is_ok(), "the app on 127.0.0.1 got the connection"); +} + +/// Whoever holds the port, on either loopback, and however it answers, never receives the +/// token: the client sends only its challenge and stops when the listener cannot prove it holds +/// the token. +#[tokio::test(flavor = "multi_thread")] +async fn a_listener_that_is_not_the_app_never_receives_the_token() { + for (kind, expected) in [ + (Impostor::Silent, "the request was not sent"), + (Impostor::Refuses, "refused challenge authentication"), + (Impostor::FakesTheChallenge, "did not prove it holds this control token"), + ] { + let mut hosts = vec!["127.0.0.1"]; + if TcpListener::bind("[::1]:0").await.is_ok() { + hosts.push("[::1]"); + } + for host in hosts { + let listener = TcpListener::bind(format!("{host}:0")).await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let (lines, error) = call_impostor(&addr, listener, kind).await; + assert_token_withheld(&lines); + assert_eq!(lines.len(), 1, "{addr}: only the challenge was sent: {lines:?}"); + assert!(lines[0].contains("auth.challenge"), "{addr}: {lines:?}"); + assert!(error.contains(expected), "{addr}: {error}"); + if !matches!(kind, Impostor::Silent) { + assert!(error.contains("the control token was not sent"), "{addr}: {error}"); + } + } + } +} + +/// The bridge authenticates to the headless `photocraft-cli serve` server by challenge. +#[tokio::test(flavor = "multi_thread")] +async fn the_bridge_authenticates_to_the_headless_server_by_challenge() { + let headless = Arc::new(Mutex::new(Headless::new())); + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = photocraft_automation::rpc::serve_tcp("127.0.0.1:0", headless, CONTROL_TOKEN.to_owned(), move |addr| { + let _ = ready_tx.send(addr); + }); + }); + let addr = ready_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + let bridge = BridgeClient::new(addr.to_string(), CONTROL_TOKEN).unwrap().with_timeout(Duration::from_secs(5)); + let methods = bridge.call("methods", json!({})).await.unwrap(); + assert!(methods.as_array().unwrap().iter().any(|m| m == "batch"), "{methods}"); + // A second bridge with another token is refused before anything derived from its token is sent. + let other = BridgeClient::new(addr.to_string(), "f".repeat(64)).unwrap().with_timeout(Duration::from_secs(5)); + let error = other.call("methods", json!({})).await.unwrap_err().to_string(); + assert!(error.contains("did not prove it holds this control token"), "{error}"); +} diff --git a/crates/automation/tests/it/main.rs b/crates/automation/tests/it/main.rs --- a/crates/automation/tests/it/main.rs +++ b/crates/automation/tests/it/main.rs @@ -4,6 +4,7 @@ mod agent_tasks; mod agent_tasks_extended; mod agent_tasks_more; mod bridge; +mod bridge_token; mod inactive_save; mod krita; mod layered_save_dirty; diff --git a/crates/automation/tests/it/mcp.rs b/crates/automation/tests/it/mcp.rs --- a/crates/automation/tests/it/mcp.rs +++ b/crates/automation/tests/it/mcp.rs @@ -468,13 +468,9 @@ async fn bridge_response_budget_drops_connection_without_retrying_the_operation( let (sock, _) = listener.accept().await.unwrap(); let (read, mut write) = sock.into_split(); let mut reader = BufReader::new(read); + crate::bridge::serve_handshake(&mut reader, &mut write).await; let mut line = String::new(); reader.read_line(&mut line).await.unwrap(); - let auth: Value = serde_json::from_str(&line).unwrap(); - let reply = json!({"id": auth["id"], "ok": true, "result": {"authenticated": true}}); - write.write_all(format!("{reply}\n").as_bytes()).await.unwrap(); - line.clear(); - reader.read_line(&mut line).await.unwrap(); let request: Value = serde_json::from_str(&line).unwrap(); assert_eq!(request["method"], "engine.execute"); let mut oversized = vec![b'x'; MAX_RESPONSE_BYTES]; @@ -912,6 +908,7 @@ async fn fake_app_with_screenshot(screenshot_png: Option>) -> (String, t let addr = listener.local_addr().unwrap().to_string(); let h = tokio::spawn(async move { let mut seen = Vec::new(); + let mut handshake = photocraft_automation::security::ServerHandshake::new(); let mut authenticated = false; let (sock, _) = listener.accept().await.unwrap(); let (r, mut w) = sock.into_split(); @@ -921,12 +918,10 @@ async fn fake_app_with_screenshot(screenshot_png: Option>) -> (String, t let id = req["id"].clone(); let method = req["method"].as_str().unwrap_or("").to_owned(); let reply = match method.as_str() { - "auth" if req["params"]["token"] == CONTROL_TOKEN => { - authenticated = true; - json!({"id": id, "ok": true, "result": {"authenticated": true}}) - } _ if !authenticated => { - json!({"id": id, "ok": false, "error": "authentication required"}) + let (reply, step) = handshake.reply(&line, CONTROL_TOKEN); + authenticated = step == photocraft_automation::security::AuthStep::Authenticated; + reply } "ui.inspect" => { json!({"id": id, "ok": true, "result": {"tool": "brush", "panels": ["layers"]}}) @@ -1005,13 +1000,9 @@ async fn bridge_lost_reply_does_not_replay_an_edit_and_next_call_reconnects() { let (socket, _) = listener.accept().await.unwrap(); let (read, mut write) = socket.into_split(); let mut reader = BufReader::new(read); + crate::bridge::serve_handshake(&mut reader, &mut write).await; let mut line = String::new(); reader.read_line(&mut line).await.unwrap(); - let (auth, authenticated) = photocraft_automation::security::authentication_reply(&line, CONTROL_TOKEN); - assert!(authenticated, "each connection must authenticate"); - write.write_all(format!("{auth}\n").as_bytes()).await.unwrap(); - line.clear(); - reader.read_line(&mut line).await.unwrap(); let request: Value = serde_json::from_str(&line).unwrap(); seen.push(request["params"]["command"].as_str().unwrap().to_owned()); let result = backend.handle(request["method"].as_str().unwrap(), request["params"].clone()).unwrap(); @@ -1167,7 +1158,8 @@ async fn bridge_rejects_wrong_token_before_control_methods() { let client = connect(PhotocraftMcp::bridge(&addr, wrong).unwrap()).await; let r = call(&client, "ui_inspect", json!({})).await; assert_eq!(r.is_error, Some(true)); - assert!(text(&r).contains("authentication required"), "{}", text(&r)); + // The app's challenge proof does not match the client's token, so nothing more is sent. + assert!(text(&r).contains("did not prove it holds this control token"), "{}", text(&r)); client.cancel().await.unwrap(); app.abort(); } diff --git a/docs/control-protocol.md b/docs/control-protocol.md --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -17,13 +17,29 @@ photocraft --control 7878 --control-token-file /private/path/photocraft-control. If the file does not exist, PhotoCraft creates it with a fresh 256-bit token. On Unix the new file is mode `0600`; on Windows, protect it with an appropriate user-only ACL. An existing file is reused. If neither a token nor token file is configured, PhotoCraft generates a token for that launch and writes it to standard error. `PHOTOCRAFT_CONTROL_TOKEN` and `PHOTOCRAFT_CONTROL_TOKEN_FILE` are the environment-variable equivalents. A flag wins over its variable and an empty variable counts as unset; with `--control` (or `PHOTOCRAFT_CONTROL_PORT`), a token together with a token file, from flags or variables, is a usage error (exit status 2). Without it no token is read, so the pair is ignored. -The first request on every TCP connection must authenticate. No other method is dispatched before this succeeds: +The first request on every TCP connection must authenticate. No other method is dispatched before this succeeds. There are two ways; a refused frame gets `{"ok": false, "error": "authentication required"}` and the connection is closed. + +**Challenge authentication** (what `photocraft-cli mcp --bridge` uses) never sends the token, and the server proves it holds the token before the client reveals anything derived from it, so a process that took the port (the app is not running yet, or has exited) learns nothing: + +```json +{"id": 1, "method": "auth.challenge", "params": {"clientNonce": "<64 hex: 32 fresh random bytes>"}} +{"id": 1, "ok": true, "result": {"serverNonce": "<64 hex>", "serverProof": "<64 hex>"}} +{"id": 2, "method": "auth", "params": {"proof": "<64 hex>"}} +{"id": 2, "ok": true, "result": {"authenticated": true}} +``` + +Both proofs are lowercase hex HMAC-SHA256, keyed by the token's 64 lowercase hex characters (as ASCII), over `