diff --git a/cli/configssh.go b/cli/configssh.go index cc723bd328..b81b39041e 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -578,6 +578,10 @@ func mergeSSHOptions( ) ( sshConfigOptions, error, ) { + if err := coderd.Validate(); err != nil { + return sshConfigOptions{}, xerrors.Errorf("invalid ssh config from coderd: %w", err) + } + // Write agent configuration. defaultOptions := []string{ "ConnectTimeout=0", diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index 0ea2ae6ea5..cf7a5bff05 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -10,6 +10,8 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/codersdk" ) func Test_sshConfigSplitOnCoderSection(t *testing.T) { @@ -302,6 +304,140 @@ func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) { } } +func Test_mergeSSHOptions_RejectsUnsafeServerConfig(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + coderd codersdk.SSHConfigResponse + wantErr string + }{ + { + name: "HostnameSuffix", + coderd: codersdk.SSHConfigResponse{ + HostnameSuffix: "coder\nHost *", + }, + wantErr: "workspace hostname suffix", + }, + { + name: "HostnamePrefix", + coderd: codersdk.SSHConfigResponse{ + HostnamePrefix: "coder.\nHost *", + }, + wantErr: "workspace hostname prefix", + }, + { + name: "ProxyCommand", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"ProxyCommand": "ssh -W %h:%p bastion"}, + }, + wantErr: `ssh config option "ProxyCommand" is not allowed`, + }, + { + name: "PermitLocalCommand", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"PermitLocalCommand": "yes"}, + }, + wantErr: `ssh config option "PermitLocalCommand" is not allowed`, + }, + { + name: "KnownHostsCommand", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"KnownHostsCommand": "echo key"}, + }, + wantErr: `ssh config option "KnownHostsCommand" is not allowed`, + }, + { + name: "PKCS11Provider", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"PKCS11Provider": "/tmp/evil.so"}, + }, + wantErr: `ssh config option "PKCS11Provider" is not allowed`, + }, + { + name: "NewlineInValue", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"UserKnownHostsFile": "/tmp/known_hosts\nHost *"}, + }, + wantErr: `ssh config option "UserKnownHostsFile" must not contain carriage return, newline, or NUL characters`, + }, + { + name: "SmartcardDevice", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"SmartcardDevice": "/path/to/lib"}, + }, + wantErr: `not allowed`, + }, + { + name: "XAuthLocation", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"XAuthLocation": "/usr/bin/xauth"}, + }, + wantErr: `not allowed`, + }, + { + name: "ProxyJump", + coderd: codersdk.SSHConfigResponse{ + SSHConfigOptions: map[string]string{"ProxyJump": "bastion.example.com"}, + }, + wantErr: `conflicts with`, + }, + { + name: "HostnameSuffixGlob", + coderd: codersdk.SSHConfigResponse{ + HostnameSuffix: "*", + }, + wantErr: `glob`, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + _, err := mergeSSHOptions(sshConfigOptions{}, tt.coderd, t.TempDir(), "/tmp/coder") + require.ErrorContains(t, err, tt.wantErr) + }) + } +} + +func Test_mergeSSHOptions_UserOptionsOverrideServerConfig(t *testing.T) { + t.Parallel() + + user := sshConfigOptions{ + userHostPrefix: "dev.", + hostnameSuffix: "local", + } + got, err := mergeSSHOptions(user, codersdk.SSHConfigResponse{ + HostnamePrefix: "coder.", + HostnameSuffix: "coder", + }, t.TempDir(), "/tmp/coder") + require.NoError(t, err) + require.Equal(t, "dev.", got.userHostPrefix) + require.Equal(t, "local", got.hostnameSuffix) +} + +func Test_mergeSSHOptions_AllowsSafeServerConfig(t *testing.T) { + t.Parallel() + + got, err := mergeSSHOptions(sshConfigOptions{}, codersdk.SSHConfigResponse{ + HostnamePrefix: "coder.", + HostnameSuffix: "coder", + SSHConfigOptions: map[string]string{ + "HostName": "example.com", + "User": "coder", + "Port": "22", + "SetEnv": "FOO=bar BAZ=qux", + "UserKnownHostsFile": "/tmp/coder_known_hosts", + }, + }, t.TempDir(), "/tmp/coder") + require.NoError(t, err) + require.Equal(t, "coder.", got.userHostPrefix) + require.Equal(t, "coder", got.hostnameSuffix) + require.Contains(t, got.sshOptions, "HostName example.com") + require.Contains(t, got.sshOptions, "SetEnv FOO=bar BAZ=qux") +} + func Test_sshConfigOptions_addOption(t *testing.T) { t.Parallel() testCases := []struct { diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 82791f02b2..b381c508a5 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -168,6 +168,63 @@ func TestConfigSSH(t *testing.T) { <-copyDone } +func TestConfigSSH_RejectsUnsafeServerConfig(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("See coder/internal#117") + } + + testCases := []struct { + name string + configSSH codersdk.SSHConfigResponse + wantErr string + }{ + { + name: "HostnameSuffix", + configSSH: codersdk.SSHConfigResponse{HostnameSuffix: "coder\nHost *"}, + wantErr: "workspace hostname suffix", + }, + { + name: "HostnamePrefix", + configSSH: codersdk.SSHConfigResponse{HostnamePrefix: "coder.\nHost *"}, + wantErr: "workspace hostname prefix", + }, + { + name: "HostnameSuffixGlob", + configSSH: codersdk.SSHConfigResponse{HostnameSuffix: "*"}, + wantErr: "glob", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + const existingConfig = "Host safe\n\tHostName safe.example.com\n" + client := coderdtest.New(t, &coderdtest.Options{ + ConfigSSH: tc.configSSH, + }) + _ = coderdtest.CreateFirstUser(t, client) + + sshConfigPath := sshConfigFileName(t) + sshConfigFileCreate(t, sshConfigPath, strings.NewReader(existingConfig)) + + inv, root := clitest.New(t, + "config-ssh", + "--ssh-config-file", sshConfigPath, + "--yes", + ) + clitest.SetupConfig(t, client, root) + + err := inv.Run() + require.Error(t, err) + require.ErrorContains(t, err, tc.wantErr) + require.Equal(t, existingConfig, sshConfigFileRead(t, sshConfigPath)) + }) + } +} + func TestConfigSSH_MissingDirectory(t *testing.T) { t.Parallel() diff --git a/cli/server.go b/cli/server.go index 9d9b9d528f..2a1fd623cd 100644 --- a/cli/server.go +++ b/cli/server.go @@ -431,6 +431,19 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. logger.Debug(ctx, "tracing closed", slog.Error(traceCloseErr)) }() + configSSHOptions, err := vals.SSHConfig.ParseOptions() + if err != nil { + return xerrors.Errorf("parse ssh config options %q: %w", vals.SSHConfig.SSHConfigOptions.String(), err) + } + sshConfigResponse := codersdk.SSHConfigResponse{ + HostnamePrefix: vals.SSHConfig.DeploymentName.String(), + HostnameSuffix: vals.WorkspaceHostnameSuffix.String(), + SSHConfigOptions: configSSHOptions, + } + if err := sshConfigResponse.Validate(); err != nil { + return xerrors.Errorf("invalid ssh config: %w", err) + } + httpServers, err := ConfigureHTTPServers(logger, inv, vals) if err != nil { return xerrors.Errorf("configure http(s): %w", err) @@ -641,20 +654,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. return xerrors.Errorf("parse real ip config: %w", err) } - configSSHOptions, err := vals.SSHConfig.ParseOptions() - if err != nil { - return xerrors.Errorf("parse ssh config options %q: %w", vals.SSHConfig.SSHConfigOptions.String(), err) - } - - // The workspace hostname suffix is always interpreted as implicitly beginning with a single dot, so it is - // a config error to explicitly include the dot. This ensures that we always interpret the suffix as a - // separate DNS label, and not just an ordinary string suffix. E.g. a suffix of 'coder' will match - // 'en.coder' but not 'encoder'. - if strings.HasPrefix(vals.WorkspaceHostnameSuffix.String(), ".") { - return xerrors.Errorf("you must omit any leading . in workspace hostname suffix: %s", - vals.WorkspaceHostnameSuffix.String()) - } - options := &coderd.Options{ AccessURL: vals.AccessURL.Value(), AppHostname: appHostname, @@ -684,14 +683,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. HTTPClient: httpClient, TemplateScheduleStore: &atomic.Pointer[schedule.TemplateScheduleStore]{}, UserQuietHoursScheduleStore: &atomic.Pointer[schedule.UserQuietHoursScheduleStore]{}, - SSHConfig: codersdk.SSHConfigResponse{ - HostnamePrefix: vals.SSHConfig.DeploymentName.String(), - SSHConfigOptions: configSSHOptions, - HostnameSuffix: vals.WorkspaceHostnameSuffix.String(), - }, - AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(), - Entitlements: entitlements.New(), - NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled. + SSHConfig: sshConfigResponse, + AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(), + Entitlements: entitlements.New(), + NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled. } if httpServers.TLSConfig != nil { options.TLSCertificates = httpServers.TLSConfig.Certificates diff --git a/cli/server_test.go b/cli/server_test.go index 08af5d7efe..5b68c68588 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -1823,6 +1823,56 @@ func TestServer(t *testing.T) { }) } +// TestServer_InvalidSSHDeploymentConfig checks that unsafe SSH config flags are +// rejected at startup, before any database connection, so these invocations +// fail fast. +func TestServer_InvalidSSHDeploymentConfig(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + flag string + wantErr string + }{ + { + name: "HostnameSuffixLeadingDot", + flag: "--workspace-hostname-suffix=.coder", + wantErr: "workspace hostname suffix", + }, + { + name: "HostnameSuffixNewline", + flag: "--workspace-hostname-suffix=coder\nHost *", + wantErr: "workspace hostname suffix", + }, + { + name: "HostnamePrefixNewline", + flag: "--ssh-hostname-prefix=coder.\nHost *", + wantErr: "workspace hostname prefix", + }, + { + name: "SSHOptionUnparseable", + flag: "--ssh-config-options=NoSeparatorOption", + wantErr: "parse ssh config options", + }, + { + name: "SSHOptionDisallowedKey", + flag: "--ssh-config-options=ProxyCommand=ssh -W %h:%p bastion", + wantErr: `ssh config option "ProxyCommand" is not allowed`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + inv, _ := clitest.New(t, "server", tc.flag) + err := inv.WithContext(ctx).Run() + require.Error(t, err) + require.ErrorContains(t, err, tc.wantErr) + }) + } +} + //nolint:tparallel,paralleltest // This test sets environment variables. func TestServer_ExternalAuthGitHubDefaultProvider(t *testing.T) { type testCase struct { diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 53ccf77f7c..63640d49f9 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -288,8 +288,12 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI. --ssh-config-options string-array, $CODER_SSH_CONFIG_OPTIONS These SSH config options will override the default SSH config options. Provide options in "key=value" or "key value" format separated by - commas.Using this incorrectly can break SSH to your deployment, use - cautiously. + commas. Using this incorrectly can break SSH to your deployment, use + cautiously. The following options are not allowed: Host, Match, + Include, ProxyCommand, ProxyJump, LocalCommand, PermitLocalCommand, + RemoteCommand, KnownHostsCommand, PKCS11Provider, SecurityKeyProvider, + SmartcardDevice, XAuthLocation. Option values must not contain + newline, carriage return, or NUL characters. --web-terminal-renderer string, $CODER_WEB_TERMINAL_RENDERER (default: canvas) The renderer to use when opening a web terminal. Valid values are @@ -298,7 +302,8 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI. --workspace-hostname-suffix string, $CODER_WORKSPACE_HOSTNAME_SUFFIX (default: coder) Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Desktop. By default it is coder, resulting in names like - myworkspace.coder. + myworkspace.coder. The suffix must not start with a dot, and must not + contain spaces, newlines, or glob characters (* and ?). CONFIG OPTIONS: Use a YAML configuration file when your server launch become unwieldy. diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 613b639553..15dd31638d 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -542,12 +542,18 @@ client: # (default: coder., type: string) sshHostnamePrefix: coder. # Workspace hostnames use this suffix in SSH config and Coder Connect on Coder - # Desktop. By default it is coder, resulting in names like myworkspace.coder. + # Desktop. By default it is coder, resulting in names like myworkspace.coder. The + # suffix must not start with a dot, and must not contain spaces, newlines, or glob + # characters (* and ?). # (default: coder, type: string) workspaceHostnameSuffix: coder # These SSH config options will override the default SSH config options. Provide - # options in "key=value" or "key value" format separated by commas.Using this - # incorrectly can break SSH to your deployment, use cautiously. + # options in "key=value" or "key value" format separated by commas. Using this + # incorrectly can break SSH to your deployment, use cautiously. The following + # options are not allowed: Host, Match, Include, ProxyCommand, ProxyJump, + # LocalCommand, PermitLocalCommand, RemoteCommand, KnownHostsCommand, + # PKCS11Provider, SecurityKeyProvider, SmartcardDevice, XAuthLocation. Option + # values must not contain newline, carriage return, or NUL characters. # (default: , type: string-array) sshConfigOptions: [] # The upgrade message to display to users when a client/server mismatch is diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 0d8a07e825..3e447a4db4 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -13,6 +13,7 @@ import ( "strconv" "strings" "time" + "unicode" "github.com/coreos/go-oidc/v3/oidc" "github.com/google/uuid" @@ -706,18 +707,115 @@ func (c SSHConfig) ParseOptions() (map[string]string, error) { return m, nil } -// ParseSSHConfigOption parses a single ssh config option into it's key/value pair. +// ParseSSHConfigOption parses a single ssh config option into its key/value pair. func ParseSSHConfigOption(opt string) (key string, value string, err error) { - // An equal sign or whitespace is the separator between the key and value. + if strings.ContainsAny(opt, "\r\n\x00") { + return "", "", xerrors.Errorf("config-ssh option %q must not contain carriage return, newline, or NUL characters", opt) + } + + // An equal sign or a space is the separator between the key and value. idx := strings.IndexFunc(opt, func(r rune) bool { return r == ' ' || r == '=' }) if idx == -1 { - return "", "", xerrors.Errorf("invalid config-ssh option %q", opt) + return "", "", xerrors.Errorf("config-ssh option %q is missing a key/value separator ('=' or ' ')", opt) } return opt[:idx], opt[idx+1:], nil } +// isSingleHostPatternToken reports whether s is safe to write as a single SSH +// host pattern token. Whitespace or control characters could break out into +// additional SSH config directives. +func isSingleHostPatternToken(s string) bool { + return !strings.ContainsFunc(s, func(r rune) bool { + return unicode.IsSpace(r) || unicode.IsControl(r) + }) +} + +// ValidateWorkspaceHostnameSuffix validates a deployment-provided SSH hostname +// suffix before it is made available to clients. +func ValidateWorkspaceHostnameSuffix(suffix string) error { + // The suffix is implicitly prefixed with a dot when matching, so a leading + // dot is a config error: it forces the suffix to be a separate DNS label + // rather than an ordinary string suffix. E.g. "coder" matches "en.coder" + // but not "encoder". + if strings.HasPrefix(suffix, ".") { + return xerrors.Errorf("workspace hostname suffix %q must not start with a leading dot", suffix) + } + if strings.ContainsAny(suffix, "*?") { + return xerrors.Errorf("workspace hostname suffix %q must not contain glob characters", suffix) + } + if !isSingleHostPatternToken(suffix) { + return xerrors.Errorf("workspace hostname suffix %q must not contain whitespace or control characters", suffix) + } + return nil +} + +// ValidateWorkspaceHostnamePrefix validates a deployment-provided SSH hostname +// prefix before it is made available to clients. Unlike the suffix, a prefix +// may legitimately contain a trailing dot (the default is "coder."), so only +// the single-token requirement is enforced. +func ValidateWorkspaceHostnamePrefix(prefix string) error { + if !isSingleHostPatternToken(prefix) { + return xerrors.Errorf("workspace hostname prefix %q must not contain whitespace or control characters", prefix) + } + return nil +} + +// ValidateSSHConfigOptions validates deployment SSH settings before they are +// written to users' local SSH configs. +func ValidateSSHConfigOptions(options map[string]string) error { + // Sort the keys so that, when several options are invalid, the surfaced + // error is deterministic across restarts rather than dependent on map + // iteration order. + keys := make([]string, 0, len(options)) + for key := range options { + keys = append(keys, key) + } + slices.Sort(keys) + for _, key := range keys { + if err := ValidateSSHConfigOption(key, options[key]); err != nil { + return err + } + } + return nil +} + +// ValidateSSHConfigOption validates one deployment SSH option before it is +// written to users' local SSH configs. +func ValidateSSHConfigOption(key, value string) error { + if key == "" { + return xerrors.New("ssh config option key must not be empty") + } + if strings.ContainsAny(key, "=\r\n\x00") || strings.ContainsFunc(key, unicode.IsSpace) { + return xerrors.Errorf("ssh config option key %q is invalid", key) + } + // These options are rejected because, written into a user's SSH config by a + // deployment, they can execute code, load shared libraries, or override + // Coder's managed SSH settings on the client machine. When extending this + // list, classify the directive against these categories; the newline and + // whitespace checks above already prevent multi-line injection, so only + // single-line dangerous directives belong here. + switch strings.ToLower(key) { + // Structural directives that escape Coder's managed block. + case "host", "match", "include", + // Directives that run an attacker-supplied command string. + "proxycommand", "localcommand", "permitlocalcommand", "remotecommand", "knownhostscommand", + // Directives that dlopen an attacker-controlled shared library. + "pkcs11provider", "securitykeyprovider", "smartcarddevice", + // Directives that execute a command for X11 authentication. + "xauthlocation": + return xerrors.Errorf("ssh config option %q is not allowed: it can execute code, load shared libraries, or override Coder's managed SSH settings on client machines", key) + // ProxyJump conflicts with Coder's managed ProxyCommand. + case "proxyjump": + return xerrors.Errorf("ssh config option %q is not allowed: it conflicts with Coder's managed ProxyCommand", key) + } + if strings.ContainsAny(value, "\r\n\x00") { + return xerrors.Errorf("ssh config option %q must not contain carriage return, newline, or NUL characters", key) + } + return nil +} + // SessionLifetime refers to "sessions" authenticating into Coderd. Coder has // multiple different session types: api keys, tokens, workspace app tokens, // agent tokens, etc. This configuration struct should be used to group all @@ -1689,7 +1787,7 @@ func (c *DeploymentValues) Options() serpent.OptionSet { } workspaceHostnameSuffix := serpent.Option{ Name: "Workspace Hostname Suffix", - Description: "Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Desktop. By default it is coder, resulting in names like myworkspace.coder.", + Description: "Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Desktop. By default it is coder, resulting in names like myworkspace.coder. The suffix must not start with a dot, and must not contain spaces, newlines, or glob characters (* and ?).", Flag: "workspace-hostname-suffix", Env: "CODER_WORKSPACE_HOSTNAME_SUFFIX", YAML: "workspaceHostnameSuffix", @@ -3562,8 +3660,13 @@ func (c *DeploymentValues) Options() serpent.OptionSet { { Name: "SSH Config Options", Description: "These SSH config options will override the default SSH config options. " + - "Provide options in \"key=value\" or \"key value\" format separated by commas." + - "Using this incorrectly can break SSH to your deployment, use cautiously.", + "Provide options in \"key=value\" or \"key value\" format separated by commas. " + + "Using this incorrectly can break SSH to your deployment, use cautiously. " + + "The following options are not allowed: " + + "Host, Match, Include, ProxyCommand, ProxyJump, LocalCommand, PermitLocalCommand, " + + "RemoteCommand, KnownHostsCommand, PKCS11Provider, SecurityKeyProvider, " + + "SmartcardDevice, XAuthLocation. " + + "Option values must not contain newline, carriage return, or NUL characters.", Flag: "ssh-config-options", Env: "CODER_SSH_CONFIG_OPTIONS", YAML: "sshConfigOptions", @@ -5239,6 +5342,23 @@ type SSHConfigResponse struct { SSHConfigOptions map[string]string `json:"ssh_config_options"` } +// Validate checks that the deployment-provided SSH configuration is safe to +// write into a user's local SSH config. Validating here ensures a deployment +// can never serve config that the client would reject. +func (r SSHConfigResponse) Validate() error { + if r.HostnamePrefix != "" { + if err := ValidateWorkspaceHostnamePrefix(r.HostnamePrefix); err != nil { + return err + } + } + if r.HostnameSuffix != "" { + if err := ValidateWorkspaceHostnameSuffix(r.HostnameSuffix); err != nil { + return err + } + } + return ValidateSSHConfigOptions(r.SSHConfigOptions) +} + // SSHConfiguration returns information about the SSH configuration for the // Coder instance. func (c *Client) SSHConfiguration(ctx context.Context) (SSHConfigResponse, error) { diff --git a/codersdk/deployment_test.go b/codersdk/deployment_test.go index 0287c0daa5..1abaa5e000 100644 --- a/codersdk/deployment_test.go +++ b/codersdk/deployment_test.go @@ -149,6 +149,270 @@ func TestDeploymentValues_HighlyConfigurable(t *testing.T) { } } +func TestParseSSHConfigOption(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + option string + wantKey string + wantValue string + wantErr bool + }{ + { + name: "ProxyCommandWithSpaces", + option: "ProxyCommand=ssh -W %h:%p bastion", + wantKey: "ProxyCommand", + wantValue: "ssh -W %h:%p bastion", + }, + { + name: "SetEnvWithEquals", + option: "SetEnv=FOO=bar BAZ=qux", + wantKey: "SetEnv", + wantValue: "FOO=bar BAZ=qux", + }, + { + name: "SetEnvWithSpaceSeparator", + option: "SetEnv FOO=bar BAZ=qux", + wantKey: "SetEnv", + wantValue: "FOO=bar BAZ=qux", + }, + { + name: "HostName", + option: "HostName example.com", + wantKey: "HostName", + wantValue: "example.com", + }, + { + name: "NewlineInValue", + option: "ProxyCommand=echo hi\nHost *", + wantErr: true, + }, + { + name: "CarriageReturnInValue", + option: "ProxyCommand=echo hi\rHost *", + wantErr: true, + }, + { + name: "NULInValue", + option: "ProxyCommand=echo hi\x00Host *", + wantErr: true, + }, + { + name: "NewlineInKey", + option: "Proxy\nCommand=value", + wantErr: true, + }, + { + name: "CarriageReturnInKey", + option: "Proxy\rCommand=value", + wantErr: true, + }, + { + name: "NULInKey", + option: "Proxy\x00Command=value", + wantErr: true, + }, + { + name: "MissingSeparator", + option: "JustAKeyNoValue", + wantErr: true, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + key, value, err := codersdk.ParseSSHConfigOption(tt.option) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.wantKey, key) + require.Equal(t, tt.wantValue, value) + }) + } +} + +func TestValidateWorkspaceHostnameSuffix(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + suffix string + wantErr bool + }{ + {name: "Coder", suffix: "coder"}, + {name: "Example", suffix: "example"}, + {name: "Dotted", suffix: "coder.example.com"}, + {name: "Empty", suffix: ""}, + {name: "LeadingDot", suffix: ".coder", wantErr: true}, + {name: "Newline", suffix: "coder\nHost *\n\tProxyCommand evil", wantErr: true}, + {name: "CarriageReturn", suffix: "coder\r\nHost *", wantErr: true}, + {name: "Space", suffix: "coder Host *", wantErr: true}, + {name: "Tab", suffix: "coder\t*", wantErr: true}, + {name: "NUL", suffix: "coder\x00", wantErr: true}, + {name: "NonBreakingSpace", suffix: "coder\u00A0suffix", wantErr: true}, + {name: "Glob", suffix: "*", wantErr: true}, + {name: "GlobPrefix", suffix: "*.*", wantErr: true}, + {name: "QuestionMark", suffix: "code?", wantErr: true}, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := codersdk.ValidateWorkspaceHostnameSuffix(tt.suffix) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestValidateWorkspaceHostnamePrefix(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + prefix string + wantErr bool + }{ + {name: "Default", prefix: "coder."}, + {name: "NoDot", prefix: "coder"}, + {name: "Empty", prefix: ""}, + {name: "LeadingDot", prefix: ".coder"}, + {name: "Newline", prefix: "coder.\nHost *\n\tProxyCommand evil", wantErr: true}, + {name: "CarriageReturn", prefix: "coder.\r\nHost *", wantErr: true}, + {name: "Space", prefix: "coder. Host *", wantErr: true}, + {name: "Tab", prefix: "coder.\t*", wantErr: true}, + {name: "NUL", prefix: "coder.\x00", wantErr: true}, + {name: "NonBreakingSpace", prefix: "coder.\u00A0x", wantErr: true}, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := codersdk.ValidateWorkspaceHostnamePrefix(tt.prefix) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestValidateSSHConfigOptions(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + options map[string]string + wantErr bool + }{ + {name: "HostName", options: map[string]string{"HostName": "example.com"}}, + {name: "User", options: map[string]string{"User": "coder"}}, + {name: "Port", options: map[string]string{"Port": "22"}}, + {name: "SetEnv", options: map[string]string{"SetEnv": "FOO=bar BAZ=qux"}}, + {name: "UserKnownHostsFile", options: map[string]string{"UserKnownHostsFile": "/tmp/coder_known_hosts"}}, + {name: "EmptyKey", options: map[string]string{"": "value"}, wantErr: true}, + {name: "NewlineInKey", options: map[string]string{"User\nProxyCommand": "evil"}, wantErr: true}, + {name: "CarriageReturnInKey", options: map[string]string{"User\rProxyCommand": "evil"}, wantErr: true}, + {name: "NULInKey", options: map[string]string{"User\x00ProxyCommand": "evil"}, wantErr: true}, + {name: "SpaceInKey", options: map[string]string{"User ProxyCommand": "evil"}, wantErr: true}, + {name: "EqualsInKey", options: map[string]string{"User=ProxyCommand": "evil"}, wantErr: true}, + {name: "Host", options: map[string]string{"Host": "*"}, wantErr: true}, + {name: "HostCaseInsensitive", options: map[string]string{"hOsT": "*"}, wantErr: true}, + {name: "Match", options: map[string]string{"Match": "all"}, wantErr: true}, + {name: "Include", options: map[string]string{"Include": "~/.ssh/config.d/*"}, wantErr: true}, + {name: "ProxyCommand", options: map[string]string{"ProxyCommand": "ssh -W %h:%p bastion"}, wantErr: true}, + {name: "ProxyCommandCaseInsensitive", options: map[string]string{"proxycommand": "ssh -W %h:%p bastion"}, wantErr: true}, + {name: "LocalCommand", options: map[string]string{"LocalCommand": "echo pwned"}, wantErr: true}, + {name: "PermitLocalCommand", options: map[string]string{"PermitLocalCommand": "yes"}, wantErr: true}, + {name: "RemoteCommand", options: map[string]string{"RemoteCommand": "some-command"}, wantErr: true}, + {name: "KnownHostsCommand", options: map[string]string{"KnownHostsCommand": "echo key"}, wantErr: true}, + {name: "PKCS11Provider", options: map[string]string{"PKCS11Provider": "/tmp/evil.so"}, wantErr: true}, + {name: "PKCS11ProviderCaseInsensitive", options: map[string]string{"pkcs11provider": "/tmp/evil.so"}, wantErr: true}, + {name: "SecurityKeyProvider", options: map[string]string{"SecurityKeyProvider": "/tmp/evil.so"}, wantErr: true}, + {name: "NewlineInValue", options: map[string]string{"UserKnownHostsFile": "/tmp/known_hosts\nHost *\nProxyCommand evil"}, wantErr: true}, + {name: "CarriageReturnInValue", options: map[string]string{"UserKnownHostsFile": "/tmp/known_hosts\r\nHost *"}, wantErr: true}, + {name: "NULInValue", options: map[string]string{"UserKnownHostsFile": "/tmp/known_hosts\x00suffix"}, wantErr: true}, + {name: "SmartcardDevice", options: map[string]string{"SmartcardDevice": "/path/to/lib"}, wantErr: true}, + {name: "XAuthLocation", options: map[string]string{"XAuthLocation": "/usr/bin/xauth"}, wantErr: true}, + {name: "ProxyJump", options: map[string]string{"ProxyJump": "bastion.example.com"}, wantErr: true}, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := codersdk.ValidateSSHConfigOptions(tt.options) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestSSHConfigResponse_Validate(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + response codersdk.SSHConfigResponse + wantErr string + }{ + { + name: "Valid", + response: codersdk.SSHConfigResponse{ + HostnamePrefix: "coder.", + HostnameSuffix: "coder", + SSHConfigOptions: map[string]string{"HostName": "example.com"}, + }, + }, + { + name: "Empty", + response: codersdk.SSHConfigResponse{}, + }, + { + name: "PrefixUnsafe", + response: codersdk.SSHConfigResponse{HostnamePrefix: "coder.\nHost *"}, + wantErr: "workspace hostname prefix", + }, + { + name: "SuffixUnsafe", + response: codersdk.SSHConfigResponse{HostnameSuffix: "coder\nHost *"}, + wantErr: "workspace hostname suffix", + }, + { + name: "OptionUnsafe", + response: codersdk.SSHConfigResponse{SSHConfigOptions: map[string]string{"ProxyCommand": "ssh -W %h:%p bastion"}}, + wantErr: `ssh config option "ProxyCommand" is not allowed`, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := tt.response.Validate() + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + return + } + require.NoError(t, err) + }) + } +} + func TestSSHConfig_ParseOptions(t *testing.T) { t.Parallel() diff --git a/docs/reference/cli/server.md b/docs/reference/cli/server.md index 2de88e4960..356358a873 100644 --- a/docs/reference/cli/server.md +++ b/docs/reference/cli/server.md @@ -1228,7 +1228,7 @@ Specify a YAML file to load configuration from. | YAML | client.workspaceHostnameSuffix | | Default | coder | -Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Desktop. By default it is coder, resulting in names like myworkspace.coder. +Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Desktop. By default it is coder, resulting in names like myworkspace.coder. The suffix must not start with a dot, and must not contain spaces, newlines, or glob characters (* and ?). ### --ssh-config-options @@ -1238,7 +1238,7 @@ Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Des | Environment | $CODER_SSH_CONFIG_OPTIONS | | YAML | client.sshConfigOptions | -These SSH config options will override the default SSH config options. Provide options in "key=value" or "key value" format separated by commas.Using this incorrectly can break SSH to your deployment, use cautiously. +These SSH config options will override the default SSH config options. Provide options in "key=value" or "key value" format separated by commas. Using this incorrectly can break SSH to your deployment, use cautiously. The following options are not allowed: Host, Match, Include, ProxyCommand, ProxyJump, LocalCommand, PermitLocalCommand, RemoteCommand, KnownHostsCommand, PKCS11Provider, SecurityKeyProvider, SmartcardDevice, XAuthLocation. Option values must not contain newline, carriage return, or NUL characters. ### --cli-upgrade-message diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index addd3dc256..801bae69c9 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -289,8 +289,12 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI. --ssh-config-options string-array, $CODER_SSH_CONFIG_OPTIONS These SSH config options will override the default SSH config options. Provide options in "key=value" or "key value" format separated by - commas.Using this incorrectly can break SSH to your deployment, use - cautiously. + commas. Using this incorrectly can break SSH to your deployment, use + cautiously. The following options are not allowed: Host, Match, + Include, ProxyCommand, ProxyJump, LocalCommand, PermitLocalCommand, + RemoteCommand, KnownHostsCommand, PKCS11Provider, SecurityKeyProvider, + SmartcardDevice, XAuthLocation. Option values must not contain + newline, carriage return, or NUL characters. --web-terminal-renderer string, $CODER_WEB_TERMINAL_RENDERER (default: canvas) The renderer to use when opening a web terminal. Valid values are @@ -299,7 +303,8 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI. --workspace-hostname-suffix string, $CODER_WORKSPACE_HOSTNAME_SUFFIX (default: coder) Workspace hostnames use this suffix in SSH config and Coder Connect on Coder Desktop. By default it is coder, resulting in names like - myworkspace.coder. + myworkspace.coder. The suffix must not start with a dot, and must not + contain spaces, newlines, or glob characters (* and ?). CONFIG OPTIONS: Use a YAML configuration file when your server launch become unwieldy.