diff --git a/cli/configssh.go b/cli/configssh.go index 00eabb9907..f40fe48453 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -192,11 +192,12 @@ func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (r //nolint:gocyclo func (r *RootCmd) configSSH() *clibase.Cmd { var ( - sshConfigFile string - sshConfigOpts sshConfigOptions - usePreviousOpts bool - dryRun bool - skipProxyCommand bool + sshConfigFile string + sshConfigOpts sshConfigOptions + usePreviousOpts bool + dryRun bool + skipProxyCommand bool + forceUnixSeparators bool ) client := new(codersdk.Client) cmd := &clibase.Cmd{ @@ -236,13 +237,13 @@ func (r *RootCmd) configSSH() *clibase.Cmd { if err != nil { return err } - escapedCoderBinary, err := sshConfigExecEscape(coderBinary) + escapedCoderBinary, err := sshConfigExecEscape(coderBinary, forceUnixSeparators) if err != nil { return xerrors.Errorf("escape coder binary for ssh failed: %w", err) } root := r.createConfig() - escapedGlobalConfig, err := sshConfigExecEscape(string(root)) + escapedGlobalConfig, err := sshConfigExecEscape(string(root), forceUnixSeparators) if err != nil { return xerrors.Errorf("escape global config for ssh failed: %w", err) } @@ -540,6 +541,19 @@ func (r *RootCmd) configSSH() *clibase.Cmd { Default: "auto", Value: clibase.EnumOf(&sshConfigOpts.waitEnum, "yes", "no", "auto"), }, + { + Flag: "force-unix-filepaths", + Env: "CODER_CONFIGSSH_UNIX_FILEPATHS", + Description: "By default, 'config-ssh' uses the os path separator when writing the ssh config. " + + "This might be an issue in Windows machine that use a unix-like shell. " + + "This flag forces the use of unix file paths (the forward slash '/').", + Value: clibase.BoolOf(&forceUnixSeparators), + // On non-windows showing this command is useless because it is a noop. + // Hide vs disable it though so if a command is copied from a Windows + // machine to a unix machine it will still work and not throw an + // "unknown flag" error. + Hidden: hideForceUnixSlashes, + }, cliui.SkipPromptOption(), } @@ -727,7 +741,31 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) { // - 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) { +// +// Additional Windows-specific notes: +// +// In some situations a Windows user could be using a unix-like shell such as +// git bash. In these situations the coder.exe is using the windows filepath +// separator (\), but the shell wants the unix filepath separator (/). +// Trying to determine if the shell is unix-like is difficult, so this function +// takes the argument 'forceUnixPath' to force the filepath to be unix-like. +// +// On actual unix machines, this is **always** a noop. Even if a windows +// path is provided. +// +// Passing a "false" for forceUnixPath will result in the filepath separator +// untouched from the original input. +// --- +// This is a control flag, and that is ok. It is a control flag +// based on the OS of the user. Making this a different file is excessive. +// nolint:revive +func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) { + if forceUnixPath { + // This is a workaround for #7639, where the filepath separator is + // incorrectly the Windows separator (\) instead of the unix separator (/). + path = filepath.ToSlash(path) + } + // 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") { diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index 201dc9fea5..732452a761 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -12,6 +12,11 @@ import ( "github.com/stretchr/testify/require" ) +func init() { + // For golden files, always show the flag. + hideForceUnixSlashes = false +} + func Test_sshConfigSplitOnCoderSection(t *testing.T) { t.Parallel() @@ -140,14 +145,14 @@ func Test_sshConfigExecEscape(t *testing.T) { 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}, + {"windows path", `C:\Program Files\Coder\bin\coder.exe`, false}, + {"no spaces", "simple", false}, + {"spaces", "path with spaces", false}, + {"quotes", "path with \"quotes\"", false}, + {"backslashes", "path with \\backslashes", false}, + {"tabs", "path with \ttabs", false}, + {"newline fails", "path with \nnewline", true}, } for _, tt := range tests { tt := tt @@ -166,7 +171,7 @@ func Test_sshConfigExecEscape(t *testing.T) { err = os.WriteFile(bin, contents, 0o755) //nolint:gosec require.NoError(t, err) - escaped, err := sshConfigExecEscape(bin) + escaped, err := sshConfigExecEscape(bin, false) if tt.wantErr { require.Error(t, err) return @@ -181,6 +186,72 @@ func Test_sshConfigExecEscape(t *testing.T) { } } +func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + path string + // Behavior is different on Windows + expWindowsPath string + expOtherPath string + forceUnix bool + wantErr bool + }{ + { + name: "windows_keep_forward_slashes_with_spaces", + // Has a space, expect quotes + path: `C:\Program Files\Coder\bin\coder.exe`, + expWindowsPath: `"C:\Program Files\Coder\bin\coder.exe"`, + expOtherPath: `"C:\Program Files\Coder\bin\coder.exe"`, + forceUnix: false, + wantErr: false, + }, + { + name: "windows_keep_forward_slashes", + path: `C:\ProgramFiles\Coder\bin\coder.exe`, + expWindowsPath: `C:\ProgramFiles\Coder\bin\coder.exe`, + expOtherPath: `C:\ProgramFiles\Coder\bin\coder.exe`, + forceUnix: false, + wantErr: false, + }, + { + name: "windows_force_unix_with_spaces", + path: `C:\Program Files\Coder\bin\coder.exe`, + expWindowsPath: `"C:/Program Files/Coder/bin/coder.exe"`, + expOtherPath: `"C:\Program Files\Coder\bin\coder.exe"`, + forceUnix: true, + wantErr: false, + }, + { + name: "windows_force_unix", + path: `C:\ProgramFiles\Coder\bin\coder.exe`, + expWindowsPath: `C:/ProgramFiles/Coder/bin/coder.exe`, + expOtherPath: `C:\ProgramFiles\Coder\bin\coder.exe`, + forceUnix: true, + wantErr: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + found, err := sshConfigExecEscape(tt.path, tt.forceUnix) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + if runtime.GOOS == "windows" { + require.Equal(t, tt.expWindowsPath, found, "(Windows) expected path") + } else { + // this is a noop on non-windows! + require.Equal(t, tt.expOtherPath, found, "(Non-Windows) expected path") + } + }) + } +} + func Test_sshConfigOptions_addOption(t *testing.T) { t.Parallel() testCases := []struct { diff --git a/cli/configssh_other.go b/cli/configssh_other.go new file mode 100644 index 0000000000..fde7cc0e47 --- /dev/null +++ b/cli/configssh_other.go @@ -0,0 +1,5 @@ +//go:build !windows + +package cli + +var hideForceUnixSlashes = true diff --git a/cli/configssh_windows.go b/cli/configssh_windows.go new file mode 100644 index 0000000000..642a388fc8 --- /dev/null +++ b/cli/configssh_windows.go @@ -0,0 +1,6 @@ +//go:build windows + +package cli + +// Must be a var for unit tests to conform behavior +var hideForceUnixSlashes = false diff --git a/cli/testdata/coder_config-ssh_--help.golden b/cli/testdata/coder_config-ssh_--help.golden index 712c958ad3..857962b271 100644 --- a/cli/testdata/coder_config-ssh_--help.golden +++ b/cli/testdata/coder_config-ssh_--help.golden @@ -15,6 +15,12 @@ Add an SSH Host entry for your workspaces "ssh coder.workspace" -n, --dry-run bool, $CODER_SSH_DRY_RUN Perform a trial run with no changes made, showing a diff at the end. + --force-unix-filepaths bool, $CODER_CONFIGSSH_UNIX_FILEPATHS + By default, 'config-ssh' uses the os path separator when writing the + ssh config. This might be an issue in Windows machine that use a + unix-like shell. This flag forces the use of unix file paths (the + forward slash '/'). + --ssh-config-file string, $CODER_SSH_CONFIG_FILE (default: ~/.ssh/config) Specifies the path to an SSH config.