diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e36d35e98..32b0d051d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,9 @@ jobs: - name: Run Windows named-pipe tests run: cargo test -p bsk --lib --locked daemon::ipc::windows::tests + - name: Run Windows daemon startup diagnostics tests + run: cargo test -p bsk --lib --locked daemon::start::windows::tests + - name: Run Windows audit storage tests run: cargo test -p bsk --lib --locked daemon::audit::tests diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index f50e63d40..fb7fa1402 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -739,6 +739,10 @@ mod m2_tests { for failure in [ DaemonStartFailure::AutoStartDisabled, DaemonStartFailure::IndependentStartFailed, + DaemonStartFailure::BreakawayUnverified, + DaemonStartFailure::ProcessCreationDenied, + DaemonStartFailure::PartialBreakaway, + DaemonStartFailure::ResumeFailed, ] { let error = anyhow::anyhow!("startup fixture cause") .context(failure) diff --git a/crates/bsk-cli/src/cli/error.rs b/crates/bsk-cli/src/cli/error.rs index 5dd588ed1..ffeafffa6 100644 --- a/crates/bsk-cli/src/cli/error.rs +++ b/crates/bsk-cli/src/cli/error.rs @@ -564,6 +564,19 @@ mod tests { DaemonStartFailure::IndependentStartFailed, "use `bsk daemon start --foreground` in a persistent host task", ), + ( + DaemonStartFailure::BreakawayUnverified, + "check access to the bsk executable", + ), + ( + DaemonStartFailure::ProcessCreationDenied, + "check access to the bsk executable", + ), + ( + DaemonStartFailure::PartialBreakaway, + "use `bsk daemon start --foreground` in a persistent host task", + ), + (DaemonStartFailure::ResumeFailed, "retry `bsk daemon start`"), ] { let error = anyhow::Error::new(std::io::Error::new( std::io::ErrorKind::PermissionDenied, diff --git a/crates/bsk-cli/src/daemon/start/windows.rs b/crates/bsk-cli/src/daemon/start/windows.rs index 1f66f5f90..fc0accde2 100644 --- a/crates/bsk-cli/src/daemon/start/windows.rs +++ b/crates/bsk-cli/src/daemon/start/windows.rs @@ -3,17 +3,19 @@ use std::ffi::{OsStr, OsString}; use std::fs::File; +use std::io; use std::os::windows::ffi::{OsStrExt, OsStringExt}; use std::path::Path; use anyhow::{Context, Result}; +use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED; use windows_sys::Win32::System::Threading::{ CREATE_BREAKAWAY_FROM_JOB, CREATE_NEW_PROCESS_GROUP, CREATE_SUSPENDED, DETACHED_PROCESS, }; use super::{DAEMON_REPLACEMENT_WAIT_ENV, DAEMONIZED_ENV, StartArgs, apply_start_args}; use crate::daemon::start_error::DaemonStartFailure; -use crate::windows_process::{self, Process}; +use crate::windows_process::{self, JobBreakaway, Process}; pub(super) fn spawn(exe: &Path, args: &StartArgs, predecessor_pid: Option) -> Result { let mut command = std::process::Command::new(exe); @@ -42,14 +44,48 @@ pub(super) fn spawn(exe: &Path, args: &StartArgs, predecessor_pid: Option) [&input, &output, &output], DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_BREAKAWAY_FROM_JOB | CREATE_SUSPENDED, ) - .context(DaemonStartFailure::IndependentStartFailed)?; - if let Err(err) = child.resume_outside_job() { - // A suspended child must never be left behind if validation/resume fails. - let _ = child.kill(); - let _ = child.wait(); - return Err(err).context(DaemonStartFailure::IndependentStartFailed); + .map_err(|err| spawn_failure(err, exe))?; + // A nested Job may allow only partial breakaway, so a successful + // CreateProcessW is not sufficient. + let failure = match child.outside_job() { + Ok(true) => match child.resume() { + Ok(()) => return Ok(child), + Err(err) => anyhow::Error::new(err).context(DaemonStartFailure::ResumeFailed), + }, + Ok(false) => { + anyhow::anyhow!("daemon process still belongs to a Job Object after breakaway") + .context(DaemonStartFailure::PartialBreakaway) + } + Err(err) => anyhow::Error::new(err).context("check the daemon's Job membership"), + }; + // A suspended child must never be left behind if validation/resume fails. + let _ = child.kill(); + let _ = child.wait(); + Err(failure) +} + +fn spawn_failure(err: io::Error, exe: &Path) -> anyhow::Error { + classify_spawn_failure(err, windows_process::current_job_breakaway().ok(), exe) +} + +/// With CREATE_BREAKAWAY_FROM_JOB, CreateProcessW fails with access denied +/// when the immediate Job forbids breakaway, but access to the executable or +/// process can be denied for other reasons. Only a Job policy that was read +/// as forbidding breakaway is reported as a Job refusal; `job` is `None` when +/// the policy could not be read. +fn classify_spawn_failure(err: io::Error, job: Option, exe: &Path) -> anyhow::Error { + let denied = err.raw_os_error() == Some(ERROR_ACCESS_DENIED as i32); + let err = anyhow::Error::new(err).context(format!("create daemon process {}", exe.display())); + if !denied { + return err; } - Ok(child) + err.context(match job { + Some(JobBreakaway::Forbidden) => DaemonStartFailure::IndependentStartFailed, + Some(JobBreakaway::Allowed | JobBreakaway::NotInJob) => { + DaemonStartFailure::ProcessCreationDenied + } + None => DaemonStartFailure::BreakawayUnverified, + }) } /// Whether [`spawn`] can start a daemon outside this process's Jobs. Creates @@ -70,15 +106,15 @@ pub(super) fn check_breakaway(exe: &Path) -> Result<()> { [&input, &output, &output], DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_BREAKAWAY_FROM_JOB | CREATE_SUSPENDED, ) - .context(DaemonStartFailure::IndependentStartFailed)?; + .map_err(|err| spawn_failure(err, exe))?; let outside = probe.outside_job(); let _ = probe.kill(); let _ = probe.wait(); if !outside.context("check the probe's Job membership")? { return Err(anyhow::anyhow!( - "breakaway from an outer Job is not allowed" + "daemon probe still belongs to a Job Object after breakaway" )) - .context(DaemonStartFailure::IndependentStartFailed); + .context(DaemonStartFailure::PartialBreakaway); } Ok(()) } @@ -112,3 +148,71 @@ pub(super) fn command_line<'a>(args: impl Iterator) -> OsStrin } OsString::from_wide(&result) } + +#[cfg(test)] +mod tests { + use windows_sys::Win32::Foundation::ERROR_FILE_NOT_FOUND; + + use super::*; + + const POLICIES: [Option; 4] = [ + Some(JobBreakaway::NotInJob), + Some(JobBreakaway::Allowed), + Some(JobBreakaway::Forbidden), + None, + ]; + + fn classified(code: u32, job: Option) -> anyhow::Error { + classify_spawn_failure( + io::Error::from_raw_os_error(code as i32), + job, + Path::new(r"C:\bsk\bsk.exe"), + ) + } + + fn assert_keeps_cause(error: &anyhow::Error, code: u32) { + let message = format!("{error:#}"); + assert!( + message.contains(r"create daemon process C:\bsk\bsk.exe"), + "{message}" + ); + assert!(message.contains(&format!("(os error {code})")), "{message}"); + } + + #[test] + fn access_denied_is_a_job_refusal_only_when_the_job_forbids_breakaway() { + for (job, expected) in [ + ( + Some(JobBreakaway::Forbidden), + DaemonStartFailure::IndependentStartFailed, + ), + ( + Some(JobBreakaway::Allowed), + DaemonStartFailure::ProcessCreationDenied, + ), + ( + Some(JobBreakaway::NotInJob), + DaemonStartFailure::ProcessCreationDenied, + ), + (None, DaemonStartFailure::BreakawayUnverified), + ] { + let error = classified(ERROR_ACCESS_DENIED, job); + assert_eq!( + error.downcast_ref::(), + Some(&expected), + "{job:?}" + ); + assert_keeps_cause(&error, ERROR_ACCESS_DENIED); + } + } + + #[test] + fn other_spawn_errors_keep_their_cause_without_job_guidance() { + for job in POLICIES { + let error = classified(ERROR_FILE_NOT_FOUND, job); + assert!(error.downcast_ref::().is_none()); + assert_keeps_cause(&error, ERROR_FILE_NOT_FOUND); + assert!(!format!("{error:#}").contains("Job Object"), "{job:?}"); + } + } +} diff --git a/crates/bsk-cli/src/daemon/start_error.rs b/crates/bsk-cli/src/daemon/start_error.rs index 12f3ec4d3..bb64d48d1 100644 --- a/crates/bsk-cli/src/daemon/start_error.rs +++ b/crates/bsk-cli/src/daemon/start_error.rs @@ -1,16 +1,52 @@ //! Recovery guidance for startup failures, preserved through anyhow contexts. -#[derive(Debug, Clone, Copy, thiserror::Error)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] pub(crate) enum DaemonStartFailure { #[error( "automatic daemon startup is disabled (BSK_AUTO_START=0); restore the daemon in its owning environment with its original configuration" )] AutoStartDisabled, + /// Process creation was denied and the immediate Job's policy was read as + /// forbidding breakaway. #[cfg(any(windows, test))] #[error( - "cannot start an independent Windows daemon; the host may prohibit Job Object breakaway; use `bsk daemon start --foreground` in a persistent host task or start from an independent terminal" + "cannot start an independent Windows daemon; the Job Object of this process does not allow breakaway; \ + use `bsk daemon start --foreground` in a persistent host task" )] IndependentStartFailed, + + /// Process creation was denied inside a Job whose policy could not be read. + #[cfg(any(windows, test))] + #[error( + "cannot start an independent Windows daemon; process creation was denied, either by a Job Object \ + that forbids breakaway or by access permissions on the executable or process; \ + check access to the bsk executable, or use `bsk daemon start --foreground` in a persistent host task" + )] + BreakawayUnverified, + + /// Process creation was denied although no Job policy forbids breakaway. + #[cfg(any(windows, test))] + #[error( + "cannot start an independent Windows daemon; process creation was denied although the Job Object \ + of this process, if any, allows breakaway; check access to the bsk executable" + )] + ProcessCreationDenied, + + /// The child was created, so the immediate Job allowed breakaway, but it + /// still belongs to a Job. Which Job holds it is not inspected. + #[cfg(any(windows, test))] + #[error( + "cannot start an independent Windows daemon; the daemon process was created but still belongs to \ + a Job Object after breakaway; use `bsk daemon start --foreground` in a persistent host task" + )] + PartialBreakaway, + + #[cfg(any(windows, test))] + #[error( + "cannot start an independent Windows daemon; the daemon process left every Job Object \ + but could not be resumed; retry `bsk daemon start`" + )] + ResumeFailed, } impl DaemonStartFailure { @@ -25,11 +61,48 @@ impl DaemonStartFailure { } #[cfg(any(windows, test))] Self::IndependentStartFailed => { - "Do not retry detached startup in this host. Run `bsk daemon start --foreground` \ - in a persistent host task that outlives client commands, or run `bsk daemon start` \ - from an independent terminal. Use the same BSK_HOME and OS user as the clients. \ - Then set BSK_AUTO_START=0 for client commands and verify `bsk status --json` \ - in a separate invocation." + "The Job Object of this process does not allow breakaway, so a detached daemon \ + cannot leave it. Do not retry detached startup in this host. Run \ + `bsk daemon start --foreground` in a persistent host task that outlives client \ + commands, or run `bsk daemon start` from an independent terminal that is not \ + inside this Job. Use the same BSK_HOME and OS user as the clients. Then set \ + BSK_AUTO_START=0 for client commands and verify `bsk status --json` in a \ + separate invocation." + } + #[cfg(any(windows, test))] + Self::BreakawayUnverified => { + "This process is in a Job Object whose breakaway policy could not be read, so \ + the cause is not confirmed: a Job restriction or denied access to the \ + executable or process. Check that this OS user can run the bsk executable named \ + above and that security software does not block it. If access is fine, run \ + `bsk daemon start --foreground` in a persistent host task that outlives client \ + commands, or run `bsk daemon start` from an independent terminal. Use the same \ + BSK_HOME and OS user as the clients. Then set BSK_AUTO_START=0 for client \ + commands and verify `bsk status --json` in a separate invocation." + } + #[cfg(any(windows, test))] + Self::ProcessCreationDenied => { + "Breakaway is permitted here, so a host Job restriction did not cause this. \ + Check that this OS user can run the bsk executable named above and that \ + security software does not block it, then retry `bsk daemon start`." + } + #[cfg(any(windows, test))] + Self::PartialBreakaway => { + "The daemon process was created, so the immediate Job Object allows breakaway, \ + but the process still belongs to a Job Object after breakaway, for example an \ + outer Job held by an IDE, a launcher or a system service. Run \ + `bsk daemon start --foreground` in a persistent host task that outlives client \ + commands. If another host or terminal does not run inside that Job, \ + `bsk daemon start` from there can succeed. Use the same BSK_HOME and OS user \ + as the clients. Then set BSK_AUTO_START=0 for client commands and verify \ + `bsk status --json` in a separate invocation." + } + #[cfg(any(windows, test))] + Self::ResumeFailed => { + "Job Object breakaway succeeded, so this is not a host Job restriction. \ + The suspended daemon process was terminated before it ran. \ + Retry `bsk daemon start`. If it keeps failing, check whether security \ + software is blocking bsk.exe." } } } diff --git a/crates/bsk-cli/src/windows_process.rs b/crates/bsk-cli/src/windows_process.rs index b9062ce07..dfe02dd66 100644 --- a/crates/bsk-cli/src/windows_process.rs +++ b/crates/bsk-cli/src/windows_process.rs @@ -14,7 +14,11 @@ use std::process::ExitStatus; use windows_sys::Win32::Foundation::{ DUPLICATE_SAME_ACCESS, DuplicateHandle, HANDLE, WAIT_OBJECT_0, WAIT_TIMEOUT, }; -use windows_sys::Win32::System::JobObjects::IsProcessInJob; +use windows_sys::Win32::System::JobObjects::{ + IsProcessInJob, JOB_OBJECT_LIMIT_BREAKAWAY_OK, JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK, + JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JobObjectExtendedLimitInformation, + QueryInformationJobObject, +}; use windows_sys::Win32::System::Threading::{ CREATE_UNICODE_ENVIRONMENT, CreateProcessW, DeleteProcThreadAttributeList, EXTENDED_STARTUPINFO_PRESENT, GetCurrentProcess, GetExitCodeProcess, GetProcessId, @@ -73,15 +77,8 @@ impl Process { Ok(!process_in_job(self.handle.as_raw_handle())?) } - /// Called only for a child created suspended. A nested Job may allow only - /// partial breakaway, so successful CreateProcessW is not sufficient. - pub(crate) fn resume_outside_job(&self) -> io::Result<()> { - if process_in_job(self.handle.as_raw_handle())? { - return Err(io::Error::new( - io::ErrorKind::PermissionDenied, - "daemon is still associated with a host Job Object after breakaway", - )); - } + /// Called only for a child created suspended. + pub(crate) fn resume(&self) -> io::Result<()> { // SAFETY: this is the primary thread returned by CreateProcessW. if unsafe { ResumeThread(self.thread.as_raw_handle()) } == u32::MAX { return Err(io::Error::last_os_error()); @@ -232,6 +229,44 @@ pub(crate) fn current_process_in_job() -> io::Result { process_in_job(unsafe { GetCurrentProcess() }) } +/// Breakaway policy of the current process's immediate Job. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum JobBreakaway { + NotInJob, + Allowed, + Forbidden, +} + +/// Only the immediate Job is visible here; outer Jobs of a nested chain are +/// not, so `Allowed` does not mean a child can leave every Job. +pub(crate) fn current_job_breakaway() -> io::Result { + if !current_process_in_job()? { + return Ok(JobBreakaway::NotInJob); + } + // SAFETY: the struct permits zero initialization. + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = unsafe { std::mem::zeroed() }; + // SAFETY: a null Job queries the immediate Job of the current process, and + // the buffer matches the requested information class. + if unsafe { + QueryInformationJobObject( + std::ptr::null_mut(), + JobObjectExtendedLimitInformation, + (&mut info as *mut JOBOBJECT_EXTENDED_LIMIT_INFORMATION).cast(), + std::mem::size_of_val(&info) as u32, + std::ptr::null_mut(), + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + let breakaway = JOB_OBJECT_LIMIT_BREAKAWAY_OK | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK; + Ok(if info.BasicLimitInformation.LimitFlags & breakaway != 0 { + JobBreakaway::Allowed + } else { + JobBreakaway::Forbidden + }) +} + fn process_in_job(process: HANDLE) -> io::Result { let mut in_job = 0; // SAFETY: a null job queries membership in any Job; the output is valid. diff --git a/crates/bsk-cli/tests/windows_daemon_start.rs b/crates/bsk-cli/tests/windows_daemon_start.rs index 3e6209431..1632bbd03 100644 --- a/crates/bsk-cli/tests/windows_daemon_start.rs +++ b/crates/bsk-cli/tests/windows_daemon_start.rs @@ -313,7 +313,10 @@ fn success(output: Output) { ); } -fn refused(output: Output) { +const JOB_FORBIDS_BREAKAWAY: &str = "does not allow breakaway"; +const PARTIAL_BREAKAWAY: &str = "still belongs to a Job Object after breakaway"; + +fn refused(output: Output, diagnosis: &str) { assert!(!output.status.success()); let text = format!( "{}{}", @@ -323,6 +326,12 @@ fn refused(output: Output) { assert!(text.contains("Job Object"), "{text}"); assert!(text.contains("persistent host task"), "{text}"); assert!(text.contains("BSK_AUTO_START=0"), "{text}"); + assert!(text.contains(diagnosis), "{text}"); + for other in [JOB_FORBIDS_BREAKAWAY, PARTIAL_BREAKAWAY] { + if other != diagnosis { + assert!(!text.contains(other), "{text}"); + } + } } // Re-exec the test binary as a gated launcher so assignment to a Job happens @@ -398,7 +407,7 @@ fn restrictive_job_refuses_explicit_and_automatic_start_without_a_daemon() { ] { let fixture = Fixture::new(); let job = Job::new(0); - refused(fixture.run(&args, &[&job])); + refused(fixture.run(&args, &[&job]), JOB_FORBIDS_BREAKAWAY); fixture.assert_stopped(); job.assert_empty(); } @@ -412,7 +421,7 @@ fn nested_jobs_require_breakaway_from_every_level() { let inner = Job::new(JOB_OBJECT_LIMIT_BREAKAWAY_OK); let output = fixture.run(&["daemon", "start", "--port", "0"], &[&outer, &inner]); if outer_flags == 0 { - refused(output); + refused(output, PARTIAL_BREAKAWAY); fixture.assert_stopped(); inner.assert_empty(); outer.assert_empty();