Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commite7b6d37

Browse files
spikecurtisEdwardAngert
authored andcommitted
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 weuse to determine whether Coder Connect is running. This is handleddifferently than the ProxyCommand, so we have a different escaperoutine, which also varies by OS.On Windows, we resort to a pretty gnarly hack, but it does work and Ifeel the only other option would be to reduce functionality such that wecould not detect the Coder Connect state.
1 parentdf8ced5 commite7b6d37

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 (osshConfigOptions)writeToBuffer(buf*bytes.Buffer)error {
115-
escapedCoderBinary,err:=sshConfigExecEscape(o.coderBinaryPath,o.forceUnixSeparators)
115+
escapedCoderBinaryProxy,err:=sshConfigProxyCommandEscape(o.coderBinaryPath,o.forceUnixSeparators)
116116
iferr!=nil {
117-
returnxerrors.Errorf("escape coder binary forssh failed: %w",err)
117+
returnxerrors.Errorf("escape coder binary forProxyCommand failed: %w",err)
118118
}
119119

120-
escapedGlobalConfig,err:=sshConfigExecEscape(o.globalConfigPath,o.forceUnixSeparators)
120+
escapedCoderBinaryMatchExec,err:=sshConfigMatchExecEscape(o.coderBinaryPath)
121121
iferr!=nil {
122-
returnxerrors.Errorf("escape global config for ssh failed: %w",err)
122+
returnxerrors.Errorf("escape coder binary for Match exec failed: %w",err)
123+
}
124+
125+
escapedGlobalConfig,err:=sshConfigProxyCommandEscape(o.globalConfigPath,o.forceUnixSeparators)
126+
iferr!=nil {
127+
returnxerrors.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
returndata,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-
funcsshConfigExecEscape(pathstring,forceUnixPathbool) (string,error) {
813+
funcsshConfigProxyCommandEscape(pathstring,forceUnixPathbool) (string,error) {
808814
ifforceUnixPath {
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
ifstrings.ContainsAny(path,"\n") {
817-
return"",xerrors.Errorf("invalid path: %s",path)
823+
return"",xerrors.Errorf("invalid path: %q",path)
818824
}
819-
// In the unlikelyeven that a path contains quotes, they must be
825+
// In the unlikelyevent that a path contains quotes, they must be
820826
// escaped so that they are not interpreted as shell quotes.
821827
ifstrings.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-
funcTest_sshConfigExecEscape(t*testing.T) {
142+
funcTest_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
iftt.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+
funcTest_sshConfigMatchExecEscape(t*testing.T) {
193+
t.Parallel()
194+
195+
tests:= []struct {
196+
namestring
197+
pathstring
198+
wantErrOtherbool
199+
wantErrWindowsbool
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:=rangetests {
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+
ifruntime.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
funcTest_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
iftt.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
varhideForceUnixSlashes=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+
funcsshConfigMatchExecEscape(pathstring) (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+
ifstrings.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+
ifstrings.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+
returnpath,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
varhideForceUnixSlashes=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+
funcsshConfigMatchExecEscape(pathstring) (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+
ifstrings.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+
ifstrings.ContainsAny(path,"\"\t") {
51+
return"",xerrors.Errorf("path must not contain quotes or tabs: %q",path)
52+
}
53+
54+
ifstrings.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+
returnpath,nil
59+
}

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp