From 33eaaf02fd3f380e99032b25e741eeeb10573cad Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Thu, 15 May 2025 13:25:48 +0200 Subject: [PATCH] libstore/ssh: remove `echo started` check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-flight `echo started` check over SSH was originally added in 577ebeaefb71020f0d6b79488602fd56ba2c1863. As it is usual with these old commits, understanding why is there a need for something is difficult. The closest thing would be > Fix a race starting the SSH master. We now wait synchronously for > the SSH master to finish starting. This prevents the SSH clients > from starting their own connections. But, we removed SSH connection sharing, so this does not apply anymore. Nonetheless, we believed this check was meant as a way to catch obvious misconfigurations or SSH failures early, before handing off to `nix-store`. However, this approach was not fruitful: it assumes the remote has a `bash`-compatible shell, `echo` behaves in a standard way, and no `ForceCommand` interferes—all of which are unreliable assumptions in practice. While the intent was to provide slightly better diagnostics (e.g. in case of SSH hanging or returning an interactive shell), in practice it does not meaningfully catch or improve real failure cases. The underlying protocol or engine can and should handle those errors more robustly anyway. In contrast, this check *does* break several legitimate workflows, including: * remote builders using `ForceCommand` wrappers (e.g. `nix-remote-build`-style setups), see , * SSHing into minimal environments lacking `bash` (e.g. initrd, busybox-based systems), * configurations that don’t default to POSIX-like shells, e.g., nushell enthusiasts. As such, we’re removing this code. Protocol mismatch errors and SSH failures can be rethought and handled more structurally elsewhere in the engine. Change-Id: I187f6881375d42ef83987a13a350c97964bbdb30 Signed-off-by: Raito Bezarius (cherry picked from commit 0dd8bf6c1c87a122d785d8efd95bec4d070c6c0e) --- .../rl-next/reliance-on-bash-for-ssh.md | 21 +++++++++++ lix/libstore/ssh.cc | 37 ++----------------- tests/nixos/default.nix | 24 +++--------- tests/nixos/remote-builds-ssh-ng.nix | 2 +- 4 files changed, 30 insertions(+), 54 deletions(-) create mode 100644 doc/manual/rl-next/reliance-on-bash-for-ssh.md diff --git a/doc/manual/rl-next/reliance-on-bash-for-ssh.md b/doc/manual/rl-next/reliance-on-bash-for-ssh.md new file mode 100644 index 000000000..fa88545eb --- /dev/null +++ b/doc/manual/rl-next/reliance-on-bash-for-ssh.md @@ -0,0 +1,21 @@ +--- +synopsis: Remove reliance on Bash for remote stores via SSH +issues: [fj#830, fj#805, fj#304] +cls: [3159] +category: "Fixes" +credits: [raito] +--- + +The pre-flight `echo started` handshake -- added years ago to catch race conditions -- has been removed. + +After removal of connection sharing in Lix 2.93, it required a Bash-compatible shell and a standard `echo`, so it failed on: + + * builders protected by `ForceCommand` wrappers (e.g. `nix-remote-build`), + * BusyBox / initrd images with no Bash, + * hosts using non-POSIX shells such as Nushell. + +The race the probe once addressed was tied to SSH connection-sharing -- since connection-sharing code has already been removed, the probe is now pointless. + +Real connection or protocol errors are now left to SSH/Nix to report directly. + +This is technically a breaking change if you had scripts that relied on the literal "started" which needs to be updated to rely on other signals, e.g., exit codes. diff --git a/lix/libstore/ssh.cc b/lix/libstore/ssh.cc index 46e719702..06ad66bc3 100644 --- a/lix/libstore/ssh.cc +++ b/lix/libstore/ssh.cc @@ -80,10 +80,11 @@ std::unique_ptr SSH::startCommand(const std::string & command) // reasonably POSIX-y semantics for the things we're about // to do next. if (fakeSSH) { - args = { "bash" }; + args = { "bash", "-c", command }; } else { - args = { "ssh", host.c_str(), "-x", "-T", "-oRemoteCommand=bash" }; + args = { "ssh", host.c_str(), "-x", "-T" }; addCommonSSHOpts(args); + args.push_back(command); } execvp(args.begin()->c_str(), stringsToCharPtrs(args).data()); @@ -96,38 +97,6 @@ std::unique_ptr SSH::startCommand(const std::string & command) in.readSide.reset(); out.writeSide.reset(); - // Once we hand off to nix-store (on the remote) and the caller (on the client), - // we lose the ability to catch SSH failing, due to Historical Architectural Decisions. - // - // We want to catch at least _some_ errors and alert the user in case of - // an obvious misconfiguration, so run a very simple command first - // to make sure things are at least somewhat operational. - // - // The exact semantics of - // - not having a shell prompt get in the way when non-interactive - // - echo doing the reasonable thing - // Are exactly why we specifically forced bash (via ssh RemoteCommand) earlier. - // We do *not* use /bin/sh because that may be busybox and busybox breaks here. - // - // FIXME: make any of this shit make sense - { - writeLine(in.writeSide.get(), "echo started"); - - std::string reply; - try { - reply = readLine(out.readSide.get()); - } catch (EndOfFile & e) { } - - if (reply != "started") { - warn("SSH to '%s' failed, stdout first line: '%s'", host, reply); - throw Error("failed to start SSH connection to '%s'", host); - } - } - - // Now that we're reasonably confident we have something vaguely resembling - // a connection, hand off to the command. - writeLine(in.writeSide.get(), fmt("exec %s", command)); - conn->out = std::move(out.readSide); conn->in = std::move(in.writeSide); diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 63fd575b6..5b03adc19 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -67,6 +67,8 @@ in }; }); + # Let's ensure that reasonably popular shells are tested for remote building. + remoteBuildsNushell = runNixOSTestFor "x86_64-linux" ({ lib, pkgs, ... }: { name = "remoteBuilds_nushell"; imports = [ ./remote-builds.nix ]; @@ -75,27 +77,11 @@ in }; }); - remoteBuildsWeirdShell = runNixOSTestFor "x86_64-linux" ({ lib, pkgs, ... }: { - name = "remoteBuilds_weird_shell"; + remoteBuildsBusybox = runNixOSTestFor "x86_64-linux" ({ lib, pkgs, ... }: { + name = "remoteBuilds_busybox"; imports = [ ./remote-builds.nix ]; builders.config = { lib, pkgs, ... }: { - # a pathologically weird shell that can do nothing BUT run bash - users.users.root.shell = pkgs.writeTextFile { - name = "watsh"; - destination = "/bin/watsh"; - executable = true; - - text = '' - #!/bin/sh - if [ "$1" = "-c" ] && [ "$2" = "bash" ]; then - exec bash - else - echo "Wat." - fi - ''; - - passthru.shellPath = "/bin/watsh"; - }; + users.users.root.shell = pkgs.busybox; }; }); diff --git a/tests/nixos/remote-builds-ssh-ng.nix b/tests/nixos/remote-builds-ssh-ng.nix index ec12f9066..669204bb0 100644 --- a/tests/nixos/remote-builds-ssh-ng.nix +++ b/tests/nixos/remote-builds-ssh-ng.nix @@ -98,7 +98,7 @@ in out = client.fail("nix-build ${expr nodes.client 1} 2>&1") assert "Host key verification failed." in out, f"No host verification error:\n{out}" - assert "warning: SSH to 'root@builder' failed, stdout first line: '''" in out, f"No details about which host:\n{out}" + assert "'ssh-ng://root@builder'" in out, f"No details about which host:\n{out}" client.succeed(f"ssh -o StrictHostKeyChecking=no {builder.name} 'echo hello world' >&2")