Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

# NH Changelog

## Unreleased

### Added

- Nh now supports the `--build-host` and `--target-host` cli arguments

## 4.0.3

### Added
Expand Down
50 changes: 41 additions & 9 deletions src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,18 @@ use subprocess::{Exec, ExitStatus, Redirection};
use thiserror::Error;
use tracing::{debug, info};

use crate::installable::Installable;
use crate::{installable::Installable, util::get_current_system};

fn ssh_wrap(cmd: Exec, ssh: Option<&str>) -> Exec {
if let Some(ssh) = ssh {
Exec::cmd("ssh")
.arg("-T")
.arg(ssh)
.stdin(cmd.to_cmdline_lossy().as_str())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that cmd.to_cmdline_lossy() doesn't pass unescaped inputs to the SSH command? I can't exactly imagine an "attack" vector here, but this seems a little dangerous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did check that i did do escaping when implementing it, not as much for security as in the case of it would break on weird input like a ' or smth, i cant vouch for their implementation of escaping being correct, but they atleast do something

} else {
cmd
}
}

#[derive(Debug)]
pub struct Command {
Expand All @@ -17,6 +28,7 @@ pub struct Command {
command: OsString,
args: Vec<OsString>,
elevate: bool,
ssh: Option<String>,
}

impl Command {
Expand All @@ -27,6 +39,7 @@ impl Command {
command: command.as_ref().to_os_string(),
args: vec![],
elevate: false,
ssh: None,
}
}

Expand All @@ -40,6 +53,11 @@ impl Command {
self
}

pub fn ssh(mut self, ssh: Option<String>) -> Self {
self.ssh = ssh;
self
}

pub fn arg<S: AsRef<OsStr>>(mut self, arg: S) -> Self {
self.args.push(arg.as_ref().to_os_string());
self
Expand Down Expand Up @@ -94,9 +112,9 @@ impl Command {
cmd.arg(&self.command).args(&self.args)
} else {
Exec::cmd(&self.command).args(&self.args)
}
.stderr(Redirection::None)
.stdout(Redirection::None);
};
let cmd =
ssh_wrap(cmd.stderr(Redirection::None), self.ssh.as_deref()).stdout(Redirection::None);

if let Some(m) = &self.message {
info!("{}", m);
Expand All @@ -106,9 +124,9 @@ impl Command {

if !self.dry {
if let Some(m) = &self.message {
cmd.join().wrap_err(m.clone())?;
cmd.capture().wrap_err(m.clone())?;
} else {
cmd.join()?;
cmd.capture()?;
}
}

Expand Down Expand Up @@ -141,6 +159,7 @@ pub struct Build {
installable: Installable,
extra_args: Vec<OsString>,
nom: bool,
builder: Option<String>,
}

impl Build {
Expand All @@ -150,6 +169,7 @@ impl Build {
installable,
extra_args: vec![],
nom: false,
builder: None,
}
}

Expand All @@ -168,6 +188,11 @@ impl Build {
self
}

pub fn builder(mut self, builder: Option<String>) -> Self {
self.builder = builder;
self
}

pub fn extra_args<I>(mut self, args: I) -> Self
where
I: IntoIterator,
Expand All @@ -192,9 +217,16 @@ impl Build {
.arg("build")
.args(&installable_args)
.args(&["--log-format", "internal-json", "--verbose"])
.args(&match &self.builder {
Some(host) => vec![
"--builders".to_string(),
format!("ssh://{host} {} - - 100", get_current_system().unwrap()),
],
None => vec![],
})
.args(&self.extra_args)
.stdout(Redirection::Pipe)
.stderr(Redirection::Merge)
.stdout(Redirection::Pipe)
| Exec::cmd("nom").args(&["--json"])
}
.stdout(Redirection::None);
Expand All @@ -205,8 +237,8 @@ impl Build {
.arg("build")
.args(&installable_args)
.args(&self.extra_args)
.stdout(Redirection::None)
.stderr(Redirection::Merge);
.stderr(Redirection::Merge)
.stdout(Redirection::None);

debug!(?cmd);
cmd.join()
Expand Down
8 changes: 8 additions & 0 deletions src/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ pub struct OsRebuildArgs {
/// Don't panic if calling nh as root
#[arg(short = 'R', long, env = "NH_BYPASS_ROOT_CHECK")]
pub bypass_root_check: bool,

/// Deploy the configuration to a different host over ssh
#[arg(long)]
pub target_host: Option<String>,

/// Build the configuration to a different host over ssh
#[arg(long)]
pub build_host: Option<String>,
}

#[derive(Debug, Args)]
Expand Down
42 changes: 35 additions & 7 deletions src/nixos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::installable::Installable;
use crate::interface::OsSubcommand::{self};
use crate::interface::{self, OsGenerationsArgs, OsRebuildArgs, OsReplArgs};
use crate::update::update;
use crate::util::ensure_ssh_key_login;
use crate::util::get_hostname;

const SYSTEM_PROFILE: &str = "/nix/var/nix/profiles/system";
Expand Down Expand Up @@ -51,6 +52,10 @@ impl OsRebuildArgs {
fn rebuild(self, variant: OsRebuildVariant) -> Result<()> {
use OsRebuildVariant::*;

if self.build_host.is_some() || self.target_host.is_some() {
ensure_ssh_key_login().unwrap();
}

let elevate = if self.bypass_root_check {
warn!("Bypassing root check, now running nix as root");
false
Expand Down Expand Up @@ -102,6 +107,7 @@ impl OsRebuildArgs {
.extra_arg("--out-link")
.extra_arg(out_path.get_path())
.extra_args(&self.extra_args)
.builder(self.build_host.clone())
.message("Building NixOS configuration")
.nom(!self.common.no_nom)
.run()?;
Expand All @@ -121,14 +127,18 @@ impl OsRebuildArgs {
Some(spec) => out_path.get_path().join("specialisation").join(spec),
};

debug!("exists: {}", target_profile.exists());

target_profile.try_exists().context("Doesn't exist")?;

Command::new("nvd")
.arg("diff")
.arg(CURRENT_PROFILE)
.arg(&target_profile)
.message("Comparing changes")
.run()?;
if self.build_host.is_none() && self.target_host.is_none() {
Command::new("nvd")
.arg("diff")
.arg(CURRENT_PROFILE)
.arg(&target_profile)
.message("Comparing changes")
.run()?;
}

if self.common.dry || matches!(variant, Build) {
if self.common.ask {
Expand All @@ -146,14 +156,28 @@ impl OsRebuildArgs {
}
}

if let Some(target_host) = &self.target_host {
Command::new("nix")
.args([
"copy",
"--to",
format!("ssh://{}", target_host).as_str(),
target_profile.to_str().unwrap(),
])
.message("Copying configuration to target")
.run()?;
};

if let Test | Switch = variant {
// !! Use the target profile aka spec-namespaced
let switch_to_configuration =
target_profile.join("bin").join("switch-to-configuration");
let switch_to_configuration = switch_to_configuration.canonicalize().unwrap();
let switch_to_configuration = switch_to_configuration.to_str().unwrap();

Command::new(switch_to_configuration)
.arg("test")
.ssh(self.target_host.clone())
.message("Activating configuration")
.elevate(elevate)
.run()?;
Expand All @@ -163,17 +187,21 @@ impl OsRebuildArgs {
Command::new("nix")
.elevate(elevate)
.args(["build", "--no-link", "--profile", SYSTEM_PROFILE])
.arg(out_path.get_path())
.arg(out_path.get_path().canonicalize().unwrap())
.ssh(self.target_host.clone())
.run()?;

// !! Use the base profile aka no spec-namespace
let switch_to_configuration = out_path
.get_path()
.join("bin")
.join("switch-to-configuration");
let switch_to_configuration = switch_to_configuration.canonicalize().unwrap();
let switch_to_configuration = switch_to_configuration.to_str().unwrap();

Command::new(switch_to_configuration)
.arg("boot")
.ssh(self.target_host.clone())
.elevate(elevate)
.message("Adding configuration to bootloader")
.run()?;
Expand Down
43 changes: 42 additions & 1 deletion src/util.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
extern crate semver;

use std::path::{Path, PathBuf};
use std::process::Command;
use std::process::{Command, Stdio};
use std::str;

use color_eyre::{eyre, Result};
Expand Down Expand Up @@ -59,6 +59,47 @@ pub fn get_nix_version() -> Result<String> {
Err(eyre::eyre!("Failed to extract version"))
}

/// Retrieves the current system we're running on in the format nix expects
///
/// This functions just runs `nix eval --impure --raw --expr 'builtins.currentSystem'` and gets the
/// output
///
/// * `Result<String>` - The current system string or an error if the version cannot be retrieved.
pub fn get_current_system() -> Result<String> {
let output = Command::new("nix")
.args([
"eval",
"--impure",
"--raw",
"--expr",
"builtins.currentSystem",
])
.output()?;
let output_str = str::from_utf8(&output.stdout)?;
Ok(output_str.to_string())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I'm really not sure about this. Surely there is a way we can get the current system purely without having to evaluate currentSystem. Maybe we can evaluate nixpkgs.hostPlatform or some modular attribute inside the evaluated configuration? CC @viperML on this for installable context related woes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually this shouldnt be needed, i can just use - to get the builders argument to use the default


/// Prompts the user for ssh key login if needed
pub fn ensure_ssh_key_login() -> Result<()> {
Copy link
Member

@NotAShelf NotAShelf May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to be too nitpicky, but there are three things I would like to note here:

  1. Execution of this function appears a little... fragile to me. It assumes ssh-add is available, and will degrade user experience if it's missing. Maybe we could check for ssh-add early and explicitly.
  2. The .unwrap() calls (e.g., Command::new("ssh-add").spawn()?.wait()?) could cause panics if errors occur. I'm not religiously opposed to unwraps and similar error handling, but for an operation as sensitive as SSH we would like to bevery explicit.
  3. This is a bit of a stretch and mostly optional. Stdio::inherit makes sense here for user interaction but could lead to unexpected behavior if nh is run in a non-interactive environment. This will likely not be the case since we don't support headless usage (not explicitly, at least) but implicit behaviour might be confusing. Take this comment with a grain of salt, I leave it to your best judgement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not nipicky at all, youre right, i should make sure to not unwrap at the call site so that we arent ever dependant on ssh-add

// ssh-add -L checks if there are any currently usable ssh keys

if Command::new("ssh-add")
.arg("-L")
.stdout(Stdio::null())
.status()?
.success()
{
return Ok(());
}
Command::new("ssh-add")
.stdin(Stdio::inherit())
.stdout(Stdio::inherit())
.stderr(Stdio::inherit())
.spawn()?
.wait()?;
Ok(())
}

pub trait MaybeTempPath: std::fmt::Debug {
fn get_path(&self) -> &Path;
}
Expand Down