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

Commit1dc0485

Browse files
authored
fix: Use smarter quoting for ProxyCommand in config-ssh (#3755)
* fix: Use smarter quoting for ProxyCommand in config-sshThis change takes better into account how OpenSSH executes`ProxyCommand`s and applies quoting accordingly.This supercedes#3664, which was reverted.Fixes#2853* fix: Ensure `~/.ssh` directory exists
1 parent0708e37 commit1dc0485

File tree

2 files changed

+134
-6
lines changed

2 files changed

+134
-6
lines changed

‎cli/configssh.go

Lines changed: 72 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,20 @@ func configSSH() *cobra.Command {
170170
// that it's possible to capture the diff.
171171
out=cmd.OutOrStderr()
172172
}
173-
binaryFile,err:=currentBinPath(out)
173+
coderBinary,err:=currentBinPath(out)
174174
iferr!=nil {
175175
returnerr
176176
}
177+
escapedCoderBinary,err:=sshConfigExecEscape(coderBinary)
178+
iferr!=nil {
179+
returnxerrors.Errorf("escape coder binary for ssh failed: %w",err)
180+
}
181+
182+
root:=createConfig(cmd)
183+
escapedGlobalConfig,err:=sshConfigExecEscape(string(root))
184+
iferr!=nil {
185+
returnxerrors.Errorf("escape global config for ssh failed: %w",err)
186+
}
177187

178188
homedir,err:=os.UserHomeDir()
179189
iferr!=nil {
@@ -238,7 +248,6 @@ func configSSH() *cobra.Command {
238248
}
239249

240250
configModified:=configRaw
241-
root:=createConfig(cmd)
242251

243252
buf:=&bytes.Buffer{}
244253
before,after:=sshConfigSplitOnCoderSection(configModified)
@@ -280,11 +289,17 @@ func configSSH() *cobra.Command {
280289
"\tLogLevel ERROR",
281290
)
282291
if!skipProxyCommand {
283-
if!wireguard {
284-
configOptions=append(configOptions,fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --stdio %s",binaryFile,root,hostname))
285-
}else {
286-
configOptions=append(configOptions,fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --wireguard --stdio %s",binaryFile,root,hostname))
292+
wgArg:=""
293+
ifwireguard {
294+
wgArg="--wireguard "
287295
}
296+
configOptions=append(
297+
configOptions,
298+
fmt.Sprintf(
299+
"\tProxyCommand %s --global-config %s ssh %s--stdio %s",
300+
escapedCoderBinary,escapedGlobalConfig,wgArg,hostname,
301+
),
302+
)
288303
}
289304

290305
_,_=buf.WriteString(strings.Join(configOptions,"\n"))
@@ -451,6 +466,11 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) {
451466
dir:=filepath.Dir(path)
452467
name:=filepath.Base(path)
453468

469+
// Ensure that e.g. the ~/.ssh directory exists.
470+
iferr=os.MkdirAll(dir,0o700);err!=nil {
471+
returnxerrors.Errorf("create directory: %w",err)
472+
}
473+
454474
// Create a tempfile in the same directory for ensuring write
455475
// operation does not fail.
456476
f,err:=os.CreateTemp(dir,fmt.Sprintf(".%s.",name))
@@ -482,6 +502,52 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) {
482502
returnnil
483503
}
484504

505+
// sshConfigExecEscape quotes the string if it contains spaces, as per
506+
// `man 5 ssh_config`. However, OpenSSH uses exec in the users shell to
507+
// run the command, and as such the formatting/escape requirements
508+
// cannot simply be covered by `fmt.Sprintf("%q", path)`.
509+
//
510+
// Always escaping the path with `fmt.Sprintf("%q", path)` usually works
511+
// on most platforms, but double quotes sometimes break on Windows 10
512+
// (see #2853). This function takes a best-effort approach to improving
513+
// compatibility and covering edge cases.
514+
//
515+
// Given the following ProxyCommand:
516+
//
517+
//ProxyCommand "/path/with space/coder" ssh --stdio work
518+
//
519+
// This is ~what OpenSSH would execute:
520+
//
521+
///bin/bash -c '"/path/with space/to/coder" ssh --stdio workspace'
522+
//
523+
// However, since it's actually an arg in C, the contents inside the
524+
// single quotes are interpreted as is, e.g. if there was a '\t', it
525+
// would be the literal string '\t', not a tab.
526+
//
527+
// See:
528+
// - https://github.com/coder/coder/issues/2853
529+
// - https://github.com/openssh/openssh-portable/blob/V_9_0_P1/sshconnect.c#L158-L167
530+
// - https://github.com/PowerShell/openssh-portable/blob/v8.1.0.0/sshconnect.c#L231-L293
531+
// - https://github.com/PowerShell/openssh-portable/blob/v8.1.0.0/contrib/win32/win32compat/w32fd.c#L1075-L1100
532+
funcsshConfigExecEscape(pathstring) (string,error) {
533+
// This is unlikely to ever happen, but newlines are allowed on
534+
// certain filesystems, but cannot be used inside ssh config.
535+
ifstrings.ContainsAny(path,"\n") {
536+
return"",xerrors.Errorf("invalid path: %s",path)
537+
}
538+
// In the unlikely even that a path contains quotes, they must be
539+
// escaped so that they are not interpreted as shell quotes.
540+
ifstrings.Contains(path,"\"") {
541+
path=strings.ReplaceAll(path,"\"","\\\"")
542+
}
543+
// A space or a tab requires quoting, but tabs must not be escaped
544+
// (\t) since OpenSSH interprets it as a literal \t, not a tab.
545+
ifstrings.ContainsAny(path,"\t") {
546+
path=fmt.Sprintf("\"%s\"",path)//nolint:gocritic // We don't want %q here.
547+
}
548+
returnpath,nil
549+
}
550+
485551
// currentBinPath returns the path to the coder binary suitable for use in ssh
486552
// ProxyCommand.
487553
funccurrentBinPath(w io.Writer) (string,error) {

‎cli/configssh_internal_test.go

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
package cli
2+
3+
import (
4+
"os"
5+
"os/exec"
6+
"path/filepath"
7+
"runtime"
8+
"strings"
9+
"testing"
10+
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
// This test tries to mimic the behavior of OpenSSH
15+
// when executing e.g. a ProxyCommand.
16+
funcTest_sshConfigExecEscape(t*testing.T) {
17+
t.Parallel()
18+
19+
tests:= []struct {
20+
namestring
21+
pathstring
22+
wantErrbool
23+
windowsbool
24+
}{
25+
{"no spaces","simple",false,true},
26+
{"spaces","path with spaces",false,true},
27+
{"quotes","path with\"quotes\"",false,false},
28+
{"backslashes","path with\\backslashes",false,false},
29+
{"tabs","path with\ttabs",false,false},
30+
{"newline fails","path with\nnewline",true,false},
31+
}
32+
for_,tt:=rangetests {
33+
tt:=tt
34+
t.Run(tt.name,func(t*testing.T) {
35+
t.Parallel()
36+
37+
ifruntime.GOOS=="windows" {
38+
t.Skip("Windows doesn't typically execute via /bin/sh or cmd.exe, so this test is not applicable.")
39+
}
40+
41+
dir:=filepath.Join(t.TempDir(),tt.path)
42+
err:=os.MkdirAll(dir,0o755)
43+
require.NoError(t,err)
44+
bin:=filepath.Join(dir,"coder")
45+
contents:= []byte("#!/bin/sh\necho yay\n")
46+
err=os.WriteFile(bin,contents,0o755)//nolint:gosec
47+
require.NoError(t,err)
48+
49+
escaped,err:=sshConfigExecEscape(bin)
50+
iftt.wantErr {
51+
require.Error(t,err)
52+
return
53+
}
54+
require.NoError(t,err)
55+
56+
b,err:=exec.Command("/bin/sh","-c",escaped).CombinedOutput()//nolint:gosec
57+
require.NoError(t,err)
58+
got:=strings.TrimSpace(string(b))
59+
require.Equal(t,"yay",got)
60+
})
61+
}
62+
}

0 commit comments

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