nixos: add `--target-host` and `--build-host` options #276

cramt · 2025-05-05T10:33:27Z

part of #254

not sure if the right move here is to invoke ln -s over ssh to make sure the out_path is symlinked correctly on the target machine, or if just canonicalize'ing the paths is the right move, i went for canonicalize just because it felt less hacky than invoking ln -s over ssh

it currently works for me on my own local server

NotAShelf · 2025-05-05T10:46:35Z

This is pretty neat. Viper and I have been discussing how we would handle remote deployments, but I guess simple was how we should've gone about it.

Not on my PC currently, so cannot provide a complete review. Until then, two things

Could.you update the CHANGELOG.md found in the repository root? You can add an ## Unreleased section and add the changes under.
Do you think the implementation could be a little more generic so that we can also-maybe-possibly consider supporting something like --build-host in the future? Not a dealbreaker, just spitballing.

Other than that I have no notes, I'll review in detail when I get back to my system.

cramt · 2025-05-05T10:49:50Z

I'll see what i can do about --build-host, should be simple enough cause its just adding a .ssh to the build command.

Another issue i did just discover is if your ssh key is password protected and you havnt dont ssh-add yet, it would be ideal if we cant prompt the user in the beginning to do ssh-add

viperML · 2025-05-05T10:52:34Z

The problem with this is that it requires one ssh connection per command, which can be a pain (related: serokell/deploy-rs#54)

I thought about using https://crates.io/crates/libssh-rs to maintain a single SSH connection, but I don't know how hard it would be to implement.

cramt · 2025-05-05T11:05:01Z

the refactor to maintain an ssh connection will probably be a pain, espeically with stuff like shelling out to nix copy, but for now i just wanted to mostly rely on the user having ssh-agent so they only need to type their password once, and then have that password type be in the beginning

cramt · 2025-05-05T11:09:33Z

oh i just realized im not handling the nvd diff correct, and that doing so is gonna be real annoying unless nvd just works over ssh

NotAShelf · 2025-05-05T11:18:19Z

I don't think it does, but @bloxx12 and I are currently working on an alternative diff implementation that I'm hoping to replace nvd with in nh as a smaller, faster implementation. Maybe we could also try taking a shot at diff over ssh?

viperML · 2025-05-05T11:23:24Z

We can disable nvd for SSH connection and merge this as is. And in the future replace nvd with an ssh-capable rewrite.

cramt · 2025-05-05T11:33:16Z

I don't think it does, but @bloxx12 and I are currently working on an alternative diff implementation that I'm hoping to replace nvd with in nh as a smaller, faster implementation. Maybe we could also try taking a shot at diff over ssh?

well diff over ssh is technically not required, if you had like an equivalent to nvd list that outputed something machine readible you can just do that on both machines, and then use the outputs to calculate the diff to display locally.

cramt · 2025-05-05T14:51:11Z

managed to get the ssh-add thingy working 🎉

NotAShelf

I like the spirit of those changes, but I really must defer to Viper here for a second opinion. We're messing with SSH, and I'd rather do everything correctly. So far I have a bunch of issues that are plaguing me. The liberal use of .unwrap() is one and I find it to be a fragile. Please consider using ? or .expect() with descriptive error messages. Speaking of, error messages would benefit from making a little more explicit. The edge cases should be rare, but not none. Lastly, and this is a little bit speculative, it’s worth validating all inputs that eventually reach shell commands. We do not want to be opening the door to weird command injections, no matter how unlikely.

Also, I'd appreciate some tests but it might be a little difficult add them without a good mock testing in place. #258 is supposed to address this... eventually.

NotAShelf · 2025-05-07T09:38:48Z

src/util.rs

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


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

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.

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.

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.

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

NotAShelf · 2025-05-07T09:47:53Z

src/util.rs

+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())
+}


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.

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

NotAShelf · 2025-05-07T09:49:32Z

src/commands.rs

+        Exec::cmd("ssh")
+            .arg("-T")
+            .arg(ssh)
+            .stdin(cmd.to_cmdline_lossy().as_str())


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.

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

NotAShelf · 2025-05-10T09:59:30Z

Sorry, there seems to be a small merge conflict. I couldn't fix it on Github's web UI very well. Would you mind fixing it? Looks like we're using both NH's own Command struct and Command from the standard library. Please use the NH's own where applicable.

cramt · 2025-05-10T10:21:23Z

np, thanks for trying, im currently visiting my in-laws, so cant promise it'll be today, so at the latest it'll be sunday

NotAShelf · 2025-05-10T10:22:57Z

No worries, I'll see what I can do if I can get to my system later. I'm also visiting family, so not sure if I'll have the time today.

cramt · 2025-05-10T10:43:28Z

managed to find some time with the laptop, to do the quick fix.

