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

* fix: Use smarter quoting for ProxyCommand in config-ssh

This 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

cli/configssh.go

@@ -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 {
@@ -238,7 +248,6 @@ func configSSH() *cobra.Command {
 			}
 
 			configModified := configRaw
-			root := createConfig(cmd)
 
 			buf := &bytes.Buffer{}
 			before, after := sshConfigSplitOnCoderSection(configModified)
@@ -280,11 +289,17 @@ func configSSH() *cobra.Command {
 						"\tLogLevel ERROR",
 					)
 					if !skipProxyCommand {
-						if !wireguard {
+						wgArg := ""
-							configOptions = append(configOptions, fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --stdio %s", binaryFile, root, hostname))
+						if wireguard {
-						} else {
+							wgArg = "--wireguard "
-							configOptions = append(configOptions, fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --wireguard --stdio %s", binaryFile, root, hostname))
 						}
+						configOptions = append(
+							configOptions,
+							fmt.Sprintf(
+								"\tProxyCommand %s --global-config %s ssh %s--stdio %s",
+								escapedCoderBinary, escapedGlobalConfig, wgArg, hostname,
+							),
+						)
 					}
 
 					_, _ = buf.WriteString(strings.Join(configOptions, "\n"))
@@ -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))
@@ -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) {

cli/configssh_internal_test.go

@@ -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)
+		})
+	}
+}