From a03bb4455cee010bbfcf7e322b10ec7e35123032 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Wed, 3 Jul 2024 16:51:25 +0200
Subject: [PATCH] Fix SSH invocation when local SHELL misbehaves

Setting it to /bin/sh will make it more predictable when users have
their favorite shell in SHELL, which might not behave as expected.
For instance, a bad rc file could send something to stdout before
our LocalCommand gets to write "started".

This may help https://github.com/NixOS/nix/issues/11010
---
 src/libstore/ssh.cc           | 33 ++++++++++++++++++++++++++++++---
 tests/nixos/remote-builds.nix | 17 ++++++++++++++++-
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc
index e5d623adf..b8c5f4d97 100644
--- a/src/libstore/ssh.cc
+++ b/src/libstore/ssh.cc
@@ -3,6 +3,7 @@
 #include "current-process.hh"
 #include "environment-variables.hh"
 #include "util.hh"
+#include "exec.hh"
 
 namespace nix {
 
@@ -44,6 +45,10 @@ void SSHMaster::addCommonSSHOpts(Strings & args)
     if (compress)
         args.push_back("-C");
 
+    // We use this to make ssh signal back to us that the connection is established.
+    // It really does run locally; see createSSHEnv which sets up SHELL to make
+    // it launch more reliably. The local command runs synchronously, so presumably
+    // the remote session won't be garbled if the local command is slow.
     args.push_back("-oPermitLocalCommand=yes");
     args.push_back("-oLocalCommand=echo started");
 }
@@ -56,6 +61,27 @@ bool SSHMaster::isMasterRunning() {
     return res.first == 0;
 }
 
+Strings createSSHEnv()
+{
+    // Copy the environment and set SHELL=/bin/sh
+    std::map<std::string, std::string> env = getEnv();
+
+    // SSH will invoke the "user" shell for -oLocalCommand, but that means
+    // $SHELL. To keep things simple and avoid potential issues with other
+    // shells, we set it to /bin/sh.
+    // Technically, we don't need that, and we could reinvoke ourselves to print
+    // "started". Self-reinvocation is tricky with library consumers, but mostly
+    // solved; refer to the development history of nixExePath in libstore/globals.cc.
+    env.insert_or_assign("SHELL", "/bin/sh");
+
+    Strings r;
+    for (auto & [k, v] : env) {
+        r.push_back(k + "=" + v);
+    }
+
+    return r;
+}
+
 std::unique_ptr<SSHMaster::Connection> SSHMaster::startCommand(
     Strings && command, Strings && extraSshArgs)
 {
@@ -104,8 +130,8 @@ std::unique_ptr<SSHMaster::Connection> SSHMaster::startCommand(
         }
 
         args.splice(args.end(), std::move(command));
-
-        execvp(args.begin()->c_str(), stringsToCharPtrs(args).data());
+        auto env = createSSHEnv();
+        nix::execvpe(args.begin()->c_str(), stringsToCharPtrs(args).data(), stringsToCharPtrs(env).data());
 
         // could not exec ssh/bash
         throw SysError("unable to execute '%s'", args.front());
@@ -172,7 +198,8 @@ Path SSHMaster::startMaster()
         if (verbosity >= lvlChatty)
             args.push_back("-v");
         addCommonSSHOpts(args);
-        execvp(args.begin()->c_str(), stringsToCharPtrs(args).data());
+        auto env = createSSHEnv();
+        nix::execvpe(args.begin()->c_str(), stringsToCharPtrs(args).data(), stringsToCharPtrs(env).data());
 
         throw SysError("unable to execute '%s'", args.front());
     }, options);
diff --git a/tests/nixos/remote-builds.nix b/tests/nixos/remote-builds.nix
index 8ddf6ad02..ab159eaad 100644
--- a/tests/nixos/remote-builds.nix
+++ b/tests/nixos/remote-builds.nix
@@ -81,6 +81,17 @@ in
             virtualisation.additionalPaths = [ config.system.build.extraUtils ];
             nix.settings.substituters = lib.mkForce [ ];
             programs.ssh.extraConfig = "ConnectTimeout 30";
+            environment.systemPackages = [
+              # `bad-shell` is used to make sure Nix works an environment with a misbehaving shell.
+              #
+              # More realistically, a bad shell would still run the command ("echo started")
+              # but considering that our solution is to avoid this shell (set via $SHELL), we
+              # don't need to bother with a more functional mock shell.
+              (pkgs.writeScriptBin "bad-shell" ''
+                #!${pkgs.runtimeShell}
+                echo "Hello, I am a broken shell"
+              '')
+            ];
           };
       };
 
@@ -114,9 +125,13 @@ in
             'echo hello world on $(hostname)' >&2
         """)
 
+      # Check that SSH uses SHELL for LocalCommand, as expected, and check that
+      # our test setup here is working. The next test will use this bad SHELL.
+      client.succeed(f"SHELL=$(which bad-shell) ssh -oLocalCommand='true' -oPermitLocalCommand=yes {builder1.name} 'echo hello world' | grep -F 'Hello, I am a broken shell'")
+
       # Perform a build and check that it was performed on the builder.
       out = client.succeed(
-        "nix-build ${expr nodes.client 1} 2> build-output",
+        "SHELL=$(which bad-shell) nix-build ${expr nodes.client 1} 2> build-output",
         "grep -q Hello build-output"
       )
       builder1.succeed(f"test -e {out}")