diff --git a/crates/spt-daemon/src/bootstrap_firewall.rs b/crates/spt-daemon/src/bootstrap_firewall.rs index c30d6b54..6e6d102c 100644 --- a/crates/spt-daemon/src/bootstrap_firewall.rs +++ b/crates/spt-daemon/src/bootstrap_firewall.rs @@ -96,10 +96,10 @@ pub fn cleanup_command() -> String { /// ranking legs by wall alone puts a script that executed no statement first. // [impl->REQ-BOOTSTRAP-FIREWALL-INVOCATION-LOG] #[cfg(any(windows, target_os = "linux"))] -fn run(label: &str, program: &str, args: &[&str]) -> Result { +fn run(label: &str, program: &str, args: &[&str], budget: std::time::Duration) -> Result { let started = std::time::Instant::now(); let mut killed = false; - let result = run_bounded(program, args, &mut killed); + let result = run_bounded(program, args, budget, &mut killed); eprintln!( "bootstrap-firewall leg={label} program={program} wall_ms={} outcome={}", started.elapsed().as_millis(), @@ -125,21 +125,26 @@ fn outcome(killed: bool, failed: bool) -> &'static str { } } -/// Match the existing local firewall probe budget, but kill/reap the child on -/// expiry rather than abandoning a worker. Pipe completion shares that deadline. +/// Bound the complete invocation with the platform's operation budget, killing +/// and reaping on expiry. Pipe completion shares the same deadline. /// /// `killed` reports whether the BUDGET expired, for the caller's log line. It is /// set only on deadline expiry: a `try_wait` failure also kills the child, but /// calling that "killed" would dress a different failure as a timeout. // [impl->REQ-HAZARD-SUBPROCESS-TIMEOUT] #[cfg(any(windows, target_os = "linux"))] -fn run_bounded(program: &str, args: &[&str], killed: &mut bool) -> Result { +fn run_bounded( + program: &str, + args: &[&str], + budget: std::time::Duration, + killed: &mut bool, +) -> Result { use std::io::Read; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; const LIMIT: u64 = 1024 * 1024; - let deadline = Instant::now() + Duration::from_secs(3); + let deadline = Instant::now() + budget; let mut command = Command::new(program); command.args(args).stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped()); // [impl->REQ-HAZARD-CHILD-CONSOLE-FLASH] @@ -220,6 +225,21 @@ mod tests { assert_eq!(outcome(true, true), "killed"); assert_eq!(outcome(true, false), "killed"); } + + // [unit->REQ-HAZARD-SUBPROCESS-TIMEOUT] + #[cfg(windows)] + #[test] + fn a_hung_firewall_child_is_killed_at_its_operation_budget() { + let mut killed = false; + let result = run_bounded( + "powershell.exe", + &["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", "Start-Sleep -Seconds 30"], + std::time::Duration::from_millis(100), + &mut killed, + ); + assert!(killed, "an unfinished child must be killed, not abandoned"); + assert!(result.is_err(), "a timed-out observation cannot certify admission"); + } use std::ffi::OsString; use std::sync::{Mutex, OnceLock}; diff --git a/crates/spt-daemon/src/bootstrap_firewall/linux.rs b/crates/spt-daemon/src/bootstrap_firewall/linux.rs index 08f615c4..89479587 100644 --- a/crates/spt-daemon/src/bootstrap_firewall/linux.rs +++ b/crates/spt-daemon/src/bootstrap_firewall/linux.rs @@ -60,7 +60,7 @@ fn command(name: &str, args: &[&str]) -> Result { let path = path .to_str() .ok_or_else(|| format!("{name} executable path is not UTF-8"))?; - run(name, path, args) + run(name, path, args, std::time::Duration::from_secs(3)) } fn unit_state(unit: &str) -> Option { diff --git a/crates/spt-daemon/src/bootstrap_firewall/windows.rs b/crates/spt-daemon/src/bootstrap_firewall/windows.rs index 31ef24c8..56ff9176 100644 --- a/crates/spt-daemon/src/bootstrap_firewall/windows.rs +++ b/crates/spt-daemon/src/bootstrap_firewall/windows.rs @@ -736,17 +736,28 @@ fn encoded(script: &str) -> String { base64::engine::general_purpose::STANDARD.encode(bytes) } +// NetSecurity legs walk the host's policy store, including the ownership queries +// inside writes and cleanup. The UDP probe's 3 s allowance killed a required +// pre-write census on a rule-heavy host in field window 5. This administrative +// operation ceiling is policy, not a guarantee for every store size. +// [impl->REQ-WEB-LAN-BOOTSTRAP-FIREWALL] +// [impl->REQ-HAZARD-SUBPROCESS-TIMEOUT] +const NETSECURITY_BUDGET: std::time::Duration = std::time::Duration::from_secs(30); + fn powershell(leg: &str, script: &str) -> Result { let command = encoded(script); super::run( leg, "powershell.exe", &["-NoLogo", "-NoProfile", "-NonInteractive", "-EncodedCommand", &command], + NETSECURITY_BUDGET, ) } -fn snapshot() -> Result { - let output = powershell("verify-query", &script(QUERY))?; +fn snapshot( + execute: &mut impl FnMut(&str, &str) -> Result, +) -> Result { + let output = execute("verify-query", &script(QUERY))?; serde_json::from_str(output.trim_start_matches('\u{feff}').trim()) .map_err(|error| format!("Cannot decode NetSecurity bootstrap rule evidence: {error}")) } @@ -766,7 +777,7 @@ pub(super) fn verify(binder: &Path, port: u16) -> Result { if port == 0 { return Err("Bootstrap firewall requires the actual bound TCP port, not port zero".into()); } - let state = snapshot()?; + let state = snapshot(&mut powershell)?; // ONE COMPOSER, ONE CENSUS (doyle's FOLD-3 condition 1): the scope the verdict // expects is derived by the same function, from the same invocation's census, // as the scope a reconcile would write. A pair matching an OLDER census is @@ -833,24 +844,35 @@ fn decide(state: &Snapshot, expected_program: &str, port: u16) -> Result Result<(), String> { + reconcile_with(binder, port, powershell) +} + +// Verify and derive any repair from one pre-write census under the caller's +// serialized helper. Fresh write-side ownership checks and post-write evidence +// remain separate; no observation is carried across an elevation boundary. +// [impl->REQ-WEB-LAN-BOOTSTRAP-FIREWALL] +fn reconcile_with( + binder: &Path, + port: u16, + mut execute: impl FnMut(&str, &str) -> Result, +) -> Result<(), String> { let binder = binder_text(binder)?; if port == 0 { return Err("Bootstrap firewall requires the actual bound TCP port, not port zero".into()); } + let observed = snapshot(&mut execute)?; + let expected = crate::firewall::normalize_path(binder); + if matches!(decide(&observed, &expected, port), Ok(true)) { + return Ok(()); + } let binder_data = base64::engine::general_purpose::STANDARD.encode(binder.as_bytes()); - // FOLD-3: THE SCOPE IS READ BEFORE THE WRITES ARE RENDERED, from the same - // query the verdict uses, because the LAN half's remotes are now a fact about - // this host rather than a constant. One extra invocation on the reconcile - // path; it cannot lengthen any single child past its own budget, which is - // where the 3000 ms is enforced. - let observed = snapshot()?; let lan = lan_scope(&observed.addresses); // THE EMITTED RULES ARE THE SPECS, not a second copy of the policy. When a // spec wants no program filter the `-Program` argument is absent entirely; // passing `Any` would be a different rule that merely reads similar. Both // halves are rendered from the same array the verdict is taken over, so a // spec added there cannot be forgotten here. - let want = desired_specs(&crate::firewall::normalize_path(binder), port, &lan); + let want = desired_specs(&expected, port, &lan); // A SILENT REWRITE IS THE SAME DEFECT AS A SILENT VERIFY (doyle's FOLD-3 // condition 1). Moving networks rewrites the LAN rule, and the reason has to // be readable afterwards or the rewrite looks like churn. @@ -873,7 +895,7 @@ Remove-Owned $binder = [System.Text.Encoding]::UTF8.GetString([Convert]::FromBase64String('{binder_data}')) {writes}"# ); - powershell("reconcile-write", &script(&body))?; + execute("reconcile-write", &script(&body))?; // The tailnet half has been written; the LAN half could not be scoped. Report // that directly rather than through `verify`, whose refusal would be wrapped // by clause (c) as "could not be verified" -- this IS a verdict, not a @@ -887,7 +909,9 @@ $binder = [System.Text.Encoding]::UTF8.GetString([Convert]::FromBase64String('{b // not wear one's face: the operator who reads "reconcile failed" reaches for // elevation and permissions, when the rules are in fact already in place and // only the observation was cut short. - match verify(Path::new(binder), port) { + match snapshot(&mut execute) + .and_then(|state| decide(&state, &expected, port)) + { Ok(true) => Ok(()), Ok(false) => Err(mismatch_message(&want)), Err(error) => Err(unverified_after_write(&error)), @@ -992,6 +1016,55 @@ pub(super) fn cleanup_command() -> String { mod tests { use super::*; + // The process boundary is the test seam: no NetSecurity command or host + // mutation runs. The pre-write observation still uses the real child runner. + // [unit->REQ-WEB-LAN-BOOTSTRAP-FIREWALL] + #[test] + fn a_rule_heavy_prewrite_query_still_reaches_reconcile_write() { + let addresses = serde_json::json!([{ + "address": "192.168.1.81", "prefixLength": 24, "addressState": "Preferred" + }]); + let before = serde_json::json!({"active": [], "addresses": addresses}).to_string(); + let after = serde_json::json!({ + "active": [ + {"name": RULE_NAME_TAILNET, "program": "Any", "ports": ["29470"], + "profile": "Any", "remotes": ["100.64.0.0/10"], "hygiene": true, + "enforcement": [1], "sourceType": "Local"}, + {"name": RULE_NAME_LAN, "program": "Any", "ports": ["29470"], + "profile": "Private,Domain", "remotes": ["192.168.1.0/24"], + "hygiene": true, "enforcement": [1], "sourceType": "Local"} + ], + "addresses": addresses + }).to_string(); + let mut written = false; + let mut queries = 0; + let result = reconcile_with(Path::new(r"C:\spt\spt.exe"), 29470, |leg, _| { + match leg { + "verify-query" => { + queries += 1; + if queries == 1 { + // Exceeds the shipped 3 s probe cap even on a quiet host. + powershell(leg, &format!( + "Start-Sleep -Milliseconds 3200; [Console]::Write('{before}')" + )) + } else if written { + Ok(after.clone()) + } else { + Err("duplicate pre-write census".into()) + } + } + "reconcile-write" => { + written = true; + Ok(String::new()) + } + other => panic!("unexpected firewall leg {other}"), + } + }); + assert!(result.is_ok(), "a completed slow census must reach admission: {result:?}"); + assert!(written, "the absent admission pair must be written"); + assert_eq!(queries, 2, "one pre-write census and a fresh post-write verification"); + } + /// The binder is the broker's captured image; anything that is not an absolute, /// NUL-free Unicode path is refused before it can reach a NetSecurity command. // [unit->REQ-WEB-LAN-BOOTSTRAP-FIREWALL] diff --git a/crates/spt/src/serveverb.rs b/crates/spt/src/serveverb.rs index f8f03054..b7d57d39 100644 --- a/crates/spt/src/serveverb.rs +++ b/crates/spt/src/serveverb.rs @@ -355,6 +355,19 @@ fn report_lan_admission(binder: Option<&Path>, port: u16, retry: bool) { lan_firewall_warning("broker did not report its actual binder executable"); return; }; + // The privileged helper serializes and re-observes the live listener before + // taking the census used for both verification and any repair. Do not spend + // a second store query here when we can enter that helper directly. + // Keep opted-out callers on the read-only observation path. + // [impl->REQ-WEB-LAN-BOOTSTRAP-FIREWALL] + if cfg!(windows) + && retry + && matches!(crate::elevation::current(), crate::elevation::Elevation::Elevated) + && spt_daemon::bootstrap_firewall::mutation_permitted().is_ok() + { + reconcile_lan_firewall(&spt_daemon::endpoint::seed_socket_name()); + return; + } let reason = match spt_daemon::bootstrap_firewall::verify(binder, port) { Ok(true) => { print_lan_admission(binder, port);