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")