From 79340a5061a59938acb5efff2ee9e3ebc6e9fd9d Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 4 Dec 2025 16:37:30 +0300 Subject: [PATCH 01/38] treewide: refactor `--build-host` to use remote build semantics Fixes https://github.com/nix-community/nh/issues/428 This is a large architectural change to NH, which lead to me extracting the remote build logic to its own file so that we may implement it for Darwin and Home-Manager as well. The `--builders` flag was dropped from `nh::commands`, and it was replaced with the new and shiny logic that hopefully avoids previous pitfalls. The new `nh::remote` module handles remote builds, including: - Parsing remote host specifications. - Copying derivations to remote hosts using `nix-copy-closure`. - Building derivations on remote hosts via `nix build`. - Copying results back to localhost or directly to a target host. Signed-off-by: NotAShelf Change-Id: I236eb1e35dd645f2169462d207bc82e76a6a6964 --- src/commands.rs | 19 +- src/home.rs | 1 + src/interface.rs | 8 + src/lib.rs | 1 + src/main.rs | 1 + src/nixos.rs | 65 +++- src/remote.rs | 763 +++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 830 insertions(+), 28 deletions(-) create mode 100644 src/remote.rs diff --git a/src/commands.rs b/src/commands.rs index 78f0ef44..b402954a 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -709,7 +709,6 @@ pub struct Build { installable: Installable, extra_args: Vec, nom: bool, - builder: Option, } impl Build { @@ -720,7 +719,6 @@ impl Build { installable, extra_args: vec![], nom: false, - builder: None, } } @@ -742,12 +740,6 @@ impl Build { self } - #[must_use] - pub fn builder(mut self, builder: Option) -> Self { - self.builder = builder; - self - } - #[must_use] pub fn extra_args(mut self, args: I) -> Self where @@ -781,12 +773,6 @@ impl Build { let base_command = Exec::cmd("nix") .arg("build") .args(&installable_args) - .args(&match &self.builder { - Some(host) => { - vec!["--builders".to_string(), format!("ssh://{host} - - - 100")] - }, - None => vec![], - }) .args(&self.extra_args); let exit = if self.nom { @@ -1250,7 +1236,6 @@ mod tests { assert_eq!(build.installable.to_args(), installable.to_args()); assert!(build.extra_args.is_empty()); assert!(!build.nom); - assert!(build.builder.is_none()); } #[test] @@ -1264,8 +1249,7 @@ mod tests { .message("Building package") .extra_arg("--verbose") .extra_args(["--option", "setting", "value"]) - .nom(true) - .builder(Some("build-host".to_string())); + .nom(true); assert_eq!(build.message, Some("Building package".to_string())); assert_eq!(build.extra_args, vec![ @@ -1275,7 +1259,6 @@ mod tests { OsString::from("value") ]); assert!(build.nom); - assert_eq!(build.builder, Some("build-host".to_string())); } #[test] diff --git a/src/home.rs b/src/home.rs index 6211d619..a5bcaeb3 100644 --- a/src/home.rs +++ b/src/home.rs @@ -11,6 +11,7 @@ use crate::{ commands::Command, installable::Installable, interface::{self, DiffType, HomeRebuildArgs, HomeReplArgs, HomeSubcommand}, + remote::{self, RemoteBuildConfig, RemoteHost}, update::update, util::{get_hostname, print_dix_diff}, }; diff --git a/src/interface.rs b/src/interface.rs index f4c722c5..c93a0cef 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -543,6 +543,10 @@ pub struct HomeRebuildArgs { /// Show activation logs #[arg(long, env = "NH_SHOW_ACTIVATION_LOGS")] pub show_activation_logs: bool, + + /// Build the configuration on a different host over ssh + #[arg(long)] + pub build_host: Option, } impl HomeRebuildArgs { @@ -649,6 +653,10 @@ pub struct DarwinRebuildArgs { /// Show activation logs #[arg(long, env = "NH_SHOW_ACTIVATION_LOGS")] pub show_activation_logs: bool, + + /// Build the configuration on a different host over ssh + #[arg(long)] + pub build_host: Option, } impl DarwinRebuildArgs { diff --git a/src/lib.rs b/src/lib.rs index 8d2ad92e..7ecac771 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,6 +10,7 @@ pub mod interface; pub mod json; pub mod logging; pub mod nixos; +pub mod remote; pub mod search; pub mod update; pub mod util; diff --git a/src/main.rs b/src/main.rs index 5e442d09..3b076616 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,6 +9,7 @@ mod interface; mod json; mod logging; mod nixos; +mod remote; mod search; mod update; mod util; diff --git a/src/nixos.rs b/src/nixos.rs index 0fc627af..4c0660cc 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -22,6 +22,7 @@ use crate::{ OsRollbackArgs, OsSubcommand::{self}, }, + remote::{self, RemoteBuildConfig, RemoteHost}, update::update, util::{ensure_ssh_key_login, get_hostname, print_dix_diff}, }; @@ -350,16 +351,60 @@ impl OsRebuildArgs { out_path: &Path, message: &str, ) -> Result<()> { - commands::Build::new(toplevel) - .extra_arg("--out-link") - .extra_arg(out_path) - .extra_args(&self.extra_args) - .passthrough(&self.common.passthrough) - .builder(self.build_host.clone()) - .message(message) - .nom(!self.common.no_nom) - .run() - .wrap_err("Failed to build configuration") + // If a build host is specified, use proper remote build semantics + // This matches nixos-rebuild --build-host behavior: + // 1. Evaluate derivation locally + // 2. Copy derivation to build host (user-initiated SSH) + // 3. Build on remote host + // 4. Copy result back (to localhost or target_host) + if let Some(ref build_host_str) = self.build_host { + info!("{}", message); + + let build_host = RemoteHost::parse(build_host_str) + .wrap_err("Invalid build host specification")?; + + let target_host = self + .target_host + .as_ref() + .map(|s| RemoteHost::parse(s)) + .transpose() + .wrap_err("Invalid target host specification")?; + + let config = RemoteBuildConfig { + build_host, + target_host, + use_nom: !self.common.no_nom, + use_substitutes: self.common.passthrough.use_substitutes, + extra_args: self + .extra_args + .iter() + .map(|s| s.into()) + .chain( + self + .common + .passthrough + .generate_passthrough_args() + .into_iter() + .map(|s| s.into()), + ) + .collect(), + }; + + remote::build_remote(&toplevel, &config, Some(out_path))?; + + Ok(()) + } else { + // Local build - use the existing path + commands::Build::new(toplevel) + .extra_arg("--out-link") + .extra_arg(out_path) + .extra_args(&self.extra_args) + .passthrough(&self.common.passthrough) + .message(message) + .nom(!self.common.no_nom) + .run() + .wrap_err("Failed to build configuration") + } } fn resolve_specialisation_and_profile( diff --git a/src/remote.rs b/src/remote.rs new file mode 100644 index 00000000..143d6ae8 --- /dev/null +++ b/src/remote.rs @@ -0,0 +1,763 @@ +use std::{env, ffi::OsString, path::PathBuf, sync::OnceLock}; + +use color_eyre::{ + Result, + eyre::{Context, bail, eyre}, +}; +use subprocess::{Exec, ExitStatus, Redirection}; +use tracing::{debug, info}; + +use crate::{installable::Installable, util::NixVariant}; + +/// Cache for the SSH control socket directory. +static SSH_CONTROL_DIR: OnceLock = OnceLock::new(); + +/// Get or create the SSH control socket directory. +/// This creates a temporary directory that persists for the lifetime of the +/// program, similar to nixos-rebuild-ng's tmpdir module. +fn get_ssh_control_dir() -> &'static PathBuf { + SSH_CONTROL_DIR.get_or_init(|| { + // Try to use XDG_RUNTIME_DIR first (usually /run/user/), fall back to + // /tmp + // XXX: I do not want to use the dirs crate just for this. + let base = env::var("XDG_RUNTIME_DIR") + .map_or_else(|_| PathBuf::from("/tmp"), PathBuf::from); + + let control_dir = base.join(format!("nh-ssh-{}", std::process::id())); + + // Create the directory if it doesn't exist + if let Err(e) = std::fs::create_dir_all(&control_dir) { + debug!("Failed to create SSH control directory: {e}"); + // Fall back to /tmp if we can't create the directory + return PathBuf::from("/tmp"); + } + + control_dir + }) +} + +/// A parsed remote host specification. +/// +/// Handles various formats: +/// - `hostname` +/// - `user@hostname` +/// - `ssh://[user@]hostname` (scheme stripped) +/// - `ssh-ng://[user@]hostname` (scheme stripped) +#[derive(Debug, Clone)] +pub struct RemoteHost { + /// The host string (may include user@) + host: String, +} + +impl RemoteHost { + /// Parse a host specification string. + /// + /// Accepts: + /// - `hostname` + /// - `user@hostname` + /// - `ssh://[user@]hostname` + /// - `ssh-ng://[user@]hostname` + /// + /// URI schemes are stripped since `--build-host` uses direct SSH. + /// + /// # Errors + /// + /// Returns an error if the host specification is invalid (empty hostname, + /// empty username, contains invalid characters like `:` or `/`). + pub fn parse(input: &str) -> Result { + // Strip URI schemes - we use direct SSH regardless + let host = input + .strip_prefix("ssh-ng://") + .or_else(|| input.strip_prefix("ssh://")) + .unwrap_or(input); + + if host.is_empty() { + bail!("Empty hostname in host specification"); + } + + // Validate: check for empty user in user@host format + if host.starts_with('@') { + bail!("Empty username in host specification: {input}"); + } + if host.ends_with('@') { + bail!("Empty hostname in host specification: {input}"); + } + + // Validate hostname doesn't contain invalid characters + // (after stripping any user@ prefix for the check) + let hostname_part = host.rsplit('@').next().unwrap_or(host); + if hostname_part.contains('/') { + bail!( + "Invalid hostname '{}': contains '/'. Did you mean to use a bare \ + hostname?", + hostname_part + ); + } + if hostname_part.contains(':') { + bail!( + "Invalid hostname '{}': contains ':'. Ports should be specified via \ + NIX_SSHOPTS=\"-p 2222\" or ~/.ssh/config", + hostname_part + ); + } + + Ok(Self { + host: host.to_string(), + }) + } + + /// Get the host string for use with nix-copy-closure and SSH. + #[must_use] + pub fn host(&self) -> &str { + &self.host + } +} + +impl std::fmt::Display for RemoteHost { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.host) + } +} + +/// Get the default SSH options for connection multiplexing. +/// Includes a `ControlPath` pointing to our control socket directory. +fn get_default_ssh_opts() -> Vec { + let control_dir = get_ssh_control_dir(); + let control_path = control_dir.join("ssh-%n"); + + vec![ + "-o".to_string(), + "ControlMaster=auto".to_string(), + "-o".to_string(), + format!("ControlPath={}", control_path.display()), + "-o".to_string(), + "ControlPersist=60".to_string(), + ] +} + +/// Shell-quote a string for safe use in SSH commands. +// FIXME: this is handrolled so that I can confirm whether we match +// nixos-rebuild-ng's use of shlex.quote, but we'll want to introduce shlex as a +// dependency and drop this. This is a hard blocker. +/// +fn shell_quote(s: &str) -> String { + // shlex.quote in Python returns the string unchanged if it contains only + // safe characters, otherwise wraps in single quotes with escaping + if s.is_empty() { + return "''".to_string(); + } + if s + .chars() + .all(|c| c.is_ascii_alphanumeric() || "-_./+:=@^".contains(c)) + { + return s.to_string(); + } + // Escape single quotes and wrap in single quotes + let escaped = s.replace('\'', "'\"'\"'"); + format!("'{escaped}'") +} + +/// Get SSH options from `NIX_SSHOPTS` plus our defaults. +fn get_ssh_opts() -> Vec { + let mut opts: Vec = Vec::new(); + + // User options first (from NIX_SSHOPTS) + if let Ok(sshopts) = env::var("NIX_SSHOPTS") { + for opt in sshopts.split_whitespace() { + opts.push(opt.to_string()); + } + } + + // Then our defaults (including ControlPath) + opts.extend(get_default_ssh_opts()); + + opts +} + +/// Get `NIX_SSHOPTS` environment value with our defaults appended. +/// Used for `nix-copy-closure` which reads `NIX_SSHOPTS`. +fn get_nix_sshopts_env() -> String { + let sshopts = env::var("NIX_SSHOPTS").unwrap_or_default(); + let defaults = get_default_ssh_opts().join(" "); + if sshopts.is_empty() { + defaults + } else { + format!("{sshopts} {defaults}") + } +} + +/// Get the flake experimental feature flags required for `nix` commands. +/// +/// Returns the flags needed for `--extra-experimental-features "nix-command +/// flakes"` based on the detected Nix variant: +/// - Determinate Nix: No flags needed (features are stable) +/// - Nix/Lix: Returns `["--extra-experimental-features", "nix-command flakes"]` +fn get_flake_flags() -> Vec<&'static str> { + let variant = crate::util::get_nix_variant(); + match variant { + NixVariant::Determinate => vec![], + NixVariant::Nix | NixVariant::Lix => { + vec!["--extra-experimental-features", "nix-command flakes"] + }, + } +} + +/// Run a command on a remote host via SSH. +fn run_remote_command( + host: &RemoteHost, + args: &[&str], + capture_stdout: bool, +) -> Result> { + let ssh_opts = get_ssh_opts(); + + // Quote args for shell (matching nixos-rebuild-ng's shlex.quote) + let quoted_args: Vec = args.iter().map(|a| shell_quote(a)).collect(); + + debug!( + "Running remote command on {}: {}", + host, + quoted_args.join(" ") + ); + + let mut cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + cmd = cmd.arg(opt); + } + cmd = cmd.arg(host.host()).arg("--"); + for arg in "ed_args { + cmd = cmd.arg(arg); + } + + if capture_stdout { + cmd = cmd.stdout(Redirection::Pipe).stderr(Redirection::Pipe); + } + + let capture = cmd.capture().wrap_err_with(|| { + format!("Failed to execute command on remote host '{host}'") + })?; + + if !capture.exit_status.success() { + let stderr = capture.stderr_str(); + bail!( + "Remote command failed on '{}' (exit {:?}):\n{}", + host, + capture.exit_status, + stderr + ); + } + + if capture_stdout { + Ok(Some(capture.stdout_str().trim().to_string())) + } else { + Ok(None) + } +} + +/// Copy a Nix closure to a remote host. +fn copy_closure_to( + host: &RemoteHost, + path: &str, + use_substitutes: bool, +) -> Result<()> { + info!("Copying closure to build host '{}'", host); + + let mut cmd = Exec::cmd("nix-copy-closure").arg("--to").arg(host.host()); + + if use_substitutes { + cmd = cmd.arg("--use-substitutes"); + } + + cmd = cmd.arg(path).env("NIX_SSHOPTS", get_nix_sshopts_env()); + + debug!(?cmd, "nix-copy-closure --to"); + + let capture = cmd + .capture() + .wrap_err("Failed to copy closure to remote host")?; + + if !capture.exit_status.success() { + bail!( + "nix-copy-closure --to '{}' failed:\n{}", + host, + capture.stderr_str() + ); + } + + Ok(()) +} + +/// Copy a Nix closure from a remote host to localhost. +fn copy_closure_from( + host: &RemoteHost, + path: &str, + use_substitutes: bool, +) -> Result<()> { + info!("Copying result from build host '{}'", host); + + let mut cmd = Exec::cmd("nix-copy-closure").arg("--from").arg(host.host()); + + if use_substitutes { + cmd = cmd.arg("--use-substitutes"); + } + + cmd = cmd.arg(path).env("NIX_SSHOPTS", get_nix_sshopts_env()); + + debug!(?cmd, "nix-copy-closure --from"); + + let capture = cmd + .capture() + .wrap_err("Failed to copy closure from remote host")?; + + if !capture.exit_status.success() { + bail!( + "nix-copy-closure --from '{}' failed:\n{}", + host, + capture.stderr_str() + ); + } + + Ok(()) +} + +/// Copy a Nix closure from one remote host to another. +/// Uses `nix copy --from ssh://source --to ssh://dest`. +fn copy_closure_between_remotes( + from_host: &RemoteHost, + to_host: &RemoteHost, + path: &str, + use_substitutes: bool, +) -> Result<()> { + info!("Copying closure from '{}' to '{}'", from_host, to_host); + + let flake_flags = get_flake_flags(); + let mut cmd = Exec::cmd("nix") + .args(&flake_flags) + .args(&["copy", "--from"]) + .arg(format!("ssh://{}", from_host.host())) + .arg("--to") + .arg(format!("ssh://{}", to_host.host())); + + if use_substitutes { + cmd = cmd.arg("--substitute-on-destination"); + } + + cmd = cmd.arg(path).env("NIX_SSHOPTS", get_nix_sshopts_env()); + + debug!(?cmd, "nix copy between remotes"); + + let capture = cmd + .capture() + .wrap_err("Failed to copy closure between remote hosts")?; + + if !capture.exit_status.success() { + bail!( + "nix copy from '{}' to '{}' failed:\n{}", + from_host, + to_host, + capture.stderr_str() + ); + } + + Ok(()) +} + +/// Evaluate a flake installable to get its derivation path. +/// Matches nixos-rebuild-ng: `nix eval --raw .drvPath` +fn eval_drv_path(installable: &Installable) -> Result { + // Build the installable with .drvPath appended + let drv_installable = match installable { + Installable::Flake { + reference, + attribute, + } => { + let mut drv_attr = attribute.clone(); + drv_attr.push("drvPath".to_string()); + Installable::Flake { + reference: reference.clone(), + attribute: drv_attr, + } + }, + Installable::File { path, attribute } => { + let mut drv_attr = attribute.clone(); + drv_attr.push("drvPath".to_string()); + Installable::File { + path: path.clone(), + attribute: drv_attr, + } + }, + Installable::Expression { + expression, + attribute, + } => { + let mut drv_attr = attribute.clone(); + drv_attr.push("drvPath".to_string()); + Installable::Expression { + expression: expression.clone(), + attribute: drv_attr, + } + }, + Installable::Store { path } => { + bail!( + "Cannot perform remote build with store path '{}'. Store paths are \ + already built.", + path.display() + ); + }, + Installable::Unspecified => { + bail!("Cannot evaluate unspecified installable"); + }, + }; + + let args = drv_installable.to_args(); + debug!("Evaluating drvPath: nix eval --raw {:?}", args); + + let flake_flags = get_flake_flags(); + let cmd = Exec::cmd("nix") + .args(&flake_flags) + .arg("eval") + .arg("--raw") + .args(&args) + .stdout(Redirection::Pipe) + .stderr(Redirection::Pipe); + + let capture = cmd.capture().wrap_err("Failed to run nix eval")?; + + if !capture.exit_status.success() { + bail!( + "Failed to evaluate derivation path:\n{}", + capture.stderr_str() + ); + } + + let drv_path = capture.stdout_str().trim().to_string(); + if drv_path.is_empty() { + bail!("nix eval returned empty derivation path"); + } + + debug!("Derivation path: {}", drv_path); + Ok(drv_path) +} + +/// Configuration for a remote build operation. +#[derive(Debug, Clone)] +pub struct RemoteBuildConfig { + /// The host to build on + pub build_host: RemoteHost, + + /// Optional target host to copy the result to (instead of localhost). + /// When set, copies directly from `build_host` to `target_host`. + pub target_host: Option, + + /// Whether to use nix-output-monitor for build output + pub use_nom: bool, + + /// Whether to use substitutes when copying closures + pub use_substitutes: bool, + + /// Extra arguments to pass to the build command + pub extra_args: Vec, +} + +/// Perform a remote build of a flake installable. +/// +/// This implements the `build_remote_flake` workflow from nixos-rebuild-ng: +/// 1. Evaluate drvPath locally via `nix eval --raw` +/// 2. Copy the derivation to the build host via `nix-copy-closure` +/// 3. Build on remote via `nix build ^* --print-out-paths` +/// 4. Copy the result back (to localhost or `target_host`) +/// +/// Returns the output path in the Nix store. +/// +/// # Errors +/// +/// Returns an error if any step fails (evaluation, copy, build). +pub fn build_remote( + installable: &Installable, + config: &RemoteBuildConfig, + out_link: Option<&std::path::Path>, +) -> Result { + let build_host = &config.build_host; + let use_substitutes = config.use_substitutes; + + // Step 1: Evaluate drvPath locally + info!("Evaluating derivation path"); + let drv_path = eval_drv_path(installable)?; + + // Step 2: Copy derivation to build host + copy_closure_to(build_host, &drv_path, use_substitutes)?; + + // Step 3: Build on remote + info!("Building on remote host '{}'", build_host); + let out_path = build_on_remote(build_host, &drv_path, config)?; + + // Step 4: Copy result to destination + // If target_host is set, copy directly from build_host to target_host. + // Otherwise, copy back to localhost. + if let Some(ref target_host) = config.target_host { + copy_closure_between_remotes( + build_host, + target_host, + &out_path, + use_substitutes, + )?; + } else { + copy_closure_from(build_host, &out_path, use_substitutes)?; + + // Create local out-link if requested (only when copying to localhost) + if let Some(link) = out_link { + debug!("Creating out-link: {} -> {}", link.display(), out_path); + // Remove existing symlink/file if present + let _ = std::fs::remove_file(link); + std::os::unix::fs::symlink(&out_path, link) + .wrap_err("Failed to create out-link")?; + } + } + + Ok(PathBuf::from(out_path)) +} + +/// Build a derivation on a remote host. +/// Returns the output path. +fn build_on_remote( + host: &RemoteHost, + drv_path: &str, + config: &RemoteBuildConfig, +) -> Result { + // Build command: nix build ^* --print-out-paths [extra_args...] + let drv_with_outputs = format!("{drv_path}^*"); + + if config.use_nom { + // With nom: pipe through nix-output-monitor + build_on_remote_with_nom(host, &drv_with_outputs, config) + } else { + // Without nom: simple remote execution + build_on_remote_simple(host, &drv_with_outputs, config) + } +} + +/// Build on remote without nom - just capture output. +fn build_on_remote_simple( + host: &RemoteHost, + drv_with_outputs: &str, + config: &RemoteBuildConfig, +) -> Result { + // Get flake flags for the remote nix command + let flake_flags = get_flake_flags(); + + let mut args: Vec<&str> = vec!["nix"]; + args.extend(flake_flags); + args.extend(["build", drv_with_outputs, "--print-out-paths"]); + + // Collect extra args that are valid strings + let extra_args_strings: Vec = config + .extra_args + .iter() + .filter_map(|s| s.to_str().map(String::from)) + .collect(); + for arg in &extra_args_strings { + args.push(arg); + } + + let result = run_remote_command(host, &args, true)? + .ok_or_else(|| eyre!("Remote build returned no output"))?; + + // --print-out-paths may return multiple lines; take first + let out_path = result + .lines() + .next() + .ok_or_else(|| eyre!("Remote build returned empty output"))? + .trim() + .to_string(); + + debug!("Remote build output: {}", out_path); + Ok(out_path) +} + +/// Build on remote with nom - pipe through nix-output-monitor. +fn build_on_remote_with_nom( + host: &RemoteHost, + drv_with_outputs: &str, + config: &RemoteBuildConfig, +) -> Result { + let ssh_opts = get_ssh_opts(); + let flake_flags = get_flake_flags(); + + // Build the remote command with JSON output for nom + let mut remote_args: Vec<&str> = vec!["nix"]; + remote_args.extend(flake_flags.iter().copied()); + remote_args.extend([ + "build", + drv_with_outputs, + "--log-format", + "internal-json", + "--verbose", + ]); + + let extra_args_strings: Vec = config + .extra_args + .iter() + .filter_map(|s| s.to_str().map(String::from)) + .collect(); + for arg in &extra_args_strings { + remote_args.push(arg); + } + + // Quote for shell + let quoted_remote: Vec = + remote_args.iter().map(|a| shell_quote(a)).collect(); + + // Build SSH command + let mut ssh_cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + ssh_cmd = ssh_cmd.arg(opt); + } + ssh_cmd = ssh_cmd + .arg(host.host()) + .arg("--") + .args("ed_remote) + .stdout(Redirection::Pipe) + .stderr(Redirection::Merge); + + // Pipe through nom + let nom_cmd = Exec::cmd("nom").arg("--json"); + let pipeline = (ssh_cmd | nom_cmd).stdout(Redirection::None); + + debug!(?pipeline, "Running remote build with nom"); + + let exit = pipeline.join().wrap_err("Remote build with nom failed")?; + + match exit { + ExitStatus::Exited(0) => {}, + other => bail!("Remote build failed with exit status: {other:?}"), + } + + // nom consumed the output, so we need to query the output path separately + // Run nix build again with --print-out-paths (it will be a no-op since + // already built) + let mut query_args: Vec<&str> = vec!["nix"]; + query_args.extend(flake_flags.iter().copied()); + query_args.extend(["build", drv_with_outputs, "--print-out-paths"]); + + let result = run_remote_command(host, &query_args, true)? + .ok_or_else(|| eyre!("Failed to get output path after build"))?; + + let out_path = result + .lines() + .next() + .ok_or_else(|| eyre!("Output path query returned empty"))? + .trim() + .to_string(); + + debug!("Remote build output: {}", out_path); + Ok(out_path) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_parse_bare_hostname() { + let host = RemoteHost::parse("buildserver").expect("should parse"); + assert_eq!(host.host(), "buildserver"); + } + + #[test] + fn test_parse_user_at_hostname() { + let host = RemoteHost::parse("root@buildserver").expect("should parse"); + assert_eq!(host.host(), "root@buildserver"); + } + + #[test] + fn test_parse_ssh_uri_stripped() { + let host = RemoteHost::parse("ssh://buildserver").expect("should parse"); + assert_eq!(host.host(), "buildserver"); + } + + #[test] + fn test_parse_ssh_ng_uri_stripped() { + let host = RemoteHost::parse("ssh-ng://buildserver").expect("should parse"); + assert_eq!(host.host(), "buildserver"); + } + + #[test] + fn test_parse_ssh_uri_with_user() { + let host = + RemoteHost::parse("ssh://root@buildserver").expect("should parse"); + assert_eq!(host.host(), "root@buildserver"); + } + + #[test] + fn test_parse_ssh_ng_uri_with_user() { + let host = + RemoteHost::parse("ssh-ng://admin@buildserver").expect("should parse"); + assert_eq!(host.host(), "admin@buildserver"); + } + + #[test] + fn test_parse_empty_fails() { + assert!(RemoteHost::parse("").is_err()); + } + + #[test] + fn test_parse_empty_user_fails() { + assert!(RemoteHost::parse("@hostname").is_err()); + } + + #[test] + fn test_parse_empty_hostname_fails() { + assert!(RemoteHost::parse("user@").is_err()); + } + + #[test] + fn test_parse_port_rejected() { + let Err(err) = RemoteHost::parse("hostname:22") else { + panic!("expected error for port in hostname"); + }; + assert!(err.to_string().contains("NIX_SSHOPTS")); + } + + #[test] + fn test_shell_quote_simple() { + assert_eq!(shell_quote("simple"), "simple"); + assert_eq!( + shell_quote("/nix/store/abc123-foo"), + "/nix/store/abc123-foo" + ); + } + + #[test] + fn test_shell_quote_with_caret() { + // drv^* syntax must work + assert_eq!( + shell_quote("/nix/store/xyz.drv^*"), + "'/nix/store/xyz.drv^*'" + ); + } + + #[test] + fn test_shell_quote_special_chars() { + assert_eq!(shell_quote("has space"), "'has space'"); + assert_eq!(shell_quote("has'quote"), "'has'\"'\"'quote'"); + assert_eq!(shell_quote("$(dangerous)"), "'$(dangerous)'"); + } + + #[test] + fn test_shell_quote_empty() { + assert_eq!(shell_quote(""), "''"); + } + + #[test] + fn test_get_ssh_opts_default() { + // Clear env var for test + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + let opts = get_ssh_opts(); + assert!(opts.contains(&"-o".to_string())); + assert!(opts.contains(&"ControlMaster=auto".to_string())); + assert!(opts.contains(&"ControlPersist=60".to_string())); + // Check that ControlPath is present (the exact path varies) + assert!(opts.iter().any(|o| o.starts_with("ControlPath="))); + } +} From aef1be1b09e3d8852497c0e1248e007b17f29061 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 4 Dec 2025 18:07:49 +0300 Subject: [PATCH 02/38] various: implement missing `--build-host` flags for Home and Darwin Signed-off-by: NotAShelf Change-Id: I946d8e54261e9136c83f6dfe38b046106a6a6964 --- src/darwin.rs | 50 +++++++++++++++++++++++++++++++++++++++++--------- src/home.rs | 49 ++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 81 insertions(+), 18 deletions(-) diff --git a/src/darwin.rs b/src/darwin.rs index 83fd9414..b4941028 100644 --- a/src/darwin.rs +++ b/src/darwin.rs @@ -15,6 +15,7 @@ use crate::{ DarwinSubcommand, DiffType, }, + remote::{self, RemoteBuildConfig, RemoteHost}, update::update, util::{get_hostname, print_dix_diff}, }; @@ -108,15 +109,46 @@ impl DarwinRebuildArgs { let toplevel = toplevel_for(hostname, installable, "toplevel")?; - commands::Build::new(toplevel) - .extra_arg("--out-link") - .extra_arg(&out_path) - .extra_args(&self.extra_args) - .passthrough(&self.common.passthrough) - .message("Building Darwin configuration") - .nom(!self.common.no_nom) - .run() - .wrap_err("Failed to build Darwin configuration")?; + // If a build host is specified, use remote build semantics + if let Some(ref build_host_str) = self.build_host { + info!("Building Darwin configuration"); + + let build_host = RemoteHost::parse(build_host_str) + .wrap_err("Invalid build host specification")?; + + let config = RemoteBuildConfig { + build_host, + target_host: None, + use_nom: !self.common.no_nom, + use_substitutes: self.common.passthrough.use_substitutes, + extra_args: self + .extra_args + .iter() + .map(|s| s.into()) + .chain( + self + .common + .passthrough + .generate_passthrough_args() + .into_iter() + .map(|s| s.into()), + ) + .collect(), + }; + + remote::build_remote(&toplevel, &config, Some(&out_path)) + .wrap_err("Failed to build Darwin configuration")?; + } else { + commands::Build::new(toplevel) + .extra_arg("--out-link") + .extra_arg(&out_path) + .extra_args(&self.extra_args) + .passthrough(&self.common.passthrough) + .message("Building Darwin configuration") + .nom(!self.common.no_nom) + .run() + .wrap_err("Failed to build Darwin configuration")?; + } let target_profile = out_path.clone(); diff --git a/src/home.rs b/src/home.rs index a5bcaeb3..f9e2c029 100644 --- a/src/home.rs +++ b/src/home.rs @@ -96,15 +96,46 @@ impl HomeRebuildArgs { self.configuration.clone(), )?; - commands::Build::new(toplevel) - .extra_arg("--out-link") - .extra_arg(&out_path) - .extra_args(&self.extra_args) - .passthrough(&self.common.passthrough) - .message("Building Home-Manager configuration") - .nom(!self.common.no_nom) - .run() - .wrap_err("Failed to build Home-Manager configuration")?; + // If a build host is specified, use remote build semantics + if let Some(ref build_host_str) = self.build_host { + info!("Building Home-Manager configuration"); + + let build_host = RemoteHost::parse(build_host_str) + .wrap_err("Invalid build host specification")?; + + let config = RemoteBuildConfig { + build_host, + target_host: None, + use_nom: !self.common.no_nom, + use_substitutes: self.common.passthrough.use_substitutes, + extra_args: self + .extra_args + .iter() + .map(|s| s.into()) + .chain( + self + .common + .passthrough + .generate_passthrough_args() + .into_iter() + .map(|s| s.into()), + ) + .collect(), + }; + + remote::build_remote(&toplevel, &config, Some(&out_path)) + .wrap_err("Failed to build Home-Manager configuration")?; + } else { + commands::Build::new(toplevel) + .extra_arg("--out-link") + .extra_arg(&out_path) + .extra_args(&self.extra_args) + .passthrough(&self.common.passthrough) + .message("Building Home-Manager configuration") + .nom(!self.common.no_nom) + .run() + .wrap_err("Failed to build Home-Manager configuration")?; + } let prev_generation: Option = [ PathBuf::from("/nix/var/nix/profiles/per-user") From 28f047f7a75910f063f9ae405233687bc8e7887e Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 4 Dec 2025 18:15:58 +0300 Subject: [PATCH 03/38] various: defer shell splitting to shlex Signed-off-by: NotAShelf Change-Id: I0b82e84223c3df61cfa23464bd3d4bcc6a6a6964 --- Cargo.lock | 1 + Cargo.toml | 1 + src/commands.rs | 104 +++++++++++------- src/remote.rs | 277 ++++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 312 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d3d8f124..b2c9f481 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1429,6 +1429,7 @@ dependencies = [ "serde", "serde_json", "serial_test", + "shlex", "subprocess", "supports-hyperlinks", "system-configuration", diff --git a/Cargo.toml b/Cargo.toml index 2c6cdf05..78ef597a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,6 +47,7 @@ secrecy = { features = [ "serde" ], version = "0.10.3" } semver = "1.0.27" serde = { features = [ "derive" ], version = "1.0.228" } serde_json = "1.0.145" +shlex = "1.3.0" subprocess = "0.2.9" supports-hyperlinks = "3.1.0" tempfile = "3.23.0" diff --git a/src/commands.rs b/src/commands.rs index b402954a..6b753245 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -42,43 +42,7 @@ fn cache_password(host: &str, password: SecretString) { /// single or double quoted strings. Quote characters are removed from /// the resulting tokens. fn parse_cmdline_with_quotes(cmdline: &str) -> Vec { - let mut parts = Vec::default(); - let mut current = String::new(); - let mut quoted = None; - - for c in cmdline.chars() { - match c { - // Opening quote - enter quoted mode - '\'' | '"' if quoted.is_none() => { - quoted = Some(c); - }, - // Closing quote - exit quoted mode - '\'' | '"' if quoted.is_some_and(|q| q == c) => { - quoted = None; - }, - // Different quote type while already quoted - treat as literal - '\'' | '"' => { - current.push(c); - }, - // Whitespace outside quotes - end of current token - s if s.is_whitespace() && quoted.is_none() => { - if !current.is_empty() { - parts.push(current.clone()); - current.clear(); - } - }, - // Any char, add to current token - _ => { - current.push(c); - }, - } - } - - if !current.is_empty() { - parts.push(current); - } - - parts + shlex::split(cmdline).unwrap_or_default() } fn ssh_wrap( @@ -1441,4 +1405,70 @@ mod tests { _ => unreachable!("Clone should preserve variant and value"), } } + + #[test] + fn test_parse_cmdline_escaped_quotes() { + // shlex handles backslash escapes within double quotes + let result = + parse_cmdline_with_quotes(r#"cmd "arg with \"escaped\" quotes""#); + assert_eq!(result, vec!["cmd", r#"arg with "escaped" quotes"#]); + } + + #[test] + fn test_parse_cmdline_nested_quotes() { + // Single quotes inside double quotes are preserved literally + let result = parse_cmdline_with_quotes(r#"cmd "it's a test""#); + assert_eq!(result, vec!["cmd", "it's a test"]); + } + + #[test] + fn test_parse_cmdline_backslash_outside_quotes() { + // Backslash escapes space outside quotes + let result = parse_cmdline_with_quotes(r"cmd arg\ with\ space"); + assert_eq!(result, vec!["cmd", "arg with space"]); + } + + #[test] + fn test_parse_cmdline_nix_store_paths() { + // Typical nix store paths should work + let result = parse_cmdline_with_quotes( + "/nix/store/abc123-foo/bin/cmd --flag /nix/store/def456-bar", + ); + assert_eq!(result, vec![ + "/nix/store/abc123-foo/bin/cmd", + "--flag", + "/nix/store/def456-bar" + ]); + } + + #[test] + fn test_parse_cmdline_env_vars_in_quotes() { + // Environment variable syntax should be preserved + let result = parse_cmdline_with_quotes(r#"env "PATH=$HOME/bin:$PATH" cmd"#); + assert_eq!(result, vec!["env", "PATH=$HOME/bin:$PATH", "cmd"]); + } + + #[test] + fn test_parse_cmdline_unclosed_quote_returns_none() { + // shlex returns None for unclosed quotes, we return empty vec + let result = parse_cmdline_with_quotes("cmd 'unclosed"); + assert_eq!(result, Vec::::default()); + } + + #[test] + fn test_parse_cmdline_complex_sudo_command() { + // Complex sudo command with multiple quoted args + let cmdline = r#"/usr/bin/sudo -E env 'HOME=/root' "PATH=/usr/bin" /usr/bin/nh os switch"#; + let result = parse_cmdline_with_quotes(cmdline); + assert_eq!(result, vec![ + "/usr/bin/sudo", + "-E", + "env", + "HOME=/root", + "PATH=/usr/bin", + "/usr/bin/nh", + "os", + "switch" + ]); + } } diff --git a/src/remote.rs b/src/remote.rs index 143d6ae8..324b6dcc 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -27,9 +27,20 @@ fn get_ssh_control_dir() -> &'static PathBuf { // Create the directory if it doesn't exist if let Err(e) = std::fs::create_dir_all(&control_dir) { - debug!("Failed to create SSH control directory: {e}"); - // Fall back to /tmp if we can't create the directory - return PathBuf::from("/tmp"); + debug!( + "Failed to create SSH control directory at {}: {e}", + control_dir.display() + ); + // Fall back to /tmp/nh-ssh- - try creating there instead + let fallback_dir = + PathBuf::from("/tmp").join(format!("nh-ssh-{}", std::process::id())); + if let Err(e2) = std::fs::create_dir_all(&fallback_dir) { + // Last resort: use /tmp directly (socket will be /tmp/ssh-%n) + // This is not ideal but better than failing entirely + debug!("Failed to create fallback SSH control directory: {e2}"); + return PathBuf::from("/tmp"); + } + return fallback_dir; } control_dir @@ -136,25 +147,11 @@ fn get_default_ssh_opts() -> Vec { } /// Shell-quote a string for safe use in SSH commands. -// FIXME: this is handrolled so that I can confirm whether we match -// nixos-rebuild-ng's use of shlex.quote, but we'll want to introduce shlex as a -// dependency and drop this. This is a hard blocker. -/// fn shell_quote(s: &str) -> String { - // shlex.quote in Python returns the string unchanged if it contains only - // safe characters, otherwise wraps in single quotes with escaping - if s.is_empty() { - return "''".to_string(); - } - if s - .chars() - .all(|c| c.is_ascii_alphanumeric() || "-_./+:=@^".contains(c)) - { - return s.to_string(); - } - // Escape single quotes and wrap in single quotes - let escaped = s.replace('\'', "'\"'\"'"); - format!("'{escaped}'") + shlex::try_quote(s).map_or_else( + |_| format!("'{}'", s.replace('\'', "'\\''")), + std::borrow::Cow::into_owned, + ) } /// Get SSH options from `NIX_SSHOPTS` plus our defaults. @@ -163,8 +160,8 @@ fn get_ssh_opts() -> Vec { // User options first (from NIX_SSHOPTS) if let Ok(sshopts) = env::var("NIX_SSHOPTS") { - for opt in sshopts.split_whitespace() { - opts.push(opt.to_string()); + if let Some(parsed) = shlex::split(&sshopts) { + opts.extend(parsed); } } @@ -176,13 +173,22 @@ fn get_ssh_opts() -> Vec { /// Get `NIX_SSHOPTS` environment value with our defaults appended. /// Used for `nix-copy-closure` which reads `NIX_SSHOPTS`. +/// +/// Note: `nix-copy-closure` splits `NIX_SSHOPTS` by whitespace without shell +/// parsing, so values containing spaces cannot be properly passed through +/// this mechanism. Users needing complex SSH options should use +/// `~/.ssh/config` instead. fn get_nix_sshopts_env() -> String { - let sshopts = env::var("NIX_SSHOPTS").unwrap_or_default(); - let defaults = get_default_ssh_opts().join(" "); - if sshopts.is_empty() { - defaults + let user_opts = env::var("NIX_SSHOPTS").unwrap_or_default(); + let default_opts = get_default_ssh_opts(); + + if user_opts.is_empty() { + default_opts.join(" ") } else { - format!("{sshopts} {defaults}") + // Append our defaults to user options + // Note: We preserve user options as-is since nix-copy-closure + // does simple whitespace splitting + format!("{} {}", user_opts, default_opts.join(" ")) } } @@ -654,6 +660,8 @@ fn build_on_remote_with_nom( #[cfg(test)] mod tests { + use serial_test::serial; + use super::*; #[test] @@ -728,17 +736,19 @@ mod tests { #[test] fn test_shell_quote_with_caret() { - // drv^* syntax must work - assert_eq!( - shell_quote("/nix/store/xyz.drv^*"), - "'/nix/store/xyz.drv^*'" - ); + // drv^* syntax - shlex quotes with single quotes but keeps ^ unquoted + // until it hits the *, which needs quoting + let quoted = shell_quote("/nix/store/xyz.drv^*"); + // The result should be safely quotable and contain the original content + assert!(quoted.contains("drv")); + assert!(quoted.contains("*")); } #[test] fn test_shell_quote_special_chars() { assert_eq!(shell_quote("has space"), "'has space'"); - assert_eq!(shell_quote("has'quote"), "'has'\"'\"'quote'"); + // shlex uses double quotes for strings containing single quotes + assert_eq!(shell_quote("has'quote"), "\"has'quote\""); assert_eq!(shell_quote("$(dangerous)"), "'$(dangerous)'"); } @@ -748,6 +758,7 @@ mod tests { } #[test] + #[serial] fn test_get_ssh_opts_default() { // Clear env var for test unsafe { @@ -760,4 +771,202 @@ mod tests { // Check that ControlPath is present (the exact path varies) assert!(opts.iter().any(|o| o.starts_with("ControlPath="))); } + + #[test] + #[serial] + fn test_get_ssh_opts_with_simple_nix_sshopts() { + unsafe { + std::env::set_var("NIX_SSHOPTS", "-p 2222 -i /path/to/key"); + } + let opts = get_ssh_opts(); + // User options should be included + assert!(opts.contains(&"-p".to_string())); + assert!(opts.contains(&"2222".to_string())); + assert!(opts.contains(&"-i".to_string())); + assert!(opts.contains(&"/path/to/key".to_string())); + // Default options should still be present + assert!(opts.contains(&"ControlMaster=auto".to_string())); + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + } + + #[test] + #[serial] + fn test_get_ssh_opts_with_quoted_nix_sshopts() { + // Test that quoted paths with spaces are handled correctly + unsafe { + std::env::set_var("NIX_SSHOPTS", r#"-i "/path/with spaces/key""#); + } + let opts = get_ssh_opts(); + // The path should be parsed as a single argument without quotes + assert!(opts.contains(&"-i".to_string())); + assert!(opts.contains(&"/path/with spaces/key".to_string())); + // Default options should still be present + assert!(opts.contains(&"ControlMaster=auto".to_string())); + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + } + + #[test] + #[serial] + fn test_get_ssh_opts_with_option_value_nix_sshopts() { + // Test -o with quoted value containing spaces + unsafe { + std::env::set_var( + "NIX_SSHOPTS", + r#"-o "ProxyCommand=ssh -W %h:%p jump""#, + ); + } + let opts = get_ssh_opts(); + assert!(opts.contains(&"-o".to_string())); + assert!(opts.contains(&"ProxyCommand=ssh -W %h:%p jump".to_string())); + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + } + + #[test] + fn test_shell_quote_roundtrip() { + // Test that quoting and then parsing gives back the original + let test_cases = vec![ + "simple", + "/nix/store/abc123-foo", + "has space", + "has'quote", + "has\"doublequote", + "$(dangerous)", + "path/with spaces/and'quotes", + ]; + + for original in test_cases { + let quoted = shell_quote(original); + // Parse the quoted string back - should give single element + let parsed = shlex::split("ed); + assert!( + parsed.is_some(), + "Failed to parse quoted string for: {original}" + ); + let parsed = parsed.expect("checked above"); + assert_eq!( + parsed.len(), + 1, + "Expected single element for: {original}, got: {parsed:?}" + ); + assert_eq!( + parsed[0], original, + "Roundtrip failed for: {original}, quoted as: {quoted}" + ); + } + } + + #[test] + fn test_shell_quote_nix_drv_output() { + // Test the drv^* syntax used by nix + let drv_path = "/nix/store/abc123.drv^*"; + let quoted = shell_quote(drv_path); + let parsed = shlex::split("ed).expect("should parse"); + assert_eq!(parsed.len(), 1); + assert_eq!(parsed[0], drv_path); + } + + #[test] + fn test_shell_quote_preserves_equals() { + // Environment variable assignments should work + let env_var = "PATH=/usr/bin:/bin"; + let quoted = shell_quote(env_var); + let parsed = shlex::split("ed).expect("should parse"); + assert_eq!(parsed[0], env_var); + } + + #[test] + fn test_shell_quote_unicode() { + // Unicode should be preserved + let unicode = "path/with/émojis/🚀"; + let quoted = shell_quote(unicode); + let parsed = shlex::split("ed).expect("should parse"); + assert_eq!(parsed[0], unicode); + } + + #[test] + #[serial] + fn test_get_nix_sshopts_env_empty() { + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + let result = get_nix_sshopts_env(); + // Should contain our defaults as space-separated values + assert!(result.contains("-o")); + assert!(result.contains("ControlMaster=auto")); + assert!(result.contains("ControlPersist=60")); + // Should contain ControlPath (exact path varies) + assert!(result.contains("ControlPath=")); + } + + #[test] + #[serial] + fn test_get_nix_sshopts_env_simple() { + unsafe { + std::env::set_var("NIX_SSHOPTS", "-p 2222"); + } + let result = get_nix_sshopts_env(); + // User options should come first + assert!(result.starts_with("-p 2222")); + // Defaults should be appended + assert!(result.contains("ControlMaster=auto")); + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + } + + #[test] + #[serial] + fn test_get_nix_sshopts_env_preserves_user_opts() { + // User options are preserved as-is (nix-copy-closure does whitespace split) + unsafe { + std::env::set_var("NIX_SSHOPTS", "-i /path/to/key -p 22"); + } + let result = get_nix_sshopts_env(); + // User options preserved at start + assert!(result.starts_with("-i /path/to/key -p 22")); + // Our defaults appended + assert!(result.contains("ControlMaster=auto")); + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + } + + #[test] + #[serial] + fn test_get_nix_sshopts_env_no_extra_quoting() { + // Verify we don't add shell quotes (nix-copy-closure doesn't parse them) + unsafe { + std::env::remove_var("NIX_SSHOPTS"); + } + let result = get_nix_sshopts_env(); + // Should NOT contain shell quote characters around our options + assert!(!result.contains("'ControlMaster")); + assert!(!result.contains("\"ControlMaster")); + // Values should be bare + assert!(result.contains("-o ControlMaster=auto")); + } + + #[test] + fn test_get_ssh_control_dir_creates_directory() { + let dir = get_ssh_control_dir(); + // The directory should exist (or be /tmp as last resort) + assert!( + dir.exists() || dir == &PathBuf::from("/tmp"), + "Control dir should exist or be /tmp fallback" + ); + // Should contain our process-specific suffix (unless /tmp fallback) + let dir_str = dir.to_string_lossy(); + if dir_str != "/tmp" { + assert!( + dir_str.contains("nh-ssh-"), + "Control dir should contain 'nh-ssh-': {dir_str}" + ); + } + } } From bd947bf7edfd25d7eedfcb9da3209f872186fcf6 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 4 Dec 2025 21:07:13 +0300 Subject: [PATCH 04/38] commands: fix error handling in nom pipeline execution Fixes a minor issue in how commands that are invalid or improperly handled are forwarded to the Nix command. Replaces `join()` with `popen()` to access individual processes in the pipeline. This way we can better check the exist status of the `nix build` process and properly propagate them. Also improves naming a little bit because why not? Signed-off-by: NotAShelf Change-Id: I8a44abf924f9c9a1c06d102e5a3f40aa6a6a6964 --- src/commands.rs | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 6b753245..6917b05a 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -739,8 +739,8 @@ impl Build { .args(&installable_args) .args(&self.extra_args); - let exit = if self.nom { - let cmd = { + if self.nom { + let pipeline = { base_command .args(&["--log-format", "internal-json", "--verbose"]) .stderr(Redirection::Merge) @@ -748,20 +748,41 @@ impl Build { | Exec::cmd("nom").args(&["--json"]) } .stdout(Redirection::None); - debug!(?cmd); - cmd.join() + debug!(?pipeline); + + // Use popen() to get access to individual processes so we can check + // nix's exit status, not nom's. The pipeline's join() only returns + // the exit status of the last command (nom), which always succeeds + // even when nix fails. + let mut processes = pipeline.popen()?; + + // Wait for all processes to finish + for proc in &mut processes { + proc.wait()?; + } + + // Check the exit status of the FIRST process (nix build) + // This is the one that matters - if nix fails, we should fail too + if let Some(nix_proc) = processes.first() { + if let Some(exit_status) = nix_proc.exit_status() { + match exit_status { + ExitStatus::Exited(0) => (), + other => bail!(ExitError(other)), + } + } + } } else { let cmd = base_command .stderr(Redirection::Merge) .stdout(Redirection::None); debug!(?cmd); - cmd.join() - }; + let exit = cmd.join(); - match exit? { - ExitStatus::Exited(0) => (), - other => bail!(ExitError(other)), + match exit? { + ExitStatus::Exited(0) => (), + other => bail!(ExitError(other)), + } } Ok(()) From 8bb85b6146f0f2a428fd944b81cf9adbb66a90c4 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 4 Dec 2025 21:37:58 +0300 Subject: [PATCH 05/38] various: simplify argument mapping Signed-off-by: NotAShelf Change-Id: Ia8506cad3352243001a281e99b8162c26a6a6964 --- src/darwin.rs | 6 +++--- src/home.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/darwin.rs b/src/darwin.rs index b4941028..d46d9ee9 100644 --- a/src/darwin.rs +++ b/src/darwin.rs @@ -1,4 +1,4 @@ -use std::{env, path::PathBuf}; +use std::{convert::Into, env, path::PathBuf}; use color_eyre::eyre::{Context, bail, eyre}; use tracing::{debug, info, warn}; @@ -124,14 +124,14 @@ impl DarwinRebuildArgs { extra_args: self .extra_args .iter() - .map(|s| s.into()) + .map(Into::into) .chain( self .common .passthrough .generate_passthrough_args() .into_iter() - .map(|s| s.into()), + .map(Into::into), ) .collect(), }; diff --git a/src/home.rs b/src/home.rs index f9e2c029..00239057 100644 --- a/src/home.rs +++ b/src/home.rs @@ -1,4 +1,4 @@ -use std::{env, ffi::OsString, path::PathBuf}; +use std::{convert::Into, env, ffi::OsString, path::PathBuf}; use color_eyre::{ Result, @@ -111,14 +111,14 @@ impl HomeRebuildArgs { extra_args: self .extra_args .iter() - .map(|s| s.into()) + .map(Into::into) .chain( self .common .passthrough .generate_passthrough_args() .into_iter() - .map(|s| s.into()), + .map(Into::into), ) .collect(), }; From 55719ceeba89fc6c7ede6f4c34fd44290086aa6d Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 5 Dec 2025 18:14:04 +0300 Subject: [PATCH 06/38] remote: add SSH reachability checks; enforce local symlink creation Tiny improvement to how remote connections are made. We now check BEFORE the connection is made, so that we can avoid all that expensive eval if it's not reachable. This is not infallible, but it is better. To fix some target-host quirks, we also have to deal with local symlinks so we enforce it locally either way. Signed-off-by: NotAShelf Change-Id: I65fd7258828459ea82fe6739383567556a6a6964 --- src/nixos.rs | 38 +++++++++++++++------- src/remote.rs | 90 +++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 103 insertions(+), 25 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index 4c0660cc..8300fa22 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -1,4 +1,5 @@ use std::{ + convert::Into, env, fs, path::{Path, PathBuf}, @@ -22,7 +23,7 @@ use crate::{ OsRollbackArgs, OsSubcommand::{self}, }, - remote::{self, RemoteBuildConfig, RemoteHost}, + remote::{self, RemoteBuildConfig, RemoteHost, check_ssh_reachability}, update::update, util::{ensure_ssh_key_login, get_hostname, print_dix_diff}, }; @@ -133,9 +134,7 @@ impl OsRebuildActivateArgs { _ => "Building NixOS configuration", }; - self - .rebuild - .execute_build_command(toplevel, &out_path, message)?; + self.rebuild.execute_build(toplevel, &out_path, message)?; let target_profile = self.rebuild.resolve_specialisation_and_profile(&out_path)?; @@ -284,7 +283,9 @@ impl OsRebuildArgs { /// - Resolving the target hostname for the build. /// /// # Returns - /// A `Result` containing a tuple: + /// + /// `Result` containing a tuple: + /// /// - `bool`: `true` if elevation is required, `false` otherwise. /// - `String`: The resolved target hostname. fn setup_build_context(&self) -> Result<(bool, String)> { @@ -345,20 +346,20 @@ impl OsRebuildArgs { ) } - fn execute_build_command( + fn execute_build( &self, toplevel: Installable, out_path: &Path, message: &str, ) -> Result<()> { - // If a build host is specified, use proper remote build semantics - // This matches nixos-rebuild --build-host behavior: + // If a build host is specified, use proper remote build semantics: + // // 1. Evaluate derivation locally // 2. Copy derivation to build host (user-initiated SSH) // 3. Build on remote host // 4. Copy result back (to localhost or target_host) if let Some(ref build_host_str) = self.build_host { - info!("{}", message); + info!("{message}"); let build_host = RemoteHost::parse(build_host_str) .wrap_err("Invalid build host specification")?; @@ -370,6 +371,19 @@ impl OsRebuildArgs { .transpose() .wrap_err("Invalid target host specification")?; + // Check SSH connectivity BEFORE starting expensive evaluation. This + // provides early feedback if remote hosts are unreachable and allows + // us to handle the error instead of letting Nix throw its own. + info!("Checking SSH connectivity to remote hosts..."); + check_ssh_reachability(&build_host) + .wrap_err(format!("Build host ({build_host}) is not reachable"))?; + + if let Some(ref target) = target_host { + check_ssh_reachability(target) + .wrap_err(format!("Target host ({target}) is not reachable"))?; + } + debug!("SSH connectivity verified"); + let config = RemoteBuildConfig { build_host, target_host, @@ -378,14 +392,14 @@ impl OsRebuildArgs { extra_args: self .extra_args .iter() - .map(|s| s.into()) + .map(Into::into) .chain( self .common .passthrough .generate_passthrough_args() .into_iter() - .map(|s| s.into()), + .map(Into::into), ) .collect(), }; @@ -496,7 +510,7 @@ impl OsRebuildArgs { _ => "Building NixOS configuration", }; - self.execute_build_command(toplevel, &out_path, message)?; + self.execute_build(toplevel, &out_path, message)?; let target_profile = self.resolve_specialisation_and_profile(&out_path)?; diff --git a/src/remote.rs b/src/remote.rs index 324b6dcc..0d5378ae 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -130,6 +130,48 @@ impl std::fmt::Display for RemoteHost { } } +/// Check if a remote host is reachable via SSH. +/// +/// Performs a quick connectivity test before expensive operations like +/// evaluation or building. +/// +/// # Errors +/// +/// Returns an error if: +/// - The SSH command fails to execute +/// - The remote host is unreachable or rejects the connection +pub fn check_ssh_reachability(host: &RemoteHost) -> Result<()> { + debug!("Checking SSH connectivity to {}", host); + + let ssh_opts = get_ssh_opts(); + + // Use ssh with a simple 'true' command and short timeout + // ConnectTimeout ensures we don't wait too long for unreachable hosts + let exit = Exec::cmd("ssh") + .args(&ssh_opts) + .args(&["-o", "ConnectTimeout=10", "-o", "BatchMode=yes"]) + .arg(host.host()) + .arg("true") + .stdout(Redirection::None) + .stderr(Redirection::Pipe) + .capture() + .wrap_err_with(|| format!("Failed to execute SSH command to {host}"))?; + + if exit.success() { + debug!("SSH connectivity to {} confirmed", host); + Ok(()) + } else { + let stderr = exit.stderr_str(); + bail!( + "Cannot reach host '{}' via SSH. Please verify:\n- The hostname is \ + correct\n- SSH key authentication is configured\n- The remote host is \ + online and accepting connections\nSSH error: {}", + host, + stderr.trim() + ) + } +} + /// Get the default SSH options for connection multiplexing. /// Includes a `ControlPath` pointing to our control socket directory. fn get_default_ssh_opts() -> Vec { @@ -506,17 +548,22 @@ pub fn build_remote( &out_path, use_substitutes, )?; - } else { + } + + // Always copy to localhost if we need to create a local out-link + // This is necessary even when target_host is set, because the calling code + // (e.g., nixos.rs) needs to resolve specialisations from the local path. + if out_link.is_some() || config.target_host.is_none() { copy_closure_from(build_host, &out_path, use_substitutes)?; + } - // Create local out-link if requested (only when copying to localhost) - if let Some(link) = out_link { - debug!("Creating out-link: {} -> {}", link.display(), out_path); - // Remove existing symlink/file if present - let _ = std::fs::remove_file(link); - std::os::unix::fs::symlink(&out_path, link) - .wrap_err("Failed to create out-link")?; - } + // Create local out-link if requested + if let Some(link) = out_link { + debug!("Creating out-link: {} -> {}", link.display(), out_path); + // Remove existing symlink/file if present + let _ = std::fs::remove_file(link); + std::os::unix::fs::symlink(&out_path, link) + .wrap_err("Failed to create out-link")?; } Ok(PathBuf::from(out_path)) @@ -630,11 +677,28 @@ fn build_on_remote_with_nom( debug!(?pipeline, "Running remote build with nom"); - let exit = pipeline.join().wrap_err("Remote build with nom failed")?; + // Use popen() to get access to individual processes so we can check + // ssh's exit status, not nom's. The pipeline's join() only returns + // the exit status of the last command (nom), which always succeeds + // even when the remote nix command fails. + let mut processes = + pipeline.popen().wrap_err("Remote build with nom failed")?; + + // Wait for all processes to finish + for proc in &mut processes { + proc.wait()?; + } - match exit { - ExitStatus::Exited(0) => {}, - other => bail!("Remote build failed with exit status: {other:?}"), + // Check the exit status of the FIRST process (ssh -> nix build) + // This is the one that matters - if the remote build fails, we should fail + // too + if let Some(ssh_proc) = processes.first() { + if let Some(exit_status) = ssh_proc.exit_status() { + match exit_status { + ExitStatus::Exited(0) => {}, + other => bail!("Remote build failed with exit status: {other:?}"), + } + } } // nom consumed the output, so we need to query the output path separately From ac4f86cdc3025165f7c626f7ffb809122224d5f6 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 8 Dec 2025 20:30:29 +0300 Subject: [PATCH 07/38] remote: consolidate remote connectivitiy checks Signed-off-by: NotAShelf Change-Id: I2366fac6ca7a72fc73eecfc0b07bd2d76a6a6964 --- src/darwin.rs | 2 ++ src/home.rs | 2 ++ src/nixos.rs | 17 +++-------------- src/remote.rs | 25 ++++++++++++++++++++++++- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/darwin.rs b/src/darwin.rs index d46d9ee9..3cbe1790 100644 --- a/src/darwin.rs +++ b/src/darwin.rs @@ -136,6 +136,8 @@ impl DarwinRebuildArgs { .collect(), }; + remote::check_remote_connectivity(&config)?; + remote::build_remote(&toplevel, &config, Some(&out_path)) .wrap_err("Failed to build Darwin configuration")?; } else { diff --git a/src/home.rs b/src/home.rs index 00239057..de07c51b 100644 --- a/src/home.rs +++ b/src/home.rs @@ -123,6 +123,8 @@ impl HomeRebuildArgs { .collect(), }; + remote::check_remote_connectivity(&config)?; + remote::build_remote(&toplevel, &config, Some(&out_path)) .wrap_err("Failed to build Home-Manager configuration")?; } else { diff --git a/src/nixos.rs b/src/nixos.rs index 8300fa22..f6ba883e 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -23,7 +23,7 @@ use crate::{ OsRollbackArgs, OsSubcommand::{self}, }, - remote::{self, RemoteBuildConfig, RemoteHost, check_ssh_reachability}, + remote::{self, RemoteBuildConfig, RemoteHost}, update::update, util::{ensure_ssh_key_login, get_hostname, print_dix_diff}, }; @@ -371,19 +371,6 @@ impl OsRebuildArgs { .transpose() .wrap_err("Invalid target host specification")?; - // Check SSH connectivity BEFORE starting expensive evaluation. This - // provides early feedback if remote hosts are unreachable and allows - // us to handle the error instead of letting Nix throw its own. - info!("Checking SSH connectivity to remote hosts..."); - check_ssh_reachability(&build_host) - .wrap_err(format!("Build host ({build_host}) is not reachable"))?; - - if let Some(ref target) = target_host { - check_ssh_reachability(target) - .wrap_err(format!("Target host ({target}) is not reachable"))?; - } - debug!("SSH connectivity verified"); - let config = RemoteBuildConfig { build_host, target_host, @@ -404,6 +391,8 @@ impl OsRebuildArgs { .collect(), }; + remote::check_remote_connectivity(&config)?; + remote::build_remote(&toplevel, &config, Some(out_path))?; Ok(()) diff --git a/src/remote.rs b/src/remote.rs index 0d5378ae..05010e58 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -506,12 +506,35 @@ pub struct RemoteBuildConfig { pub extra_args: Vec, } +/// Check SSH connectivity to remote hosts specified in the build config. +/// +/// Checks connectivity to the build host and optionally the target host. +/// Provides early feedback before starting expensive operations. +/// +/// # Errors +/// +/// Returns an error if any host is unreachable. +pub fn check_remote_connectivity(config: &RemoteBuildConfig) -> Result<()> { + info!("Checking SSH connectivity to remote hosts..."); + check_ssh_reachability(&config.build_host).wrap_err(format!( + "Build host ({}) is not reachable", + config.build_host + ))?; + + if let Some(ref target) = config.target_host { + check_ssh_reachability(target) + .wrap_err(format!("Target host ({target}) is not reachable"))?; + } + debug!("SSH connectivity verified"); + Ok(()) +} + /// Perform a remote build of a flake installable. /// /// This implements the `build_remote_flake` workflow from nixos-rebuild-ng: /// 1. Evaluate drvPath locally via `nix eval --raw` /// 2. Copy the derivation to the build host via `nix-copy-closure` -/// 3. Build on remote via `nix build ^* --print-out-paths` +/// 3. Build on remote host via `nix build ^* --print-out-paths` /// 4. Copy the result back (to localhost or `target_host`) /// /// Returns the output path in the Nix store. From 6f8357dc5a5ea81108697e8bd6245aadba48505d Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 8 Dec 2025 11:50:12 +0300 Subject: [PATCH 08/38] remote: add a hostname method for normalizing compared hostnames Signed-off-by: NotAShelf Change-Id: If5d2072431348a8468150abf15a7a2a06a6a6964 --- src/remote.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index 05010e58..9172f1bd 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -61,6 +61,13 @@ pub struct RemoteHost { } impl RemoteHost { + /// Get the hostname part (without user@ prefix). + #[must_use] + pub fn hostname(&self) -> &str { + #[allow(clippy::unwrap_used)] + self.host.rsplit('@').next().unwrap() + } + /// Parse a host specification string. /// /// Accepts: @@ -562,15 +569,17 @@ pub fn build_remote( let out_path = build_on_remote(build_host, &drv_path, config)?; // Step 4: Copy result to destination - // If target_host is set, copy directly from build_host to target_host. - // Otherwise, copy back to localhost. + // If target_host is set and different from build_host, copy directly from + // build_host to target_host. Otherwise, copy back to localhost. if let Some(ref target_host) = config.target_host { - copy_closure_between_remotes( - build_host, - target_host, - &out_path, - use_substitutes, - )?; + if build_host.hostname() != target_host.hostname() { + copy_closure_between_remotes( + build_host, + target_host, + &out_path, + use_substitutes, + )?; + } } // Always copy to localhost if we need to create a local out-link From ac353fad1997cbf133e11ab79fda2d786842c1e9 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 8 Dec 2025 12:06:44 +0300 Subject: [PATCH 09/38] remote: attempt to reduce remote copy roundtrips Signed-off-by: NotAShelf Change-Id: I9eb3904e832e58e0f4ac306d537f7dee6a6a6964 --- src/nixos.rs | 57 ++++++++++++++++------------ src/remote.rs | 102 +++++++++++++++++++++++++++++++++++++------------- 2 files changed, 109 insertions(+), 50 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index f6ba883e..23a355f3 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -186,21 +186,25 @@ impl OsRebuildActivateArgs { } if let Some(target_host) = &self.rebuild.target_host { - Command::new("nix") - .args([ - "copy", - "--to", - format!("ssh://{target_host}").as_str(), - match target_profile.to_str() { - Some(s) => s, - None => { - return Err(eyre!("target_profile path is not valid UTF-8")); + // Only copy if the output path exists locally (i.e., was copied back from + // remote build) + if out_path.exists() { + Command::new("nix") + .args([ + "copy", + "--to", + format!("ssh://{target_host}").as_str(), + match target_profile.to_str() { + Some(s) => s, + None => { + return Err(eyre!("target_profile path is not valid UTF-8")); + }, }, - }, - ]) - .message("Copying configuration to target") - .with_required_env() - .run()?; + ]) + .message("Copying configuration to target") + .with_required_env() + .run()?; + } } let switch_to_configuration = target_profile @@ -431,16 +435,23 @@ impl OsRebuildArgs { debug!("Output path: {out_path:?}"); debug!("Target profile path: {}", target_profile.display()); - debug!("Target profile exists: {}", target_profile.exists()); - if !target_profile - .try_exists() - .context("Failed to check if target profile exists")? - { - return Err(eyre!( - "Target profile path does not exist: {}", - target_profile.display() - )); + // If out_path doesn't exist locally, assume it's remote and skip existence + // check + if out_path.exists() { + debug!("Target profile exists: {}", target_profile.exists()); + + if !target_profile + .try_exists() + .context("Failed to check if target profile exists")? + { + return Err(eyre!( + "Target profile path does not exist: {}", + target_profile.display() + )); + } + } else { + debug!("Output path is remote, skipping local existence check"); } Ok(target_profile) diff --git a/src/remote.rs b/src/remote.rs index 9172f1bd..a16c0304 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -13,6 +13,7 @@ use crate::{installable::Installable, util::NixVariant}; static SSH_CONTROL_DIR: OnceLock = OnceLock::new(); /// Get or create the SSH control socket directory. +/// /// This creates a temporary directory that persists for the lifetime of the /// program, similar to nixos-rebuild-ng's tmpdir module. fn get_ssh_control_dir() -> &'static PathBuf { @@ -582,10 +583,17 @@ pub fn build_remote( } } - // Always copy to localhost if we need to create a local out-link - // This is necessary even when target_host is set, because the calling code - // (e.g., nixos.rs) needs to resolve specialisations from the local path. - if out_link.is_some() || config.target_host.is_none() { + // Copy to localhost if we need to create a local out-link or if target_host + // is not set. When target_host is set and the same as build_host, skip + // copying to localhost to avoid inefficiency, assuming specialisation + // resolution can be handled remotely or is not needed. + if out_link.is_some() + || config.target_host.is_none() + || config + .target_host + .as_ref() + .is_some_and(|th| th.hostname() != build_host.hostname()) + { copy_closure_from(build_host, &out_path, use_substitutes)?; } @@ -756,10 +764,56 @@ fn build_on_remote_with_nom( #[cfg(test)] mod tests { + use proptest::prelude::*; use serial_test::serial; use super::*; + proptest! { + #[test] + fn hostname_always_returns_suffix_after_last_at(s in "\\PC*") { + let host = RemoteHost { host: s.clone() }; + let expected = s.rsplit('@').next().unwrap(); + prop_assert_eq!(host.hostname(), expected); + } + + #[test] + fn hostname_is_substring_of_host(s in "\\PC*") { + let host = RemoteHost { host: s.clone() }; + prop_assert!(s.contains(host.hostname())); + } + + #[test] + fn hostname_no_at_means_whole_string(s in "[^@]*") { + let host = RemoteHost { host: s.clone() }; + prop_assert_eq!(host.hostname(), s); + } + + #[test] + fn hostname_with_user(user in "[a-zA-Z0-9_]+", hostname in "[a-zA-Z0-9_.-]+") { + let full = format!("{}@{}", user, hostname); + let host = RemoteHost { host: full }; + prop_assert_eq!(host.hostname(), hostname); + } + + #[test] + fn parse_valid_bare_hostname(hostname in "[a-zA-Z0-9_.-]+") { + let result = RemoteHost::parse(&hostname); + prop_assert!(result.is_ok()); + let host = result.unwrap(); + prop_assert_eq!(host.hostname(), hostname); + } + + #[test] + fn parse_valid_user_at_hostname(user in "[a-zA-Z0-9_]+", hostname in "[a-zA-Z0-9_.-]+") { + let full = format!("{}@{}", user, hostname); + let result = RemoteHost::parse(&full); + prop_assert!(result.is_ok()); + let host = result.unwrap(); + prop_assert_eq!(host.hostname(), hostname); + } + } + #[test] fn test_parse_bare_hostname() { let host = RemoteHost::parse("buildserver").expect("should parse"); @@ -830,29 +884,6 @@ mod tests { ); } - #[test] - fn test_shell_quote_with_caret() { - // drv^* syntax - shlex quotes with single quotes but keeps ^ unquoted - // until it hits the *, which needs quoting - let quoted = shell_quote("/nix/store/xyz.drv^*"); - // The result should be safely quotable and contain the original content - assert!(quoted.contains("drv")); - assert!(quoted.contains("*")); - } - - #[test] - fn test_shell_quote_special_chars() { - assert_eq!(shell_quote("has space"), "'has space'"); - // shlex uses double quotes for strings containing single quotes - assert_eq!(shell_quote("has'quote"), "\"has'quote\""); - assert_eq!(shell_quote("$(dangerous)"), "'$(dangerous)'"); - } - - #[test] - fn test_shell_quote_empty() { - assert_eq!(shell_quote(""), "''"); - } - #[test] #[serial] fn test_get_ssh_opts_default() { @@ -1048,6 +1079,23 @@ mod tests { assert!(result.contains("-o ControlMaster=auto")); } + #[test] + fn test_hostname_comparison_for_same_host() { + let host1 = RemoteHost::parse("user1@host.example").unwrap(); + let host2 = RemoteHost::parse("user2@host.example").unwrap(); + let host3 = RemoteHost::parse("host.example").unwrap(); + let host4 = RemoteHost::parse("other.host").unwrap(); + + assert_eq!(host1.hostname(), "host.example"); + assert_eq!(host2.hostname(), "host.example"); + assert_eq!(host3.hostname(), "host.example"); + assert_eq!(host4.hostname(), "other.host"); + + assert_eq!(host1.hostname(), host2.hostname()); + assert_eq!(host1.hostname(), host3.hostname()); + assert_ne!(host1.hostname(), host4.hostname()); + } + #[test] fn test_get_ssh_control_dir_creates_directory() { let dir = get_ssh_control_dir(); From 2add284ccb68afe2ebad7cf6062e6fb21d908775 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sat, 20 Dec 2025 16:09:33 +0300 Subject: [PATCH 10/38] remote: handle non-UTF8 strings more explicitly Signed-off-by: NotAShelf Change-Id: Idf201a23f71795e0caea9813280084036a6a6964 --- src/remote.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index a16c0304..405d1e6d 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -641,12 +641,16 @@ fn build_on_remote_simple( args.extend(flake_flags); args.extend(["build", drv_with_outputs, "--print-out-paths"]); - // Collect extra args that are valid strings + // Convert extra args to strings, fail if any are non-UTF-8 let extra_args_strings: Vec = config .extra_args .iter() - .filter_map(|s| s.to_str().map(String::from)) - .collect(); + .map(|s| { + s.to_str() + .map(String::from) + .ok_or_else(|| eyre!("Extra argument is not valid UTF-8: {:?}", s)) + }) + .collect::>>()?; for arg in &extra_args_strings { args.push(arg); } @@ -686,11 +690,16 @@ fn build_on_remote_with_nom( "--verbose", ]); + // Convert extra args to strings, fail if any are non-UTF-8 let extra_args_strings: Vec = config .extra_args .iter() - .filter_map(|s| s.to_str().map(String::from)) - .collect(); + .map(|s| { + s.to_str() + .map(String::from) + .ok_or_else(|| eyre!("Extra argument is not valid UTF-8: {:?}", s)) + }) + .collect::>>()?; for arg in &extra_args_strings { remote_args.push(arg); } From deac4d7323cf2dca19eb217ba7a269e282fdfd23 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Wed, 24 Dec 2025 14:12:51 +0300 Subject: [PATCH 11/38] remote: optimize decision matrix to reduce number of connections Signed-off-by: NotAShelf Change-Id: I73a94927d1270b4a499bb22b8220a1326a6a6964 --- src/remote.rs | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index 405d1e6d..9ae76595 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -570,10 +570,25 @@ pub fn build_remote( let out_path = build_on_remote(build_host, &drv_path, config)?; // Step 4: Copy result to destination - // If target_host is set and different from build_host, copy directly from - // build_host to target_host. Otherwise, copy back to localhost. + // + // Optimizes copy paths based on hostname comparison: + // - When build_host != target_host: copy build -> target, then build -> local + // - When build_host == target_host: skip redundant copies, only copy to local + // if out-link is needed + // - When target_host is None: always copy build -> local + let target_is_build_host = config + .target_host + .as_ref() + .is_some_and(|th| th.hostname() == build_host.hostname()); + + // Copy from build_host to target_host if they differ if let Some(ref target_host) = config.target_host { - if build_host.hostname() != target_host.hostname() { + if target_is_build_host { + debug!( + "Skipping copy from build host to target host (same host: {})", + build_host.hostname() + ); + } else { copy_closure_between_remotes( build_host, target_host, @@ -583,18 +598,17 @@ pub fn build_remote( } } - // Copy to localhost if we need to create a local out-link or if target_host - // is not set. When target_host is set and the same as build_host, skip - // copying to localhost to avoid inefficiency, assuming specialisation - // resolution can be handled remotely or is not needed. - if out_link.is_some() - || config.target_host.is_none() - || config - .target_host - .as_ref() - .is_some_and(|th| th.hostname() != build_host.hostname()) - { + // Copy to localhost only when necessary to avoid ping-pong effect + let need_local_copy = + config.target_host.is_none() || !target_is_build_host || out_link.is_some(); + + if need_local_copy { copy_closure_from(build_host, &out_path, use_substitutes)?; + } else { + debug!( + "Skipping copy to localhost (build_host == target_host, no out-link \ + needed)" + ); } // Create local out-link if requested From 6ad3d11b3b957c49a1d10b458a172ae0abcfdad7 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Wed, 24 Dec 2025 14:35:45 +0300 Subject: [PATCH 12/38] remote: optimize SSH connections and add proper cleanup Remove the redundant and poor connectivity checks that added overhead without any tangible benefit, and implement SSH ControlMaster cleanup on program exit. This reduces the number of SSH connections made during remote operations and makes sure SSH control processes are properly terminated Signed-off-by: NotAShelf Change-Id: Ideb1825cb7e8302316d7d25b64e7859b6a6a6964 --- src/darwin.rs | 3 +- src/home.rs | 3 +- src/nixos.rs | 3 +- src/remote.rs | 174 +++++++++++++++++++++++++++++++------------------- 4 files changed, 115 insertions(+), 68 deletions(-) diff --git a/src/darwin.rs b/src/darwin.rs index 3cbe1790..bd82f48c 100644 --- a/src/darwin.rs +++ b/src/darwin.rs @@ -136,7 +136,8 @@ impl DarwinRebuildArgs { .collect(), }; - remote::check_remote_connectivity(&config)?; + // Initialize SSH control - guard will cleanup connections on drop + let _ssh_guard = remote::init_ssh_control(); remote::build_remote(&toplevel, &config, Some(&out_path)) .wrap_err("Failed to build Darwin configuration")?; diff --git a/src/home.rs b/src/home.rs index de07c51b..19d8be29 100644 --- a/src/home.rs +++ b/src/home.rs @@ -123,7 +123,8 @@ impl HomeRebuildArgs { .collect(), }; - remote::check_remote_connectivity(&config)?; + // Initialize SSH control - guard will cleanup connections on drop + let _ssh_guard = remote::init_ssh_control(); remote::build_remote(&toplevel, &config, Some(&out_path)) .wrap_err("Failed to build Home-Manager configuration")?; diff --git a/src/nixos.rs b/src/nixos.rs index 23a355f3..c390aa48 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -395,7 +395,8 @@ impl OsRebuildArgs { .collect(), }; - remote::check_remote_connectivity(&config)?; + // Initialize SSH control - guard will cleanup connections on drop + let _ssh_guard = remote::init_ssh_control(); remote::build_remote(&toplevel, &config, Some(out_path))?; diff --git a/src/remote.rs b/src/remote.rs index 9ae76595..16c6c3fb 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -9,6 +9,94 @@ use tracing::{debug, info}; use crate::{installable::Installable, util::NixVariant}; +/// Guard that cleans up SSH `ControlMaster` sockets on drop. +/// +/// This ensures SSH control connections are properly closed when remote +/// operations complete, preventing lingering SSH processes. +#[must_use] +pub struct SshControlGuard { + control_dir: PathBuf, +} + +impl Drop for SshControlGuard { + fn drop(&mut self) { + cleanup_ssh_control_sockets(&self.control_dir); + } +} + +/// Clean up SSH `ControlMaster` sockets in the control directory. +/// +/// Iterates through all ssh-* control sockets and sends the "exit" command +/// to close the master connection. Errors are logged but not propagated. +fn cleanup_ssh_control_sockets(control_dir: &std::path::Path) { + debug!( + "Cleaning up SSH control sockets in {}", + control_dir.display() + ); + + // Read directory entries + let entries = match std::fs::read_dir(control_dir) { + Ok(entries) => entries, + Err(e) => { + // Directory might not exist if no SSH connections were made + debug!( + "Could not read SSH control directory {}: {}", + control_dir.display(), + e + ); + return; + }, + }; + + for entry in entries.flatten() { + let path = entry.path(); + + // Only process files starting with "ssh-" + if let Some(filename) = path.file_name().and_then(|n| n.to_str()) { + if filename.starts_with("ssh-") { + debug!("Closing SSH control socket: {}", path.display()); + + // Run: ssh -o ControlPath= -O exit dummyhost + let result = Exec::cmd("ssh") + .args(&["-o", &format!("ControlPath={}", path.display())]) + .args(&["-O", "exit", "dummyhost"]) + .stdout(Redirection::Pipe) + .stderr(Redirection::Pipe) + .capture(); + + match result { + Ok(capture) => { + if !capture.exit_status.success() { + // This is normal if the connection was already closed + debug!( + "SSH control socket cleanup exited with status {:?} for {}", + capture.exit_status, + path.display() + ); + } + }, + Err(e) => { + tracing::warn!( + "Failed to close SSH control socket at {}: {}", + path.display(), + e + ); + }, + } + } + } + } +} + +/// Initialize SSH control socket management. +/// +/// Returns a guard that will clean up SSH `ControlMaster` connections when +/// dropped. The guard should be held for the duration of remote operations. +pub fn init_ssh_control() -> SshControlGuard { + let control_dir = get_ssh_control_dir().clone(); + SshControlGuard { control_dir } +} + /// Cache for the SSH control socket directory. static SSH_CONTROL_DIR: OnceLock = OnceLock::new(); @@ -138,48 +226,6 @@ impl std::fmt::Display for RemoteHost { } } -/// Check if a remote host is reachable via SSH. -/// -/// Performs a quick connectivity test before expensive operations like -/// evaluation or building. -/// -/// # Errors -/// -/// Returns an error if: -/// - The SSH command fails to execute -/// - The remote host is unreachable or rejects the connection -pub fn check_ssh_reachability(host: &RemoteHost) -> Result<()> { - debug!("Checking SSH connectivity to {}", host); - - let ssh_opts = get_ssh_opts(); - - // Use ssh with a simple 'true' command and short timeout - // ConnectTimeout ensures we don't wait too long for unreachable hosts - let exit = Exec::cmd("ssh") - .args(&ssh_opts) - .args(&["-o", "ConnectTimeout=10", "-o", "BatchMode=yes"]) - .arg(host.host()) - .arg("true") - .stdout(Redirection::None) - .stderr(Redirection::Pipe) - .capture() - .wrap_err_with(|| format!("Failed to execute SSH command to {host}"))?; - - if exit.success() { - debug!("SSH connectivity to {} confirmed", host); - Ok(()) - } else { - let stderr = exit.stderr_str(); - bail!( - "Cannot reach host '{}' via SSH. Please verify:\n- The hostname is \ - correct\n- SSH key authentication is configured\n- The remote host is \ - online and accepting connections\nSSH error: {}", - host, - stderr.trim() - ) - } -} - /// Get the default SSH options for connection multiplexing. /// Includes a `ControlPath` pointing to our control socket directory. fn get_default_ssh_opts() -> Vec { @@ -514,29 +560,6 @@ pub struct RemoteBuildConfig { pub extra_args: Vec, } -/// Check SSH connectivity to remote hosts specified in the build config. -/// -/// Checks connectivity to the build host and optionally the target host. -/// Provides early feedback before starting expensive operations. -/// -/// # Errors -/// -/// Returns an error if any host is unreachable. -pub fn check_remote_connectivity(config: &RemoteBuildConfig) -> Result<()> { - info!("Checking SSH connectivity to remote hosts..."); - check_ssh_reachability(&config.build_host).wrap_err(format!( - "Build host ({}) is not reachable", - config.build_host - ))?; - - if let Some(ref target) = config.target_host { - check_ssh_reachability(target) - .wrap_err(format!("Target host ({target}) is not reachable"))?; - } - debug!("SSH connectivity verified"); - Ok(()) -} - /// Perform a remote build of a flake installable. /// /// This implements the `build_remote_flake` workflow from nixos-rebuild-ng: @@ -1136,4 +1159,25 @@ mod tests { ); } } + + #[test] + fn test_init_ssh_control_returns_guard() { + // Verify that init_ssh_control() returns a guard + // and that the guard holds the correct control directory + let guard = init_ssh_control(); + let expected_dir = get_ssh_control_dir(); + + // Verify the guard holds the same directory + assert_eq!(guard.control_dir, *expected_dir); + } + + #[test] + fn test_ssh_control_guard_drop() { + // Verify that dropping the guard doesn't panic + // We can't easily test the actual cleanup without creating real SSH + // connections, but we can at least verify the Drop implementation runs + let guard = init_ssh_control(); + drop(guard); + // If this completes without panic, the Drop impl is at least safe + } } From 8ff64d3a757d860b19534c69b42d0da49e28792a Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 29 Dec 2025 11:43:52 +0300 Subject: [PATCH 13/38] commands: consolidate duplicate logic; drop unused cmdline parser Signed-off-by: NotAShelf Change-Id: Ia782562b87e2614a390d8f435114142b6a6a6964 --- src/commands.rs | 53 +++++++++++++++++++++---------------------------- src/remote.rs | 38 ++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 48 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 6917b05a..ac4a2c2d 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -36,15 +36,6 @@ fn cache_password(host: &str, password: SecretString) { guard.insert(host.to_string(), password); } -/// Parse a command line string respecting quoted arguments. -/// -/// Splits the command line by whitespace while preserving spaces within -/// single or double quoted strings. Quote characters are removed from -/// the resulting tokens. -fn parse_cmdline_with_quotes(cmdline: &str) -> Vec { - shlex::split(cmdline).unwrap_or_default() -} - fn ssh_wrap( cmd: Exec, ssh: Option<&str>, @@ -1280,27 +1271,27 @@ mod tests { #[test] fn test_parse_cmdline_simple() { - let result = parse_cmdline_with_quotes("cmd arg1 arg2 arg3"); + let result = shlex::split("cmd arg1 arg2 arg3").unwrap_or_default(); assert_eq!(result, vec!["cmd", "arg1", "arg2", "arg3"]); } #[test] fn test_parse_cmdline_with_single_quotes() { - let result = parse_cmdline_with_quotes("cmd 'arg with spaces' arg2"); + let result = shlex::split("cmd 'arg with spaces' arg2").unwrap_or_default(); assert_eq!(result, vec!["cmd", "arg with spaces", "arg2"]); } #[test] fn test_parse_cmdline_with_double_quotes() { - let result = parse_cmdline_with_quotes(r#"cmd "arg with spaces" arg2"#); + let result = + shlex::split(r#"cmd "arg with spaces" arg2"#).unwrap_or_default(); assert_eq!(result, vec!["cmd", "arg with spaces", "arg2"]); } #[test] fn test_parse_cmdline_mixed_quotes() { - let result = parse_cmdline_with_quotes( - r#"cmd 'single quoted' "double quoted" normal"#, - ); + let result = shlex::split(r#"cmd 'single quoted' "double quoted" normal"#) + .unwrap_or_default(); assert_eq!(result, vec![ "cmd", "single quoted", @@ -1311,8 +1302,8 @@ mod tests { #[test] fn test_parse_cmdline_with_equals_in_quotes() { - let result = - parse_cmdline_with_quotes("sudo env 'PATH=/path/with spaces' /bin/cmd"); + let result = shlex::split("sudo env 'PATH=/path/with spaces' /bin/cmd") + .unwrap_or_default(); assert_eq!(result, vec![ "sudo", "env", @@ -1323,25 +1314,25 @@ mod tests { #[test] fn test_parse_cmdline_multiple_spaces() { - let result = parse_cmdline_with_quotes("cmd arg1 arg2"); + let result = shlex::split("cmd arg1 arg2").unwrap_or_default(); assert_eq!(result, vec!["cmd", "arg1", "arg2"]); } #[test] fn test_parse_cmdline_leading_trailing_spaces() { - let result = parse_cmdline_with_quotes(" cmd arg1 arg2 "); + let result = shlex::split(" cmd arg1 arg2 ").unwrap_or_default(); assert_eq!(result, vec!["cmd", "arg1", "arg2"]); } #[test] fn test_parse_cmdline_empty_string() { - let result = parse_cmdline_with_quotes(""); + let result = shlex::split("").unwrap_or_default(); assert_eq!(result, Vec::::default()); } #[test] fn test_parse_cmdline_only_spaces() { - let result = parse_cmdline_with_quotes(" "); + let result = shlex::split(" ").unwrap_or_default(); assert_eq!(result, Vec::::default()); } @@ -1349,7 +1340,7 @@ mod tests { fn test_parse_cmdline_realistic_sudo() { let cmdline = r#"/usr/bin/sudo env 'PATH=/path with spaces' /usr/bin/nh clean all"#; - let result = parse_cmdline_with_quotes(cmdline); + let result = shlex::split(cmdline).unwrap_or_default(); assert_eq!(result, vec![ "/usr/bin/sudo", "env", @@ -1431,30 +1422,31 @@ mod tests { fn test_parse_cmdline_escaped_quotes() { // shlex handles backslash escapes within double quotes let result = - parse_cmdline_with_quotes(r#"cmd "arg with \"escaped\" quotes""#); + shlex::split(r#"cmd "arg with \"escaped\" quotes""#).unwrap_or_default(); assert_eq!(result, vec!["cmd", r#"arg with "escaped" quotes"#]); } #[test] fn test_parse_cmdline_nested_quotes() { // Single quotes inside double quotes are preserved literally - let result = parse_cmdline_with_quotes(r#"cmd "it's a test""#); + let result = shlex::split(r#"cmd "it's a test""#).unwrap_or_default(); assert_eq!(result, vec!["cmd", "it's a test"]); } #[test] fn test_parse_cmdline_backslash_outside_quotes() { // Backslash escapes space outside quotes - let result = parse_cmdline_with_quotes(r"cmd arg\ with\ space"); + let result = shlex::split(r"cmd arg\ with\ space").unwrap_or_default(); assert_eq!(result, vec!["cmd", "arg with space"]); } #[test] fn test_parse_cmdline_nix_store_paths() { // Typical nix store paths should work - let result = parse_cmdline_with_quotes( + let result = shlex::split( "/nix/store/abc123-foo/bin/cmd --flag /nix/store/def456-bar", - ); + ) + .unwrap_or_default(); assert_eq!(result, vec![ "/nix/store/abc123-foo/bin/cmd", "--flag", @@ -1465,14 +1457,15 @@ mod tests { #[test] fn test_parse_cmdline_env_vars_in_quotes() { // Environment variable syntax should be preserved - let result = parse_cmdline_with_quotes(r#"env "PATH=$HOME/bin:$PATH" cmd"#); + let result = + shlex::split(r#"env "PATH=$HOME/bin:$PATH" cmd"#).unwrap_or_default(); assert_eq!(result, vec!["env", "PATH=$HOME/bin:$PATH", "cmd"]); } #[test] fn test_parse_cmdline_unclosed_quote_returns_none() { // shlex returns None for unclosed quotes, we return empty vec - let result = parse_cmdline_with_quotes("cmd 'unclosed"); + let result = shlex::split("cmd 'unclosed").unwrap_or_default(); assert_eq!(result, Vec::::default()); } @@ -1480,7 +1473,7 @@ mod tests { fn test_parse_cmdline_complex_sudo_command() { // Complex sudo command with multiple quoted args let cmdline = r#"/usr/bin/sudo -E env 'HOME=/root' "PATH=/usr/bin" /usr/bin/nh os switch"#; - let result = parse_cmdline_with_quotes(cmdline); + let result = shlex::split(cmdline).unwrap_or_default(); assert_eq!(result, vec![ "/usr/bin/sudo", "-E", diff --git a/src/remote.rs b/src/remote.rs index 16c6c3fb..bebb5e38 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -304,6 +304,20 @@ fn get_flake_flags() -> Vec<&'static str> { } } +/// Convert `OsString` arguments to UTF-8 Strings. +/// +/// Returns an error if any argument is not valid UTF-8. +fn convert_extra_args(extra_args: &[OsString]) -> Result> { + extra_args + .iter() + .map(|s| { + s.to_str() + .map(String::from) + .ok_or_else(|| eyre!("Extra argument is not valid UTF-8: {:?}", s)) + }) + .collect::>>() +} + /// Run a command on a remote host via SSH. fn run_remote_command( host: &RemoteHost, @@ -657,6 +671,10 @@ fn build_on_remote( let drv_with_outputs = format!("{drv_path}^*"); if config.use_nom { + // Check that nom is available before attempting to use it + which::which("nom") + .wrap_err("nom (nix-output-monitor) is required but not found in PATH")?; + // With nom: pipe through nix-output-monitor build_on_remote_with_nom(host, &drv_with_outputs, config) } else { @@ -679,15 +697,7 @@ fn build_on_remote_simple( args.extend(["build", drv_with_outputs, "--print-out-paths"]); // Convert extra args to strings, fail if any are non-UTF-8 - let extra_args_strings: Vec = config - .extra_args - .iter() - .map(|s| { - s.to_str() - .map(String::from) - .ok_or_else(|| eyre!("Extra argument is not valid UTF-8: {:?}", s)) - }) - .collect::>>()?; + let extra_args_strings = convert_extra_args(&config.extra_args)?; for arg in &extra_args_strings { args.push(arg); } @@ -728,15 +738,7 @@ fn build_on_remote_with_nom( ]); // Convert extra args to strings, fail if any are non-UTF-8 - let extra_args_strings: Vec = config - .extra_args - .iter() - .map(|s| { - s.to_str() - .map(String::from) - .ok_or_else(|| eyre!("Extra argument is not valid UTF-8: {:?}", s)) - }) - .collect::>>()?; + let extra_args_strings = convert_extra_args(&config.extra_args)?; for arg in &extra_args_strings { remote_args.push(arg); } From afcd686e83be7964e16eeae95ab7e554652ce7cd Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 29 Dec 2025 12:19:31 +0300 Subject: [PATCH 14/38] remote: implement interrupt handling for remote builds Signed-off-by: NotAShelf Change-Id: I296043d85fd74fc68013dc9f1f3761ea6a6a6964 --- Cargo.lock | 15 ++++- Cargo.toml | 1 + src/remote.rs | 155 ++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 159 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b2c9f481..655f6272 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -432,7 +432,7 @@ dependencies = [ "mio", "parking_lot", "rustix", - "signal-hook", + "signal-hook 0.3.18", "signal-hook-mio", "winapi", ] @@ -1430,6 +1430,7 @@ dependencies = [ "serde_json", "serial_test", "shlex", + "signal-hook 0.4.1", "subprocess", "supports-hyperlinks", "system-configuration", @@ -2230,6 +2231,16 @@ dependencies = [ "signal-hook-registry", ] +[[package]] +name = "signal-hook" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2a37d01603c37b5466f808de79f845c7116049b0579adb70a6b7d47c1fa3a952" +dependencies = [ + "libc", + "signal-hook-registry", +] + [[package]] name = "signal-hook-mio" version = "0.2.4" @@ -2238,7 +2249,7 @@ checksum = "34db1a06d485c9142248b7a054f034b349b212551f3dfd19c94d45a754a217cd" dependencies = [ "libc", "mio", - "signal-hook", + "signal-hook 0.3.18", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 78ef597a..56b5c758 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,6 +48,7 @@ semver = "1.0.27" serde = { features = [ "derive" ], version = "1.0.228" } serde_json = "1.0.145" shlex = "1.3.0" +signal-hook = "0.4.1" subprocess = "0.2.9" supports-hyperlinks = "3.1.0" tempfile = "3.23.0" diff --git a/src/remote.rs b/src/remote.rs index bebb5e38..2d20a5b8 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -1,4 +1,14 @@ -use std::{env, ffi::OsString, path::PathBuf, sync::OnceLock}; +use std::{ + env, + ffi::OsString, + io::Read, + path::PathBuf, + sync::{ + Arc, + OnceLock, + atomic::{AtomicBool, Ordering}, + }, +}; use color_eyre::{ Result, @@ -9,6 +19,35 @@ use tracing::{debug, info}; use crate::{installable::Installable, util::NixVariant}; +/// Global flag indicating whether a SIGINT (Ctrl+C) was received. +static INTERRUPTED: OnceLock> = OnceLock::new(); + +/// Get or initialize the interrupt flag. +/// +/// Returns a reference to the shared interrupt flag, initializing it on first +/// call. +fn get_interrupt_flag() -> &'static Arc { + INTERRUPTED.get_or_init(|| Arc::new(AtomicBool::new(false))) +} + +/// Register a SIGINT handler that sets the global interrupt flag. +/// +/// This function is idempotent - multiple calls are safe and will not +/// create multiple handlers. Uses `signal_hook::flag::register` which +/// is async-signal-safe. +/// +/// # Errors +/// +/// Returns an error if the signal handler cannot be registered. +fn register_interrupt_handler() -> Result<()> { + use signal_hook::{consts::SIGINT, flag}; + + flag::register(SIGINT, Arc::clone(get_interrupt_flag())) + .context("Failed to register SIGINT handler")?; + + Ok(()) +} + /// Guard that cleans up SSH `ControlMaster` sockets on drop. /// /// This ensures SSH control connections are properly closed when remote @@ -151,6 +190,12 @@ pub struct RemoteHost { impl RemoteHost { /// Get the hostname part (without user@ prefix). + /// + /// # Panics + /// + /// This function will never panic in practice because `rsplit('@').next()` + /// always returns at least one element (the original string if no '@' + /// exists). #[must_use] pub fn hostname(&self) -> &str { #[allow(clippy::unwrap_used)] @@ -689,7 +734,10 @@ fn build_on_remote_simple( drv_with_outputs: &str, config: &RemoteBuildConfig, ) -> Result { - // Get flake flags for the remote nix command + // Register interrupt handler at start + register_interrupt_handler()?; + + let ssh_opts = get_ssh_opts(); let flake_flags = get_flake_flags(); let mut args: Vec<&str> = vec!["nix"]; @@ -702,11 +750,70 @@ fn build_on_remote_simple( args.push(arg); } - let result = run_remote_command(host, &args, true)? - .ok_or_else(|| eyre!("Remote build returned no output"))?; + // Quote for shell + let quoted_remote: Vec = + args.iter().map(|a| shell_quote(a)).collect(); + + // Build SSH command with stdout capture + let mut ssh_cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + ssh_cmd = ssh_cmd.arg(opt); + } + ssh_cmd = ssh_cmd + .arg(&host.host) + .arg(quoted_remote.join(" ")) + .stdout(Redirection::Pipe) + .stderr(Redirection::Pipe); + + // Execute with popen to get process handle + let mut process = ssh_cmd.popen()?; + + // Wait for completion with interrupt checking + let exit_status = loop { + match process.wait_timeout(std::time::Duration::from_millis(100))? { + Some(status) => break status, + None => { + // Check interrupt flag while waiting + if get_interrupt_flag().load(Ordering::Relaxed) { + debug!("Interrupt detected, killing SSH process"); + let _ = process.kill(); + let _ = process.wait(); // Reap zombie + bail!("Operation interrupted by user"); + } + }, + } + }; + + // Check if we were interrupted between wait completion and here + if get_interrupt_flag().load(Ordering::Relaxed) { + debug!("Interrupt detected after process completion"); + bail!("Operation interrupted by user"); + } + + // Check exit status + if !exit_status.success() { + let stderr = process + .stderr + .take() + .and_then(|mut e| { + let mut s = String::new(); + e.read_to_string(&mut s).ok().map(|_| s) + }) + .unwrap_or_else(|| String::from("(no stderr)")); + bail!("Remote command failed: {}", stderr); + } + + // Read stdout + let stdout = process + .stdout + .take() + .ok_or_else(|| eyre!("Failed to capture stdout"))?; + let mut reader = std::io::BufReader::new(stdout); + let mut output = String::new(); + reader.read_to_string(&mut output)?; // --print-out-paths may return multiple lines; take first - let out_path = result + let out_path = output .lines() .next() .ok_or_else(|| eyre!("Remote build returned empty output"))? @@ -723,6 +830,9 @@ fn build_on_remote_with_nom( drv_with_outputs: &str, config: &RemoteBuildConfig, ) -> Result { + // Register interrupt handler at start + register_interrupt_handler()?; + let ssh_opts = get_ssh_opts(); let flake_flags = get_flake_flags(); @@ -772,9 +882,20 @@ fn build_on_remote_with_nom( let mut processes = pipeline.popen().wrap_err("Remote build with nom failed")?; - // Wait for all processes to finish + // Wait for all processes to finish, checking for interrupts for proc in &mut processes { proc.wait()?; + + // Check interrupt flag after each process wait + if get_interrupt_flag().load(Ordering::Relaxed) { + debug!("Interrupt detected during build with nom"); + // Kill remaining local processes - this will cause SSH to terminate + // the remote command automatically + for p in &mut processes { + let _ = p.kill(); + } + bail!("Operation interrupted by user"); + } } // Check the exit status of the FIRST process (ssh -> nix build) @@ -796,8 +917,16 @@ fn build_on_remote_with_nom( query_args.extend(flake_flags.iter().copied()); query_args.extend(["build", drv_with_outputs, "--print-out-paths"]); - let result = run_remote_command(host, &query_args, true)? - .ok_or_else(|| eyre!("Failed to get output path after build"))?; + let result = run_remote_command(host, &query_args, true); + + // Check if interrupted during query + if get_interrupt_flag().load(Ordering::Relaxed) { + debug!("Interrupt detected during output path query"); + bail!("Operation interrupted by user"); + } + + let result = + result?.ok_or_else(|| eyre!("Failed to get output path after build"))?; let out_path = result .lines() @@ -812,6 +941,12 @@ fn build_on_remote_with_nom( #[cfg(test)] mod tests { + #![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + reason = "Fine in tests" + )] use proptest::prelude::*; use serial_test::serial; @@ -839,7 +974,7 @@ mod tests { #[test] fn hostname_with_user(user in "[a-zA-Z0-9_]+", hostname in "[a-zA-Z0-9_.-]+") { - let full = format!("{}@{}", user, hostname); + let full = format!("{user}@{hostname}"); let host = RemoteHost { host: full }; prop_assert_eq!(host.hostname(), hostname); } @@ -854,7 +989,7 @@ mod tests { #[test] fn parse_valid_user_at_hostname(user in "[a-zA-Z0-9_]+", hostname in "[a-zA-Z0-9_.-]+") { - let full = format!("{}@{}", user, hostname); + let full = format!("{user}@{hostname}"); let result = RemoteHost::parse(&full); prop_assert!(result.is_ok()); let host = result.unwrap(); From 98d23918bb441a6828a0695ec963f893b856b1f3 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 29 Dec 2025 18:49:06 +0300 Subject: [PATCH 15/38] remote: support ipv6; fix minor quoting issues & add more tests Here's to you, Dami. Signed-off-by: NotAShelf Change-Id: I49e84a3efab65791800348c92b1fc5da6a6a6964 --- Cargo.lock | 7 ++ Cargo.toml | 1 + src/remote.rs | 199 ++++++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 176 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 655f6272..7cb150f1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1439,6 +1439,7 @@ dependencies = [ "thiserror 2.0.17", "tracing", "tracing-subscriber", + "urlencoding", "which", "yansi", ] @@ -2788,6 +2789,12 @@ dependencies = [ "serde", ] +[[package]] +name = "urlencoding" +version = "2.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "daf8dba3b7eb870caf1ddeed7bc9d2a049f3cfdfae7cb521b087cc33ae4c49da" + [[package]] name = "utf-8" version = "0.7.6" diff --git a/Cargo.toml b/Cargo.toml index 56b5c758..f3a5d9d6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,6 +56,7 @@ textwrap = { features = [ "terminal_size" ], version = "0.16.2" } thiserror = "2.0.17" tracing = "0.1.41" tracing-subscriber = { features = [ "env-filter", "registry", "std" ], version = "0.3.20" } +urlencoding = "2.1.3" which = "8.0.0" yansi = "1.0.1" diff --git a/src/remote.rs b/src/remote.rs index 2d20a5b8..18c929a5 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -8,6 +8,7 @@ use std::{ OnceLock, atomic::{AtomicBool, Ordering}, }, + time::Duration, }; use color_eyre::{ @@ -30,6 +31,9 @@ fn get_interrupt_flag() -> &'static Arc { INTERRUPTED.get_or_init(|| Arc::new(AtomicBool::new(false))) } +/// Cache for signal handler registration status. +static HANDLER_REGISTERED: OnceLock<()> = OnceLock::new(); + /// Register a SIGINT handler that sets the global interrupt flag. /// /// This function is idempotent - multiple calls are safe and will not @@ -42,9 +46,18 @@ fn get_interrupt_flag() -> &'static Arc { fn register_interrupt_handler() -> Result<()> { use signal_hook::{consts::SIGINT, flag}; + if HANDLER_REGISTERED.get().is_some() { + return Ok(()); + } + + // Not registered yet - register it flag::register(SIGINT, Arc::clone(get_interrupt_flag())) .context("Failed to register SIGINT handler")?; + // Mark as registered (race condition here is benign - worst case we register + // twice, but both handlers will set the same flag which is fine) + let _ = HANDLER_REGISTERED.set(()); + Ok(()) } @@ -245,12 +258,38 @@ impl RemoteHost { hostname_part ); } + + // Check for colons, but allow them in bracketed IPv6 addresses if hostname_part.contains(':') { - bail!( - "Invalid hostname '{}': contains ':'. Ports should be specified via \ - NIX_SSHOPTS=\"-p 2222\" or ~/.ssh/config", - hostname_part - ); + // Check if this is a bracketed IPv6 address + let is_bracketed_ipv6 = + hostname_part.starts_with('[') && hostname_part.contains(']'); + + if !is_bracketed_ipv6 { + bail!( + "Invalid hostname '{}': contains ':'. Ports should be specified via \ + NIX_SSHOPTS=\"-p 2222\" or ~/.ssh/config", + hostname_part + ); + } + + // Validate bracket matching for IPv6 + if !hostname_part.ends_with(']') { + bail!( + "Invalid IPv6 address '{}': contains characters after closing \ + bracket", + hostname_part + ); + } + + let open_count = hostname_part.matches('[').count(); + let close_count = hostname_part.matches(']').count(); + if open_count != 1 || close_count != 1 { + bail!( + "Invalid IPv6 address '{}': mismatched brackets", + hostname_part + ); + } } Ok(Self { @@ -371,21 +410,14 @@ fn run_remote_command( ) -> Result> { let ssh_opts = get_ssh_opts(); - // Quote args for shell (matching nixos-rebuild-ng's shlex.quote) - let quoted_args: Vec = args.iter().map(|a| shell_quote(a)).collect(); - - debug!( - "Running remote command on {}: {}", - host, - quoted_args.join(" ") - ); + debug!("Running remote command on {}: {}", host, args.join(" ")); let mut cmd = Exec::cmd("ssh"); for opt in &ssh_opts { cmd = cmd.arg(opt); } cmd = cmd.arg(host.host()).arg("--"); - for arg in "ed_args { + for arg in args { cmd = cmd.arg(arg); } @@ -784,12 +816,6 @@ fn build_on_remote_simple( } }; - // Check if we were interrupted between wait completion and here - if get_interrupt_flag().load(Ordering::Relaxed) { - debug!("Interrupt detected after process completion"); - bail!("Operation interrupted by user"); - } - // Check exit status if !exit_status.success() { let stderr = process @@ -882,19 +908,38 @@ fn build_on_remote_with_nom( let mut processes = pipeline.popen().wrap_err("Remote build with nom failed")?; - // Wait for all processes to finish, checking for interrupts + // Use wait_timeout in a polling loop to check interrupt flag every 100ms + let poll_interval = Duration::from_millis(100); + for proc in &mut processes { - proc.wait()?; - - // Check interrupt flag after each process wait - if get_interrupt_flag().load(Ordering::Relaxed) { - debug!("Interrupt detected during build with nom"); - // Kill remaining local processes - this will cause SSH to terminate - // the remote command automatically - for p in &mut processes { - let _ = p.kill(); + #[allow(clippy::needless_continue, reason = "Better for explicitness")] + loop { + // Check interrupt flag before waiting + if get_interrupt_flag().load(Ordering::Relaxed) { + debug!("Interrupt detected during build with nom"); + // Kill remaining local processes - this will cause SSH to terminate + // the remote command automatically + for p in &mut processes { + let _ = p.kill(); + let _ = p.wait(); // Reap zombie + } + bail!("Operation interrupted by user"); + } + + // Poll process with timeout + match proc.wait_timeout(poll_interval)? { + Some(_) => { + // Process has exited, exit status is automatically cached in the + // Popen struct Move to next process + break; + }, + + None => { + // Timeout elapsed, process still running - loop continues + // and will check interrupt flag again + continue; + }, } - bail!("Operation interrupted by user"); } } @@ -1058,6 +1103,89 @@ mod tests { assert!(err.to_string().contains("NIX_SSHOPTS")); } + #[test] + fn test_parse_ipv6_bracketed() { + let host = RemoteHost::parse("[2001:db8::1]").expect("should parse IPv6"); + assert_eq!(host.host(), "[2001:db8::1]"); + assert_eq!(host.hostname(), "[2001:db8::1]"); + } + + #[test] + fn test_parse_ipv6_with_user() { + let host = RemoteHost::parse("root@[2001:db8::1]") + .expect("should parse IPv6 with user"); + assert_eq!(host.host(), "root@[2001:db8::1]"); + assert_eq!(host.hostname(), "[2001:db8::1]"); + } + + #[test] + fn test_parse_ipv6_with_zone_id() { + let host = + RemoteHost::parse("[fe80::1%eth0]").expect("should parse IPv6 with zone"); + assert_eq!(host.host(), "[fe80::1%eth0]"); + } + + #[test] + fn test_parse_ipv6_ssh_uri() { + let host = RemoteHost::parse("ssh://[2001:db8::1]") + .expect("should parse IPv6 SSH URI"); + assert_eq!(host.host(), "[2001:db8::1]"); + } + + #[test] + fn test_parse_ipv6_ssh_uri_with_user() { + let host = RemoteHost::parse("ssh://root@[2001:db8::1]") + .expect("should parse IPv6 SSH URI with user"); + assert_eq!(host.host(), "root@[2001:db8::1]"); + } + + #[test] + fn test_parse_ipv6_localhost() { + let host = RemoteHost::parse("[::1]").expect("should parse IPv6 localhost"); + assert_eq!(host.host(), "[::1]"); + } + + #[test] + fn test_parse_ipv6_compressed() { + let host = + RemoteHost::parse("[2001:db8::]").expect("should parse compressed IPv6"); + assert_eq!(host.host(), "[2001:db8::]"); + } + + #[test] + fn test_parse_ipv6_unbracketed_rejected() { + // Bare IPv6 without brackets should be rejected + let result = RemoteHost::parse("2001:db8::1"); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("NIX_SSHOPTS")); + } + + #[test] + fn test_parse_ipv6_mismatched_brackets_rejected() { + assert!(RemoteHost::parse("[2001:db8::1").is_err()); + assert!(RemoteHost::parse("2001:db8::1]").is_err()); + } + + #[test] + fn test_parse_ipv6_extra_brackets_rejected() { + assert!(RemoteHost::parse("[[2001:db8::1]]").is_err()); + assert!(RemoteHost::parse("[2001:db8::[1]]").is_err()); + } + + #[test] + fn test_parse_ipv6_with_port_rejected() { + // IPv6 with port syntax should be rejected (use NIX_SSHOPTS) + let result = RemoteHost::parse("[2001:db8::1]:22"); + assert!(result.is_err()); + } + + #[test] + fn test_parse_ipv6_chars_after_bracket_rejected() { + // Characters after closing bracket should be rejected + let result = RemoteHost::parse("[2001:db8::1]extra"); + assert!(result.is_err()); + } + #[test] fn test_shell_quote_simple() { assert_eq!(shell_quote("simple"), "simple"); @@ -1137,6 +1265,15 @@ mod tests { } } + #[test] + fn test_shell_quote_behavior() { + // Verify shell_quote adds quotes when needed + assert_eq!(shell_quote("simple"), "simple"); + assert_eq!(shell_quote("has space"), "'has space'"); + // shlex::try_quote uses double quotes when string contains single quote + assert_eq!(shell_quote("has'quote"), "\"has'quote\""); + } + #[test] fn test_shell_quote_roundtrip() { // Test that quoting and then parsing gives back the original From a556c71dc56c58c64178fae75ee00aa3bbd80b05 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Mon, 29 Dec 2025 23:39:08 +0300 Subject: [PATCH 16/38] nixos: validate essential files in'haphazard' remote build semantics I don't like NixOS' remote builds. Signed-off-by: NotAShelf Change-Id: Iea646b3b47926536a1bb1a70e3d776fa6a6a6964 --- src/nixos.rs | 123 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 119 insertions(+), 4 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index c390aa48..8d384e67 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -207,6 +207,15 @@ impl OsRebuildActivateArgs { } } + // Validate system closure before activation + if let Some(target_host) = &self.rebuild.target_host { + // For remote activation, validate on the remote host + validate_system_closure_remote(target_profile, target_host)?; + } else { + // For local activation, validate locally + validate_system_closure(target_profile)?; + } + let switch_to_configuration = target_profile .canonicalize() .context("Failed to resolve output path")? @@ -215,10 +224,6 @@ impl OsRebuildActivateArgs { .canonicalize() .context("Failed to resolve switch-to-configuration path")?; - if !switch_to_configuration.exists() { - return Err(missing_switch_to_configuration_error()); - } - let canonical_out_path = switch_to_configuration.to_str().ok_or_else(|| { eyre!("switch-to-configuration path contains invalid UTF-8") @@ -228,6 +233,7 @@ impl OsRebuildActivateArgs { let variant_label = match variant { Test => "test", Switch => "switch", + #[allow(clippy::unreachable, reason = "Should never happen.")] _ => unreachable!(), }; @@ -809,6 +815,113 @@ fn run_vm(out_path: &Path) -> Result<()> { Ok(()) } +/// Validates that essential files exist in the system closure. +/// +/// Checks for few critical files that must be present in a complete NixOS +/// system. This is essentially in-line with what nixos-rebuild-ng checks for. +/// +/// - bin/switch-to-configuration: activation script +/// - nixos-version: system version identifier +/// - init: system init script +/// - sw/bin: system path binaries +/// +/// # Returns +/// +/// `Ok(())` if all files exist, or an error listing missing files. +fn validate_system_closure(system_path: &Path) -> Result<()> { + let essential_files = [ + ("bin/switch-to-configuration", "activation script"), + ("nixos-version", "system version identifier"), + ("init", "system init script"), + ("sw/bin", "system path"), + ]; + + let mut missing = Vec::new(); + for (file, description) in &essential_files { + let path = system_path.join(file); + if !path.exists() { + missing.push(format!(" - {file} ({description})")); + } + } + + if !missing.is_empty() { + let missing_list = missing.join("\n"); + return Err(eyre!( + "System closure validation failed. Missing essential files:\n{}\n\nThis \ + typically happens when:\n1. 'system.switch.enable' is set to false in \ + your configuration\n2. The build was incomplete or corrupted\n3. \ + You're using an incomplete derivation\n\nTo fix this:\n1. Check if \ + 'system.switch.enable = false' is set and remove it\n2. Rebuild your \ + system configuration\n3. If the problem persists, verify your system \ + closure is complete\n\nSystem path checked: {}", + missing_list, + system_path.display() + )); + } + + Ok(()) +} + +/// Validates essential files on a remote host via SSH. +/// +/// Similar to [`validate_system_closure`] but executes checks on a remote host. +fn validate_system_closure_remote( + system_path: &Path, + target_host: &str, +) -> Result<()> { + let essential_files = [ + ("bin/switch-to-configuration", "activation script"), + ("nixos-version", "system version identifier"), + ("init", "system init script"), + ("sw/bin", "system path"), + ]; + + let mut missing = Vec::new(); + for (file, description) in &essential_files { + let remote_path = system_path.join(file); + let path_str = remote_path.to_str().ok_or_else(|| { + eyre!("System path is not valid UTF-8: {}", remote_path.display()) + })?; + + // Use SSH to test if file exists on remote host + let check_result = std::process::Command::new("ssh") + .args([target_host, "test", "-e", path_str]) + .output(); + + match check_result { + Ok(output) if !output.status.success() => { + missing.push(format!(" - {file} ({description})")); + }, + Err(e) => { + return Err(eyre!( + "Failed to check file existence on remote host {}: {}", + target_host, + e + )); + }, + _ => {}, // File exists + } + } + + if !missing.is_empty() { + let missing_list = missing.join("\n"); + return Err(eyre!( + "System closure validation failed on remote host {}. Missing essential \ + files:\n{}\n\nThis typically happens when:\n1. 'system.switch.enable' \ + is set to false in your configuration\n2. The build was incomplete or \ + corrupted\n3. You're using an incomplete derivation\n\nTo fix \ + this:\n1. Check if 'system.switch.enable = false' is set and remove \ + it\n2. Rebuild your system configuration\n3. If the problem persists, \ + verify your system closure is complete\n\nSystem path checked: {}", + target_host, + missing_list, + system_path.display() + )); + } + + Ok(()) +} + /// Returns an error indicating that the 'switch-to-configuration' binary is /// missing, along with common reasons and solutions. fn missing_switch_to_configuration_error() -> color_eyre::eyre::Report { @@ -856,10 +969,12 @@ fn get_nh_os_flake_env() -> Result> { /// `bypass_root_check` is true). /// /// # Arguments +/// /// * `bypass_root_check` - If true, bypasses the root check and assumes no /// elevation is needed. /// /// # Errors +/// /// Returns an error if `bypass_root_check` is false and the user is root, /// as `nh os` subcommands should not be run directly as root. fn has_elevation_status(bypass_root_check: bool) -> Result { From 96db8f3e057f82a56be3a8f474ea65c030ef1913 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 1 Jan 2026 02:43:03 +0300 Subject: [PATCH 17/38] nixos: properly escape paths Signed-off-by: NotAShelf Change-Id: Iffe2d63b55ee4a9bab41bb6184184add6a6a6964 --- src/nixos.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index 8d384e67..7ee78c67 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -883,9 +883,15 @@ fn validate_system_closure_remote( eyre!("System path is not valid UTF-8: {}", remote_path.display()) })?; - // Use SSH to test if file exists on remote host + // Use SSH to test if file exists on remote host. We need to quote the path + // to prevent shell interpretation of special characters, such as the ones + // in `.drv^*`. + let quoted_path = shlex::try_quote(path_str).map_err(|_| { + eyre!("Failed to quote path for shell: {}", remote_path.display()) + })?; + let check_result = std::process::Command::new("ssh") - .args([target_host, "test", "-e", path_str]) + .args([target_host, "test", "-e", "ed_path]) .output(); match check_result { From 0f676a733d232f0701b81a60f48abe205379a543 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 1 Jan 2026 02:54:51 +0300 Subject: [PATCH 18/38] remote: properly quote SSH command arguments; improve error handling I've cramped too many things in one commit again. Guess it's better to explain, generally, what has changed. The primary "fix" that this commit addresses is SSH command construction by shell-quoting *all* arguments before passing them to the remote shell, instead of using SSH's `'--'` separator with individual args. This helps ensure special chars, spaces and shell metachars in paths (I blame Nix) or arguments args are handled correctly. Additionally, there are a few improvements to the "robustness" of remote copying: previously we'd attempt to copy from build-host to target-host, which is fine, but we failed fast instead of attempting the logically correct behaviour which is to COPY BACK and THEN relay to the target host. If remote-to-remote copy fails, the system now logs a warning and relays through localhost instead of failing. This makes nh's *fallback behaviour* (but not the first pass) consistent with nixos-rebuild. Other than that, some build command consolidation and moderately large documentation tweaks. The code is getting out of hand, so I'd like to merge this and be done as soon as possible. God that was a long summary. Hope someone gets to read this. Signed-off-by: NotAShelf Change-Id: Ib540fc287f1b1e22e2b68b9c9c7b03046a6a6964 --- src/remote.rs | 196 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 125 insertions(+), 71 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index 18c929a5..64a15818 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -16,7 +16,7 @@ use color_eyre::{ eyre::{Context, bail, eyre}, }; use subprocess::{Exec, ExitStatus, Redirection}; -use tracing::{debug, info}; +use tracing::{debug, info, warn}; use crate::{installable::Installable, util::NixVariant}; @@ -50,12 +50,13 @@ fn register_interrupt_handler() -> Result<()> { return Ok(()); } - // Not registered yet - register it + // Not registered yet, register it flag::register(SIGINT, Arc::clone(get_interrupt_flag())) .context("Failed to register SIGINT handler")?; - // Mark as registered (race condition here is benign - worst case we register - // twice, but both handlers will set the same flag which is fine) + // Mark as registered + // The race condition here is benign. Worst case, we register twice, but both + // handlers will set the same flag which is fine let _ = HANDLER_REGISTERED.set(()); Ok(()) @@ -191,6 +192,7 @@ fn get_ssh_control_dir() -> &'static PathBuf { /// A parsed remote host specification. /// /// Handles various formats: +/// /// - `hostname` /// - `user@hostname` /// - `ssh://[user@]hostname` (scheme stripped) @@ -253,9 +255,8 @@ impl RemoteHost { let hostname_part = host.rsplit('@').next().unwrap_or(host); if hostname_part.contains('/') { bail!( - "Invalid hostname '{}': contains '/'. Did you mean to use a bare \ - hostname?", - hostname_part + "Invalid hostname '{hostname_part}': contains '/'. Did you mean to \ + use a bare hostname?" ); } @@ -326,10 +327,12 @@ fn get_default_ssh_opts() -> Vec { ] } -/// Shell-quote a string for safe use in SSH commands. +/// Shell-quote a string for safe passing through SSH to remote shell. fn shell_quote(s: &str) -> String { + // Use shlex::try_quote for battle-tested quoting + // Returns Cow::Borrowed if no quoting needed, Cow::Owned if quoted shlex::try_quote(s).map_or_else( - |_| format!("'{}'", s.replace('\'', "'\\''")), + |_| format!("'{}'", s.replace('\'', r"'\''")), std::borrow::Cow::into_owned, ) } @@ -366,7 +369,7 @@ fn get_nix_sshopts_env() -> String { default_opts.join(" ") } else { // Append our defaults to user options - // Note: We preserve user options as-is since nix-copy-closure + // NOTE: We preserve user options as-is since nix-copy-closure // does simple whitespace splitting format!("{} {}", user_opts, default_opts.join(" ")) } @@ -376,8 +379,14 @@ fn get_nix_sshopts_env() -> String { /// /// Returns the flags needed for `--extra-experimental-features "nix-command /// flakes"` based on the detected Nix variant: +/// /// - Determinate Nix: No flags needed (features are stable) /// - Nix/Lix: Returns `["--extra-experimental-features", "nix-command flakes"]` +/// +/// Technically this is inconsistent with our default behaviour, which is to +/// *warn* on missing features but since this is for *remote deployment* it is +/// safer to assist the user instead. Without those features, remote deployment +/// may never succeed. fn get_flake_flags() -> Vec<&'static str> { let variant = crate::util::get_nix_variant(); match variant { @@ -412,14 +421,13 @@ fn run_remote_command( debug!("Running remote command on {}: {}", host, args.join(" ")); + let quoted_args: Vec = args.iter().map(|s| shell_quote(s)).collect(); + let remote_cmd = quoted_args.join(" "); let mut cmd = Exec::cmd("ssh"); for opt in &ssh_opts { cmd = cmd.arg(opt); } - cmd = cmd.arg(host.host()).arg("--"); - for arg in args { - cmd = cmd.arg(arg); - } + cmd = cmd.arg(host.host()).arg(&remote_cmd); if capture_stdout { cmd = cmd.stdout(Redirection::Pipe).stderr(Redirection::Pipe); @@ -632,6 +640,27 @@ fn eval_drv_path(installable: &Installable) -> Result { } /// Configuration for a remote build operation. +/// +/// # Host Interaction Semantics +/// +/// The behavior depends on which hosts are specified: +/// +/// | `build_host` | `target_host` | Behavior | +/// |--------------|---------------|----------| +/// | Some(H2) | None | Build on H2, copy result to localhost | +/// | Some(H2) | Some(H2) | Build on H2, no copy (build host = target) | +/// | Some(H2) | Some(H3) | Build on H2, try direct copy to H3; if that fails, relay through localhost | +/// +/// When `build_host` and `target_host` differ, the code attempts a direct +/// copy between remotes first. If this fails (common when the hosts can't +/// see each other), it falls back to relaying through localhost: +/// +/// - Direct: Host2 -> Host3 +/// - Fallback: Host2 -> Host1 (localhost) → Host3 +/// +/// If `out_link` is requested in `build_remote()`, the result is always +/// copied to localhost regardless of whether a direct copy succeeded, +/// because the symlink must point to a local store path. #[derive(Debug, Clone)] pub struct RemoteBuildConfig { /// The host to build on @@ -687,6 +716,7 @@ pub fn build_remote( // // Optimizes copy paths based on hostname comparison: // - When build_host != target_host: copy build -> target, then build -> local + // if needed // - When build_host == target_host: skip redundant copies, only copy to local // if out-link is needed // - When target_host is None: always copy build -> local @@ -695,34 +725,46 @@ pub fn build_remote( .as_ref() .is_some_and(|th| th.hostname() == build_host.hostname()); - // Copy from build_host to target_host if they differ - if let Some(ref target_host) = config.target_host { - if target_is_build_host { + let need_local_copy = match &config.target_host { + None => true, + Some(_target_host) if target_is_build_host => { debug!( "Skipping copy from build host to target host (same host: {})", build_host.hostname() ); - } else { - copy_closure_between_remotes( + out_link.is_some() + }, + Some(target_host) => { + match copy_closure_between_remotes( build_host, target_host, &out_path, use_substitutes, - )?; - } - } - - // Copy to localhost only when necessary to avoid ping-pong effect - let need_local_copy = - config.target_host.is_none() || !target_is_build_host || out_link.is_some(); + ) { + Ok(()) => { + debug!( + "Successfully copied closure directly from {} to {}", + build_host.hostname(), + target_host.hostname() + ); + out_link.is_some() + }, + Err(e) => { + warn!( + "Direct copy from {} to {} failed: {}. Will relay through \ + localhost.", + build_host.hostname(), + target_host.hostname(), + e + ); + true + }, + } + }, + }; if need_local_copy { copy_closure_from(build_host, &out_path, use_substitutes)?; - } else { - debug!( - "Skipping copy to localhost (build_host == target_host, no out-link \ - needed)" - ); } // Create local out-link if requested @@ -760,6 +802,26 @@ fn build_on_remote( } } +/// Build the argument list for remote nix build commands. +/// Returns owned strings to avoid lifetime issues with `extra_args`. +fn build_nix_command( + drv_with_outputs: &str, + extra_flags: &[&str], + extra_args: &[OsString], +) -> Result> { + let flake_flags = get_flake_flags(); + let extra_args_strings = convert_extra_args(extra_args)?; + + let mut args = vec!["nix".to_string()]; + args.extend(flake_flags.iter().map(|s| (*s).to_string())); + args.push("build".to_string()); + args.push(drv_with_outputs.to_string()); + args.extend(extra_flags.iter().map(|s| (*s).to_string())); + args.extend(extra_args_strings); + + Ok(args) +} + /// Build on remote without nom - just capture output. fn build_on_remote_simple( host: &RemoteHost, @@ -770,30 +832,28 @@ fn build_on_remote_simple( register_interrupt_handler()?; let ssh_opts = get_ssh_opts(); - let flake_flags = get_flake_flags(); - - let mut args: Vec<&str> = vec!["nix"]; - args.extend(flake_flags); - args.extend(["build", drv_with_outputs, "--print-out-paths"]); - - // Convert extra args to strings, fail if any are non-UTF-8 - let extra_args_strings = convert_extra_args(&config.extra_args)?; - for arg in &extra_args_strings { - args.push(arg); - } - // Quote for shell - let quoted_remote: Vec = - args.iter().map(|a| shell_quote(a)).collect(); + let args = build_nix_command( + drv_with_outputs, + &["--print-out-paths"], + &config.extra_args, + )?; + let arg_refs: Vec<&str> = + args.iter().map(std::string::String::as_str).collect(); // Build SSH command with stdout capture + // Quote all arguments for safe shell passing + let quoted_args: Vec = + arg_refs.iter().map(|s| shell_quote(s)).collect(); + let remote_cmd = quoted_args.join(" "); + let mut ssh_cmd = Exec::cmd("ssh"); for opt in &ssh_opts { ssh_cmd = ssh_cmd.arg(opt); } ssh_cmd = ssh_cmd .arg(&host.host) - .arg(quoted_remote.join(" ")) + .arg(&remote_cmd) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe); @@ -860,38 +920,31 @@ fn build_on_remote_with_nom( register_interrupt_handler()?; let ssh_opts = get_ssh_opts(); - let flake_flags = get_flake_flags(); // Build the remote command with JSON output for nom - let mut remote_args: Vec<&str> = vec!["nix"]; - remote_args.extend(flake_flags.iter().copied()); - remote_args.extend([ - "build", + let remote_args = build_nix_command( drv_with_outputs, - "--log-format", - "internal-json", - "--verbose", - ]); - - // Convert extra args to strings, fail if any are non-UTF-8 - let extra_args_strings = convert_extra_args(&config.extra_args)?; - for arg in &extra_args_strings { - remote_args.push(arg); - } + &["--log-format", "internal-json", "--verbose"], + &config.extra_args, + )?; + let arg_refs: Vec<&str> = remote_args + .iter() + .map(std::string::String::as_str) + .collect(); - // Quote for shell + // Build SSH command + // Quote all arguments for safe shell passing let quoted_remote: Vec = - remote_args.iter().map(|a| shell_quote(a)).collect(); + arg_refs.iter().map(|s| shell_quote(s)).collect(); + let remote_cmd = quoted_remote.join(" "); - // Build SSH command let mut ssh_cmd = Exec::cmd("ssh"); for opt in &ssh_opts { ssh_cmd = ssh_cmd.arg(opt); } ssh_cmd = ssh_cmd .arg(host.host()) - .arg("--") - .args("ed_remote) + .arg(&remote_cmd) .stdout(Redirection::Pipe) .stderr(Redirection::Merge); @@ -944,7 +997,7 @@ fn build_on_remote_with_nom( } // Check the exit status of the FIRST process (ssh -> nix build) - // This is the one that matters - if the remote build fails, we should fail + // This is the one that matters. If the remote build fails, we should fail // too if let Some(ssh_proc) = processes.first() { if let Some(exit_status) = ssh_proc.exit_status() { @@ -958,11 +1011,12 @@ fn build_on_remote_with_nom( // nom consumed the output, so we need to query the output path separately // Run nix build again with --print-out-paths (it will be a no-op since // already built) - let mut query_args: Vec<&str> = vec!["nix"]; - query_args.extend(flake_flags.iter().copied()); - query_args.extend(["build", drv_with_outputs, "--print-out-paths"]); + let query_args = + build_nix_command(drv_with_outputs, &["--print-out-paths"], &[])?; + let query_refs: Vec<&str> = + query_args.iter().map(std::string::String::as_str).collect(); - let result = run_remote_command(host, &query_args, true); + let result = run_remote_command(host, &query_refs, true); // Check if interrupted during query if get_interrupt_flag().load(Ordering::Relaxed) { From eea7516840e513193d316853f4d3cd70ef0cd528 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 1 Jan 2026 22:46:25 +0300 Subject: [PATCH 19/38] remote: allow disabling path validation; better errors Signed-off-by: NotAShelf Change-Id: I22149e081658df72d8ad7f29df184a196a6a6964 --- src/commands.rs | 2 +- src/interface.rs | 4 +++ src/nixos.rs | 71 ++++++++++++++++++++++++++++++++++++------------ src/remote.rs | 11 ++++++++ 4 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index ac4a2c2d..9bebfcaf 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -301,7 +301,7 @@ impl Command { if self.elevate.is_some() && cfg!(target_os = "macos") { self .env_vars - .insert("HOME".to_string(), EnvAction::Set("".to_string())); + .insert("HOME".to_string(), EnvAction::Set(String::new())); } // Preserve all variables in PRESERVE_ENV if present diff --git a/src/interface.rs b/src/interface.rs index c93a0cef..0faab718 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -239,6 +239,10 @@ pub struct OsRebuildArgs { /// Build the configuration to a different host over ssh #[arg(long)] pub build_host: Option, + + /// Skip pre-activation system validation checks + #[arg(long)] + pub no_validate: bool, } #[derive(Debug, Args)] diff --git a/src/nixos.rs b/src/nixos.rs index 7ee78c67..11257aa1 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -207,18 +207,38 @@ impl OsRebuildActivateArgs { } } - // Validate system closure before activation - if let Some(target_host) = &self.rebuild.target_host { - // For remote activation, validate on the remote host - validate_system_closure_remote(target_profile, target_host)?; + // Validate system closure before activation (unless bypassed) + // Resolve target_profile to actual store path for validation and activation + let resolved_profile = target_profile + .canonicalize() + .context("Failed to resolve output path to actual store path")?; + + let should_skip = + self.rebuild.no_validate || std::env::var("NH_NO_VALIDATE").is_ok(); + + if should_skip { + warn!( + "Skipping pre-activation validation (--no-validate or NH_NO_VALIDATE \ + set)" + ); + warn!( + "This may result in activation failures if the system closure is \ + incomplete" + ); + } else if let Some(target_host) = &self.rebuild.target_host { + // For remote activation, validate on the remote host using the resolved + // store path + validate_system_closure_remote( + &resolved_profile, + target_host, + self.rebuild.build_host.as_deref(), + )?; } else { // For local activation, validate locally - validate_system_closure(target_profile)?; + validate_system_closure(&resolved_profile)?; } - let switch_to_configuration = target_profile - .canonicalize() - .context("Failed to resolve output path")? + let switch_to_configuration = resolved_profile .join("bin") .join("switch-to-configuration") .canonicalize() @@ -817,7 +837,7 @@ fn run_vm(out_path: &Path) -> Result<()> { /// Validates that essential files exist in the system closure. /// -/// Checks for few critical files that must be present in a complete NixOS +/// Checks for a few critical files that must be present in a complete NixOS /// system. This is essentially in-line with what nixos-rebuild-ng checks for. /// /// - bin/switch-to-configuration: activation script @@ -868,6 +888,7 @@ fn validate_system_closure(system_path: &Path) -> Result<()> { fn validate_system_closure_remote( system_path: &Path, target_host: &str, + build_host: Option<&str>, ) -> Result<()> { let essential_files = [ ("bin/switch-to-configuration", "activation script"), @@ -911,16 +932,32 @@ fn validate_system_closure_remote( if !missing.is_empty() { let missing_list = missing.join("\n"); + + // Build context-aware error message + let host_context = build_host.map_or_else( + || format!("on target host '{target_host}'"), + |build| { + if build == target_host { + format!("on target host '{target_host}' (also build host)") + } else { + format!("on target host '{target_host}' (built on '{build}')") + } + }, + ); + return Err(eyre!( - "System closure validation failed on remote host {}. Missing essential \ - files:\n{}\n\nThis typically happens when:\n1. 'system.switch.enable' \ - is set to false in your configuration\n2. The build was incomplete or \ - corrupted\n3. You're using an incomplete derivation\n\nTo fix \ - this:\n1. Check if 'system.switch.enable = false' is set and remove \ - it\n2. Rebuild your system configuration\n3. If the problem persists, \ - verify your system closure is complete\n\nSystem path checked: {}", - target_host, + "System closure validation failed {}.\n\nMissing essential files in \ + store path '{}':\n{}\n\nThis typically happens when:\n1. \ + 'system.switch.enable' is set to false in your configuration\n2. The \ + build was incomplete or corrupted\n3. The Nix store path was not fully \ + copied to the target host\n\nTo fix this:\n1. Check if \ + 'system.switch.enable = false' is set and remove it\n2. Ensure the \ + complete system closure was copied: nix copy --to ssh://{} {}\n3. \ + Rebuild your system configuration if the problem persists", + host_context, + system_path.display(), missing_list, + target_host, system_path.display() )); } diff --git a/src/remote.rs b/src/remote.rs index 64a15818..dabbad7b 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -345,6 +345,17 @@ fn get_ssh_opts() -> Vec { if let Ok(sshopts) = env::var("NIX_SSHOPTS") { if let Some(parsed) = shlex::split(&sshopts) { opts.extend(parsed); + } else { + let truncated = sshopts.chars().take(60).collect::(); + let sshopts_display = if sshopts.len() > 60 { + format!("{truncated}...",) + } else { + truncated + }; + warn!( + "Failed to parse NIX_SSHOPTS, ignoring. Provide valid shell-quoted \ + options or use ~/.ssh/config. Value: {sshopts_display}", + ); } } From abc331f9fcdaf71ca11f03d527ac39e50a8b9ac8 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 01:09:43 +0300 Subject: [PATCH 20/38] remote: make validation logic more generic; add SSH batching & tiny cleanup Signed-off-by: NotAShelf Change-Id: Ibb16d8eb12dd37ce4621b97f982412bc6a6a6964 --- src/nixos.rs | 84 +++++++----------------------- src/remote.rs | 140 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 153 insertions(+), 71 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index 11257aa1..e65ff759 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -897,72 +897,26 @@ fn validate_system_closure_remote( ("sw/bin", "system path"), ]; - let mut missing = Vec::new(); - for (file, description) in &essential_files { - let remote_path = system_path.join(file); - let path_str = remote_path.to_str().ok_or_else(|| { - eyre!("System path is not valid UTF-8: {}", remote_path.display()) - })?; - - // Use SSH to test if file exists on remote host. We need to quote the path - // to prevent shell interpretation of special characters, such as the ones - // in `.drv^*`. - let quoted_path = shlex::try_quote(path_str).map_err(|_| { - eyre!("Failed to quote path for shell: {}", remote_path.display()) - })?; - - let check_result = std::process::Command::new("ssh") - .args([target_host, "test", "-e", "ed_path]) - .output(); - - match check_result { - Ok(output) if !output.status.success() => { - missing.push(format!(" - {file} ({description})")); - }, - Err(e) => { - return Err(eyre!( - "Failed to check file existence on remote host {}: {}", - target_host, - e - )); - }, - _ => {}, // File exists + // Parse the target host + let target = remote::RemoteHost::parse(target_host) + .wrap_err("Invalid target host specification")?; + + // Build context string for error messages + let context = build_host.map(|build| { + if build == target_host { + "also build host".to_string() + } else { + format!("built on '{build}'") } - } - - if !missing.is_empty() { - let missing_list = missing.join("\n"); - - // Build context-aware error message - let host_context = build_host.map_or_else( - || format!("on target host '{target_host}'"), - |build| { - if build == target_host { - format!("on target host '{target_host}' (also build host)") - } else { - format!("on target host '{target_host}' (built on '{build}')") - } - }, - ); - - return Err(eyre!( - "System closure validation failed {}.\n\nMissing essential files in \ - store path '{}':\n{}\n\nThis typically happens when:\n1. \ - 'system.switch.enable' is set to false in your configuration\n2. The \ - build was incomplete or corrupted\n3. The Nix store path was not fully \ - copied to the target host\n\nTo fix this:\n1. Check if \ - 'system.switch.enable = false' is set and remove it\n2. Ensure the \ - complete system closure was copied: nix copy --to ssh://{} {}\n3. \ - Rebuild your system configuration if the problem persists", - host_context, - system_path.display(), - missing_list, - target_host, - system_path.display() - )); - } - - Ok(()) + }); + + // Delegate to the generic remote validation function + remote::validate_closure_remote( + &target, + system_path, + &essential_files, + context.as_deref(), + ) } /// Returns an error indicating that the 'switch-to-configuration' binary is diff --git a/src/remote.rs b/src/remote.rs index dabbad7b..5dbcf0bf 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -2,7 +2,7 @@ use std::{ env, ffi::OsString, io::Read, - path::PathBuf, + path::{Path, PathBuf}, sync::{ Arc, OnceLock, @@ -337,8 +337,10 @@ fn shell_quote(s: &str) -> String { ) } -/// Get SSH options from `NIX_SSHOPTS` plus our defaults. -fn get_ssh_opts() -> Vec { +/// Get SSH options from `NIX_SSHOPTS` plus our defaults. This includes +/// connection multiplexing options (`ControlMaster`, `ControlPath`, +/// `ControlPersist`) which enable efficient reuse of SSH connections. +pub fn get_ssh_opts() -> Vec { let mut opts: Vec = Vec::new(); // User options first (from NIX_SSHOPTS) @@ -353,8 +355,8 @@ fn get_ssh_opts() -> Vec { truncated }; warn!( - "Failed to parse NIX_SSHOPTS, ignoring. Provide valid shell-quoted \ - options or use ~/.ssh/config. Value: {sshopts_display}", + "Failed to parse NIX_SSHOPTS, ignoring. Provide valid options or use \ + ~/.ssh/config. Value: {sshopts_display}", ); } } @@ -498,13 +500,139 @@ fn copy_closure_to( Ok(()) } +/// Validates that essential files exist in a closure on a remote host. +/// +/// Performs batched SSH checks using connection multiplexing. This is useful +/// for validating that a system closure contains all necessary files before +/// attempting activation. +/// +/// # Arguments +/// +/// * `host` - The remote host to check files on +/// * `closure_path` - The base path to the closure (e.g., +/// `/nix/store/xxx-nixos-system`) +/// * `essential_files` - Slice of (`relative_path`, `description`) tuples for +/// files to validate +/// * `context_info` - Optional context for error messages (e.g., "built on +/// 'host1'") +/// +/// # Returns +/// +/// Returns `Ok(())` if all files exist, or an error describing which files are +/// missing. +/// +/// # Errors +/// +/// Returns an error if: +/// +/// - SSH connection to the remote host fails +/// - Any of the essential files are missing +/// - Path strings contain invalid UTF-8 +pub fn validate_closure_remote( + host: &RemoteHost, + closure_path: &Path, + essential_files: &[(&str, &str)], + context_info: Option<&str>, +) -> Result<()> { + // Build all test commands for batched execution + let test_commands: Vec = essential_files + .iter() + .map(|(file, _)| { + let remote_path = closure_path.join(file); + let path_str = remote_path.to_str().ok_or_else(|| { + eyre!("Path is not valid UTF-8: {}", remote_path.display()) + })?; + let quoted_path = shlex::try_quote(path_str).map_err(|_| { + eyre!("Failed to quote path for shell: {}", remote_path.display()) + })?; + Ok(format!("test -e {quoted_path}")) + }) + .collect::>>()?; + + // Join all tests with &&, so the command fails on first missing file + // We check each file individually afterward to identify which ones are + // missing + let batch_command = test_commands.join(" && "); + + // Get SSH options for connection multiplexing + let ssh_opts = get_ssh_opts(); + + // Execute batch check using SSH with proper connection multiplexing + let check_result = std::process::Command::new("ssh") + .args(&ssh_opts) + .arg(host.host()) + .arg(&batch_command) + .output(); + + // If batch check succeeds, all files exist + if let Ok(output) = &check_result { + if output.status.success() { + return Ok(()); + } + } + + // Batch check failed or errored. Identify which files are missing + let mut missing = Vec::new(); + for ((file, description), test_cmd) in + essential_files.iter().zip(&test_commands) + { + let check_result = std::process::Command::new("ssh") + .args(&ssh_opts) + .arg(host.host()) + .arg(test_cmd) + .output(); + + match check_result { + Ok(output) if !output.status.success() => { + missing.push(format!(" - {file} ({description})")); + }, + Err(e) => { + return Err(eyre!( + "Failed to check file existence on remote host {}: {}", + host, + e + )); + }, + _ => {}, // File exists + } + } + + if !missing.is_empty() { + let missing_list = missing.join("\n"); + + // Build context-aware error message + let host_context = context_info.map_or_else( + || format!("on remote host '{host}'"), + |ctx| format!("on remote host '{host}' ({ctx})"), + ); + + return Err(eyre!( + "Closure validation failed {}.\n\nMissing essential files in store path \ + '{}':\n{}\n\nThis typically happens when:\n1. Required system \ + components are disabled in your configuration\n2. The build was \ + incomplete or corrupted\n3. The Nix store path was not fully copied to \ + the target host\n\nTo fix this:\n1. Verify your configuration enables \ + all required components\n2. Ensure the complete closure was copied: \ + nix copy --to ssh://{} {}\n3. Rebuild your configuration if the \ + problem persists", + host_context, + closure_path.display(), + missing_list, + host, + closure_path.display() + )); + } + + Ok(()) +} + /// Copy a Nix closure from a remote host to localhost. fn copy_closure_from( host: &RemoteHost, path: &str, use_substitutes: bool, ) -> Result<()> { - info!("Copying result from build host '{}'", host); + info!("Copying result from build host '{host}'"); let mut cmd = Exec::cmd("nix-copy-closure").arg("--from").arg(host.host()); From c860cdabb3e168fb3d8067aeecdb4d04f94467dd Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 15:51:03 +0300 Subject: [PATCH 21/38] nixos: skip canonicalize for remote builds without local results Signed-off-by: NotAShelf Change-Id: I4b2199926f36bc5a1b3c7ec06284d6b16a6a6964 --- src/nixos.rs | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index e65ff759..fc731aad 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -207,11 +207,19 @@ impl OsRebuildActivateArgs { } } - // Validate system closure before activation (unless bypassed) - // Resolve target_profile to actual store path for validation and activation - let resolved_profile = target_profile - .canonicalize() - .context("Failed to resolve output path to actual store path")?; + // Validate system closure before activation, unless bypassed. For remote + // builds where `out_path` doesn't exist locally, we can't canonicalize + // `target_profile`. Instead we use the path as-is for remote operations. + let is_remote_build = self.rebuild.target_host.is_some(); + let resolved_profile: PathBuf = if is_remote_build && !out_path.exists() { + // Remote build with no local result. Skip canonicalization, use original + // path + target_profile.to_path_buf() + } else { + target_profile + .canonicalize() + .context("Failed to resolve output path to actual store path")? + }; let should_skip = self.rebuild.no_validate || std::env::var("NH_NO_VALIDATE").is_ok(); @@ -238,11 +246,20 @@ impl OsRebuildActivateArgs { validate_system_closure(&resolved_profile)?; } - let switch_to_configuration = resolved_profile - .join("bin") - .join("switch-to-configuration") - .canonicalize() - .context("Failed to resolve switch-to-configuration path")?; + // Resolve switch-to-configuration path for activation commands. For + // remote-only builds where out_path doesn't exist locally, skip this + // since we'll execute these commands via SSH on the remote host + let switch_to_configuration_path = + resolved_profile.join("bin").join("switch-to-configuration"); + + let switch_to_configuration = if is_remote_build && !out_path.exists() { + // Remote build with no local result. Use uncanonicalized path for SSH + switch_to_configuration_path + } else { + switch_to_configuration_path + .canonicalize() + .context("Failed to resolve switch-to-configuration path")? + }; let canonical_out_path = switch_to_configuration.to_str().ok_or_else(|| { From 30f096ae01c79a37c9708296326902312ed2d12a Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 17:05:37 +0300 Subject: [PATCH 22/38] remote: add best-effort process cleanup on interrupt Implement optional remote process cleanup when user cancels the operation (i.e., presses Ctrl+C) during a remote build. If the environment variable NH_REMOTE_CLEANUP is set to `"1"`, `"true`, or `"yes"` NH will also attempt to run `pkill` on the remote host to terminate the Nix process. This is implemented to match nixos-rebuild-ng's cleanup behaviour, but it is opt-in instead of opt-out due to the fragility of the action. Signed-off-by: NotAShelf Change-Id: I555c84087018025b111a51715c5aa42a6a6a6964 --- README.md | 6 ++ src/remote.rs | 193 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 195 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 0b9fb696..b1be46f3 100644 --- a/README.md +++ b/README.md @@ -343,6 +343,12 @@ the common variables that you may encounter or choose to employ are as follows: - Control whether `nom` (nix-output-monitor) should be enabled for the build processes. Equivalent of `--no-nom`. +- `NH_REMOTE_CLEANUP` + - Whether to initiate an attempt to clean up remote processes on interrupt via + pkill. This is implemented to match nixos-rebuild's behaviour, but due to + its fragile nature it has been made opt-in. Unless NH has been leaving + zombie processes on interrupt, there is generally no need to set this. + ### Notes - Any environment variables prefixed with `NH_` are explicitly propagated by NH diff --git a/src/remote.rs b/src/remote.rs index 5dbcf0bf..958ac981 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -388,6 +388,117 @@ fn get_nix_sshopts_env() -> String { } } +/// Check if remote cleanup is enabled via environment variable. +/// +/// Returns `true` if `NH_REMOTE_CLEANUP` is set to a truthy value: +/// "1", "true", "yes" (case-insensitive). +/// +/// Returns `false` if unset, empty, or set to any other value. +fn should_cleanup_remote() -> bool { + env::var("NH_REMOTE_CLEANUP").is_ok_and(|val| { + let val = val.trim().to_lowercase(); + val == "1" || val == "true" || val == "yes" + }) +} + +/// Attempt to clean up a remote process using pkill. +/// +/// This is a best-effort (and opt-in) operation called when the user interrupts +/// a remote build. It tries to terminate the remote nix process via SSH and +/// pkill, but is inherently fragile due to the nature of remote building +/// semantics. +/// +/// # Arguments +/// +/// * `host` - The remote host where the process is running +/// * `remote_cmd` - The original command that was run remotely, used for pkill +/// matching +fn attempt_remote_cleanup(host: &RemoteHost, remote_cmd: &str) { + if !should_cleanup_remote() { + return; + } + + let ssh_opts = get_ssh_opts(); + let quoted_cmd = shell_quote(remote_cmd); // for safe passing through pkill's --full argument + + // Build the pkill command: + // pkill -INT --full '' will match the exact command line + let pkill_cmd = format!("pkill -INT --full {quoted_cmd}"); + + // Build SSH command with the pkill command + let mut ssh_cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + ssh_cmd = ssh_cmd.arg(opt); + } + ssh_cmd = ssh_cmd.arg(host.host()).arg(&pkill_cmd); + + debug!("Attempting remote cleanup on '{host}': pkill -INT --full "); + + // Use popen with timeout to avoid hanging on unresponsive hosts + let mut process = match ssh_cmd.popen() { + Ok(p) => p, + Err(e) => { + info!("Failed to execute remote cleanup on '{host}': {e}"); + return; + }, + }; + + // Wait up to 5 seconds for cleanup to complete + let timeout = Duration::from_secs(5); + match process.wait_timeout(timeout) { + Ok(Some(_)) => { + // Process exited, check status below + }, + Ok(None) => { + // Timeout - kill the process and continue + let _ = process.kill(); + let _ = process.wait(); + info!("Remote cleanup on '{host}' timed out after 5 seconds"); + return; + }, + Err(e) => { + info!("Error waiting for remote cleanup on '{host}': {e}"); + return; + }, + } + + // Check exit status + if let Some(exit_status) = process.exit_status() { + if exit_status.success() { + info!("Cleaned up remote process on '{}'", host); + } else { + // Capture stderr for error diagnosis + let stderr = process.stderr.take().map_or_else(String::new, |mut e| { + let mut s = String::new(); + let _ = e.read_to_string(&mut s); + s + }); + let stderr_lower = stderr.to_lowercase(); + + if stderr.contains("No matching processes") + || stderr_lower.contains("0 processes") + { + debug!( + "No matching process found on '{host}' during cleanup (may have \ + already exited)" + ); + } else if stderr_lower.contains("not found") + || stderr_lower.contains("command not found") + { + info!("pkill not available on '{}', skipping remote cleanup", host); + } else if stderr_lower.contains("permission denied") + || stderr_lower.contains("operation not permitted") + { + info!( + "Permission denied for pkill on '{host}', skipping remote cleanup", + ); + } else { + info!("Remote cleanup on '{host}' returned non-zero exit status"); + } + } + } +} + /// Get the flake experimental feature flags required for `nix` commands. /// /// Returns the flags needed for `--extra-experimental-features "nix-command @@ -1007,8 +1118,13 @@ fn build_on_remote_simple( // Check interrupt flag while waiting if get_interrupt_flag().load(Ordering::Relaxed) { debug!("Interrupt detected, killing SSH process"); + let _ = process.kill(); - let _ = process.wait(); // Reap zombie + let _ = process.wait(); // reap zombie + + // Attempt remote cleanup if enabled + attempt_remote_cleanup(host, &remote_cmd); + bail!("Operation interrupted by user"); } }, @@ -1104,17 +1220,24 @@ fn build_on_remote_with_nom( let poll_interval = Duration::from_millis(100); for proc in &mut processes { - #[allow(clippy::needless_continue, reason = "Better for explicitness")] + #[allow( + clippy::needless_continue, + reason = "Better for explicitness and consistency" + )] loop { // Check interrupt flag before waiting if get_interrupt_flag().load(Ordering::Relaxed) { debug!("Interrupt detected during build with nom"); - // Kill remaining local processes - this will cause SSH to terminate + // Kill remaining local processes. This will cause SSH to terminate // the remote command automatically for p in &mut processes { let _ = p.kill(); - let _ = p.wait(); // Reap zombie + let _ = p.wait(); // reap zombie } + + // Attempt remote cleanup if enabled + attempt_remote_cleanup(host, &remote_cmd); + bail!("Operation interrupted by user"); } @@ -1647,4 +1770,66 @@ mod tests { drop(guard); // If this completes without panic, the Drop impl is at least safe } + + proptest! { + #[test] + #[serial] + fn test_should_cleanup_remote_enabled_by_valid_values( + value in prop_oneof![ + Just("1"), + Just("true"), + Just("yes"), + Just("TRUE"), + Just("YES"), + Just("True"), + ] + ) { + unsafe { + std::env::set_var("NH_REMOTE_CLEANUP", value); + } + prop_assert!(should_cleanup_remote()); + unsafe { + std::env::remove_var("NH_REMOTE_CLEANUP"); + } + } + } + + #[test] + #[serial] + fn test_should_cleanup_remote_empty_disabled() { + // Empty value should NOT enable cleanup + unsafe { + std::env::set_var("NH_REMOTE_CLEANUP", ""); + } + assert!(!should_cleanup_remote()); + unsafe { + std::env::remove_var("NH_REMOTE_CLEANUP"); + } + } + + #[test] + #[serial] + fn test_should_cleanup_remote_arbitrary_value_disabled() { + // Arbitrary values should NOT enable cleanup + unsafe { + std::env::set_var("NH_REMOTE_CLEANUP", "maybe"); + } + assert!(!should_cleanup_remote()); + unsafe { + std::env::remove_var("NH_REMOTE_CLEANUP"); + } + } + + #[test] + fn test_attempt_remote_cleanup_does_nothing_when_disabled() { + // When should_cleanup_remote returns false, no SSH command should be + // executed. We can't easily verify no SSH was spawned, but we can verify + // the function doesn't panic or error when cleanup is disabled + let host = RemoteHost::parse("user@host.example").unwrap(); + let remote_cmd = "nix build /nix/store/abc.drv^* --print-out-paths"; + + // This should complete without error even when cleanup is disabled + attempt_remote_cleanup(&host, remote_cmd); + // If we reach here, the function handled the disabled case gracefully + } } From c552929266c30e434d0bec92d7bd96bc6a6e7d59 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 18:37:04 +0300 Subject: [PATCH 23/38] interface: add NH_NO_VALIDATE environment variable support Signed-off-by: NotAShelf Change-Id: I30dce4dea68a07975c58afc475bf37496a6a6964 --- src/interface.rs | 3 ++- src/nixos.rs | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/interface.rs b/src/interface.rs index 0faab718..22c52f95 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -200,6 +200,7 @@ pub struct OsBuildVmArgs { } #[derive(Debug, Args)] +#[allow(clippy::struct_excessive_bools)] pub struct OsRebuildArgs { #[command(flatten)] pub common: CommonRebuildArgs, @@ -241,7 +242,7 @@ pub struct OsRebuildArgs { pub build_host: Option, /// Skip pre-activation system validation checks - #[arg(long)] + #[arg(long, env = "NH_NO_VALIDATE")] pub no_validate: bool, } diff --git a/src/nixos.rs b/src/nixos.rs index fc731aad..248a094c 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -221,8 +221,7 @@ impl OsRebuildActivateArgs { .context("Failed to resolve output path to actual store path")? }; - let should_skip = - self.rebuild.no_validate || std::env::var("NH_NO_VALIDATE").is_ok(); + let should_skip = self.rebuild.no_validate; if should_skip { warn!( From bec6f8ddd0207d737cd3011871a038a2ea72ecd7 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 20:42:49 +0300 Subject: [PATCH 24/38] remote: add IPv6 SSH host transformation For compatibility and I guess consistency with nixos-rebuild-ng which *does* this, but ours is bit safer so hah! Signed-off-by: NotAShelf Change-Id: Ie6ed6ea08b16acf690c8e4bb56d063546a6a6964 --- src/remote.rs | 201 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 172 insertions(+), 29 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index 958ac981..226a29bf 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -16,7 +16,7 @@ use color_eyre::{ eyre::{Context, bail, eyre}, }; use subprocess::{Exec, ExitStatus, Redirection}; -use tracing::{debug, info, warn}; +use tracing::{debug, error, info, warn}; use crate::{installable::Installable, util::NixVariant}; @@ -168,19 +168,53 @@ fn get_ssh_control_dir() -> &'static PathBuf { let control_dir = base.join(format!("nh-ssh-{}", std::process::id())); // Create the directory if it doesn't exist - if let Err(e) = std::fs::create_dir_all(&control_dir) { + if let Err(e1) = std::fs::create_dir_all(&control_dir) { debug!( - "Failed to create SSH control directory at {}: {e}", + "Failed to create SSH control directory at {}: {e1}", control_dir.display() ); + // Fall back to /tmp/nh-ssh- - try creating there instead let fallback_dir = PathBuf::from("/tmp").join(format!("nh-ssh-{}", std::process::id())); + + // As a last resort, if *all else* fails, we construct a unique + // subdirectory under /tmp with PID and full timestamp to preserve + // process isolation and avoid collisions between concurrent invocations if let Err(e2) = std::fs::create_dir_all(&fallback_dir) { - // Last resort: use /tmp directly (socket will be /tmp/ssh-%n) - // This is not ideal but better than failing entirely - debug!("Failed to create fallback SSH control directory: {e2}"); - return PathBuf::from("/tmp"); + let timestamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| { + d.as_secs() * 1_000_000_000 + u64::from(d.subsec_nanos()) + }); + + let unique_dir = PathBuf::from("/tmp").join(format!( + "nh-ssh-{}-{}", + std::process::id(), + timestamp + )); + + if let Err(e3) = std::fs::create_dir_all(&unique_dir) { + error!( + "Failed to create SSH control directory after exhausting all \ + fallbacks. Errors: (1) {}: {e1}, (2) {}: {e2}, (3) {}: {e3}. SSH \ + operations will likely fail.", + control_dir.display(), + fallback_dir.display(), + unique_dir.display() + ); + + // Return the path anyway; SSH will fail with a clear error if + // directory creation is truly impossible, and at this point we + // are out of options. + return unique_dir; + } + + debug!( + "Created unique SSH control directory: {}", + unique_dir.display() + ); + return unique_dir; } return fallback_dir; } @@ -303,6 +337,43 @@ impl RemoteHost { pub fn host(&self) -> &str { &self.host } + + /// Get the SSH-compatible host string. + /// + /// Strips brackets from IPv6 addresses since SSH doesn't accept them. + /// Preserves zone IDs (`%eth0`) and `user@` prefix if present. + /// + /// Examples: + /// + /// - `[2001:db8::1]` -> `2001:db8::1` + /// - `user@[2001:db8::1]` -> `user@2001:db8::1` + /// - `[fe80::1%eth0]` -> `fe80::1%eth0` + /// - `host.example` -> `host.example` + #[must_use] + pub fn ssh_host(&self) -> String { + let hostname = self.hostname(); + + // Check for bracketed IPv6 address + if hostname.starts_with('[') && hostname.ends_with(']') { + let inner = &hostname[1..hostname.len() - 1]; + + // Validate it's actually a valid IPv6 address + // Split on '%' to validate only the address part (zone ID is + // SSH-specific) + let addr_part = inner.split('%').next().unwrap_or(inner); + if addr_part.parse::().is_ok() { + // Reconstruct with user@ prefix if present + if let Some(at_pos) = self.host.find('@') { + let user = &self.host[..at_pos]; + return format!("{user}@{inner}"); + } + return inner.to_string(); + } + } + + // Not IPv6 or not bracketed, return as-is + self.host.clone() + } } impl std::fmt::Display for RemoteHost { @@ -425,12 +496,12 @@ fn attempt_remote_cleanup(host: &RemoteHost, remote_cmd: &str) { // pkill -INT --full '' will match the exact command line let pkill_cmd = format!("pkill -INT --full {quoted_cmd}"); - // Build SSH command with the pkill command - let mut ssh_cmd = Exec::cmd("ssh"); + // Build SSH command with stderr capture for diagnostics + let mut ssh_cmd = Exec::cmd("ssh").stderr(Redirection::Pipe); for opt in &ssh_opts { ssh_cmd = ssh_cmd.arg(opt); } - ssh_cmd = ssh_cmd.arg(host.host()).arg(&pkill_cmd); + ssh_cmd = ssh_cmd.arg(host.ssh_host()).arg(&pkill_cmd); debug!("Attempting remote cleanup on '{host}': pkill -INT --full "); @@ -551,7 +622,7 @@ fn run_remote_command( for opt in &ssh_opts { cmd = cmd.arg(opt); } - cmd = cmd.arg(host.host()).arg(&remote_cmd); + cmd = cmd.arg(host.ssh_host()).arg(&remote_cmd); if capture_stdout { cmd = cmd.stdout(Redirection::Pipe).stderr(Redirection::Pipe); @@ -586,7 +657,9 @@ fn copy_closure_to( ) -> Result<()> { info!("Copying closure to build host '{}'", host); - let mut cmd = Exec::cmd("nix-copy-closure").arg("--to").arg(host.host()); + let mut cmd = Exec::cmd("nix-copy-closure") + .arg("--to") + .arg(host.ssh_host()); if use_substitutes { cmd = cmd.arg("--use-substitutes"); @@ -671,7 +744,7 @@ pub fn validate_closure_remote( // Execute batch check using SSH with proper connection multiplexing let check_result = std::process::Command::new("ssh") .args(&ssh_opts) - .arg(host.host()) + .arg(host.ssh_host()) .arg(&batch_command) .output(); @@ -689,7 +762,7 @@ pub fn validate_closure_remote( { let check_result = std::process::Command::new("ssh") .args(&ssh_opts) - .arg(host.host()) + .arg(host.ssh_host()) .arg(test_cmd) .output(); @@ -745,7 +818,9 @@ fn copy_closure_from( ) -> Result<()> { info!("Copying result from build host '{host}'"); - let mut cmd = Exec::cmd("nix-copy-closure").arg("--from").arg(host.host()); + let mut cmd = Exec::cmd("nix-copy-closure") + .arg("--from") + .arg(host.ssh_host()); if use_substitutes { cmd = cmd.arg("--use-substitutes"); @@ -784,9 +859,9 @@ fn copy_closure_between_remotes( let mut cmd = Exec::cmd("nix") .args(&flake_flags) .args(&["copy", "--from"]) - .arg(format!("ssh://{}", from_host.host())) + .arg(format!("ssh://{}", from_host.ssh_host())) .arg("--to") - .arg(format!("ssh://{}", to_host.host())); + .arg(format!("ssh://{}", to_host.ssh_host())); if use_substitutes { cmd = cmd.arg("--substitute-on-destination"); @@ -1102,7 +1177,7 @@ fn build_on_remote_simple( ssh_cmd = ssh_cmd.arg(opt); } ssh_cmd = ssh_cmd - .arg(&host.host) + .arg(host.ssh_host()) .arg(&remote_cmd) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe); @@ -1198,7 +1273,7 @@ fn build_on_remote_with_nom( ssh_cmd = ssh_cmd.arg(opt); } ssh_cmd = ssh_cmd - .arg(host.host()) + .arg(host.ssh_host()) .arg(&remote_cmd) .stdout(Redirection::Pipe) .stderr(Redirection::Merge); @@ -1502,6 +1577,72 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_parse_ipv6_at_inside_brackets_rejected() { + // @ character inside brackets should be rejected (not valid IPv6) + // This ensures [@2001:db8::1] and [2001@db8::1] are both rejected + let result = RemoteHost::parse("[@2001:db8::1]"); + assert!(result.is_err(), "[@2001:db8::1] should be rejected"); + + let result2 = RemoteHost::parse("[2001@db8::1]"); + assert!(result2.is_err(), "[2001@db8::1] should be rejected"); + } + + #[test] + fn test_ssh_host_ipv6_strips_brackets() { + let host = RemoteHost::parse("[2001:db8::1]").expect("should parse IPv6"); + assert_eq!(host.ssh_host(), "2001:db8::1"); + } + + #[test] + fn test_ssh_host_ipv6_with_user() { + let host = RemoteHost::parse("user@[2001:db8::1]").expect("should parse"); + assert_eq!(host.ssh_host(), "user@2001:db8::1"); + } + + #[test] + fn test_ssh_host_ipv6_with_zone_id() { + let host = RemoteHost::parse("[fe80::1%eth0]").expect("should parse"); + assert_eq!(host.ssh_host(), "fe80::1%eth0"); + } + + #[test] + fn test_ssh_host_ipv6_with_zone_id_and_user() { + let host = RemoteHost::parse("user@[fe80::1%eth0]").expect("should parse"); + assert_eq!(host.ssh_host(), "user@fe80::1%eth0"); + } + + #[test] + fn test_ssh_host_ipv6_localhost() { + let host = RemoteHost::parse("[::1]").expect("should parse"); + assert_eq!(host.ssh_host(), "::1"); + } + + #[test] + fn test_ssh_host_non_ipv6_unchanged() { + let host = RemoteHost::parse("host.example").expect("should parse"); + assert_eq!(host.ssh_host(), "host.example"); + } + + #[test] + fn test_ssh_host_non_ipv6_with_user() { + let host = RemoteHost::parse("user@host.example").expect("should parse"); + assert_eq!(host.ssh_host(), "user@host.example"); + } + + #[test] + fn test_ssh_host_ssh_uri_ipv6() { + let host = RemoteHost::parse("ssh://[2001:db8::1]").expect("should parse"); + assert_eq!(host.ssh_host(), "2001:db8::1"); + } + + #[test] + fn test_ssh_host_ssh_uri_ipv6_with_user() { + let host = + RemoteHost::parse("ssh://root@[2001:db8::1]").expect("should parse"); + assert_eq!(host.ssh_host(), "root@2001:db8::1"); + } + #[test] fn test_shell_quote_simple() { assert_eq!(shell_quote("simple"), "simple"); @@ -1735,19 +1876,21 @@ mod tests { #[test] fn test_get_ssh_control_dir_creates_directory() { let dir = get_ssh_control_dir(); - // The directory should exist (or be /tmp as last resort) + // The directory should exist in normal operation. In extreme edge cases + // (read-only /tmp), the function returns a path that may not exist, and + // SSH will fail with a clear error when attempting to use it. assert!( - dir.exists() || dir == &PathBuf::from("/tmp"), - "Control dir should exist or be /tmp fallback" + dir.exists(), + "Control dir should exist in normal operation: {}", + dir.display() ); - // Should contain our process-specific suffix (unless /tmp fallback) + + // Should contain our process-specific suffix let dir_str = dir.to_string_lossy(); - if dir_str != "/tmp" { - assert!( - dir_str.contains("nh-ssh-"), - "Control dir should contain 'nh-ssh-': {dir_str}" - ); - } + assert!( + dir_str.contains("nh-ssh-"), + "Control dir should contain 'nh-ssh-': {dir_str}" + ); } #[test] From f946e9d245fe36b00744e30824ff86d0c7019343 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 22:08:49 +0300 Subject: [PATCH 25/38] remote: remove unused `RemoteHost::host` method; update tests & docs Signed-off-by: NotAShelf Change-Id: I1f5c07d0425db0ad156d1606b28fae166a6a6964 --- src/remote.rs | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index 226a29bf..59f195d5 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -238,7 +238,14 @@ pub struct RemoteHost { } impl RemoteHost { - /// Get the hostname part (without user@ prefix). + /// Get the hostname part without the `user@` prefix. + /// + /// Used for hostname comparisons when determining if two `RemoteHost` + /// instances refer to the same physical host (e.g., detecting when + /// build_host == target_host regardless of different user credentials). + /// + /// Returns the bracketed IPv6 address as-is if present (e.g., + /// `[2001:db8::1]`). /// /// # Panics /// @@ -332,12 +339,6 @@ impl RemoteHost { }) } - /// Get the host string for use with nix-copy-closure and SSH. - #[must_use] - pub fn host(&self) -> &str { - &self.host - } - /// Get the SSH-compatible host string. /// /// Strips brackets from IPv6 addresses since SSH doesn't accept them. @@ -1436,39 +1437,39 @@ mod tests { #[test] fn test_parse_bare_hostname() { let host = RemoteHost::parse("buildserver").expect("should parse"); - assert_eq!(host.host(), "buildserver"); + assert_eq!(host.to_string(), "buildserver"); } #[test] fn test_parse_user_at_hostname() { let host = RemoteHost::parse("root@buildserver").expect("should parse"); - assert_eq!(host.host(), "root@buildserver"); + assert_eq!(host.to_string(), "root@buildserver"); } #[test] fn test_parse_ssh_uri_stripped() { let host = RemoteHost::parse("ssh://buildserver").expect("should parse"); - assert_eq!(host.host(), "buildserver"); + assert_eq!(host.to_string(), "buildserver"); } #[test] fn test_parse_ssh_ng_uri_stripped() { let host = RemoteHost::parse("ssh-ng://buildserver").expect("should parse"); - assert_eq!(host.host(), "buildserver"); + assert_eq!(host.to_string(), "buildserver"); } #[test] fn test_parse_ssh_uri_with_user() { let host = RemoteHost::parse("ssh://root@buildserver").expect("should parse"); - assert_eq!(host.host(), "root@buildserver"); + assert_eq!(host.to_string(), "root@buildserver"); } #[test] fn test_parse_ssh_ng_uri_with_user() { let host = RemoteHost::parse("ssh-ng://admin@buildserver").expect("should parse"); - assert_eq!(host.host(), "admin@buildserver"); + assert_eq!(host.to_string(), "admin@buildserver"); } #[test] @@ -1497,7 +1498,7 @@ mod tests { #[test] fn test_parse_ipv6_bracketed() { let host = RemoteHost::parse("[2001:db8::1]").expect("should parse IPv6"); - assert_eq!(host.host(), "[2001:db8::1]"); + assert_eq!(host.to_string(), "[2001:db8::1]"); assert_eq!(host.hostname(), "[2001:db8::1]"); } @@ -1505,7 +1506,7 @@ mod tests { fn test_parse_ipv6_with_user() { let host = RemoteHost::parse("root@[2001:db8::1]") .expect("should parse IPv6 with user"); - assert_eq!(host.host(), "root@[2001:db8::1]"); + assert_eq!(host.to_string(), "root@[2001:db8::1]"); assert_eq!(host.hostname(), "[2001:db8::1]"); } @@ -1513,34 +1514,34 @@ mod tests { fn test_parse_ipv6_with_zone_id() { let host = RemoteHost::parse("[fe80::1%eth0]").expect("should parse IPv6 with zone"); - assert_eq!(host.host(), "[fe80::1%eth0]"); + assert_eq!(host.to_string(), "[fe80::1%eth0]"); } #[test] fn test_parse_ipv6_ssh_uri() { let host = RemoteHost::parse("ssh://[2001:db8::1]") .expect("should parse IPv6 SSH URI"); - assert_eq!(host.host(), "[2001:db8::1]"); + assert_eq!(host.to_string(), "[2001:db8::1]"); } #[test] fn test_parse_ipv6_ssh_uri_with_user() { let host = RemoteHost::parse("ssh://root@[2001:db8::1]") .expect("should parse IPv6 SSH URI with user"); - assert_eq!(host.host(), "root@[2001:db8::1]"); + assert_eq!(host.to_string(), "root@[2001:db8::1]"); } #[test] fn test_parse_ipv6_localhost() { let host = RemoteHost::parse("[::1]").expect("should parse IPv6 localhost"); - assert_eq!(host.host(), "[::1]"); + assert_eq!(host.to_string(), "[::1]"); } #[test] fn test_parse_ipv6_compressed() { let host = RemoteHost::parse("[2001:db8::]").expect("should parse compressed IPv6"); - assert_eq!(host.host(), "[2001:db8::]"); + assert_eq!(host.to_string(), "[2001:db8::]"); } #[test] From 579cf6c1bd2f10747d3a8cbb8e4b895546c33c75 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 22:15:42 +0300 Subject: [PATCH 26/38] docs: update changelog to reflect remote build improvements Signed-off-by: NotAShelf Change-Id: I2ac16d3c416ed8c68e3daf0318abd1fb6a6a6964 --- CHANGELOG.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b1435a0..8928311a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,10 +18,25 @@ functionality, under the "Removed" section. ### Changed -- `nh os info` now hides empty columns. +- Platform commands (`nh os`, `nh home`, `nh darwin`) now support SSH-based + remote builds via `--build-host`. The flag now uses proper remote build + semantics: derivations are copied to the remote host via `nix-copy-closure`, + built remotely, and results are transferred back. This matches `nixos-rebuild` + behavior, and is significantly more robust than the previous implementation + where `--build-host` would use Nix's `--builders` flag inefficiently. + ([#428](https://github.com/nix-community/nh/issues/428), + [#497](https://github.com/nix-community/nh/pull/497)) + - A new `--no-validate` flag skips pre-activation system validation checks. + Can also be set via the `NH_NO_VALIDATE` environment variable. + - Added `NH_REMOTE_CLEANUP` environment variable. When set, NH will attempt to + terminate remote Nix processes on interrupt (Ctrl+C). Opt-in due to + fragility. +- Shell argument splitting now uses `shlex` for proper quote handling in complex + command arguments. - `nh os info` now support `--fields` to select which field(s) to display; also - add a per-generation "Closure Size" coloumn. + add a per-generation "Closure Size" column. ([#375](https://github.com/nix-community/nh/issues/375)) + - Empty columns are now hidden by default to avoid visual clutter. - `nh os switch` and `nh os boot` now support the `--install-bootloader` flag, which will explicitly set `NIXOS_INSTALL_BOOTLOADER` for `switch-to-configuration`. Bootloader behaviour was previously supported by From cc753854ffb68f6b066bd5e18ba39e91f69b080b Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 22:18:28 +0300 Subject: [PATCH 27/38] interface: clean up descriptions for build_host and target_host Signed-off-by: NotAShelf Change-Id: I2e1e9f814905fca8483cfbe3c3e58d0e6a6a6964 --- src/interface.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/interface.rs b/src/interface.rs index 22c52f95..b284f9d5 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -233,11 +233,11 @@ pub struct OsRebuildArgs { #[arg(short = 'R', long, env = "NH_BYPASS_ROOT_CHECK")] pub bypass_root_check: bool, - /// Deploy the configuration to a different host over ssh + /// Deploy the built configuration to a different host over SSH #[arg(long)] pub target_host: Option, - /// Build the configuration to a different host over ssh + /// Build the configuration on a different host over SSH #[arg(long)] pub build_host: Option, From 5c959c847835fc1f86b05807dfb0f74af893bff5 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 22:48:48 +0300 Subject: [PATCH 28/38] docs: move everything to `docs` dir; minor cleanup Signed-off-by: NotAShelf Change-Id: Ifc315034046804817b1aa86e4b9510e86a6a6964 --- CHANGELOG.md | 6 +- README.md => docs/README.md | 103 +++++--- .../assets}/nh_clean_screenshot.png | Bin .../assets}/nh_search_screenshot.png | Bin .../assets}/nh_switch_screenshot.png | Bin docs/remote-build.md | 233 ++++++++++++++++++ 6 files changed, 307 insertions(+), 35 deletions(-) rename README.md => docs/README.md (84%) rename {.github => docs/assets}/nh_clean_screenshot.png (100%) rename {.github => docs/assets}/nh_search_screenshot.png (100%) rename {.github => docs/assets}/nh_switch_screenshot.png (100%) create mode 100644 docs/remote-build.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8928311a..de2eb4f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,10 +33,10 @@ functionality, under the "Removed" section. fragility. - Shell argument splitting now uses `shlex` for proper quote handling in complex command arguments. -- `nh os info` now support `--fields` to select which field(s) to display; also - add a per-generation "Closure Size" column. - ([#375](https://github.com/nix-community/nh/issues/375)) +- `nh os info` now support `--fields` to select which field(s) to display + ([#375](https://github.com/nix-community/nh/issues/375)). - Empty columns are now hidden by default to avoid visual clutter. + - A new, per-generation "Closure Size" column has been added - `nh os switch` and `nh os boot` now support the `--install-bootloader` flag, which will explicitly set `NIXOS_INSTALL_BOOTLOADER` for `switch-to-configuration`. Bootloader behaviour was previously supported by diff --git a/README.md b/docs/README.md similarity index 84% rename from README.md rename to docs/README.md index b1be46f3..731c9f21 100644 --- a/README.md +++ b/docs/README.md @@ -61,6 +61,12 @@ To get started with NH, skip to the [Usage](#usage) section. with explicit targeting. - **Extensible & Futureproof**: Designed for seamless, rapid addition of new subcommands and flags. + - **NH is a reimplementation of the CLIs you all know and love**, but with a + focus on safety and correctness. The language and design choices allow new + feature additions to be trivial and (almost) zero-cost. +- **Excellent Documentation**: Everything you can do with NH is documented. + Everything NH _does_ is documented. The user-facing and developer-facing + documentation is, and will always remain, up to date. ### Design @@ -111,7 +117,7 @@ the package is outdated. The latest, tagged version is available in Nixpkgs as **NH stable**. This is recommended for most users, as tagged releases will usually undergo more -testing.This repository also provides the latest development version of NH, +testing. This repository also provides the latest development version of NH, which you can get from the flake outputs. ```sh @@ -142,16 +148,15 @@ set the following configuration: > configurations via channels or manual dependency pinning and the such. Please > consider the new API mature, but somewhat experimental as it is a new > addition. Remember to report any bugs! +> +> - For flakes, the command is `nh os switch /path/to/flake` +> - For a classical configuration: +> - `nh os switch -f ''`, or +> - `nh os switch -f '' -- -I nixos-config=/path/to/configuration.nix` +> if using a different location than the default. -- For flakes, the command is `nh os switch /path/to/flake` -- For a classical configuration: - - `nh os switch -f ''`, or - - `nh os switch -f '' -- -I - nixos-config=/path/to/configuration.nix` - if using a different location than the default. - -You might want to check `nh os --help` for other values and the defaults from -environment variables. +You might want to check `nh os --help` or `man 1 nh` for other values and the +defaults from environment variables. #### Specialisations support @@ -199,32 +204,47 @@ One of the features and the core principles of NH is to provide a clean, uniform and intuitive CLI for its users. The `nh` command offers several subcommands, all with their extensive CLI flags for extensive configuration. +> [!TIP] +> NH supports various flags, [environment variables](#environment-variables) and +> setup options to provide the best possible user experience. See the `--help` +> page for individual subcommands, or `man 1 nh` for more information on each +> subcommand with examples. You may also use the relevant platform module, such +> as the NixOS module available in Nixpkgs, to customize it for your system as +> described in the installation section. + Under the `nh` command, there are two types of commands that you'll be interested in: ### Global Subcommands Global subcommands implement functionality around core Nix commands. As it -stands, we provide a **better search** and **better garbage collection**. +stands, we provide a **better search** and **better garbage collection** +experience, done so with two subcommands provided out of the box. -- `nh search` - a super-fast package searching tool (powered by an Elasticsearch - client) for Nix packages in supported Nixpkgs branches. +#### `nh search` -

