Merge "libstore/ssh: remove echo started check" into main

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.

lix/libstore/ssh.cc

@@ -80,10 +80,11 @@ std::unique_ptr<SSH::Connection> 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::Connection> 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);
 

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;
     };
   });
 

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