Skip to content

Commit d47a53d

Browse files
spikecurtisspikecurtis
authored
fix: handle paths with spaces in Match exec clause of SSH config (#18266)
fixes #18199 Corrects handling of paths with spaces in the `Match !exec` clause we use to determine whether Coder Connect is running. This is handled differently than the ProxyCommand, so we have a different escape routine, which also varies by OS. On Windows, we resort to a pretty gnarly hack, but it does work and I feel the only other option would be to reduce functionality such that we could not detect the Coder Connect state.
1 parent 709f374 commit d47a53d

File tree

4 files changed

+161
-14
lines changed

4 files changed

+161
-14
lines changed

‎cli/configssh.go‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -112,14 +112,19 @@ func (o sshConfigOptions) equal(other sshConfigOptions) bool {
112112
}
113113

114114
func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error {
115-
escapedCoderBinary, err := sshConfigExecEscape(o.coderBinaryPath, o.forceUnixSeparators)
115+
escapedCoderBinaryProxy, err := sshConfigProxyCommandEscape(o.coderBinaryPath, o.forceUnixSeparators)
116116
if err != nil {
117-
return xerrors.Errorf("escape coder binary for ssh failed: %w", err)
117+
return xerrors.Errorf("escape coder binary for ProxyCommand failed: %w", err)
118118
}
119119

120-
escapedGlobalConfig, err := sshConfigExecEscape(o.globalConfigPath, o.forceUnixSeparators)
120+
escapedCoderBinaryMatchExec, err := sshConfigMatchExecEscape(o.coderBinaryPath)
121121
if err != nil {
122-
return xerrors.Errorf("escape global config for ssh failed: %w", err)
122+
return xerrors.Errorf("escape coder binary for Match exec failed: %w", err)
123+
}
124+
125+
escapedGlobalConfig, err := sshConfigProxyCommandEscape(o.globalConfigPath, o.forceUnixSeparators)
126+
if err != nil {
127+
return xerrors.Errorf("escape global config for ProxyCommand failed: %w", err)
123128
}
124129

125130
rootFlags := fmt.Sprintf("--global-config %s", escapedGlobalConfig)
@@ -155,7 +160,7 @@ func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error {
155160
_, _ = buf.WriteString("\t")
156161
_, _ = fmt.Fprintf(buf,
157162
"ProxyCommand %s %s ssh --stdio%s --ssh-host-prefix %s %%h",
158-
escapedCoderBinary, rootFlags, flags, o.userHostPrefix,
163+
escapedCoderBinaryProxy, rootFlags, flags, o.userHostPrefix,
159164
)
160165
_, _ = buf.WriteString("\n")
161166
}
@@ -174,11 +179,11 @@ func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error {
174179
// the ^^ options should always apply, but we only want to use the proxy command if Coder Connect is not running.
175180
if !o.skipProxyCommand {
176181
_, _ = fmt.Fprintf(buf, "\nMatch host *.%s !exec \"%s connect exists %%h\"\n",
177-
o.hostnameSuffix, escapedCoderBinary)
182+
o.hostnameSuffix, escapedCoderBinaryMatchExec)
178183
_, _ = buf.WriteString("\t")
179184
_, _ = fmt.Fprintf(buf,
180185
"ProxyCommand %s %s ssh --stdio%s --hostname-suffix %s %%h",
181-
escapedCoderBinary, rootFlags, flags, o.hostnameSuffix,
186+
escapedCoderBinaryProxy, rootFlags, flags, o.hostnameSuffix,
182187
)
183188
_, _ = buf.WriteString("\n")
184189
}
@@ -759,7 +764,8 @@ func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after []
759764
return data, nil, nil, nil
760765
}
761766

762-
// sshConfigExecEscape quotes the string if it contains spaces, as per
767+
// sshConfigProxyCommandEscape prepares the path for use in ProxyCommand.
768+
// It quotes the string if it contains spaces, as per
763769
// `man 5 ssh_config`. However, OpenSSH uses exec in the users shell to
764770
// run the command, and as such the formatting/escape requirements
765771
// cannot simply be covered by `fmt.Sprintf("%q", path)`.
@@ -804,7 +810,7 @@ func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after []
804810
// This is a control flag, and that is ok. It is a control flag
805811
// based on the OS of the user. Making this a different file is excessive.
806812
// nolint:revive
807-
func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) {
813+
func sshConfigProxyCommandEscape(path string, forceUnixPath bool) (string, error) {
808814
if forceUnixPath {
809815
// This is a workaround for #7639, where the filepath separator is
810816
// incorrectly the Windows separator (\) instead of the unix separator (/).
@@ -814,9 +820,9 @@ func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) {
814820
// This is unlikely to ever happen, but newlines are allowed on
815821
// certain filesystems, but cannot be used inside ssh config.
816822
if strings.ContainsAny(path, "\n") {
817-
return "", xerrors.Errorf("invalid path: %s", path)
823+
return "", xerrors.Errorf("invalid path: %q", path)
818824
}
819-
// In the unlikely even that a path contains quotes, they must be
825+
// In the unlikely event that a path contains quotes, they must be
820826
// escaped so that they are not interpreted as shell quotes.
821827
if strings.Contains(path, "\"") {
822828
path = strings.ReplaceAll(path, "\"", "\\\"")

‎cli/configssh_internal_test.go‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func Test_sshConfigSplitOnCoderSection(t *testing.T) {
139139
// This test tries to mimic the behavior of OpenSSH
140140
// when executing e.g. a ProxyCommand.
141141
// nolint:tparallel
142-
func Test_sshConfigExecEscape(t *testing.T) {
142+
func Test_sshConfigProxyCommandEscape(t *testing.T) {
143143
t.Parallel()
144144

145145
tests := []struct {
@@ -171,7 +171,7 @@ func Test_sshConfigExecEscape(t *testing.T) {
171171
err = os.WriteFile(bin, contents, 0o755) //nolint:gosec
172172
require.NoError(t, err)
173173

174-
escaped, err := sshConfigExecEscape(bin, false)
174+
escaped, err := sshConfigProxyCommandEscape(bin, false)
175175
if tt.wantErr {
176176
require.Error(t, err)
177177
return
@@ -186,6 +186,63 @@ func Test_sshConfigExecEscape(t *testing.T) {
186186
}
187187
}
188188

189+
// This test tries to mimic the behavior of OpenSSH
190+
// when executing e.g. a match exec command.
191+
// nolint:tparallel
192+
func Test_sshConfigMatchExecEscape(t *testing.T) {
193+
t.Parallel()
194+
195+
tests := []struct {
196+
name string
197+
path string
198+
wantErrOther bool
199+
wantErrWindows bool
200+
}{
201+
{"no spaces", "simple", false, false},
202+
{"spaces", "path with spaces", false, false},
203+
{"quotes", "path with \"quotes\"", true, true},
204+
{"backslashes", "path with\\backslashes", false, false},
205+
{"tabs", "path with \ttabs", false, true},
206+
{"newline fails", "path with \nnewline", true, true},
207+
}
208+
// nolint:paralleltest // Fixes a flake
209+
for _, tt := range tests {
210+
tt := tt
211+
t.Run(tt.name, func(t *testing.T) {
212+
cmd := "/bin/sh"
213+
arg := "-c"
214+
contents := []byte("#!/bin/sh\necho yay\n")
215+
if runtime.GOOS == "windows" {
216+
cmd = "cmd.exe"
217+
arg = "/c"
218+
contents = []byte("@echo yay\n")
219+
}
220+
221+
dir := filepath.Join(t.TempDir(), tt.path)
222+
bin := filepath.Join(dir, "coder.bat") // Windows will treat it as batch, Linux doesn't care
223+
escaped, err := sshConfigMatchExecEscape(bin)
224+
if (runtime.GOOS == "windows" && tt.wantErrWindows) || (runtime.GOOS != "windows" && tt.wantErrOther) {
225+
require.Error(t, err)
226+
return
227+
}
228+
require.NoError(t, err)
229+
230+
err = os.MkdirAll(dir, 0o755)
231+
require.NoError(t, err)
232+
233+
err = os.WriteFile(bin, contents, 0o755) //nolint:gosec
234+
require.NoError(t, err)
235+
236+
// OpenSSH processes %% escape sequences into %
237+
escaped = strings.ReplaceAll(escaped, "%%", "%")
238+
b, err := exec.Command(cmd, arg, escaped).CombinedOutput() //nolint:gosec
239+
require.NoError(t, err)
240+
got := strings.TrimSpace(string(b))
241+
require.Equal(t, "yay", got)
242+
})
243+
}
244+
}
245+
189246
func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
190247
t.Parallel()
191248

@@ -236,7 +293,7 @@ func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
236293
tt := tt
237294
t.Run(tt.name, func(t *testing.T) {
238295
t.Parallel()
239-
found, err := sshConfigExecEscape(tt.path, tt.forceUnix)
296+
found, err := sshConfigProxyCommandEscape(tt.path, tt.forceUnix)
240297
if tt.wantErr {
241298
require.Error(t, err)
242299
return

‎cli/configssh_other.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,35 @@
22

33
package cli
44

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

‎cli/configssh_windows.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,58 @@
22

33
package cli
44

5+
import (
6+
"fmt"
7+
"strings"
8+
9+
"golang.org/x/xerrors"
10+
)
11+
512
// Must be a var for unit tests to conform behavior
613
var hideForceUnixSlashes = false
14+
15+
// sshConfigMatchExecEscape prepares the path for use in `Match exec` statement.
16+
//
17+
// OpenSSH parses the Match line with a very simple tokenizer that accepts "-enclosed strings for the exec command, and
18+
// has no supported escape sequences for ". This means we cannot include " within the command to execute.
19+
//
20+
// To make matters worse, on Windows, OpenSSH passes the string directly to cmd.exe for execution, and as far as I can
21+
// tell, the only supported way to call a path that has spaces in it is to surround it with ".
22+
//
23+
// So, we can't actually include " directly, but here is a horrible workaround:
24+
//
25+
// "for /f %%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a connect exists %h"
26+
//
27+
// The key insight here is to store the character " in a variable (%a in this case, but the % itself needs to be
28+
// escaped, so it becomes %%a), and then use that variable to construct the double-quoted path:
29+
//
30+
// %%aC:\Program Files\Coder\bin\coder.exe%%a.
31+
//
32+
// How do we generate a single " character without actually using that character? I couldn't find any command in cmd.exe
33+
// to do it, but powershell.exe can convert ASCII to characters like this: `[char]34` (where 34 is the code point for ").
34+
//
35+
// Other notes:
36+
// - @ in `@cmd.exe` suppresses echoing it, so you don't get this command printed
37+
// - we need another invocation of cmd.exe (e.g. `do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a`). Without
38+
// it the double-quote gets interpreted as part of the path, and you get: '"C:\Program' is not recognized.
39+
// Constructing the string and then passing it to another instance of cmd.exe does this trick here.
40+
// - OpenSSH passes the `Match exec` command to cmd.exe regardless of whether the user has a unix-like shell like
41+
// git bash, so we don't have a `forceUnixPath` option like for the ProxyCommand which does respect the user's
42+
// configured shell on Windows.
43+
func sshConfigMatchExecEscape(path string) (string, error) {
44+
// This is unlikely to ever happen, but newlines are allowed on
45+
// certain filesystems, but cannot be used inside ssh config.
46+
if strings.ContainsAny(path, "\n") {
47+
return "", xerrors.Errorf("invalid path: %s", path)
48+
}
49+
// Windows does not allow double-quotes or tabs in paths. If we get one it is an error.
50+
if strings.ContainsAny(path, "\"\t") {
51+
return "", xerrors.Errorf("path must not contain quotes or tabs: %q", path)
52+
}
53+
54+
if strings.ContainsAny(path, " ") {
55+
// c.f. function comment for how this works.
56+
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.
57+
}
58+
return path, nil
59+
}

0 commit comments

Comments
 (0)