it would require quite a few additions to the internal NH command struct work for the ensure_ssh_key_login function as we need the exit code for the first command and we need to inherit stdio and stdout in the second command, so its probably simplest to just stick to the std lib command

NotAShelf · 2025-05-10T11:39:41Z

LGTM. I have a few notes, but I think I've nitted this PR enough-- I can cover those later on my own accord. Is this ready to merge?

cramt · 2025-05-10T11:43:51Z

for my sake its ready to merge

NotAShelf · 2025-05-10T11:44:14Z

Thank you for your efforts :)

khaneliman · 2025-05-11T03:54:33Z

Seems like this broke compare changes diff and introduced a warning on every build saying it's guessing host name and it's building for a VM.

NotAShelf · 2025-05-11T05:08:00Z

I'm doing a little cleanup in the platform commands right now. It will be pushed to a new branch later today, and should avoid the breakage as I think I caught the issue and fixed it anyway.

emenel · 2025-05-15T19:24:25Z

Seems like this broke compare changes diff and introduced a warning on every build saying it's guessing host name and it's building for a VM.

fwiw i'm still seeing this message using the latest version from the repo's flake.

NotAShelf · 2025-05-15T19:55:12Z

Please see if #281 resolves the issue for you. I'm currently preoccupied with personal projects and can't make too much progress on that PR, but it should already contain the fix for the error reported.

cramt added 2 commits May 5, 2025 12:30

add --target_host option

477b751

Merge branch 'master' into add_target_host_option

22f08fa

cramt added 3 commits May 5, 2025 15:22

build_host and changelog

34e2aa5

using the no-op on nvd broke things

20219ad

ensure that user has working ssh key in agent

e7eebb5

cramt changed the title ~~add --target_host option~~ add --target_host and --build_host options May 5, 2025

cramt changed the title ~~add --target_host and --build_host options~~ nixos: add --target_host and --build_host options May 5, 2025

NotAShelf reviewed May 7, 2025

View reviewed changes

cramt and others added 3 commits May 7, 2025 15:57

no need to fetch currentSystem and dont error if ssh-add isnt there

280e852

delete unused import

067380d

Merge branch 'master' into add_target_host_option

83574dd

alias std command as StdCommand to not cause namespace clashes

75e09d0

NotAShelf changed the title ~~nixos: add --target_host and --build_host options~~ nixos: add --target-host and --build-host options May 10, 2025

NotAShelf approved these changes May 10, 2025

View reviewed changes

NotAShelf merged commit c248d2c into nix-community:master May 10, 2025
2 checks passed

cramt deleted the add_target_host_option branch May 10, 2025 11:44

NotAShelf mentioned this pull request May 9, 2025

Feature parity with nixos-rebuild #254

Open

20 tasks

Uh oh!

nixos: add --target-host and --build-host options #276

nixos: add --target-host and --build-host options #276

Uh oh!

Conversation

cramt commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NotAShelf commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cramt commented May 5, 2025

Uh oh!

viperML commented May 5, 2025

Uh oh!

cramt commented May 5, 2025

Uh oh!

cramt commented May 5, 2025

Uh oh!

NotAShelf commented May 5, 2025

Uh oh!

viperML commented May 5, 2025

Uh oh!

cramt commented May 5, 2025

Uh oh!

cramt commented May 5, 2025

Uh oh!

NotAShelf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotAShelf May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramt May 7, 2025

Choose a reason for hiding this comment

Uh oh!

NotAShelf May 7, 2025

Choose a reason for hiding this comment

Uh oh!

cramt May 7, 2025

Choose a reason for hiding this comment

Uh oh!

NotAShelf May 7, 2025

Choose a reason for hiding this comment

Uh oh!

cramt May 7, 2025

Choose a reason for hiding this comment

Uh oh!

NotAShelf commented May 10, 2025

Uh oh!

cramt commented May 10, 2025

Uh oh!

NotAShelf commented May 10, 2025

Uh oh!

cramt commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NotAShelf commented May 10, 2025

Uh oh!

cramt commented May 10, 2025

Uh oh!

Uh oh!

NotAShelf commented May 10, 2025

Uh oh!

khaneliman commented May 11, 2025

Uh oh!

NotAShelf commented May 11, 2025

Uh oh!

emenel commented May 15, 2025

Uh oh!

NotAShelf commented May 15, 2025

Uh oh!

Uh oh!

nixos: add `--target-host` and `--build-host` options #276

nixos: add `--target-host` and `--build-host` options #276

cramt commented May 5, 2025 •

edited

Loading

NotAShelf commented May 5, 2025 •

edited

Loading

NotAShelf left a comment •

edited

Loading

NotAShelf May 7, 2025 •

edited

Loading

cramt commented May 10, 2025 •

edited

Loading