From 227e4d349f212b82a4ed594653e0ff7f7b28a327 Mon Sep 17 00:00:00 2001
From: K900 <me@0upti.me>
Date: Tue, 22 Apr 2025 09:40:21 +0300
Subject: [PATCH] libstore/ssh: fix the SSH connectivity check with non-POSIXy
 shells

SSH runs the command provided to it in the user's login shell.
That's a feature and is impossible to bypass (trust me, we tried).

The previous implementation of `echo started; nix-store --stdio`
broke with shells that treat `;` differently, e.g. nushell,
which eats the output of everything but the last command in a chain.

In the more general case, this means that a sufficiently weird shell
can do _anything_ it wants to the command we pass via SSH, so
we're forced to rely on as little functionality as possible.

The subset we're hereby settling on is just "running `bash` runs bash".
We then run bash, in non-interactive mode, which gives us a somewhat
consistent environment do to things in.

This whole thing is extremely cursed, but fixing it _correctly_
requires pretty much entirely rewriting how remote stores work,
and I'm not doing that right now.

Fixes #805.

Change-Id: Icac846e8cd821cbca91860ddaa0f657b4317dbf8
Co-authored-by: eldritch horrors <pennae@lix.systems>
---
 lix/libstore/ssh.cc     | 30 +++++++++++++++++++++++++-----
 tests/nixos/default.nix | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/lix/libstore/ssh.cc b/lix/libstore/ssh.cc
index 80c11aea7..46e719702 100644
--- a/lix/libstore/ssh.cc
+++ b/lix/libstore/ssh.cc
@@ -76,14 +76,16 @@ std::unique_ptr<SSH::Connection> SSH::startCommand(const std::string & command)
 
         Strings args;
 
+        // We specifically spawn bash here, to (hopefully) get
+        // reasonably POSIX-y semantics for the things we're about
+        // to do next.
         if (fakeSSH) {
-            args = { "bash", "-c" };
+            args = { "bash" };
         } else {
-            args = { "ssh", host.c_str(), "-x" };
+            args = { "ssh", host.c_str(), "-x", "-T", "-oRemoteCommand=bash" };
             addCommonSSHOpts(args);
         }
 
-        args.push_back(fmt("echo started; %s", command));
         execvp(args.begin()->c_str(), stringsToCharPtrs(args).data());
 
         // could not exec ssh/bash
@@ -94,9 +96,23 @@ std::unique_ptr<SSH::Connection> SSH::startCommand(const std::string & command)
     in.readSide.reset();
     out.writeSide.reset();
 
-    // Wait for the SSH connection to be established,
-    // So that we don't overwrite the password prompt with our progress bar.
+    // 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());
@@ -108,6 +124,10 @@ std::unique_ptr<SSH::Connection> SSH::startCommand(const std::string & command)
         }
     }
 
+    // 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 2463ae5cc..92ec74ca4 100644
--- a/tests/nixos/default.nix
+++ b/tests/nixos/default.nix
@@ -64,6 +64,38 @@ in
     };
   });
 
+  remoteBuildsNushell = runNixOSTestFor "x86_64-linux" ({ lib, pkgs, ... }: {
+    name = "remoteBuilds_nushell";
+    imports = [ ./remote-builds.nix ];
+    builders.config = { lib, pkgs, ... }: {
+      users.users.root.shell = pkgs.nushell;
+    };
+  });
+
+  remoteBuildsWeirdShell = runNixOSTestFor "x86_64-linux" ({ lib, pkgs, ... }: {
+    name = "remoteBuilds_weird_shell";
+    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";
+      };
+    };
+  });
+
   # Test our Nix as a builder for clients that are older
 
   remoteBuilds_local_2_3 = runNixOSTestFor "x86_64-linux" ({ lib, pkgs, ... }: {