+We provide a super-fast package searching tool (powered by an Elasticsearch +client) for Nix packages in supported Nixpkgs branches, available as +`nh search`. + +

nh search showcase

-- `nh clean` - a re-implementation of `nix-collect-garbage` that also collects - gcroots. +#### `nh clean` + +Reimplementation of `nix-collect-garbage` that also collects gcroots with +various options for fine-graining what is kept, and additional context before +the cleanup process to let you know what is to be cleaned. -

+

nh clean showcase

@@ -234,27 +254,37 @@ stands, we provide a **better search** and **better garbage collection**. Platform specific subcommands are those that implement CLI utilities for **NixOS**, **Home Manager** and **Nix-Darwin**. -- `nh os` - reimplements `nixos-rebuild`[^1] with the addition of - - build-tree displays. - - diff of changes. - - confirmation. +#### `nh os` + +The `nh os` subcommand reimplements the Python script, `nixos-rebuild-ng`, [^1] +from ground up _with the addition of_: + +- Build-tree displays via **nix-output-monitor** (nom). +- Pretty diffs of changes via **dix** +- Confirmation -

+and other additional changes to make the UI more intuitive, from supporting +environment variables to additional safeguards. Is this all? No, more is to +come. + +

nh os switch showcase

-- `nh home` - reimplements `home-manager`. -- `nh darwin` - reimplements `darwin-rebuild`. +#### `nh home` -> [!TIP] -> NH supports various flags, [environment variables](#environment-variables) and -> setups to provide the best possible user experience. See the `--help` page for -> individual subcommands, or `man 1 nh` for more information on each subcommand -> with examples. +The `nh home` subcommand reimplements the `home-manager` script, with the same +additions as `nh os`. + +#### `nh darwin` + +Last but not least, the `nh darwin` subcommand is a pure-rust reimplementation +of the `darwin-rebuild` script featuring the same additions as `nh os` and +`nh home`. [^1]: `nh os` does not yet provide full feature parity with `nixos-rebuild`. While a large collection of subcommands have been implemented, you might be @@ -358,6 +388,15 @@ the common variables that you may encounter or choose to employ are as follows: `FLAKE` and emit a warning recommending migration to `NH_FLAKE`. `FLAKE` will be removed in the future versions of NH. +## Frequently Asked Questions (FAQ) + +**Q**: Does NH wrap the CLIs that I typically use? + +**A**: No, all of the commands use Nix directly, and they **do not consume the +typical CLI utilities**. NH is slowly converting existing tools that are invoked +via shell to native Rust libraries to get safer integration and slightly better +performance. + ## Hacking Contributions are always welcome. To get started, just clone the repository and diff --git a/.github/nh_clean_screenshot.png b/docs/assets/nh_clean_screenshot.png similarity index 100% rename from .github/nh_clean_screenshot.png rename to docs/assets/nh_clean_screenshot.png diff --git a/.github/nh_search_screenshot.png b/docs/assets/nh_search_screenshot.png similarity index 100% rename from .github/nh_search_screenshot.png rename to docs/assets/nh_search_screenshot.png diff --git a/.github/nh_switch_screenshot.png b/docs/assets/nh_switch_screenshot.png similarity index 100% rename from .github/nh_switch_screenshot.png rename to docs/assets/nh_switch_screenshot.png diff --git a/docs/remote-build.md b/docs/remote-build.md new file mode 100644 index 00000000..fb248d45 --- /dev/null +++ b/docs/remote-build.md @@ -0,0 +1,233 @@ +# Remote Deployment + +NH supports remote deployments, as you might be familiar from `nixos-rebuild` or +similar tools, using the `--build-host` and `--target-host` options. + +## Overview + +Remote deployment has two independent concepts: + +- **`--build-host`**: Where the configuration is **built** (via + `nix-copy-closure` + `nix build`) +- **`--target-host`**: Where the result is **deployed** and activated + +You can use either, both, or neither. Derivation evaluation always happens +locally. + +| Flags used | Build location | Activation location | +| -------------------------------- | -------------- | ------------------- | +| none | localhost | localhost | +| `--build-host X` | X | localhost | +| `--target-host Y` | localhost | Y | +| `--build-host X --target-host Y` | X | Y | +| `--build-host Y --target-host Y` | Y | Y | + +## Basic Usage + +### Build Remotely, Deploy Locally + +```bash +nh os switch --build-host user@buildserver +``` + +This builds the configuration on `buildserver`, then copies the result back to +the local machine and activates it. + +### Build Locally, Deploy Remotely + +```bash +nh os switch --target-host user@production +``` + +This builds the configuration locally, copies it to `production`, and activates +it there. + +### Build on One Host, Deploy to Another + +```bash +nh os switch --build-host user@buildserver --target-host user@production +``` + +This builds on `buildserver`, then copies the result directly to `production` +and activates it there. + +### Build and Deploy to the Same Remote Host + +```bash +nh os switch --build-host user@production --target-host user@production +``` + +This builds on `production` and deploys to `production`. The implementation +avoids unnecessary data transfers by detecting when both hosts are the same. + +## Host Specification Format + +Hosts can be specified in several formats: + +- `hostname` - connects as the current user +- `user@hostname` - connects as the specified user +- `ssh://hostname` or `ssh://user@hostname` - URI format (scheme is stripped) +- `ssh-ng://hostname` or `ssh-ng://user@hostname` - Nix store URI format (scheme + is stripped) +- IPv6 addresses must use bracketed notation: `[2001:db8::1]` or + `user@[2001:db8::1]` + +> [!NOTE] +> Due to restrictions of Nix's SSH remote handling, ports cannot be specified in +> the host string. Use `NIX_SSHOPTS="-p 2222"` or configure ports in +> `~/.ssh/config` for the host you are building on/deploying to. + +## SSH Configuration + +### Authentication + +Remote deployment connects via SSH as the user running `nh`, not as `root`. This +means: + +- Your SSH keys and agent are used +- You can use keys with passphrases (via ssh-agent) +- You can use interactive authentication if needed + +This differs from `nix build --builders`, which connects via the `nix-daemon` +running as `root`. + +### Connection Multiplexing + +`nh` uses SSH's `ControlMaster` feature to share connections: + +```plaintext +ControlMaster=auto +ControlPath=/nh-ssh-%n +ControlPersist=60 +``` + +This reduces overhead when multiple SSH operations are performed. The first SSH +connection to a host creates a master connection, and subsequent operations to +the same host reuse it, avoiding repeated authentication. + +SSH control connections are automatically cleaned up when `nh` completes, +ensuring no lingering SSH processes remain. + +### Custom SSH Options + +Use the `NIX_SSHOPTS` environment variable to pass additional SSH options: + +```bash +NIX_SSHOPTS="-p 2222 -i ~/.ssh/custom_key" nh os switch --build-host user@host +``` + +Options in `NIX_SSHOPTS` are merged with the default options. For persistent +configuration, use `~/.ssh/config`: + +```plaintext +Host buildserver + HostName 192.168.1.100 + Port 2222 + User builder + IdentityFile ~/.ssh/builder_key +``` + +Then simply use: + +```bash +nh os switch --build-host buildserver +``` + +## Environment Variables + +### NH_REMOTE_CLEANUP + +When set, nh will attempt to terminate remote Nix processes when you press +Ctrl+C during a remote build. This uses `pkill` on the remote host to clean up +the build process. + +```bash +export NH_REMOTE_CLEANUP=1 +nh os switch --build-host user@buildserver +``` + +Valid values: `1`, `true`, `yes` (case-insensitive). + +This feature is **opt-in** because it is inherently fragile - remote process +cleanup depends on SSH still being functional and `pkill` being available. You +may still see zombie processes on the remote host if the connection drops before +cleanup can complete. + +### NH_NO_VALIDATE + +When set, skips pre-activation system validation checks. Useful when the target +host's store path isn't accessible from the local machine (e.g., building +remotely and deploying to a different target). + +```bash +export NH_NO_VALIDATE=1 +nh os switch --build-host user@buildserver --target-host user@production +``` + +## How Remote Builds Work + +When you use `--build-host`, `nh` follows this process: + +1. **Evaluate** the derivation path locally using + `nix eval --raw .drvPath` +2. **Copy derivation** to the build host using `nix-copy-closure --to` +3. **Build remotely** by running `nix build ^* --print-out-paths` on the + build host +4. **Copy result** back based on the deployment scenario (see below) + +### Copy Optimization + +To avoid unnecessary network transfers, `nh` optimizes copies based on your +configuration: + + + +| Scenario | Copy Path | +| -------------------------------------------- | -------------------------------------------------- | +| Build remote, no target | `build -> local` | +| Build remote, target = different host | `build -> target`, `build -> local` (for out-link) | +| Build remote, target = build host | `(nothing)` (already on target) | +| Build remote, target = build host + out-link | `build -> local` (only for out-link) | + + + +If `--build-host` and `--target-host` differ, NH will attempt a quick connection +from the build host to the target host to see if it can handle the copy directly +without relaying over localhost. This operation **will not fail the remote build +process**, and NH will simply relay over the orchestrator, i.e., the host you +have ran `nh os build` on. This is implemented as a minor convenience function, +and has zero negative effect over your builds. Instead, it may optimize the +number of connections when all hosts are connected over Tailscale, for example. + +When `--build-host` and `--target-host` point to the same machine, the result +stays on that machine unless you need a local out-link (symlink to the build +result). + +For security, you are _encouraged to be explicit_ in your hostnames and not +trust the DNS blindly. + +## Substitutes + +Use `--use-substitutes` to allow remote hosts to fetch pre-built binaries from +binary caches instead of building everything: + +```bash +nh os switch --build-host buildserver --use-substitutes +``` + +This passes: + +- `--use-substitutes` to `nix-copy-closure` +- `--substitute-on-destination` to `nix copy` (when copying between two remote + hosts) + +## Build Output + +### nix-output-monitor + +By default, build output is shown directly. While the NH package is wrapped with +nix-output-monitor, you will need `nix-output-monitor` available on the build +host if you want NH to be able to use it. + +If `nix-output-monitor` creates issues for whatever reason, you may disable it +with `--no-nom`. From 00e1d23df83ad5167bea49d7cd57cc1a3dfd8cb4 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 2 Jan 2026 22:55:00 +0300 Subject: [PATCH 29/38] xtask: populate ENVIRONMENT section in manpages Signed-off-by: NotAShelf Change-Id: I6cfae3f78880cd471815bcd4b438823c6a6a6964 --- xtask/src/man.rs | 116 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/xtask/src/man.rs b/xtask/src/man.rs index 92f9b9e3..c1a961ff 100644 --- a/xtask/src/man.rs +++ b/xtask/src/man.rs @@ -46,6 +46,122 @@ pub fn generate(out_dir: &str) -> Result<(), String> { .to_writer(&mut buffer) .map_err(|e| format!("Failed to write exit status section: {}", e))?; + // ENVIRONMENT section + let env_vars = [ + ( + "NH_NO_CHECKS", + "When set (any non-empty value), skips startup checks such as Nix \ + version and experimental feature validation. Useful for generating \ + completions or running in constrained build environments.", + ), + ( + "NH_FLAKE", + "Preferred path/reference to a directory containing your flake.nix used \ + by NH when running flake-based commands. Historically FLAKE was used; \ + NH will migrate FLAKE into NH_FLAKE if present.", + ), + ( + "NH_OS_FLAKE", + "Command-specific flake reference for nh os commands. Takes precedence \ + over NH_FLAKE.", + ), + ( + "NH_HOME_FLAKE", + "Command-specific flake reference for nh home commands. Takes \ + precedence over NH_FLAKE.", + ), + ( + "NH_DARWIN_FLAKE", + "Command-specific flake reference for nh darwin commands. Takes \ + precedence over NH_FLAKE.", + ), + ( + "NH_SUDO_ASKPASS", + "Path to a program used as SUDO_ASKPASS when NH self-elevates with sudo.", + ), + ( + "NH_PRESERVE_ENV", + "Controls whether environment variables marked for preservation are \ + passed to elevated commands. Set to \"0\" to disable, \"1\" to force. \ + If unset, defaults to enabled.", + ), + ( + "NH_SHOW_ACTIVATION_LOGS", + "Controls whether activation output is displayed. By default, \ + activation output is hidden. Setting to \"1\" shows full logs.", + ), + ( + "NH_LOG", + "Sets the tracing/log filter for NH. Uses tracing_subscriber format \ + (e.g., nh=trace).", + ), + ( + "NH_NOM", + "Control whether nix-output-monitor (nom) is enabled for build \ + processes. Equivalent of --no-nom.", + ), + ( + "NH_REMOTE_CLEANUP", + "Whether to clean up remote processes on interrupt via pkill. Opt-in \ + due to fragile behavior.", + ), + ( + "NIXOS_INSTALL_BOOTLOADER", + "Forwarded to switch-to-configuration. If true, forces bootloader \ + installation. Also available via --install-bootloader.", + ), + ( + "NIX_SSHOPTS", + "SSH options passed to Nix commands for remote builds.", + ), + ( + "NIX_CONFIG", + "Nix configuration options passed to all Nix commands.", + ), + ( + "NIX_REMOTE", + "Remote store configuration for Nix operations.", + ), + ( + "NIX_SSL_CERT_FILE", + "SSL certificate file for Nix HTTPS operations.", + ), + ("NIX_USER_CONF_FILES", "User configuration files for Nix."), + ]; + let mut sect = Roff::new(); + sect.control("SH", ["ENVIRONMENT"]); + for (var, desc) in env_vars { + sect.control("IP", [var]).text([roman(desc)]); + } + sect + .to_writer(&mut buffer) + .map_err(|e| format!("Failed to write environment section: {}", e))?; + + // FILES section + let files = [ + ( + "/nix/var/nix/profiles/system", + "System profile directory containing system generations.", + ), + ( + "/run/current-system", + "Symlink to the currently active system profile.", + ), + ( + "/etc/specialisation", + "NixOS specialisation detection. Contains the active specialisation \ + name.", + ), + ]; + let mut sect = Roff::new(); + sect.control("SH", ["FILES"]); + for (path, desc) in files { + sect.control("IP", [path]).text([roman(desc)]); + } + sect + .to_writer(&mut buffer) + .map_err(|e| format!("Failed to write files section: {}", e))?; + // EXAMPLES section let examples = [ ( From a868222d2cecdd28392e71cf3e9ee190a5376049 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sun, 4 Jan 2026 12:13:56 +0300 Subject: [PATCH 30/38] various: fix minor typos Signed-off-by: NotAShelf Change-Id: I1bf3cecd4d5a8d055cbe43b5075372426a6a6964 --- src/commands.rs | 8 ++++---- src/interface.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 9bebfcaf..79ecfe62 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -741,10 +741,10 @@ impl Build { .stdout(Redirection::None); debug!(?pipeline); - // Use popen() to get access to individual processes so we can check - // nix's exit status, not nom's. The pipeline's join() only returns + // Use `popen()` to get access to individual processes so we can check + // Nix's exit status, not nom's. The pipeline's `join()` only returns // the exit status of the last command (nom), which always succeeds - // even when nix fails. + // even when Nix fails. let mut processes = pipeline.popen()?; // Wait for all processes to finish @@ -753,7 +753,7 @@ impl Build { } // Check the exit status of the FIRST process (nix build) - // This is the one that matters - if nix fails, we should fail too + // This is the one that matters. If Nix fails, we should fail as well if let Some(nix_proc) = processes.first() { if let Some(exit_status) = nix_proc.exit_status() { match exit_status { diff --git a/src/interface.rs b/src/interface.rs index b284f9d5..8a6f326f 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -549,7 +549,7 @@ pub struct HomeRebuildArgs { #[arg(long, env = "NH_SHOW_ACTIVATION_LOGS")] pub show_activation_logs: bool, - /// Build the configuration on a different host over ssh + /// Build the configuration on a different host over SSH #[arg(long)] pub build_host: Option, } @@ -659,7 +659,7 @@ pub struct DarwinRebuildArgs { #[arg(long, env = "NH_SHOW_ACTIVATION_LOGS")] pub show_activation_logs: bool, - /// Build the configuration on a different host over ssh + /// Build the configuration on a different host over SSH #[arg(long)] pub build_host: Option, } From 38bd0d20d7f778191c36a1145182923b204e0112 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sun, 4 Jan 2026 12:25:15 +0300 Subject: [PATCH 31/38] treewide: consolidate remote activation logic into `nh::remote` Signed-off-by: NotAShelf Change-Id: I3a52c45ee7fd9f9ab27bed587a2c57336a6a6964 --- src/commands.rs | 9 -- src/darwin.rs | 16 +- src/home.rs | 29 +++- src/nixos.rs | 154 ++++++++++++------- src/remote.rs | 346 ++++++++++++++++++++++++++++++++++++++++++- src/util/platform.rs | 21 --- 6 files changed, 476 insertions(+), 99 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 79ecfe62..427c5556 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -191,13 +191,6 @@ impl Command { self } - /// Set the SSH target for remote command execution. - #[must_use] - pub fn ssh(mut self, ssh: Option) -> Self { - self.ssh = ssh; - self - } - /// Add a single argument to the command. #[must_use] pub fn arg>(mut self, arg: S) -> Self { @@ -858,7 +851,6 @@ mod tests { .dry(true) .show_output(true) .elevate(Some(ElevationStrategy::Force("sudo"))) - .ssh(Some("host".to_string())) .message("test message") .arg("arg1") .args(["arg2", "arg3"]); @@ -866,7 +858,6 @@ mod tests { assert!(cmd.dry); assert!(cmd.show_output); assert_eq!(cmd.elevate, Some(ElevationStrategy::Force("sudo"))); - assert_eq!(cmd.ssh, Some("host".to_string())); assert_eq!(cmd.message, Some("test message".to_string())); assert_eq!(cmd.args, vec![ OsString::from("arg1"), diff --git a/src/darwin.rs b/src/darwin.rs index bd82f48c..7fde98b3 100644 --- a/src/darwin.rs +++ b/src/darwin.rs @@ -26,9 +26,23 @@ const CURRENT_PROFILE: &str = "/run/current-system"; impl DarwinArgs { /// Run the `darwin` subcommand. /// + /// # Parameters + /// + /// * `self` - The Darwin operation arguments + /// * `elevation` - The privilege elevation strategy (sudo/doas/none) + /// + /// # Returns + /// + /// Returns `Ok(())` if the operation succeeds. + /// /// # Errors /// - /// Returns an error if the operation fails. + /// Returns an error if: + /// + /// - Build or activation operations fail + /// - Remote operations encounter network or SSH issues + /// - Nix evaluation or building fails + /// - File system operations fail #[cfg_attr(feature = "hotpath", hotpath::measure)] pub fn run(self, elevation: ElevationStrategy) -> Result<()> { use DarwinRebuildVariant::{Build, Switch}; diff --git a/src/home.rs b/src/home.rs index 19d8be29..1f860e8b 100644 --- a/src/home.rs +++ b/src/home.rs @@ -19,9 +19,22 @@ use crate::{ impl interface::HomeArgs { /// Run the `home` subcommand. /// + /// # Parameters + /// + /// * `self` - The Home Manager operation arguments + /// + /// # Returns + /// + /// Returns `Ok(())` if the operation succeeds. + /// /// # Errors /// - /// Returns an error if the operation fails. + /// Returns an error if: + /// + /// - Build or activation operations fail + /// - Remote operations encounter network or SSH issues + /// - Nix evaluation or building fails + /// - File system operations fail #[cfg_attr(feature = "hotpath", hotpath::measure)] pub fn run(self) -> Result<()> { use HomeRebuildVariant::{Build, Switch}; @@ -157,12 +170,13 @@ impl HomeRebuildArgs { let spec_location = PathBuf::from(std::env::var("HOME")?) .join(".local/share/home-manager/specialisation"); - let current_specialisation = if let Some(s) = spec_location.to_str() { - std::fs::read_to_string(s).ok() - } else { - tracing::warn!("spec_location path is not valid UTF-8"); - None - }; + let current_specialisation = spec_location.to_str().map_or_else( + || { + tracing::warn!("spec_location path is not valid UTF-8"); + None + }, + |s| std::fs::read_to_string(s).ok(), + ); let target_specialisation = if self.no_specialisation { None @@ -425,6 +439,7 @@ where } }, Installable::Store { .. } => {}, + #[allow(clippy::unreachable, reason = "Should never happen")] Installable::Unspecified => { unreachable!( "Unspecified installable should have been resolved before calling \ diff --git a/src/nixos.rs b/src/nixos.rs index 248a094c..515c46b5 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -34,6 +34,25 @@ const CURRENT_PROFILE: &str = "/run/current-system"; const SPEC_LOCATION: &str = "/etc/specialisation"; impl interface::OsArgs { + /// Executes the NixOS subcommand. + /// + /// # Parameters + /// + /// * `self` - The NixOS operation arguments + /// * `elevation` - The privilege elevation strategy (sudo/doas/none) + /// + /// # Returns + /// + /// Returns `Ok(())` if the operation succeeds. + /// + /// # Errors + /// + /// Returns an error if: + /// + /// - Build or activation operations fail + /// - Remote operations encounter network or SSH issues + /// - Nix evaluation or building fails + /// - File system operations fail #[cfg_attr(feature = "hotpath", hotpath::measure)] pub fn run(self, elevation: ElevationStrategy) -> Result<()> { use OsRebuildVariant::{Boot, Build, Switch, Test}; @@ -189,21 +208,14 @@ impl OsRebuildActivateArgs { // Only copy if the output path exists locally (i.e., was copied back from // remote build) if out_path.exists() { - Command::new("nix") - .args([ - "copy", - "--to", - format!("ssh://{target_host}").as_str(), - match target_profile.to_str() { - Some(s) => s, - None => { - return Err(eyre!("target_profile path is not valid UTF-8")); - }, - }, - ]) - .message("Copying configuration to target") - .with_required_env() - .run()?; + let target = RemoteHost::parse(target_host) + .wrap_err("Invalid target host specification")?; + remote::copy_to_remote( + &target, + target_profile, + self.rebuild.common.passthrough.use_substitutes, + ) + .context("Failed to copy configuration to target host")?; } } @@ -266,52 +278,88 @@ impl OsRebuildActivateArgs { })?; if let Test | Switch = variant { - let variant_label = match variant { - Test => "test", - Switch => "switch", - #[allow(clippy::unreachable, reason = "Should never happen.")] - _ => unreachable!(), - }; - - Command::new(canonical_out_path) - .arg("test") - .ssh(self.rebuild.target_host.clone()) - .message("Activating configuration") - .elevate(elevate.then_some(elevation.clone())) - .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) - .with_required_env() - .show_output(self.show_activation_logs) - .run() - .wrap_err(format!("Activation ({variant_label}) failed"))?; + if let Some(target_host) = &self.rebuild.target_host { + let target = RemoteHost::parse(target_host) + .wrap_err("Invalid target host specification")?; + + let activation_type = match variant { + Test => remote::ActivationType::Test, + Switch => remote::ActivationType::Switch, + #[allow(clippy::unreachable, reason = "Should never happen.")] + _ => unreachable!(), + }; + + remote::activate_remote( + &target, + &resolved_profile, + &remote::ActivateRemoteConfig { + platform: remote::Platform::NixOS, + activation_type, + install_bootloader: false, + show_logs: self.show_activation_logs, + sudo: elevate, + }, + ) + .wrap_err(format!( + "Activation ({}) failed", + activation_type.as_str() + ))?; + } else { + Command::new(canonical_out_path) + .arg("test") + .message("Activating configuration") + .elevate(elevate.then_some(elevation.clone())) + .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) + .with_required_env() + .show_output(self.show_activation_logs) + .run() + .wrap_err("Activation (test) failed")?; + } debug!("Completed {variant:?} operation with output path: {out_path:?}"); } if let Boot | Switch = variant { - Command::new("nix") - .elevate(elevate.then_some(elevation.clone())) - .args(["build", "--no-link", "--profile", SYSTEM_PROFILE]) - .arg(canonical_out_path) - .ssh(self.rebuild.target_host.clone()) - .with_required_env() - .run() - .wrap_err("Failed to set system profile")?; + if let Some(target_host) = &self.rebuild.target_host { + let target = RemoteHost::parse(target_host) + .wrap_err("Invalid target host specification")?; + + remote::activate_remote( + &target, + &resolved_profile, + &remote::ActivateRemoteConfig { + platform: remote::Platform::NixOS, + activation_type: remote::ActivationType::Boot, + install_bootloader: self.rebuild.install_bootloader, + show_logs: false, + sudo: elevate, + }, + ) + .wrap_err("Bootloader activation failed")?; + } else { + Command::new("nix") + .elevate(elevate.then_some(elevation.clone())) + .args(["build", "--no-link", "--profile", SYSTEM_PROFILE]) + .arg(canonical_out_path) + .with_required_env() + .run() + .wrap_err("Failed to set system profile")?; - let mut cmd = Command::new(switch_to_configuration) - .arg("boot") - .ssh(self.rebuild.target_host.clone()) - .elevate(elevate.then_some(elevation)) - .message("Adding configuration to bootloader") - .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]); + let mut cmd = Command::new(switch_to_configuration) + .arg("boot") + .elevate(elevate.then_some(elevation)) + .message("Adding configuration to bootloader") + .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]); - if self.rebuild.install_bootloader { - cmd = cmd.set_env("NIXOS_INSTALL_BOOTLOADER", "1"); - } + if self.rebuild.install_bootloader { + cmd = cmd.set_env("NIXOS_INSTALL_BOOTLOADER", "1"); + } - cmd - .with_required_env() - .run() - .wrap_err("Bootloader activation failed")?; + cmd + .with_required_env() + .run() + .wrap_err("Bootloader activation failed")?; + } } debug!("Completed {variant:?} operation with output path: {out_path:?}"); diff --git a/src/remote.rs b/src/remote.rs index 59f195d5..6c505a87 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -242,7 +242,7 @@ impl RemoteHost { /// /// Used for hostname comparisons when determining if two `RemoteHost` /// instances refer to the same physical host (e.g., detecting when - /// build_host == target_host regardless of different user credentials). + /// `build_host` == `target_host` regardless of different user credentials). /// /// Returns the bracketed IPv6 address as-is if present (e.g., /// `[2001:db8::1]`). @@ -846,6 +846,70 @@ fn copy_closure_from( Ok(()) } +/// Copy a Nix closure from localhost to a remote host. +/// +/// Uses `nix copy --to ssh://host` to transfer a store path and its +/// dependencies from the local Nix store to a remote machine via SSH. +/// +/// When `use_substitutes` is enabled, the remote host will attempt to fetch +/// missing paths from configured binary caches instead of transferring them +/// over SSH, which can significantly improve performance and reduce bandwidth +/// usage. +/// +/// # Arguments +/// +/// * `host` - The remote host to copy the closure to. SSH connection +/// multiplexing and options from `NIX_SSHOPTS` are automatically applied. +/// * `path` - The store path to copy (e.g., `/nix/store/xxx-nixos-system`). All +/// dependencies (the complete closure) are copied automatically. +/// * `use_substitutes` - When `true`, adds `--substitute-on-destination` to +/// allow the remote host to fetch missing paths from binary caches instead of +/// transferring them over SSH. +/// +/// # Returns +/// +/// Returns `Ok(())` on success, or an error if the copy operation fails. +/// +/// # Errors +/// +/// Returns an error if: +/// +/// - The SSH connection to the remote host fails +/// - The `nix copy` command fails (e.g., insufficient disk space on remote, +/// network issues, authentication failures) +/// - The path does not exist in the local store +pub fn copy_to_remote( + host: &RemoteHost, + path: &Path, + use_substitutes: bool, +) -> Result<()> { + info!("Copying closure to remote host '{}'", host); + + let flake_flags = get_flake_flags(); + let mut cmd = Exec::cmd("nix") + .args(&flake_flags) + .args(&["copy", "--to"]) + .arg(format!("ssh://{}", host.ssh_host())); + + if use_substitutes { + cmd = cmd.arg("--substitute-on-destination"); + } + + cmd = cmd.arg(path).env("NIX_SSHOPTS", get_nix_sshopts_env()); + + debug!(?cmd, "nix copy --to"); + + let capture = cmd + .capture() + .wrap_err("Failed to copy closure to remote host")?; + + if !capture.exit_status.success() { + bail!("nix copy --to '{}' failed:\n{}", host, capture.stderr_str()); + } + + Ok(()) +} + /// Copy a Nix closure from one remote host to another. /// Uses `nix copy --from ssh://source --to ssh://dest`. fn copy_closure_between_remotes( @@ -888,6 +952,256 @@ fn copy_closure_between_remotes( Ok(()) } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// Represents the type of activation to perform on a remote system. +/// +/// This determines which action the system's activation script will execute. +pub enum ActivationType { + /// Run the configuration in a test mode without activating + Test, + + /// Atomically switch to the new configuration + Switch, + + /// Make the new configuration the default boot option + Boot, +} + +impl ActivationType { + /// Get the string representation used by activation scripts. + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Self::Test => "test", + Self::Switch => "switch", + Self::Boot => "boot", + } + } +} + +/// Represents the target platform for remote operations. +/// +/// This enum allows the remote module to support multiple platforms while +/// keeping the implementation generic. Currently only NixOS is implemented. +/// Other platforms can be added in the future. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Platform { + /// NixOS system configuration + NixOS, + // TODO: Add Darwin and HomeManager support + // + // To add support for other platforms: + // + // 1. Add the platform variant to this enum + // 2. Implement platform-specific activation logic in a (private) function + // 3. Update `activate_remote()` to dispatch to the new platform handler + // Darwin, + // HomeManager, +} + +/// Configuration for remote activation operations. +#[derive(Debug)] +pub struct ActivateRemoteConfig { + /// The target platform for activation + pub platform: Platform, + + /// The type of activation to perform + pub activation_type: ActivationType, + + /// Whether to install the bootloader during activation + pub install_bootloader: bool, + + /// Whether to show output logs during activation + pub show_logs: bool, + + /// Whether to use sudo for activation commands + pub sudo: bool, +} + +/// Activate a system configuration on a remote host. +/// +/// Currently only supports NixOS. +/// +/// # Arguments +/// +/// * `host` - The remote host to activate on +/// * `system_profile` - The path to the NixOS system profile (e.g., +/// /nix/var/nix/profiles/system) +/// * `config` - Activation configuration options +/// +/// # Errors +/// +/// Returns an error if SSH connection fails or activation commands fail. +pub fn activate_remote( + host: &RemoteHost, + system_profile: &Path, + config: &ActivateRemoteConfig, +) -> Result<()> { + match config.platform { + Platform::NixOS => activate_nixos_remote(host, system_profile, config), + // TODO: + // Platform::Darwin => activate_darwin_remote(host, system_profile, config), + // Platform::HomeManager => activate_home_remote(host, system_profile, + // config), + } +} + +/// Activate a NixOS system configuration on a remote host. +/// +/// Handles the SSH commands required to activate a NixOS system. Supports +/// test, switch, and boot activation types. +/// +/// # Arguments +/// +/// * `host` - The remote host to activate on +/// * `system_profile` - The path to the NixOS system profile +/// * `config` - Activation configuration options +/// +/// # Errors +/// +/// Returns an error if SSH connection fails or activation commands fail. +fn activate_nixos_remote( + host: &RemoteHost, + system_profile: &Path, + config: &ActivateRemoteConfig, +) -> Result<()> { + let ssh_opts = get_ssh_opts(); + + let switch_to_config = system_profile.join("bin/switch-to-configuration"); + + let switch_path_str = switch_to_config.to_str().ok_or_else(|| { + eyre!("switch-to-configuration path contains invalid UTF-8") + })?; + + match config.activation_type { + ActivationType::Test | ActivationType::Switch => { + let action = config.activation_type.as_str(); + + let mut ssh_cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + ssh_cmd = ssh_cmd.arg(opt); + } + ssh_cmd = ssh_cmd.arg(host.ssh_host()); + + // Build the remote command with proper quoting + let remote_cmd = if config.sudo { + format!("sudo {} {}", shell_quote(switch_path_str), action) + } else { + format!("{} {}", shell_quote(switch_path_str), action) + }; + + ssh_cmd = ssh_cmd.arg(remote_cmd); + + if config.show_logs { + ssh_cmd = ssh_cmd + .stdout(Redirection::Merge) + .stderr(Redirection::Merge); + } + + debug!(?ssh_cmd, "Activating NixOS configuration"); + + let capture = ssh_cmd + .capture() + .wrap_err("Failed to activate NixOS configuration")?; + + if !capture.exit_status.success() { + bail!( + "Activation ({}) failed on '{}':\n{}", + action, + host, + capture.stderr_str() + ); + } + }, + + ActivationType::Boot => { + let mut profile_ssh_cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + profile_ssh_cmd = profile_ssh_cmd.arg(opt); + } + profile_ssh_cmd = profile_ssh_cmd.arg(host.ssh_host()); + + // Build the remote command with proper quoting + let profile_remote_cmd = if config.sudo { + format!( + "sudo nix build --no-link --profile {} {}", + NIXOS_SYSTEM_PROFILE, + system_profile.display() + ) + } else { + format!( + "nix build --no-link --profile {} {}", + NIXOS_SYSTEM_PROFILE, + system_profile.display() + ) + }; + + profile_ssh_cmd = profile_ssh_cmd.arg(profile_remote_cmd); + + debug!(?profile_ssh_cmd, "Setting NixOS profile"); + + let profile_capture = profile_ssh_cmd + .capture() + .wrap_err("Failed to set NixOS profile")?; + + if !profile_capture.exit_status.success() { + bail!( + "Failed to set system profile on '{}':\n{}", + host, + profile_capture.stderr_str() + ); + } + + let mut boot_ssh_cmd = Exec::cmd("ssh"); + for opt in &ssh_opts { + boot_ssh_cmd = boot_ssh_cmd.arg(opt); + } + boot_ssh_cmd = boot_ssh_cmd.arg(host.ssh_host()); + + // Build the remote command with proper quoting + let boot_remote_cmd = if config.install_bootloader { + if config.sudo { + format!( + "sudo NIXOS_INSTALL_BOOTLOADER=1 {} boot", + shell_quote(switch_path_str) + ) + } else { + format!( + "NIXOS_INSTALL_BOOTLOADER=1 {} boot", + shell_quote(switch_path_str) + ) + } + } else if config.sudo { + format!("sudo {} boot", shell_quote(switch_path_str)) + } else { + format!("{} boot", shell_quote(switch_path_str)) + }; + + boot_ssh_cmd = boot_ssh_cmd.arg(boot_remote_cmd); + + debug!(?boot_ssh_cmd, "Bootloader activation"); + + let boot_capture = boot_ssh_cmd + .capture() + .wrap_err("Bootloader activation failed")?; + + if !boot_capture.exit_status.success() { + bail!( + "Bootloader activation failed on '{}':\n{}", + host, + boot_capture.stderr_str() + ); + } + }, + } + + Ok(()) +} + +/// System profile path for NixOS. +/// Used by remote activation functions. +const NIXOS_SYSTEM_PROFILE: &str = "/nix/var/nix/profiles/system"; + /// Evaluate a flake installable to get its derivation path. /// Matches nixos-rebuild-ng: `nix eval --raw .drvPath` fn eval_drv_path(installable: &Installable) -> Result { @@ -1058,7 +1372,14 @@ pub fn build_remote( "Skipping copy from build host to target host (same host: {})", build_host.hostname() ); - out_link.is_some() + + // When build_host == target_host and both are remote, the result is + // already where it needs to be. No need to copy to localhost even if + // out_link is requested, since the closure will be activated remotely. + // This is a little confusing, but frankly, respecting --out-link to + // create a local path while everything happens remotely is a bit + // more confusing. + false }, Some(target_host) => { match copy_closure_between_remotes( @@ -1093,13 +1414,22 @@ pub fn build_remote( copy_closure_from(build_host, &out_path, use_substitutes)?; } - // Create local out-link if requested + // Create local out-link if requested and the result is in local store + // When build_host == target_host (both remote), skip out-link creation + // since the closure is remote and won't be copied to localhost if let Some(link) = out_link { - debug!("Creating out-link: {} -> {}", link.display(), out_path); - // Remove existing symlink/file if present - let _ = std::fs::remove_file(link); - std::os::unix::fs::symlink(&out_path, link) - .wrap_err("Failed to create out-link")?; + if need_local_copy { + debug!("Creating out-link: {} -> {}", link.display(), out_path); + // Remove existing symlink/file if present + let _ = std::fs::remove_file(link); + std::os::unix::fs::symlink(&out_path, link) + .wrap_err("Failed to create out-link")?; + } else { + debug!( + "Skipping out-link creation: result is on remote host and not copied \ + to localhost" + ); + } } Ok(PathBuf::from(out_path)) diff --git a/src/util/platform.rs b/src/util/platform.rs index 048db99c..606c2839 100644 --- a/src/util/platform.rs +++ b/src/util/platform.rs @@ -490,27 +490,6 @@ pub fn get_target_hostname( Ok((target_hostname, hostname_mismatch)) } -/// Common function to activate configurations in `NixOS` -pub fn activate_nixos_configuration( - target_profile: &Path, - variant: &str, - target_host: Option, - elevate: bool, - message: &str, -) -> Result<()> { - let switch_to_configuration = target_profile.join("bin").join("switch-to-configuration"); - let switch_to_configuration = switch_to_configuration.canonicalize().map_err(|e| { - color_eyre::eyre::eyre!("Failed to canonicalize switch-to-configuration path: {}", e) - })?; - - commands::Command::new(switch_to_configuration) - .arg(variant) - .ssh(target_host) - .message(message) - .elevate(elevate) - .run() -} - /// Configuration options for rebuilding workflows pub struct RebuildWorkflowConfig<'a> { /// The Nix installable representing the configuration From 260799845e426bfbd6e79e41ddf340075286da41 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sun, 4 Jan 2026 17:41:14 +0300 Subject: [PATCH 32/38] treewide: change elevation program to elevation strategy This is a massive change, and it primarily concerns `--elevation-*program*` not making any sense for remote operations. The change to *strategy* allows the flag to be more descriptive in terms of how it operates, and allows us to add new strategies to handle. Fixes https://github.com/nix-community/nh/issues/434 Signed-off-by: NotAShelf Change-Id: I9e51838007feca7d2d914402d7f11dbc6a6a6964 --- CHANGELOG.md | 13 ++- src/commands.rs | 174 ++++++++++++++++++++++++++-------- src/interface.rs | 18 +++- src/main.rs | 14 ++- src/nixos.rs | 91 ++++++++++++------ src/remote.rs | 236 +++++++++++++++++++++++++++++++++++++++-------- 6 files changed, 432 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de2eb4f5..d6b1befa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,13 @@ functionality, under the "Removed" section. ### Changed +- `--elevation-program` flag was renamed to `--elevation-strategy` with support + for `'none'` (no elevation) and `'passwordless'` (for remote hosts with + `NOPASSWD` configured) values. The old flag name remains available as an alias + for backward compatibility. It may be removed at a later version. + ([#434](https://github.com/nix-community/nh/issues/434)) + - Multi-program remote elevation support: `sudo`, `doas`, `run0`, and `pkexec` + are now supported with correct flags for each program - Platform commands (`nh os`, `nh home`, `nh darwin`) now support SSH-based remote builds via `--build-host`. The flag now uses proper remote build semantics: derivations are copied to the remote host via `nix-copy-closure`, @@ -33,7 +40,7 @@ functionality, under the "Removed" section. fragility. - Shell argument splitting now uses `shlex` for proper quote handling in complex command arguments. -- `nh os info` now support `--fields` to select which field(s) to display +- `nh os info` now supports `--fields` to select which field(s) to display ([#375](https://github.com/nix-community/nh/issues/375)). - Empty columns are now hidden by default to avoid visual clutter. - A new, per-generation "Closure Size" column has been added @@ -63,7 +70,7 @@ functionality, under the "Removed" section. variable. - `nh search` displays a link to the `package.nix` file on the nixpkgs GitHub, and also fixes the existing links so that they no longer brokenly point to a - non-existent file path on nix flake systems. + non-existent file path on Nix flake systems. ### Fixed @@ -87,6 +94,8 @@ functionality, under the "Removed" section. the installable such as (`./flake.nix#myHost`) in the past and lead to confusing behaviour for those unfamiliar. Such arguments are now normalized with a warning if NH can parse them. + - Password caching now works across all remote operations. + - Empty password validation prevents invalid credential caching. ### Removed diff --git a/src/commands.rs b/src/commands.rs index 427c5556..674da5ec 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -20,20 +20,44 @@ use crate::{installable::Installable, interface::NixBuildPassthroughArgs}; static PASSWORD_CACHE: OnceLock>> = OnceLock::new(); -fn get_cached_password(host: &str) -> Option { +/// Retrieves a cached password for the specified host. +/// +/// # Arguments +/// +/// * `host` - The host identifier (e.g., "user@hostname" or "hostname") to look +/// up in the cache +/// +/// # Returns +/// +/// * `Some(SecretString)` - If a password for the host exists in the cache +/// * `None` - If no password has been cached for this host +pub fn get_cached_password(host: &str) -> Result> { let cache = PASSWORD_CACHE.get_or_init(|| Mutex::new(HashMap::new())); let guard = cache .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - guard.get(host).cloned() + .map_err(|_| eyre::eyre!("Password cache lock poisoned"))?; + Ok(guard.get(host).cloned()) } -fn cache_password(host: &str, password: SecretString) { +/// Stores a password in the cache for the specified host. +/// +/// The password is stored as a `SecretString` to ensure secure memory +/// handling. Cached passwords persist for the lifetime of the program and can +/// be retrieved using [`get_cached_password`]. +/// +/// # Arguments +/// +/// * `host` - The host identifier (e.g., "user@hostname" or "hostname") to +/// associate with the password +/// * `password` - The password to cache, wrapped in a `SecretString` for secure +/// handling +pub fn cache_password(host: &str, password: SecretString) -> Result<()> { let cache = PASSWORD_CACHE.get_or_init(|| Mutex::new(HashMap::new())); let mut guard = cache .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); + .map_err(|_| eyre::eyre!("Password cache lock poisoned"))?; guard.insert(host.to_string(), password); + Ok(()) } fn ssh_wrap( @@ -70,35 +94,76 @@ pub enum EnvAction { Remove, } -/// Strategy for choosing a privilege elevation program. -/// - `Auto`: try supported programs in fallback order. -/// - `Prefer(PathBuf)`: try the specified program, then fallback. -/// - `Force(&'static str)`: use only the specified program, error if not -/// available. -#[allow(dead_code)] +/// Strategy for handling privilege elevation when running commands. +/// +/// This enum defines how `nh` should handle privilege elevation for commands +/// that require root access (e.g., `switch-to-configuration`). #[derive(Debug, Clone, PartialEq, Eq)] pub enum ElevationStrategy { + /// Automatically detect and use the first available elevation program + /// (tries doas -> sudo -> run0 -> pkexec in order). Uses askpass helper if + /// available. Auto, + + /// Try the specified elevation program first, fall back to `Auto` if not + /// found. Prefer(PathBuf), + + /// Use only the specified program name. + #[allow(dead_code)] Force(&'static str), + + /// Do not use any elevation program. Commands run without privilege + /// escalation. This will fail for commands requiring root unless the user is + /// already root or the system has other privilege mechanisms configured. + None, + + /// Use elevation program but skip password prompting. For remote hosts with + /// passwordless sudo (NOPASSWD in sudoers) or similar configurations. The + /// elevation command runs without `--stdin` or password input. + Passwordless, } impl ElevationStrategy { + /// Resolves the elevation strategy to an actual program path. + /// + /// Attempts to find an appropriate privilege elevation program based on the + /// strategy variant and system availability. + /// + /// # Returns + /// + /// Returns `Ok(PathBuf)` containing the path to the elevation program binary. + /// + /// # Errors + /// + /// Returns an error if: + /// + /// - `None` variant: Always fails (elevation is disabled via + /// `--elevation-strategy=none`) + /// - `Force` variant: The specified program is not found in PATH + /// - Other variants: No suitable elevation programs are available on the + /// system pub fn resolve(&self) -> Result { match self { - Self::Auto => Self::choice(), + Self::Auto | Self::Passwordless => Self::choice(), Self::Prefer(program) => { which(program).or_else(|_| { - let auto = Self::choice()?; warn!( - "{} not found. Using {} instead", - program.to_string_lossy(), - auto.to_string_lossy() + ?program, + "Preferred elevation program not found, falling back to \ + auto-detection" ); - Ok(auto) + Self::choice() }) }, - Self::Force(program) => Ok(program.into()), + Self::Force(program_name) => { + which(program_name).context(format!( + "Forced elevation program '{program_name}' not found in PATH" + )) + }, + // Only reachable if resolve() is called directly. Safe since callers + // check is_some() before invoking resolve(). + Self::None => bail!("Elevation disabled via --elevation-strategy=none"), } } @@ -145,6 +210,7 @@ impl ElevationStrategy { } #[derive(Debug)] +#[allow(clippy::struct_field_names)] pub struct Command { dry: bool, message: Option, @@ -356,23 +422,29 @@ impl Command { /// /// Panics: If called when `self.elevate` is `None` fn build_sudo_cmd(&self) -> Result { - let elevation_program = self + let elevation_strategy = self .elevate .as_ref() - .ok_or_else(|| eyre::eyre!("Command not found for elevation"))? + .ok_or_else(|| eyre::eyre!("Command not found for elevation"))?; + + let elevation_program = elevation_strategy .resolve() .context("Failed to resolve elevation program")?; let mut cmd = Exec::cmd(&elevation_program); - // Use NH_SUDO_ASKPASS program for sudo if present + // Use NH_SUDO_ASKPASS program for sudo if present, but NOT for + // Passwordless variant (Passwordless expects NOPASSWD config without + // password input) let program_name = elevation_program .file_name() .and_then(|name| name.to_str()) .ok_or_else(|| { eyre::eyre!("Failed to determine elevation program name") })?; - if program_name == "sudo" { + if program_name == "sudo" + && !matches!(elevation_strategy, ElevationStrategy::Passwordless) + { if let Ok(askpass) = std::env::var("NH_SUDO_ASKPASS") { cmd = cmd.env("SUDO_ASKPASS", askpass).arg("-A"); } @@ -392,10 +464,8 @@ impl Command { match action { EnvAction::Set(value) => Some(format!("{key}={value}")), EnvAction::Preserve if preserve_env => { - match std::env::var(key) { - Ok(value) => Some(format!("{key}={value}")), - Err(_) => None, - } + std::env::var(key) + .map_or(None, |value| Some(format!("{key}={value}"))) }, _ => None, } @@ -438,10 +508,8 @@ impl Command { match action { EnvAction::Set(value) => Some(format!("{key}={value}")), EnvAction::Preserve if preserve_env => { - match std::env::var(key) { - Ok(value) => Some(format!("{key}={value}")), - Err(_) => None, - } + std::env::var(key) + .map_or(None, |value| Some(format!("{key}={value}"))) }, _ => None, } @@ -503,15 +571,14 @@ impl Command { /// Panics if the command result is unexpectedly None. #[cfg_attr(feature = "hotpath", hotpath::measure)] pub fn run(&self) -> Result<()> { - // Prompt for sudo password if needed for remote deployment - // FIXME: this implementation only covers Sudo. I *think* doas and run0 are - // able to read from stdin, but needs to be tested and possibly - // mitigated. + // Prompt for elevation password if needed for remote deployment. + // Note: Only sudo supports stdin password input. For remote deployments + // with doas/run0, use --elevation-strategy=passwordless instead. let sudo_password = if self.ssh.is_some() && self.elevate.is_some() { let host = self.ssh.as_ref().ok_or_else(|| { eyre::eyre!("SSH host is None but elevation is required") })?; - if let Some(cached_password) = get_cached_password(host) { + if let Some(cached_password) = get_cached_password(host)? { Some(cached_password) } else { let password = @@ -519,8 +586,11 @@ impl Command { .without_confirmation() .prompt() .context("Failed to read sudo password")?; + if password.is_empty() { + bail!("Password cannot be empty"); + } let secret_password = SecretString::new(password.into()); - cache_password(host, secret_password.clone()); + cache_password(host, secret_password.clone())?; Some(secret_password) } } else { @@ -1037,11 +1107,10 @@ mod tests { .build_sudo_cmd() .expect("build_sudo_cmd should succeed in test"); - // Platform-agnostic: 'sudo' may not be the first token if env vars are - // injected (e.g., NH_SUDO_ASKPASS). Accept any command line where - // 'sudo' is present as a token. + // Platform-agnostic: 'sudo' may be a full path or just the program name. + // Accept any command line where a token ends with 'sudo'. let cmdline = sudo_exec.to_cmdline_lossy(); - assert!(cmdline.split_whitespace().any(|tok| tok == "sudo")); + assert!(cmdline.split_whitespace().any(|tok| tok.ends_with("sudo"))); } #[test] @@ -1106,6 +1175,31 @@ mod tests { assert!(cmdline.contains("TEST_VAR=test_value")); } + #[test] + fn test_elevation_strategy_passwordless_resolves() { + let strategy = ElevationStrategy::Passwordless; + let result = strategy.resolve(); + + // Passwordless should resolve to an elevation program just like Auto + assert!(result.is_ok()); + let program = result.unwrap(); + assert!(!program.as_os_str().is_empty()); + } + + #[test] + fn test_build_sudo_cmd_passwordless_no_stdin() { + let cmd = + Command::new("test").elevate(Some(ElevationStrategy::Force("sudo"))); + + let sudo_exec = + cmd.build_sudo_cmd().expect("build_sudo_cmd should succeed"); + let cmdline = sudo_exec.to_cmdline_lossy(); + + // Regular sudo should have --stdin and --prompt= flags + // (Note: Force("sudo") behaves like regular sudo, not Passwordless) + assert!(cmdline.contains("sudo")); + } + #[test] #[serial] fn test_build_sudo_cmd_with_remove_vars() { diff --git a/src/interface.rs b/src/interface.rs index 8a6f326f..0038bf84 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -51,9 +51,21 @@ pub struct Main { /// more detailed logs. pub verbosity: clap_verbosity_flag::Verbosity, - #[arg(short, long, global = true, env = "NH_ELEVATION_PROGRAM", value_hint = clap::ValueHint::CommandName)] - /// Choose what privilege elevation program should be used - pub elevation_program: Option, + #[arg( + short, + long, + global = true, + env = "NH_ELEVATION_STRATEGY", + value_hint = clap::ValueHint::CommandName, + alias = "elevation-program" + )] + /// Choose the privilege elevation strategy. + /// + /// Can be a path to an elevation program (e.g., /usr/bin/sudo), + /// or one of: 'none' (no elevation), + /// 'passwordless' (use elevation without password prompt for remote hosts + /// with NOPASSWD configured) + pub elevation_strategy: Option, #[command(subcommand)] pub command: NHCommand, diff --git a/src/main.rs b/src/main.rs index 3b076616..48fea177 100644 --- a/src/main.rs +++ b/src/main.rs @@ -41,9 +41,17 @@ fn main() -> Result<()> { // added to setup_environment in the future. checks::verify_variables()?; - let elevation = args - .elevation_program - .map_or(ElevationStrategy::Auto, ElevationStrategy::Prefer); + let elevation = + args + .elevation_strategy + .as_ref() + .map_or(ElevationStrategy::Auto, |path| { + match path.to_str() { + Some("none") => ElevationStrategy::None, + Some("passwordless") => ElevationStrategy::Passwordless, + _ => ElevationStrategy::Prefer(path.clone()), + } + }); args.command.run(elevation) } diff --git a/src/nixos.rs b/src/nixos.rs index 515c46b5..956a068f 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -70,7 +70,7 @@ impl interface::OsArgs { if args.common.ask || args.common.dry { warn!("`--ask` and `--dry` have no effect for `nh os build`"); } - args.build_only(&Build, None, elevation) + args.build_only(&Build, None, &elevation) }, OsSubcommand::BuildVm(args) => args.build_vm(elevation), OsSubcommand::Repl(args) => args.run(), @@ -107,7 +107,7 @@ impl OsBuildVmArgs { // Show warning if no hostname was explicitly provided for VM builds if self.common.hostname.is_none() { - let (_, target_hostname) = self.common.setup_build_context()?; + let (_, target_hostname) = self.common.setup_build_context(&elevation)?; tracing::warn!( "Guessing system is {target_hostname} for a VM image. If this isn't \ intended, use --hostname to change." @@ -117,7 +117,7 @@ impl OsBuildVmArgs { self.common.build_only( &OsRebuildVariant::BuildVm, Some(&attr), - elevation, + &elevation, )?; // If --run flag is set, execute the VM @@ -139,7 +139,8 @@ impl OsRebuildActivateArgs { ) -> Result<()> { use OsRebuildVariant::{Build, BuildVm}; - let (elevate, target_hostname) = self.rebuild.setup_build_context()?; + let (elevate, target_hostname) = + self.rebuild.setup_build_context(&elevation)?; let (out_path, _tempdir_guard) = self.rebuild.determine_output_path(variant)?; @@ -153,7 +154,8 @@ impl OsRebuildActivateArgs { _ => "Building NixOS configuration", }; - self.rebuild.execute_build(toplevel, &out_path, message)?; + let actual_store_path = + self.rebuild.execute_build(toplevel, &out_path, message)?; let target_profile = self.rebuild.resolve_specialisation_and_profile(&out_path)?; @@ -177,6 +179,7 @@ impl OsRebuildActivateArgs { variant, &out_path, &target_profile, + actual_store_path.as_deref(), elevate, elevation, )?; @@ -189,6 +192,7 @@ impl OsRebuildActivateArgs { variant: &OsRebuildVariant, out_path: &Path, target_profile: &Path, + actual_store_path: Option<&Path>, elevate: bool, elevation: ElevationStrategy, ) -> Result<()> { @@ -220,14 +224,19 @@ impl OsRebuildActivateArgs { } // Validate system closure before activation, unless bypassed. For remote - // builds where `out_path` doesn't exist locally, we can't canonicalize - // `target_profile`. Instead we use the path as-is for remote operations. + // builds, use the actual store path returned from the build. For local + // builds, canonicalize the target_profile. let is_remote_build = self.rebuild.target_host.is_some(); - let resolved_profile: PathBuf = if is_remote_build && !out_path.exists() { - // Remote build with no local result. Skip canonicalization, use original - // path + let resolved_profile: PathBuf = if let Some(store_path) = actual_store_path + { + // Remote build - use the actual store path from the build output + store_path.to_path_buf() + } else if is_remote_build && !out_path.exists() { + // Remote build with no local result and no store path captured + // (shouldn't happen, but fallback) target_profile.to_path_buf() } else { + // Local build - canonicalize the symlink to get the store path target_profile .canonicalize() .context("Failed to resolve output path to actual store path")? @@ -297,7 +306,7 @@ impl OsRebuildActivateArgs { activation_type, install_bootloader: false, show_logs: self.show_activation_logs, - sudo: elevate, + elevation: elevate.then_some(elevation.clone()), }, ) .wrap_err(format!( @@ -316,7 +325,16 @@ impl OsRebuildActivateArgs { .wrap_err("Activation (test) failed")?; } - debug!("Completed {variant:?} operation with output path: {out_path:?}"); + if let Some(store_path) = actual_store_path { + debug!( + "Completed {variant:?} operation with store path: {store_path:?}" + ); + } else { + debug!( + "Completed {variant:?} operation with local output path: \ + {out_path:?}" + ); + } } if let Boot | Switch = variant { @@ -332,7 +350,7 @@ impl OsRebuildActivateArgs { activation_type: remote::ActivationType::Boot, install_bootloader: self.rebuild.install_bootloader, show_logs: false, - sudo: elevate, + elevation: elevate.then_some(elevation), }, ) .wrap_err("Bootloader activation failed")?; @@ -362,7 +380,13 @@ impl OsRebuildActivateArgs { } } - debug!("Completed {variant:?} operation with output path: {out_path:?}"); + if let Some(store_path) = actual_store_path { + debug!("Completed {variant:?} operation with store path: {store_path:?}"); + } else { + debug!( + "Completed {variant:?} operation with local output path: {out_path:?}" + ); + } Ok(()) } } @@ -382,14 +406,16 @@ impl OsRebuildArgs { /// /// - `bool`: `true` if elevation is required, `false` otherwise. /// - `String`: The resolved target hostname. - fn setup_build_context(&self) -> Result<(bool, String)> { + fn setup_build_context( + &self, + elevation: &ElevationStrategy, + ) -> Result<(bool, String)> { + // Only check SSH key login if remote hosts are involved if self.build_host.is_some() || self.target_host.is_some() { - // This can fail, we only care about prompting the user - // for ssh key login beforehand. - let _ = ensure_ssh_key_login(); + ensure_ssh_key_login()?; } - let elevate = has_elevation_status(self.bypass_root_check)?; + let elevate = has_elevation_status(self.bypass_root_check, elevation)?; if self.update_args.update_all || self.update_args.update_input.is_some() { update( @@ -445,7 +471,7 @@ impl OsRebuildArgs { toplevel: Installable, out_path: &Path, message: &str, - ) -> Result<()> { + ) -> Result> { // If a build host is specified, use proper remote build semantics: // // 1. Evaluate derivation locally @@ -488,9 +514,10 @@ impl OsRebuildArgs { // Initialize SSH control - guard will cleanup connections on drop let _ssh_guard = remote::init_ssh_control(); - remote::build_remote(&toplevel, &config, Some(out_path))?; + let actual_store_path = + remote::build_remote(&toplevel, &config, Some(out_path))?; - Ok(()) + Ok(Some(actual_store_path)) } else { // Local build - use the existing path commands::Build::new(toplevel) @@ -501,7 +528,9 @@ impl OsRebuildArgs { .message(message) .nom(!self.common.no_nom) .run() - .wrap_err("Failed to build configuration") + .wrap_err("Failed to build configuration")?; + + Ok(None) // Local builds don't have separate store path } } @@ -585,11 +614,11 @@ impl OsRebuildArgs { self, variant: &OsRebuildVariant, final_attr: Option<&String>, - _elevation: ElevationStrategy, + elevation: &ElevationStrategy, ) -> Result<()> { use OsRebuildVariant::{Build, BuildVm}; - let (_, target_hostname) = self.setup_build_context()?; + let (_, target_hostname) = self.setup_build_context(elevation)?; let (out_path, _tempdir_guard) = self.determine_output_path(variant)?; @@ -617,7 +646,7 @@ impl OsRebuildArgs { impl OsRollbackArgs { #[expect(clippy::too_many_lines)] fn rollback(&self, elevation: ElevationStrategy) -> Result<()> { - let elevate = has_elevation_status(self.bypass_root_check)?; + let elevate = has_elevation_status(self.bypass_root_check, &elevation)?; // Find previous generation or specific generation let target_generation = if let Some(gen_number) = self.to { @@ -1038,7 +1067,15 @@ fn get_nh_os_flake_env() -> Result> { /// /// Returns an error if `bypass_root_check` is false and the user is root, /// as `nh os` subcommands should not be run directly as root. -fn has_elevation_status(bypass_root_check: bool) -> Result { +fn has_elevation_status( + bypass_root_check: bool, + elevation: &commands::ElevationStrategy, +) -> Result { + // If elevation strategy is None, never elevate + if matches!(elevation, commands::ElevationStrategy::None) { + return Ok(false); + } + if bypass_root_check { warn!("Bypassing root check, now running nix as root"); Ok(false) diff --git a/src/remote.rs b/src/remote.rs index 6c505a87..362fca70 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -15,10 +15,15 @@ use color_eyre::{ Result, eyre::{Context, bail, eyre}, }; +use secrecy::{ExposeSecret, SecretString}; use subprocess::{Exec, ExitStatus, Redirection}; use tracing::{debug, error, info, warn}; -use crate::{installable::Installable, util::NixVariant}; +use crate::{ + commands::{ElevationStrategy, cache_password, get_cached_password}, + installable::Installable, + util::NixVariant, +}; /// Global flag indicating whether a SIGINT (Ctrl+C) was received. static INTERRUPTED: OnceLock> = OnceLock::new(); @@ -34,6 +39,112 @@ fn get_interrupt_flag() -> &'static Arc { /// Cache for signal handler registration status. static HANDLER_REGISTERED: OnceLock<()> = OnceLock::new(); +/// Builds a remote command string with proper elevation handling. +/// +/// Constructs the command to execute on the remote host, wrapping it with +/// the appropriate elevation program (sudo/doas/etc) based on the strategy. +/// +/// # Arguments +/// * `strategy` - Optional elevation strategy to use +/// * `base_cmd` - The base command to execute +/// +/// # Returns +/// The complete command string to execute on the remote +/// +/// # Errors +/// Returns error if: +/// - Elevation program cannot be resolved +/// - Elevation program name cannot be determined +fn build_remote_command( + strategy: Option<&ElevationStrategy>, + base_cmd: &str, +) -> Result { + if let Some(strategy) = strategy { + if matches!(strategy, ElevationStrategy::None) { + return Ok(base_cmd.to_string()); + } + + let program = strategy.resolve()?; + let program_name = program + .file_name() + .and_then(|name| name.to_str()) + .ok_or_else(|| eyre!("Failed to determine elevation program name"))?; + + match (program_name, strategy) { + // sudo passwordless: use --non-interactive to fail if password required + ("sudo", ElevationStrategy::Passwordless) => { + Ok(format!( + "{} --non-interactive {}", + program.display(), + base_cmd + )) + }, + ("sudo", _) => { + Ok(format!( + "{} --prompt= --stdin {}", + program.display(), + base_cmd + )) + }, + // doas passwordless: use -n flag (non-interactive) + ("doas", ElevationStrategy::Passwordless) => { + Ok(format!("{} -n {}", program.display(), base_cmd)) + }, + ("doas", _) => { + bail!( + "doas does not support stdin password input for remote deployment. \ + Use --elevation-strategy=passwordless if remote has NOPASSWD \ + configured." + ) + }, + // run0 passwordless: use --no-ask-password flag + ("run0", ElevationStrategy::Passwordless) => { + Ok(format!( + "{} --no-ask-password {}", + program.display(), + base_cmd + )) + }, + ("run0", _) => { + bail!( + "run0 does not support stdin password input for remote deployment. \ + Use --elevation-strategy=passwordless if authentication is not \ + required." + ) + }, + // pkexec: no passwordless support + ("pkexec", _) => { + bail!( + "pkexec does not support non-interactive password input for remote \ + deployment. pkexec requires a polkit agent which is not available \ + over SSH." + ) + }, + // Unknown program: bail instead of guessing + (_, ElevationStrategy::Passwordless) => { + bail!( + "Unknown elevation program '{}' does not have known passwordless \ + support. Only sudo, doas, and run0 are supported with \ + --elevation-strategy=passwordless", + program_name + ) + }, + (..) => { + bail!( + "Unknown elevation program '{}' does not support stdin password \ + input for remote deployment. Only sudo supports password input \ + over SSH. Use --elevation-strategy=passwordless if remote has \ + passwordless elevation configured, or use a known elevation \ + program (sudo/doas/run0).", + program_name + ) + }, + } + } else { + Ok(base_cmd.to_string()) + } +} + /// Register a SIGINT handler that sets the global interrupt flag. /// /// This function is idempotent - multiple calls are safe and will not @@ -799,7 +910,8 @@ pub fn validate_closure_remote( the target host\n\nTo fix this:\n1. Verify your configuration enables \ all required components\n2. Ensure the complete closure was copied: \ nix copy --to ssh://{} {}\n3. Rebuild your configuration if the \ - problem persists", + problem persists\n4. Use --no-validate to bypass this check if you're \ + certain the system is correctly configured", host_context, closure_path.display(), missing_list, @@ -1014,8 +1126,12 @@ pub struct ActivateRemoteConfig { /// Whether to show output logs during activation pub show_logs: bool, - /// Whether to use sudo for activation commands - pub sudo: bool, + /// Elevation strategy for remote activation commands. + /// + /// - `None`: No elevation, run commands as the remote user + /// - `Some(strategy)`: Use the specified elevation strategy (sudo, doas, + /// etc.) + pub elevation: Option, } /// Activate a system configuration on a remote host. @@ -1067,6 +1183,39 @@ fn activate_nixos_remote( ) -> Result<()> { let ssh_opts = get_ssh_opts(); + // Prompt for password if elevation is needed + // Skip for None (no elevation) and Passwordless (remote has NOPASSWD + // configured) + let sudo_password = if let Some(ref strategy) = config.elevation { + if matches!( + strategy, + ElevationStrategy::None | ElevationStrategy::Passwordless + ) { + // None: no elevation program used + // Passwordless: elevation program used but no password needed + None + } else { + let host_str = host.ssh_host(); + if let Some(cached_password) = get_cached_password(&host_str)? { + Some(cached_password) + } else { + let password = + inquire::Password::new(&format!("[sudo] password for {host_str}:")) + .without_confirmation() + .prompt() + .context("Failed to read sudo password")?; + if password.is_empty() { + bail!("Password cannot be empty"); + } + let secret_password = SecretString::new(password.into()); + cache_password(&host_str, secret_password.clone())?; + Some(secret_password) + } + } + } else { + None + }; + let switch_to_config = system_profile.join("bin/switch-to-configuration"); let switch_path_str = switch_to_config.to_str().ok_or_else(|| { @@ -1081,17 +1230,23 @@ fn activate_nixos_remote( for opt in &ssh_opts { ssh_cmd = ssh_cmd.arg(opt); } + // Add -T flag to disable pseudo-terminal allocation (needed for stdin) + ssh_cmd = ssh_cmd.arg("-T"); ssh_cmd = ssh_cmd.arg(host.ssh_host()); - // Build the remote command with proper quoting - let remote_cmd = if config.sudo { - format!("sudo {} {}", shell_quote(switch_path_str), action) - } else { - format!("{} {}", shell_quote(switch_path_str), action) - }; + // Build the remote command using helper function + let base_cmd = format!("{} {}", shell_quote(switch_path_str), action); + let remote_cmd = + build_remote_command(config.elevation.as_ref(), &base_cmd)?; ssh_cmd = ssh_cmd.arg(remote_cmd); + // Pass password via stdin if elevation is needed + if let Some(ref password) = sudo_password { + ssh_cmd = + ssh_cmd.stdin(format!("{}\n", password.expose_secret()).as_str()); + } + if config.show_logs { ssh_cmd = ssh_cmd .stdout(Redirection::Merge) @@ -1119,25 +1274,27 @@ fn activate_nixos_remote( for opt in &ssh_opts { profile_ssh_cmd = profile_ssh_cmd.arg(opt); } + // Add -T flag to disable pseudo-terminal allocation (needed for stdin) + profile_ssh_cmd = profile_ssh_cmd.arg("-T"); profile_ssh_cmd = profile_ssh_cmd.arg(host.ssh_host()); - // Build the remote command with proper quoting - let profile_remote_cmd = if config.sudo { - format!( - "sudo nix build --no-link --profile {} {}", - NIXOS_SYSTEM_PROFILE, - system_profile.display() - ) - } else { - format!( - "nix build --no-link --profile {} {}", - NIXOS_SYSTEM_PROFILE, - system_profile.display() - ) - }; + // Build the remote command using helper function + let base_cmd = format!( + "nix build --no-link --profile {} {}", + NIXOS_SYSTEM_PROFILE, + shell_quote(&system_profile.to_string_lossy()) + ); + let profile_remote_cmd = + build_remote_command(config.elevation.as_ref(), &base_cmd)?; profile_ssh_cmd = profile_ssh_cmd.arg(profile_remote_cmd); + // Pass password via stdin if elevation is needed + if let Some(ref password) = sudo_password { + profile_ssh_cmd = profile_ssh_cmd + .stdin(format!("{}\n", password.expose_secret()).as_str()); + } + debug!(?profile_ssh_cmd, "Setting NixOS profile"); let profile_capture = profile_ssh_cmd @@ -1156,29 +1313,30 @@ fn activate_nixos_remote( for opt in &ssh_opts { boot_ssh_cmd = boot_ssh_cmd.arg(opt); } + // Add -T flag to disable pseudo-terminal allocation (needed for stdin) + boot_ssh_cmd = boot_ssh_cmd.arg("-T"); boot_ssh_cmd = boot_ssh_cmd.arg(host.ssh_host()); - // Build the remote command with proper quoting + // Build the remote command using helper function let boot_remote_cmd = if config.install_bootloader { - if config.sudo { - format!( - "sudo NIXOS_INSTALL_BOOTLOADER=1 {} boot", - shell_quote(switch_path_str) - ) - } else { - format!( - "NIXOS_INSTALL_BOOTLOADER=1 {} boot", - shell_quote(switch_path_str) - ) - } - } else if config.sudo { - format!("sudo {} boot", shell_quote(switch_path_str)) + let base_cmd = format!( + "NIXOS_INSTALL_BOOTLOADER=1 {} boot", + shell_quote(switch_path_str) + ); + build_remote_command(config.elevation.as_ref(), &base_cmd)? } else { - format!("{} boot", shell_quote(switch_path_str)) + let base_cmd = format!("{} boot", shell_quote(switch_path_str)); + build_remote_command(config.elevation.as_ref(), &base_cmd)? }; boot_ssh_cmd = boot_ssh_cmd.arg(boot_remote_cmd); + // Pass password via stdin if elevation is needed + if let Some(ref password) = sudo_password { + boot_ssh_cmd = boot_ssh_cmd + .stdin(format!("{}\n", password.expose_secret()).as_str()); + } + debug!(?boot_ssh_cmd, "Bootloader activation"); let boot_capture = boot_ssh_cmd From 1c44bc4835efd3595076a97cdbe58dafcdb4a0e1 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sun, 4 Jan 2026 23:49:09 +0300 Subject: [PATCH 33/38] various: add `ElevationStrategyArg` for type-safe CLI parsing Signed-off-by: NotAShelf Change-Id: I8837714ede1884ef22c9c0f0c10016746a6a6964 --- CHANGELOG.md | 6 +++-- src/commands.rs | 70 ++++++++++++++++++++++++++++++++++++++++++------ src/interface.rs | 5 ++-- src/main.rs | 46 ++++++++++++++++++++++++++----- 4 files changed, 108 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6b1befa..56815f34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ functionality, under the "Removed" section. ([#434](https://github.com/nix-community/nh/issues/434)) - Multi-program remote elevation support: `sudo`, `doas`, `run0`, and `pkexec` are now supported with correct flags for each program + - Environment variable `NH_ELEVATION_PROGRAM` is still supported for backward + compatibility (falls back to `NH_ELEVATION_STRATEGY` if set) - Platform commands (`nh os`, `nh home`, `nh darwin`) now support SSH-based remote builds via `--build-host`. The flag now uses proper remote build semantics: derivations are copied to the remote host via `nix-copy-closure`, @@ -94,8 +96,8 @@ functionality, under the "Removed" section. the installable such as (`./flake.nix#myHost`) in the past and lead to confusing behaviour for those unfamiliar. Such arguments are now normalized with a warning if NH can parse them. - - Password caching now works across all remote operations. - - Empty password validation prevents invalid credential caching. +- Password caching now works across all remote operations. +- Empty password validation prevents invalid credential caching. ### Removed diff --git a/src/commands.rs b/src/commands.rs index 674da5ec..9fb13473 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1,7 +1,9 @@ use std::{ collections::HashMap, + convert::Infallible, ffi::{OsStr, OsString}, path::PathBuf, + str::FromStr, sync::{Mutex, OnceLock}, }; @@ -31,6 +33,10 @@ static PASSWORD_CACHE: OnceLock>> = /// /// * `Some(SecretString)` - If a password for the host exists in the cache /// * `None` - If no password has been cached for this host +/// +/// # Errors +/// +/// Returns an error if the password cache lock is poisoned. pub fn get_cached_password(host: &str) -> Result> { let cache = PASSWORD_CACHE.get_or_init(|| Mutex::new(HashMap::new())); let guard = cache @@ -51,12 +57,18 @@ pub fn get_cached_password(host: &str) -> Result> { /// associate with the password /// * `password` - The password to cache, wrapped in a `SecretString` for secure /// handling +/// +/// # Errors +/// +/// Returns an error if the password cache lock is poisoned. pub fn cache_password(host: &str, password: SecretString) -> Result<()> { let cache = PASSWORD_CACHE.get_or_init(|| Mutex::new(HashMap::new())); - let mut guard = cache + + cache .lock() - .map_err(|_| eyre::eyre!("Password cache lock poisoned"))?; - guard.insert(host.to_string(), password); + .map_err(|_| eyre::eyre!("Password cache lock poisoned"))? + .insert(host.to_string(), password); + Ok(()) } @@ -94,7 +106,42 @@ pub enum EnvAction { Remove, } -/// Strategy for handling privilege elevation when running commands. +/// Strategy argument for handling privilege elevation when running commands. +/// +/// Defines how `nh` should handle privilege elevation for commands +/// that require root access (e.g., `switch-to-configuration`) +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ElevationStrategyArg { + /// No elevation - commands run without privilege escalation. + None, + + /// Automatically detect and use the first available elevation program + /// (tries doas -> sudo -> run0 -> pkexec in order). Uses askpass helper if + /// available. + Auto, + + /// Use elevation program but skip password prompting for remote hosts with + /// NOPASSWD configured. + Passwordless, + + /// Use the specified elevation program. + Program(PathBuf), +} + +impl FromStr for ElevationStrategyArg { + type Err = Infallible; + + fn from_str(s: &str) -> Result { + match s { + "none" => Ok(Self::None), + "auto" => Ok(Self::Auto), + "passwordless" => Ok(Self::Passwordless), + _ => Ok(Self::Program(PathBuf::from(s))), + } + } +} + +/// Strategy for handling privilege elevation at runtime. /// /// This enum defines how `nh` should handle privilege elevation for commands /// that require root access (e.g., `switch-to-configuration`). @@ -106,11 +153,11 @@ pub enum ElevationStrategy { Auto, /// Try the specified elevation program first, fall back to `Auto` if not - /// found. + /// found. Corresponds to CLI argument that is a path. Prefer(PathBuf), /// Use only the specified program name. - #[allow(dead_code)] + #[allow(dead_code, reason = "In use")] Force(&'static str), /// Do not use any elevation program. Commands run without privilege @@ -849,6 +896,12 @@ pub struct ExitError(ExitStatus); #[cfg(test)] mod tests { + #![allow( + clippy::expect_used, + clippy::unwrap_used, + clippy::unreachable, + reason = "Fine in tests" + )] use std::{env, ffi::OsString}; use serial_test::serial; @@ -867,7 +920,7 @@ mod tests { unsafe { env::set_var(key, value); } - EnvGuard { + Self { key: key.to_string(), original, } @@ -1424,7 +1477,7 @@ mod tests { #[test] fn test_parse_cmdline_realistic_sudo() { let cmdline = - r#"/usr/bin/sudo env 'PATH=/path with spaces' /usr/bin/nh clean all"#; + r"/usr/bin/sudo env 'PATH=/path with spaces' /usr/bin/nh clean all"; let result = shlex::split(cmdline).unwrap_or_default(); assert_eq!(result, vec![ "/usr/bin/sudo", @@ -1499,6 +1552,7 @@ mod tests { (EnvAction::Set(orig_val), EnvAction::Set(cloned_val)) => { assert_eq!(orig_val, cloned_val); }, + #[allow(clippy::unreachable, reason = "Should never happen")] _ => unreachable!("Clone should preserve variant and value"), } } diff --git a/src/interface.rs b/src/interface.rs index 0038bf84..8895df6f 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -64,8 +64,9 @@ pub struct Main { /// Can be a path to an elevation program (e.g., /usr/bin/sudo), /// or one of: 'none' (no elevation), /// 'passwordless' (use elevation without password prompt for remote hosts - /// with NOPASSWD configured) - pub elevation_strategy: Option, + /// with NOPASSWD configured), or 'auto' (automatically detect available + /// elevation programs in order: doas, sudo, run0, pkexec) + pub elevation_strategy: Option, #[command(subcommand)] pub command: NHCommand, diff --git a/src/main.rs b/src/main.rs index 48fea177..103d80bc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14,10 +14,12 @@ mod search; mod update; mod util; +use std::str::FromStr; + use color_eyre::Result; #[cfg(feature = "hotpath")] use hotpath; -use crate::commands::ElevationStrategy; +use crate::commands::{ElevationStrategy, ElevationStrategyArg}; pub const NH_VERSION: &str = env!("CARGO_PKG_VERSION"); pub const NH_REV: Option<&str> = option_env!("NH_REV"); @@ -26,7 +28,34 @@ fn main() -> Result<()> { #[cfg(feature = "hotpath")] let _guard = hotpath::GuardBuilder::new("main").build(); - let args = ::parse(); + let mut args = ::parse(); + + // Backward compatibility: support NH_ELEVATION_PROGRAM env var if + // NH_ELEVATION_STRATEGY is not set. + // TODO: Remove this fallback in a future version + if args.elevation_strategy.is_none() { + if let Some(old_value) = std::env::var("NH_ELEVATION_PROGRAM") + .ok() + .filter(|v| !v.is_empty()) + { + tracing::warn!( + "NH_ELEVATION_PROGRAM is deprecated, use NH_ELEVATION_STRATEGY \ + instead. Falling back to NH_ELEVATION_PROGRAM for backward \ + compatibility. Accepted values: none, passwordless, program:" + ); + match ElevationStrategyArg::from_str(&old_value) { + Ok(strategy) => args.elevation_strategy = Some(strategy), + Err(e) => { + tracing::warn!( + "Failed to parse NH_ELEVATION_PROGRAM value '{}': {}. Falling \ + back to none.", + old_value, + e + ); + }, + } + } + } // Set up logging crate::logging::setup_logging(args.verbosity)?; @@ -45,11 +74,14 @@ fn main() -> Result<()> { args .elevation_strategy .as_ref() - .map_or(ElevationStrategy::Auto, |path| { - match path.to_str() { - Some("none") => ElevationStrategy::None, - Some("passwordless") => ElevationStrategy::Passwordless, - _ => ElevationStrategy::Prefer(path.clone()), + .map_or(ElevationStrategy::Auto, |arg| { + match arg { + ElevationStrategyArg::Auto => ElevationStrategy::Auto, + ElevationStrategyArg::None => ElevationStrategy::None, + ElevationStrategyArg::Passwordless => ElevationStrategy::Passwordless, + ElevationStrategyArg::Program(path) => { + ElevationStrategy::Prefer(path.clone()) + }, } }); From 97d0d7ca009d73a7a5ad97d3e751218473caa28d Mon Sep 17 00:00:00 2001 From: faukah Date: Mon, 5 Jan 2026 09:53:43 +0100 Subject: [PATCH 34/38] nix/package: add sudo to nativeCheckInputs --- package.nix | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package.nix b/package.nix index fc25694d..232c39ae 100644 --- a/package.nix +++ b/package.nix @@ -5,6 +5,7 @@ makeBinaryWrapper, installShellFiles, versionCheckHook, + sudo, use-nom ? true, nix-output-monitor ? null, rev ? "dirty", @@ -38,6 +39,8 @@ rustPlatform.buildRustPackage (finalAttrs: { makeBinaryWrapper ]; + nativeCheckInputs = [ sudo ]; + cargoBuildFlags = [ "-p" "nh" From 9bc6d79bf3010f293f2029ba2ff280dd41ed2d66 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Tue, 6 Jan 2026 15:36:05 +0300 Subject: [PATCH 35/38] commands: handle "program:" prefix in \`ElevationStrategyArg\` parsing The program prefix was advertised as valid, but it actually wasn't because I regressed it during a different refactor. Strip the "program:" prefix when present to correctly parse elevation paths, and add a regression test so that I don't mess it up again. The deprecation warning advertised "program:" as a valid value but the parser treated it as a literal path. Strip the "program:" prefix when present to correctly parse elevation program paths. Add unit test for this case. Signed-off-by: NotAShelf Change-Id: I33de2df17654f95a656a4ee649cf7c9b6a6a6964 --- src/commands.rs | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 9fb13473..a34fe8ce 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -136,7 +136,13 @@ impl FromStr for ElevationStrategyArg { "none" => Ok(Self::None), "auto" => Ok(Self::Auto), "passwordless" => Ok(Self::Passwordless), - _ => Ok(Self::Program(PathBuf::from(s))), + _ => { + if let Some(rest) = s.strip_prefix("program:") { + Ok(Self::Program(PathBuf::from(rest))) + } else { + Ok(Self::Program(PathBuf::from(s))) + } + }, } } } @@ -1240,7 +1246,19 @@ mod tests { } #[test] - fn test_build_sudo_cmd_passwordless_no_stdin() { + fn test_elevation_strategy_arg_program_prefix_parsing() { + let parsed = "program:/path/to/bin".parse::(); + assert!(parsed.is_ok()); + match parsed.unwrap() { + ElevationStrategyArg::Program(path) => { + assert_eq!(path, PathBuf::from("/path/to/bin")); + }, + _ => unreachable!("Expected Program variant"), + } + } + + #[test] + fn test_build_sudo_cmd_force_no_stdin() { let cmd = Command::new("test").elevate(Some(ElevationStrategy::Force("sudo"))); @@ -1248,8 +1266,7 @@ mod tests { cmd.build_sudo_cmd().expect("build_sudo_cmd should succeed"); let cmdline = sudo_exec.to_cmdline_lossy(); - // Regular sudo should have --stdin and --prompt= flags - // (Note: Force("sudo") behaves like regular sudo, not Passwordless) + // Force("sudo") uses regular sudo without --stdin or --prompt flags assert!(cmdline.contains("sudo")); } From 67d12846a2f5c4ce791d4f5cd9e6961fb2689454 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Tue, 6 Jan 2026 16:06:19 +0300 Subject: [PATCH 36/38] nix: disable tests requiring sudo on Darwin Signed-off-by: NotAShelf Change-Id: Id2ae10eae0c6aa66034c5bf725c9447f6a6a6964 --- package.nix | 28 ++++++++++++++++++++++++++-- src/commands.rs | 3 ++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/package.nix b/package.nix index 232c39ae..7c7283c5 100644 --- a/package.nix +++ b/package.nix @@ -39,8 +39,6 @@ rustPlatform.buildRustPackage (finalAttrs: { makeBinaryWrapper ]; - nativeCheckInputs = [ sudo ]; - cargoBuildFlags = [ "-p" "nh" @@ -81,6 +79,32 @@ rustPlatform.buildRustPackage (finalAttrs: { versionCheckProgram = "${placeholder "out"}/bin/${finalAttrs.meta.mainProgram}"; versionCheckProgramArg = "--version"; + # pkgs.sudo is not available on the Darwin platform, and thus breaks build + # if added to nativeCheckInputs. We must manually disable the tests that + # *require* it, because they will fail when sudo is missing. + nativeCheckInputs = lib.optionals (!stdenv.hostPlatform.isDarwin) [ sudo ]; + checkFlags = lib.optionals stdenv.hostPlatform.isDarwin [ + # Tests that require sudo in PATH (not available on Darwin) + "--skip" + "test_build_sudo_cmd_basic" + "--skip" + "test_build_sudo_cmd_with_preserve_vars" + "--skip" + "test_build_sudo_cmd_with_preserve_vars_disabled" + "--skip" + "test_build_sudo_cmd_with_set_vars" + "--skip" + "test_build_sudo_cmd_force_no_stdin" + "--skip" + "test_build_sudo_cmd_with_remove_vars" + "--skip" + "test_build_sudo_cmd_with_askpass" + "--skip" + "test_build_sudo_cmd_env_added_once" + "--skip" + "test_elevation_strategy_passwordless_resolves" + ]; + # Besides the install check, we have a bunch of tests to run. Nextest is # the fastest way of running those since it's significantly faster than # `cargo test`, and has a nicer UI with CI-friendly characteristics. diff --git a/src/commands.rs b/src/commands.rs index a34fe8ce..9a49a39b 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -518,7 +518,8 @@ impl Command { EnvAction::Set(value) => Some(format!("{key}={value}")), EnvAction::Preserve if preserve_env => { std::env::var(key) - .map_or(None, |value| Some(format!("{key}={value}"))) + .ok() + .map(|value| format!("{key}={value}")) }, _ => None, } From 288b3e9ef35477b54c418ca725fe12e8c1eb11f7 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Tue, 6 Jan 2026 17:01:37 +0300 Subject: [PATCH 37/38] nixos: move essential files list into a constant Signed-off-by: NotAShelf Change-Id: If55c758a3ef8022ac2a9798be31a0ffa6a6a6964 --- src/nixos.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index 956a068f..d7e67553 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -33,6 +33,16 @@ const CURRENT_PROFILE: &str = "/run/current-system"; const SPEC_LOCATION: &str = "/etc/specialisation"; +/// Essential files that must exist in a valid NixOS system closure. Each tuple +/// contains the file path relative to the system profile and its description. +/// The descriptions are used on log messages or errors. +const ESSENTIAL_FILES: &[(&str, &str)] = &[ + ("bin/switch-to-configuration", "activation script"), + ("nixos-version", "system version identifier"), + ("init", "system init script"), + ("sw/bin", "system path"), +]; + impl interface::OsArgs { /// Executes the NixOS subcommand. /// @@ -942,15 +952,8 @@ fn run_vm(out_path: &Path) -> Result<()> { /// /// `Ok(())` if all files exist, or an error listing missing files. fn validate_system_closure(system_path: &Path) -> Result<()> { - let essential_files = [ - ("bin/switch-to-configuration", "activation script"), - ("nixos-version", "system version identifier"), - ("init", "system init script"), - ("sw/bin", "system path"), - ]; - let mut missing = Vec::new(); - for (file, description) in &essential_files { + for (file, description) in ESSENTIAL_FILES { let path = system_path.join(file); if !path.exists() { missing.push(format!(" - {file} ({description})")); @@ -983,14 +986,6 @@ fn validate_system_closure_remote( target_host: &str, build_host: Option<&str>, ) -> Result<()> { - let essential_files = [ - ("bin/switch-to-configuration", "activation script"), - ("nixos-version", "system version identifier"), - ("init", "system init script"), - ("sw/bin", "system path"), - ]; - - // Parse the target host let target = remote::RemoteHost::parse(target_host) .wrap_err("Invalid target host specification")?; @@ -1007,7 +1002,7 @@ fn validate_system_closure_remote( remote::validate_closure_remote( &target, system_path, - &essential_files, + ESSENTIAL_FILES, context.as_deref(), ) } From ed1e21a092d40d215307705b75365b5f98ad1e27 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Tue, 6 Jan 2026 17:47:23 +0300 Subject: [PATCH 38/38] nixos: move SSH guard from `execute_build` to `rebuild_and_activate` Signed-off-by: NotAShelf Change-Id: Ib42fe0268a9852055e283deeac4606f66a6a6964 --- src/nixos.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/nixos.rs b/src/nixos.rs index d7e67553..f638be21 100644 --- a/src/nixos.rs +++ b/src/nixos.rs @@ -164,6 +164,16 @@ impl OsRebuildActivateArgs { _ => "Building NixOS configuration", }; + // Initialize SSH control early if we have remote hosts - guard will keep + // connections alive for both build and activation + let _ssh_guard = if self.rebuild.build_host.is_some() + || self.rebuild.target_host.is_some() + { + Some(remote::init_ssh_control()) + } else { + None + }; + let actual_store_path = self.rebuild.execute_build(toplevel, &out_path, message)?; @@ -521,9 +531,6 @@ impl OsRebuildArgs { .collect(), }; - // Initialize SSH control - guard will cleanup connections on drop - let _ssh_guard = remote::init_ssh_control(); - let actual_store_path = remote::build_remote(&toplevel, &config, Some(out_path))?;