fix: handle paths with spaces in Match exec clause of SSH config (cherry-pick coder#18266) (coder#18778)

Co-authored-by: Spike Curtis <spike@coder.com>
fixes coder#18199

Author: gcp-cherry-pick-bot[bot]

cli/configssh.go

@@ -112,14 +112,19 @@ func (o sshConfigOptions) equal(other sshConfigOptions) bool {
 }
 
 func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error {
-	escapedCoderBinary, err := sshConfigExecEscape(o.coderBinaryPath, o.forceUnixSeparators)
+	escapedCoderBinaryProxy, err := sshConfigProxyCommandEscape(o.coderBinaryPath, o.forceUnixSeparators)
 	if err != nil {
-		return xerrors.Errorf("escape coder binary for ssh failed: %w", err)
+		return xerrors.Errorf("escape coder binary for ProxyCommand failed: %w", err)
 	}
 
-	escapedGlobalConfig, err := sshConfigExecEscape(o.globalConfigPath, o.forceUnixSeparators)
+	escapedCoderBinaryMatchExec, err := sshConfigMatchExecEscape(o.coderBinaryPath)
 	if err != nil {
-		return xerrors.Errorf("escape global config for ssh failed: %w", err)
+		return xerrors.Errorf("escape coder binary for Match exec failed: %w", err)
+	}
+
+	escapedGlobalConfig, err := sshConfigProxyCommandEscape(o.globalConfigPath, o.forceUnixSeparators)
+	if err != nil {
+		return xerrors.Errorf("escape global config for ProxyCommand failed: %w", err)
 	}
 
 	rootFlags := fmt.Sprintf("--global-config %s", escapedGlobalConfig)
@@ -155,7 +160,7 @@ func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error {
 			_, _ = buf.WriteString("\t")
 			_, _ = fmt.Fprintf(buf,
 				"ProxyCommand %s %s ssh --stdio%s --ssh-host-prefix %s %%h",
-				escapedCoderBinary, rootFlags, flags, o.userHostPrefix,
+				escapedCoderBinaryProxy, rootFlags, flags, o.userHostPrefix,
 			)
 			_, _ = buf.WriteString("\n")
 		}
@@ -174,11 +179,11 @@ func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error {
 	// the ^^ options should always apply, but we only want to use the proxy command if Coder Connect is not running.
 	if !o.skipProxyCommand {
 		_, _ = fmt.Fprintf(buf, "\nMatch host *.%s !exec \"%s connect exists %%h\"\n",
-			o.hostnameSuffix, escapedCoderBinary)
+			o.hostnameSuffix, escapedCoderBinaryMatchExec)
 		_, _ = buf.WriteString("\t")
 		_, _ = fmt.Fprintf(buf,
 			"ProxyCommand %s %s ssh --stdio%s --hostname-suffix %s %%h",
-			escapedCoderBinary, rootFlags, flags, o.hostnameSuffix,
+			escapedCoderBinaryProxy, rootFlags, flags, o.hostnameSuffix,
 		)
 		_, _ = buf.WriteString("\n")
 	}
@@ -759,7 +764,8 @@ func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after []
 	return data, nil, nil, nil
 }
 
-// sshConfigExecEscape quotes the string if it contains spaces, as per
+// sshConfigProxyCommandEscape prepares the path for use in ProxyCommand.
+// It quotes the string if it contains spaces, as per
 // `man 5 ssh_config`. However, OpenSSH uses exec in the users shell to
 // run the command, and as such the formatting/escape requirements
 // cannot simply be covered by `fmt.Sprintf("%q", path)`.
@@ -804,7 +810,7 @@ func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after []
 // This is a control flag, and that is ok. It is a control flag
 // based on the OS of the user. Making this a different file is excessive.
 // nolint:revive
-func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) {
+func sshConfigProxyCommandEscape(path string, forceUnixPath bool) (string, error) {
 	if forceUnixPath {
 		// This is a workaround for #7639, where the filepath separator is
 		// incorrectly the Windows separator (\) instead of the unix separator (/).
@@ -814,9 +820,9 @@ func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) {
 	// This is unlikely to ever happen, but newlines are allowed on
 	// certain filesystems, but cannot be used inside ssh config.
 	if strings.ContainsAny(path, "\n") {
-		return "", xerrors.Errorf("invalid path: %s", path)
+		return "", xerrors.Errorf("invalid path: %q", path)
 	}
-	// In the unlikely even that a path contains quotes, they must be
+	// In the unlikely event that a path contains quotes, they must be
 	// escaped so that they are not interpreted as shell quotes.
 	if strings.Contains(path, "\"") {
 		path = strings.ReplaceAll(path, "\"", "\\\"")

cli/configssh_internal_test.go

@@ -139,7 +139,7 @@ func Test_sshConfigSplitOnCoderSection(t *testing.T) {
 // This test tries to mimic the behavior of OpenSSH
 // when executing e.g. a ProxyCommand.
 // nolint:tparallel
-func Test_sshConfigExecEscape(t *testing.T) {
+func Test_sshConfigProxyCommandEscape(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -171,7 +171,7 @@ func Test_sshConfigExecEscape(t *testing.T) {
 			err = os.WriteFile(bin, contents, 0o755) //nolint:gosec
 			require.NoError(t, err)
 
-			escaped, err := sshConfigExecEscape(bin, false)
+			escaped, err := sshConfigProxyCommandEscape(bin, false)
 			if tt.wantErr {
 				require.Error(t, err)
 				return
@@ -186,6 +186,63 @@ func Test_sshConfigExecEscape(t *testing.T) {
 	}
 }
 
+// This test tries to mimic the behavior of OpenSSH
+// when executing e.g. a match exec command.
+// nolint:tparallel
+func Test_sshConfigMatchExecEscape(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name           string
+		path           string
+		wantErrOther   bool
+		wantErrWindows bool
+	}{
+		{"no spaces", "simple", false, false},
+		{"spaces", "path with spaces", false, false},
+		{"quotes", "path with \"quotes\"", true, true},
+		{"backslashes", "path with\\backslashes", false, false},
+		{"tabs", "path with \ttabs", false, true},
+		{"newline fails", "path with \nnewline", true, true},
+	}
+	// nolint:paralleltest // Fixes a flake
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			cmd := "/bin/sh"
+			arg := "-c"
+			contents := []byte("#!/bin/sh\necho yay\n")
+			if runtime.GOOS == "windows" {
+				cmd = "cmd.exe"
+				arg = "/c"
+				contents = []byte("@echo yay\n")
+			}
+
+			dir := filepath.Join(t.TempDir(), tt.path)
+			bin := filepath.Join(dir, "coder.bat") // Windows will treat it as batch, Linux doesn't care
+			escaped, err := sshConfigMatchExecEscape(bin)
+			if (runtime.GOOS == "windows" && tt.wantErrWindows) || (runtime.GOOS != "windows" && tt.wantErrOther) {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			err = os.MkdirAll(dir, 0o755)
+			require.NoError(t, err)
+
+			err = os.WriteFile(bin, contents, 0o755) //nolint:gosec
+			require.NoError(t, err)
+
+			// OpenSSH processes %% escape sequences into %
+			escaped = strings.ReplaceAll(escaped, "%%", "%")
+			b, err := exec.Command(cmd, arg, escaped).CombinedOutput() //nolint:gosec
+			require.NoError(t, err)
+			got := strings.TrimSpace(string(b))
+			require.Equal(t, "yay", got)
+		})
+	}
+}
+
 func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
 	t.Parallel()
 
@@ -236,7 +293,7 @@ func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			found, err := sshConfigExecEscape(tt.path, tt.forceUnix)
+			found, err := sshConfigProxyCommandEscape(tt.path, tt.forceUnix)
 			if tt.wantErr {
 				require.Error(t, err)
 				return

cli/configssh_other.go

@@ -2,4 +2,35 @@
 
 package cli
 
+import (
+	"strings"
+
+	"golang.org/x/xerrors"
+)
+
 var hideForceUnixSlashes = true
+
+// sshConfigMatchExecEscape prepares the path for use in `Match exec` statement.
+//
+// OpenSSH parses the Match line with a very simple tokenizer that accepts "-enclosed strings for the exec command, and
+// has no supported escape sequences for ". This means we cannot include " within the command to execute.
+func sshConfigMatchExecEscape(path string) (string, error) {
+	// This is unlikely to ever happen, but newlines are allowed on
+	// certain filesystems, but cannot be used inside ssh config.
+	if strings.ContainsAny(path, "\n") {
+		return "", xerrors.Errorf("invalid path: %s", path)
+	}
+	// Quotes are allowed in path names on unix-like file systems, but OpenSSH's parsing of `Match exec` doesn't allow
+	// them.
+	if strings.Contains(path, `"`) {
+		return "", xerrors.Errorf("path must not contain quotes: %q", path)
+	}
+
+	// OpenSSH passes the match exec string directly to the user's shell. sh, bash and zsh accept spaces, tabs and
+	// backslashes simply escaped by a `\`. It's hard to predict exactly what more exotic shells might do, but this
+	// should work for macOS and most Linux distros in their default configuration.
+	path = strings.ReplaceAll(path, `\`, `\\`) // must be first, since later replacements add backslashes.
+	path = strings.ReplaceAll(path, " ", "\\ ")
+	path = strings.ReplaceAll(path, "\t", "\\\t")
+	return path, nil
+}

cli/configssh_windows.go

@@ -2,5 +2,58 @@
 
 package cli
 
+import (
+	"fmt"
+	"strings"
+
+	"golang.org/x/xerrors"
+)
+
 // Must be a var for unit tests to conform behavior
 var hideForceUnixSlashes = false
+
+// sshConfigMatchExecEscape prepares the path for use in `Match exec` statement.
+//
+// OpenSSH parses the Match line with a very simple tokenizer that accepts "-enclosed strings for the exec command, and
+// has no supported escape sequences for ". This means we cannot include " within the command to execute.
+//
+// To make matters worse, on Windows, OpenSSH passes the string directly to cmd.exe for execution, and as far as I can
+// tell, the only supported way to call a path that has spaces in it is to surround it with ".
+//
+// So, we can't actually include " directly, but here is a horrible workaround:
+//
+// "for /f %%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a connect exists %h"
+//
+// The key insight here is to store the character " in a variable (%a in this case, but the % itself needs to be
+// escaped, so it becomes %%a), and then use that variable to construct the double-quoted path:
+//
+// %%aC:\Program Files\Coder\bin\coder.exe%%a.
+//
+// How do we generate a single " character without actually using that character? I couldn't find any command in cmd.exe
+// to do it, but powershell.exe can convert ASCII to characters like this: `[char]34` (where 34 is the code point for ").
+//
+// Other notes:
+//   - @ in `@cmd.exe` suppresses echoing it, so you don't get this command printed
+//   - we need another invocation of cmd.exe (e.g. `do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a`). Without
+//     it the double-quote gets interpreted as part of the path, and you get: '"C:\Program' is not recognized.
+//     Constructing the string and then passing it to another instance of cmd.exe does this trick here.
+//   - OpenSSH passes the `Match exec` command to cmd.exe regardless of whether the user has a unix-like shell like
+//     git bash, so we don't have a `forceUnixPath` option like for the ProxyCommand which does respect the user's
+//     configured shell on Windows.
+func sshConfigMatchExecEscape(path string) (string, error) {
+	// This is unlikely to ever happen, but newlines are allowed on
+	// certain filesystems, but cannot be used inside ssh config.
+	if strings.ContainsAny(path, "\n") {
+		return "", xerrors.Errorf("invalid path: %s", path)
+	}
+	// Windows does not allow double-quotes or tabs in paths. If we get one it is an error.
+	if strings.ContainsAny(path, "\"\t") {
+		return "", xerrors.Errorf("path must not contain quotes or tabs: %q", path)
+	}
+
+	if strings.ContainsAny(path, " ") {
+		// c.f. function comment for how this works.
+		path = fmt.Sprintf("for /f %%%%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%%%a%s%%%%a", path) //nolint:gocritic // We don't want %q here.
+	}
+	return path, nil
+}