From 9d5be5ec8ebca0749ca722eefda932f9ba54ff29 Mon Sep 17 00:00:00 2001 From: Michael Higgins <55243596+Higgs32584@users.noreply.github.com> Date: Wed, 11 Nov 2026 01:35:17 -0500 Subject: [PATCH] Bridge: never send the control token to a listener that is not the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `photocraft-cli mcp --bridge` sent the control token in clear as its first frame, to whatever had accepted the connection: a `[::1]` listener reached through a `localhost` lookup, or any process holding `127.0.0.1:` while the app was not listening. This PR fixes both cases: - **Literal loopback only.** `BridgeClient` parses its address into a socket address and connects to it. Only `127.0.0.1:` and `[::1]:` are accepted. `localhost:` means `127.0.0.1:` (what the desktop app binds) and is never looked up. - **Challenge authentication.** The bridge authenticates with `auth.challenge {clientNonce}` → `{serverNonce, serverProof}` → `auth {proof}`. Both proofs are HMAC-SHA256 keyed by the token over both nonces, each under its own label. The client checks the server's proof before it sends its own and never falls back to sending the token. The token never crosses the wire, so a listener that is not the app gets only a random nonce. - **One shared server implementation.** `security::ServerHandshake` replaces `authentication_reply`, and both the desktop control server and `photocraft-cli serve` use it. The servers still accept the one-frame `auth {token}`, so existing scripts and older clients keep working. A repeat `auth.challenge` after authenticating gets "already authenticated", like a repeat `auth`. - HMAC comes from RustCrypto's `hmac` 0.12 with `sha2` 0.10 (`sha2` was already in the lockfile). The proofs are compared without an early exit. - `docs/control-protocol.md` documents the handshake, the exact HMAC message, and the bridge's address rules. The book's protocol summary is updated too. Compatibility: a new bridge needs an app from the same release. Against an older app it fails with "refused challenge authentication … the control token was not sent". It does not downgrade. Known limit: the channel is plain TCP with no TLS, so nothing binds the handshake to the connection. A party sitting between a bridge and a real app could relay the handshake without learning the token. On loopback that needs it to hold the port the bridge dials while the app listens on another, which the literal-address rule above narrows. Older clients that still send `auth {token}` keep their old exposure until they upgrade. Closes #9777 ## Tests New in `crates/automation/tests/it/bridge_token.rs`: - `localhost_reaches_the_ipv4_app_not_an_ipv6_listener`: a listener on `[::1]:P` gets no connection, and the app on `127.0.0.1:P` does. - `a_listener_that_is_not_the_app_never_receives_the_token`: on both loopbacks, a listener that stays silent, refuses, or fakes the challenge receives only the challenge frame, never the token. The error says why. - `the_bridge_authenticates_to_the_headless_server_by_challenge`: end to end against `rpc::serve_tcp`. A wrong token is refused before its proof is sent. Unit tests: the handshake in both directions, and that wrong, missing or replayed proofs, the token after a challenge, a second challenge and malformed nonces are all refused (`security.rs`). `loopback_socket` address parsing (`bridge.rs`). The desktop server answering a challenge, then dispatching (`control_server.rs`). The existing bridge and MCP fakes now run the real `ServerHandshake`. Run locally (Windows 11, targeted tests only): - **Fail before.** I put upstream's `crates/automation/src/bridge.rs` (the old client) back and ran `cargo test -p photocraft-automation --test it bridge_token`: **3 failed**. The `[::1]` listener received `{"method":"auth","params":{"token":""}}`, a `127.0.0.1` listener received the raw token, and the headless test got "authentication required" instead of the refusal. The original audit tests (`audit_nv6_bridge_sends_token_to_ipv6_impostor`, `audit_nv6_bridge_sends_token_to_any_ipv4_holder`) fail the same way on upstream/main. - **Pass after.** - `cargo test -p photocraft-automation --lib --test it -- bridge security tcp`: 23 + 21 passed. This covers the new tests, the existing bridge, MCP-bridge, security and serve TCP tests, and the legacy-token tests that still pass unchanged. - `cargo test -p photocraft control_server`: 6 passed, including the new challenge test. The legacy-token tests still pass. - `cargo clippy -p photocraft-automation --all-targets -- -D warnings`: clean. - `cargo fmt`: clean. - `Cargo.lock` adds only `hmac` 0.12.1, plus `subtle` under `digest` (already in the lock). Checked with `cargo metadata --locked`. Not run here: the full gate (`cargo xtask ...`, the workspace-wide tests, wasm, clippy on the desktop app crate). That is left to CI. API note: the workspace-internal `security::authentication_reply` is replaced by `ServerHandshake::reply`, which returns an `AuthStep`. ## Summary by CodeRabbit * **Security** * Control connections support challenge-response authentication, verifying possession of the configured token without sending it. Token-in-frame authentication remains available for scripts. * The MCP bridge accepts literal loopback addresses and maps `localhost` directly to IPv4 loopback. It rejects servers that refuse or cannot complete challenge authentication. * **Bug Fixes** * Rejected authentication exchanges close the connection; successful authentication allows requests to proceed. * **Documentation** * Control protocol and bridge authentication behavior are documented. --- Cargo.lock | 12 ++ apps/photocraft/src/control_server.rs | 56 ++++- book/src/automation/control-protocol.md | 2 +- crates/automation/Cargo.toml | 3 + crates/automation/src/bridge.rs | 79 ++++++- crates/automation/src/rpc.rs | 17 +- crates/automation/src/security.rs | 234 ++++++++++++++++++--- crates/automation/src/server.rs | 22 +- crates/automation/tests/it/bridge.rs | 29 ++- crates/automation/tests/it/bridge_token.rs | 141 +++++++++++++ crates/automation/tests/it/main.rs | 1 + crates/automation/tests/it/mcp.rs | 33 +-- crates/automation/tests/mcp_wait.rs | 26 ++- docs/control-protocol.md | 21 +- 14 files changed, 593 insertions(+), 83 deletions(-) create mode 100644 crates/automation/tests/it/bridge_token.rs diff --git a/Cargo.lock b/Cargo.lock index 0aeea498e3..df19473077 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1736,6 +1736,7 @@ checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" dependencies = [ "block-buffer", "crypto-common", + "subtle", ] [[package]] @@ -3076,6 +3077,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" @@ -5390,6 +5400,7 @@ dependencies = [ "cap-std", "clap", "getrandom 0.3.4", + "hmac", "photocraft-codecs", "photocraft-color", "photocraft-compose", @@ -5408,6 +5419,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "sha2", "thiserror 2.0.21", "tokio", "wgpu", diff --git a/apps/photocraft/src/control_server.rs b/apps/photocraft/src/control_server.rs index 4834327b8e..ae7fa8a32e 100644 --- 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; @@ -66,6 +66,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; @@ -326,4 +327,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 index c81c205843..418fa9f50e 100644 --- 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 index bee8984e1c..bbc0aa6bb6 100644 --- a/crates/automation/Cargo.toml +++ b/crates/automation/Cargo.toml @@ -34,6 +34,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 index 1f1a5e0509..78e136150c 100644 --- a/crates/automation/src/bridge.rs +++ b/crates/automation/src/bridge.rs @@ -4,7 +4,13 @@ //! dropped before its reply — or before 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()))?; @@ -177,6 +184,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. @@ -256,6 +308,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 index 2f6b7ea818..d12d912e97 100644 --- 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}; @@ -375,6 +375,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)? { @@ -392,19 +393,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 index 7fff71c67a..481ff3d1c0 100644 --- 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<()> { } } +/// Lowercase hexadecimal, two characters per byte. +fn hex(bytes: &[u8]) -> String { + const HEX: &[u8; 16] = b"0123456789abcdef"; + 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); + } + 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 { - 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); - 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); - } - Ok(token) + 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,107 @@ 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())) +} + +/// 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 { + /// A handshake for a freshly accepted connection: no challenge issued yet. + 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` frame on an already authenticated connection. Returns `None` for any -/// other frame, which the caller dispatches as usual. +/// 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 +414,78 @@ 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) + } + + /// An `auth.challenge` frame carrying `client_nonce`. + fn challenge_line(client_nonce: &str) -> String { + json!({"id": 1, "method": CHALLENGE_METHOD, "params": {"clientNonce": client_nonce}}).to_string() + } + + /// An `auth` frame carrying the client's challenge `proof`. + 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); + } + + /// NV-6: after a challenge, a wrong, missing or token-shaped proof, the token itself, a second + /// challenge and a control method are all refused, and a malformed nonce gets no challenge. + #[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(); @@ -404,6 +578,8 @@ mod tests { assert!(authenticated, "{reply}"); } + /// A repeat `auth` or `auth.challenge` on an authenticated connection is answered "already + /// authenticated"; any other frame is left to the dispatcher. #[test] fn repeat_auth_on_an_authenticated_connection_says_so() { let line = json!({"id": 3, "method": AUTH_METHOD, "params": {"token": "anything"}}).to_string(); @@ -411,6 +587,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/src/server.rs b/crates/automation/src/server.rs index 0ab39bc658..6ec00b27ab 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -1546,6 +1546,25 @@ mod tests { server.await.unwrap().unwrap(); } + /// Answer the bridge's challenge authentication on one fake-app connection, frame by frame, + /// as the real control servers do. Returns whether the connection authenticated. + async fn fake_app_handshake(lines: &mut tokio::io::Lines, write: &mut W, token: &str) -> bool + where + R: tokio::io::AsyncBufRead + Unpin, + W: tokio::io::AsyncWrite + Unpin, + { + use tokio::io::AsyncWriteExt; + let mut handshake = crate::security::ServerHandshake::new(); + while let Ok(Some(line)) = lines.next_line().await { + let (reply, step) = handshake.reply(&line, token); + write.write_all(format!("{reply}\n").as_bytes()).await.unwrap(); + if step != crate::security::AuthStep::Challenged { + return step == crate::security::AuthStep::Authenticated; + } + } + false + } + /// The same over the bridge (#8130): a dropped wait sends `jobs.cancel` for the app's job. #[tokio::test(flavor = "multi_thread")] async fn dropping_a_bridged_wait_cancels_the_apps_job() { @@ -1563,8 +1582,7 @@ mod tests { let (socket, _) = listener.accept().await.unwrap(); let (read, mut write) = socket.into_split(); let mut lines = tokio::io::BufReader::new(read).lines(); - let (auth, _) = crate::security::authentication_reply(&lines.next_line().await.unwrap().unwrap(), TOKEN); - write.write_all(format!("{auth}\n").as_bytes()).await.unwrap(); + fake_app_handshake(&mut lines, &mut write, TOKEN).await; while let Ok(Some(line)) = lines.next_line().await { let request: Value = serde_json::from_str(&line).unwrap(); let result = match request["method"].as_str().unwrap() { diff --git a/crates/automation/tests/it/bridge.rs b/crates/automation/tests/it/bridge.rs index 460a33e264..ac6f07cbbe 100644 --- 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 index 0000000000..b1e3d849a9 --- /dev/null +++ b/crates/automation/tests/it/bridge_token.rs @@ -0,0 +1,141 @@ +//! 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) +} + +/// Fail if any line a listener received contains the control token. +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 index 2c66b134b7..ed98e61b6b 100644 --- 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 gpu_backend; mod inactive_save; mod krita; diff --git a/crates/automation/tests/it/mcp.rs b/crates/automation/tests/it/mcp.rs index 9f791c487f..12cc75f19f 100644 --- a/crates/automation/tests/it/mcp.rs +++ b/crates/automation/tests/it/mcp.rs @@ -577,6 +577,8 @@ async fn bridge_doc_save_forwards_maximize_compatibility() { app.abort(); } +/// An oversized app reply drops the connection with a budget error and never retries the +/// operation on a fresh, authenticated connection. #[tokio::test(flavor = "multi_thread")] async fn bridge_response_budget_drops_connection_without_retrying_the_operation() { use photocraft_automation::{BridgeClient, budgets::MAX_RESPONSE_BYTES}; @@ -586,13 +588,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]; @@ -1068,11 +1066,16 @@ async fn fake_app() -> (String, tokio::task::JoinHandle>) { fake_app_with_screenshot(None).await } +/// A fake app on one connection: it authenticates with the real [`ServerHandshake`], then +/// answers the control methods the bridge tests use, returning the requests it saw. +/// +/// [`ServerHandshake`]: photocraft_automation::security::ServerHandshake async fn fake_app_with_screenshot(screenshot_png: Option>) -> (String, tokio::task::JoinHandle>) { let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); 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(); @@ -1082,12 +1085,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"]}}) @@ -1148,6 +1149,8 @@ async fn bridge_forwards_to_control_protocol() { app.abort(); } +/// #1007: an edit whose reply is lost is not resent, and the next call reconnects and +/// authenticates again. #[tokio::test(flavor = "multi_thread")] async fn bridge_lost_reply_does_not_replay_an_edit_and_next_call_reconnects() { use photocraft_automation::{BridgeClient, Headless}; @@ -1166,13 +1169,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(); @@ -1321,6 +1320,7 @@ fn bridge_rejects_non_loopback() { assert!(PhotocraftMcp::bridge("localhost:1", "short").is_err()); } +/// A bridge with the wrong token stops at the app's challenge proof, before any control method. #[tokio::test(flavor = "multi_thread")] async fn bridge_rejects_wrong_token_before_control_methods() { let (addr, app) = fake_app().await; @@ -1328,7 +1328,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/crates/automation/tests/mcp_wait.rs b/crates/automation/tests/mcp_wait.rs index 797b1d141a..f5200472ee 100644 --- a/crates/automation/tests/mcp_wait.rs +++ b/crates/automation/tests/mcp_wait.rs @@ -4,7 +4,7 @@ use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; -use photocraft_automation::security::authentication_reply; +use photocraft_automation::security::{AuthStep, ServerHandshake}; use photocraft_automation::{Backend, BridgeClient, PhotocraftMcp}; use serde_json::{Value, json}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader, DuplexStream, Lines}; @@ -183,7 +183,27 @@ struct FakeApp { log: Vec, } +/// Answer the bridge's challenge authentication on one fake-app connection, frame by frame, as +/// the real control servers do. Returns whether the connection authenticated. +async fn authenticate(lines: &mut Lines, write: &mut W) -> bool +where + R: tokio::io::AsyncBufRead + Unpin, + W: tokio::io::AsyncWrite + Unpin, +{ + let mut handshake = ServerHandshake::new(); + while let Ok(Some(line)) = lines.next_line().await { + let (reply, step) = handshake.reply(&line, TOKEN); + write.write_all(format!("{reply}\n").as_bytes()).await.unwrap(); + if step != AuthStep::Challenged { + return step == AuthStep::Authenticated; + } + } + false +} + impl FakeApp { + /// Listen on a fresh loopback port and serve every connection: authenticate it, then answer + /// its requests through [`FakeApp::answer`]. Returns the address and the shared state. async fn serve(runs: Duration, lose_start: bool) -> (String, Arc>) { let app = Arc::new(Mutex::new(FakeApp { runs, @@ -203,9 +223,7 @@ impl FakeApp { tokio::spawn(async move { let (read, mut write) = socket.into_split(); let mut lines = BufReader::new(read).lines(); - let (auth, ok) = authentication_reply(&lines.next_line().await.unwrap().unwrap(), TOKEN); - assert!(ok, "each connection must authenticate"); - write.write_all(format!("{auth}\n").as_bytes()).await.unwrap(); + assert!(authenticate(&mut lines, &mut write).await, "each connection must authenticate"); while let Ok(Some(line)) = lines.next_line().await { let request: Value = serde_json::from_str(&line).unwrap(); let Some((result, after)) = app.lock().unwrap().answer(&request) else { return }; diff --git a/docs/control-protocol.md b/docs/control-protocol.md index e4cbc8a2eb..70d7efab32 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -17,13 +17,28 @@ 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 `