Merge pull request #11021 from hercules-ci/issue-11010

Fix SSH invocation when local SHELL misbehaves

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));
-
+        auto env = createSSHEnv();
-        execvp(args.begin()->c_str(), stringsToCharPtrs(args).data());
+        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);

src/libutil/unix/exec.hh

@@ -0,0 +1,13 @@
+#pragma once
+
+namespace nix {
+
+/**
+ * `execvpe` is a GNU extension, so we need to implement it for other POSIX
+ * platforms.
+ *
+ * We use our own implementation unconditionally for consistency.
+ */
+int execvpe(const char * file0, char * const argv[], char * const envp[]);
+
+}

src/libutil/unix/meson.build

@@ -13,6 +13,7 @@ sources += files(
 include_dirs += include_directories('.')
 
 headers += files(
+  'exec.hh',
   'monitor-fd.hh',
   'signals-impl.hh',
 )

src/libutil/unix/processes.cc

@@ -1,5 +1,6 @@
 #include "current-process.hh"
 #include "environment-variables.hh"
+#include "executable-path.hh"
 #include "signals.hh"
 #include "processes.hh"
 #include "finally.hh"
@@ -419,4 +420,10 @@ bool statusOk(int status)
     return WIFEXITED(status) && WEXITSTATUS(status) == 0;
 }
 
+int execvpe(const char * file0, char * const argv[], char * const envp[])
+{
+    auto file = ExecutablePath::load().findPath(file0).string();
+    return execve(file.c_str(), argv, envp);
+}
+
 }

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