mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix!: validate HostnameSuffix and SSHConfigOptions' (#26154)
- Adds server-side and client-side validation for CODER_CONFIGSSH_HOSTNAME_SUFFIX and CODER_SSH_CONFIG_OPTIONS. - **Server-side breaking change:** invalid values for either of these will cause `coderd` to exit with an error. - Client-side: `coder config-ssh` will exit with an error if it detects invalid config. - Adds tests for the above Local smoke-testing: ran `develop.sh --env-file <path to an env file containing badness>`. Validated that server startup failed as expected. > 🤖 Generated by Coder Agents with supervision from a human. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
Cian Johnston
co-authored by
Copilot Autofix powered by AI
Copilot Autofix powered by AI
parent
1dc12f8ae7
commit
a26c46a3bf
+126
-6
@@ -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) {
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user