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

fix: Use smarter quoting for ProxyCommand in config-ssh#3755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mafredri merged 9 commits intomainfrommafredri/fix-coder-configssh-proxycmd-quoting
Aug 30, 2022
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 72 additions & 6 deletionscli/configssh.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -170,10 +170,20 @@ func configSSH() *cobra.Command {
// that it's possible to capture the diff.
out = cmd.OutOrStderr()
}
binaryFile, err := currentBinPath(out)
coderBinary, err := currentBinPath(out)
if err != nil {
return err
}
escapedCoderBinary, err := sshConfigExecEscape(coderBinary)
if err != nil {
return xerrors.Errorf("escape coder binary for ssh failed: %w", err)
}

root := createConfig(cmd)
escapedGlobalConfig, err := sshConfigExecEscape(string(root))
if err != nil {
return xerrors.Errorf("escape global config for ssh failed: %w", err)
}

homedir, err := os.UserHomeDir()
if err != nil {
Expand DownExpand Up@@ -238,7 +248,6 @@ func configSSH() *cobra.Command {
}

configModified := configRaw
root := createConfig(cmd)

buf := &bytes.Buffer{}
before, after := sshConfigSplitOnCoderSection(configModified)
Expand DownExpand Up@@ -280,11 +289,17 @@ func configSSH() *cobra.Command {
"\tLogLevel ERROR",
)
if !skipProxyCommand {
if !wireguard {
configOptions = append(configOptions, fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --stdio %s", binaryFile, root, hostname))
} else {
configOptions = append(configOptions, fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --wireguard --stdio %s", binaryFile, root, hostname))
wgArg := ""
if wireguard {
wgArg = "--wireguard "
}
configOptions = append(
configOptions,
fmt.Sprintf(
"\tProxyCommand %s --global-config %s ssh %s--stdio %s",
escapedCoderBinary, escapedGlobalConfig, wgArg, hostname,
),
)
}

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

// Ensure that e.g. the ~/.ssh directory exists.
if err = os.MkdirAll(dir, 0o700); err != nil {
return xerrors.Errorf("create directory: %w", err)
}

// Create a tempfile in the same directory for ensuring write
// operation does not fail.
f, err := os.CreateTemp(dir, fmt.Sprintf(".%s.", name))
Expand DownExpand Up@@ -482,6 +502,52 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) {
return nil
}

// sshConfigExecEscape 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)`.
//
// Always escaping the path with `fmt.Sprintf("%q", path)` usually works
// on most platforms, but double quotes sometimes break on Windows 10
// (see #2853). This function takes a best-effort approach to improving
// compatibility and covering edge cases.
//
// Given the following ProxyCommand:
//
//ProxyCommand "/path/with space/coder" ssh --stdio work
//
// This is ~what OpenSSH would execute:
//
///bin/bash -c '"/path/with space/to/coder" ssh --stdio workspace'
//
// However, since it's actually an arg in C, the contents inside the
// single quotes are interpreted as is, e.g. if there was a '\t', it
// would be the literal string '\t', not a tab.
//
// See:
// - https://github.com/coder/coder/issues/2853
// - https://github.com/openssh/openssh-portable/blob/V_9_0_P1/sshconnect.c#L158-L167
// - https://github.com/PowerShell/openssh-portable/blob/v8.1.0.0/sshconnect.c#L231-L293
// - https://github.com/PowerShell/openssh-portable/blob/v8.1.0.0/contrib/win32/win32compat/w32fd.c#L1075-L1100
func sshConfigExecEscape(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)
}
// In the unlikely even 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, "\"", "\\\"")
}
// A space or a tab requires quoting, but tabs must not be escaped
// (\t) since OpenSSH interprets it as a literal \t, not a tab.
if strings.ContainsAny(path, " \t") {
path = fmt.Sprintf("\"%s\"", path) //nolint:gocritic // We don't want %q here.
}
return path, nil
}

// currentBinPath returns the path to the coder binary suitable for use in ssh
// ProxyCommand.
func currentBinPath(w io.Writer) (string, error) {
Expand Down
62 changes: 62 additions & 0 deletionscli/configssh_internal_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
package cli

import (
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"testing"

"github.com/stretchr/testify/require"
)

// This test tries to mimic the behavior of OpenSSH
// when executing e.g. a ProxyCommand.
func Test_sshConfigExecEscape(t *testing.T) {
t.Parallel()

tests := []struct {
name string
path string
wantErr bool
windows bool
}{
{"no spaces", "simple", false, true},
{"spaces", "path with spaces", false, true},
{"quotes", "path with \"quotes\"", false, false},
{"backslashes", "path with \\backslashes", false, false},
{"tabs", "path with \ttabs", false, false},
{"newline fails", "path with \nnewline", true, false},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

if runtime.GOOS == "windows" {
t.Skip("Windows doesn't typically execute via /bin/sh or cmd.exe, so this test is not applicable.")
}

dir := filepath.Join(t.TempDir(), tt.path)
err := os.MkdirAll(dir, 0o755)
require.NoError(t, err)
bin := filepath.Join(dir, "coder")
contents := []byte("#!/bin/sh\necho yay\n")
err = os.WriteFile(bin, contents, 0o755) //nolint:gosec
require.NoError(t, err)

escaped, err := sshConfigExecEscape(bin)
if tt.wantErr {
require.Error(t, err)
return
}
require.NoError(t, err)

b, err := exec.Command("/bin/sh", "-c", escaped).CombinedOutput() //nolint:gosec
require.NoError(t, err)
got := strings.TrimSpace(string(b))
require.Equal(t, "yay", got)
})
}
}
[8]ページ先頭
©2009-2025 Movatter.jp