diff --git a/cli/configssh.go b/cli/configssh.go index cfea6b377f6ee..c1be60b604a9e 100644 --- a/cli/configssh.go +++ b/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, "\"", "\\\"") diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index d3eee395de0a3..acf534e7ae157 100644 --- a/cli/configssh_internal_test.go +++ b/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 diff --git a/cli/configssh_other.go b/cli/configssh_other.go index fde7cc0e47e63..07417487e8c8f 100644 --- a/cli/configssh_other.go +++ b/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 +} diff --git a/cli/configssh_windows.go b/cli/configssh_windows.go index 642a388fc873c..5df0d6b50c00e 100644 --- a/cli/configssh_windows.go +++ b/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 +}