Skip to content

Commit 2dcde52

Browse files
jdomeracki-coderjohnstcnCopilot
authored
fix!: validate HostnameSuffix and SSHConfigOptions' (#26154) (#26225)
Backport of #26154 to `release/2.32`. _Generated by Coder Agents on behalf of @jdomeracki-coder._ Co-authored-by: Cian Johnston <cian@coder.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent eb297f3 commit 2dcde52

11 files changed

Lines changed: 681 additions & 39 deletions

File tree

‎cli/configssh.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -583,6 +583,10 @@ func mergeSSHOptions(
583583
) (
584584
sshConfigOptions, error,
585585
) {
586+
if err := coderd.Validate(); err != nil {
587+
return sshConfigOptions{}, xerrors.Errorf("invalid ssh config from coderd: %w", err)
588+
}
589+
586590
// Write agent configuration.
587591
defaultOptions := []string{
588592
"ConnectTimeout=0",

‎cli/configssh_internal_test.go‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import (
1010
"testing"
1111

1212
"github.com/stretchr/testify/require"
13+
14+
"github.com/coder/coder/v2/codersdk"
1315
)
1416

1517
func init() {
@@ -307,6 +309,140 @@ func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
307309
}
308310
}
309311

312+
func Test_mergeSSHOptions_RejectsUnsafeServerConfig(t *testing.T) {
313+
t.Parallel()
314+
315+
testCases := []struct {
316+
name string
317+
coderd codersdk.SSHConfigResponse
318+
wantErr string
319+
}{
320+
{
321+
name: "HostnameSuffix",
322+
coderd: codersdk.SSHConfigResponse{
323+
HostnameSuffix: "coder\nHost *",
324+
},
325+
wantErr: "workspace hostname suffix",
326+
},
327+
{
328+
name: "HostnamePrefix",
329+
coderd: codersdk.SSHConfigResponse{
330+
HostnamePrefix: "coder.\nHost *",
331+
},
332+
wantErr: "workspace hostname prefix",
333+
},
334+
{
335+
name: "ProxyCommand",
336+
coderd: codersdk.SSHConfigResponse{
337+
SSHConfigOptions: map[string]string{"ProxyCommand": "ssh -W %h:%p bastion"},
338+
},
339+
wantErr: `ssh config option "ProxyCommand" is not allowed`,
340+
},
341+
{
342+
name: "PermitLocalCommand",
343+
coderd: codersdk.SSHConfigResponse{
344+
SSHConfigOptions: map[string]string{"PermitLocalCommand": "yes"},
345+
},
346+
wantErr: `ssh config option "PermitLocalCommand" is not allowed`,
347+
},
348+
{
349+
name: "KnownHostsCommand",
350+
coderd: codersdk.SSHConfigResponse{
351+
SSHConfigOptions: map[string]string{"KnownHostsCommand": "echo key"},
352+
},
353+
wantErr: `ssh config option "KnownHostsCommand" is not allowed`,
354+
},
355+
{
356+
name: "PKCS11Provider",
357+
coderd: codersdk.SSHConfigResponse{
358+
SSHConfigOptions: map[string]string{"PKCS11Provider": "/tmp/evil.so"},
359+
},
360+
wantErr: `ssh config option "PKCS11Provider" is not allowed`,
361+
},
362+
{
363+
name: "NewlineInValue",
364+
coderd: codersdk.SSHConfigResponse{
365+
SSHConfigOptions: map[string]string{"UserKnownHostsFile": "/tmp/known_hosts\nHost *"},
366+
},
367+
wantErr: `ssh config option "UserKnownHostsFile" must not contain carriage return, newline, or NUL characters`,
368+
},
369+
{
370+
name: "SmartcardDevice",
371+
coderd: codersdk.SSHConfigResponse{
372+
SSHConfigOptions: map[string]string{"SmartcardDevice": "/path/to/lib"},
373+
},
374+
wantErr: `not allowed`,
375+
},
376+
{
377+
name: "XAuthLocation",
378+
coderd: codersdk.SSHConfigResponse{
379+
SSHConfigOptions: map[string]string{"XAuthLocation": "/usr/bin/xauth"},
380+
},
381+
wantErr: `not allowed`,
382+
},
383+
{
384+
name: "ProxyJump",
385+
coderd: codersdk.SSHConfigResponse{
386+
SSHConfigOptions: map[string]string{"ProxyJump": "bastion.example.com"},
387+
},
388+
wantErr: `conflicts with`,
389+
},
390+
{
391+
name: "HostnameSuffixGlob",
392+
coderd: codersdk.SSHConfigResponse{
393+
HostnameSuffix: "*",
394+
},
395+
wantErr: `glob`,
396+
},
397+
}
398+
399+
for _, tt := range testCases {
400+
t.Run(tt.name, func(t *testing.T) {
401+
t.Parallel()
402+
403+
_, err := mergeSSHOptions(sshConfigOptions{}, tt.coderd, t.TempDir(), "/tmp/coder")
404+
require.ErrorContains(t, err, tt.wantErr)
405+
})
406+
}
407+
}
408+
409+
func Test_mergeSSHOptions_UserOptionsOverrideServerConfig(t *testing.T) {
410+
t.Parallel()
411+
412+
user := sshConfigOptions{
413+
userHostPrefix: "dev.",
414+
hostnameSuffix: "local",
415+
}
416+
got, err := mergeSSHOptions(user, codersdk.SSHConfigResponse{
417+
HostnamePrefix: "coder.",
418+
HostnameSuffix: "coder",
419+
}, t.TempDir(), "/tmp/coder")
420+
require.NoError(t, err)
421+
require.Equal(t, "dev.", got.userHostPrefix)
422+
require.Equal(t, "local", got.hostnameSuffix)
423+
}
424+
425+
func Test_mergeSSHOptions_AllowsSafeServerConfig(t *testing.T) {
426+
t.Parallel()
427+
428+
got, err := mergeSSHOptions(sshConfigOptions{}, codersdk.SSHConfigResponse{
429+
HostnamePrefix: "coder.",
430+
HostnameSuffix: "coder",
431+
SSHConfigOptions: map[string]string{
432+
"HostName": "example.com",
433+
"User": "coder",
434+
"Port": "22",
435+
"SetEnv": "FOO=bar BAZ=qux",
436+
"UserKnownHostsFile": "/tmp/coder_known_hosts",
437+
},
438+
}, t.TempDir(), "/tmp/coder")
439+
require.NoError(t, err)
440+
require.Equal(t, "coder.", got.userHostPrefix)
441+
require.Equal(t, "coder", got.hostnameSuffix)
442+
require.Contains(t, got.sshOptions, "HostName example.com")
443+
require.Contains(t, got.sshOptions, "SetEnv FOO=bar BAZ=qux")
444+
}
445+
310446
func Test_sshConfigOptions_addOption(t *testing.T) {
311447
t.Parallel()
312448
testCases := []struct {

‎cli/configssh_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,63 @@ func TestConfigSSH(t *testing.T) {
169169
<-copyDone
170170
}
171171

172+
func TestConfigSSH_RejectsUnsafeServerConfig(t *testing.T) {
173+
t.Parallel()
174+
175+
if runtime.GOOS == "windows" {
176+
t.Skip("See coder/internal#117")
177+
}
178+
179+
testCases := []struct {
180+
name string
181+
configSSH codersdk.SSHConfigResponse
182+
wantErr string
183+
}{
184+
{
185+
name: "HostnameSuffix",
186+
configSSH: codersdk.SSHConfigResponse{HostnameSuffix: "coder\nHost *"},
187+
wantErr: "workspace hostname suffix",
188+
},
189+
{
190+
name: "HostnamePrefix",
191+
configSSH: codersdk.SSHConfigResponse{HostnamePrefix: "coder.\nHost *"},
192+
wantErr: "workspace hostname prefix",
193+
},
194+
{
195+
name: "HostnameSuffixGlob",
196+
configSSH: codersdk.SSHConfigResponse{HostnameSuffix: "*"},
197+
wantErr: "glob",
198+
},
199+
}
200+
201+
for _, tc := range testCases {
202+
t.Run(tc.name, func(t *testing.T) {
203+
t.Parallel()
204+
205+
const existingConfig = "Host safe\n\tHostName safe.example.com\n"
206+
client := coderdtest.New(t, &coderdtest.Options{
207+
ConfigSSH: tc.configSSH,
208+
})
209+
_ = coderdtest.CreateFirstUser(t, client)
210+
211+
sshConfigPath := sshConfigFileName(t)
212+
sshConfigFileCreate(t, sshConfigPath, strings.NewReader(existingConfig))
213+
214+
inv, root := clitest.New(t,
215+
"config-ssh",
216+
"--ssh-config-file", sshConfigPath,
217+
"--yes",
218+
)
219+
clitest.SetupConfig(t, client, root)
220+
221+
err := inv.Run()
222+
require.Error(t, err)
223+
require.ErrorContains(t, err, tc.wantErr)
224+
require.Equal(t, existingConfig, sshConfigFileRead(t, sshConfigPath))
225+
})
226+
}
227+
}
228+
172229
func TestConfigSSH_MissingDirectory(t *testing.T) {
173230
t.Parallel()
174231

‎cli/server.go‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,19 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
427427
logger.Debug(ctx, "tracing closed", slog.Error(traceCloseErr))
428428
}()
429429

430+
configSSHOptions, err := vals.SSHConfig.ParseOptions()
431+
if err != nil {
432+
return xerrors.Errorf("parse ssh config options %q: %w", vals.SSHConfig.SSHConfigOptions.String(), err)
433+
}
434+
sshConfigResponse := codersdk.SSHConfigResponse{
435+
HostnamePrefix: vals.SSHConfig.DeploymentName.String(),
436+
HostnameSuffix: vals.WorkspaceHostnameSuffix.String(),
437+
SSHConfigOptions: configSSHOptions,
438+
}
439+
if err := sshConfigResponse.Validate(); err != nil {
440+
return xerrors.Errorf("invalid ssh config: %w", err)
441+
}
442+
430443
httpServers, err := ConfigureHTTPServers(logger, inv, vals)
431444
if err != nil {
432445
return xerrors.Errorf("configure http(s): %w", err)
@@ -624,20 +637,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
624637
return xerrors.Errorf("parse real ip config: %w", err)
625638
}
626639

627-
configSSHOptions, err := vals.SSHConfig.ParseOptions()
628-
if err != nil {
629-
return xerrors.Errorf("parse ssh config options %q: %w", vals.SSHConfig.SSHConfigOptions.String(), err)
630-
}
631-
632-
// The workspace hostname suffix is always interpreted as implicitly beginning with a single dot, so it is
633-
// a config error to explicitly include the dot. This ensures that we always interpret the suffix as a
634-
// separate DNS label, and not just an ordinary string suffix. E.g. a suffix of 'coder' will match
635-
// 'en.coder' but not 'encoder'.
636-
if strings.HasPrefix(vals.WorkspaceHostnameSuffix.String(), ".") {
637-
return xerrors.Errorf("you must omit any leading . in workspace hostname suffix: %s",
638-
vals.WorkspaceHostnameSuffix.String())
639-
}
640-
641640
options := &coderd.Options{
642641
AccessURL: vals.AccessURL.Value(),
643642
AppHostname: appHostname,
@@ -667,14 +666,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
667666
HTTPClient: httpClient,
668667
TemplateScheduleStore: &atomic.Pointer[schedule.TemplateScheduleStore]{},
669668
UserQuietHoursScheduleStore: &atomic.Pointer[schedule.UserQuietHoursScheduleStore]{},
670-
SSHConfig: codersdk.SSHConfigResponse{
671-
HostnamePrefix: vals.SSHConfig.DeploymentName.String(),
672-
SSHConfigOptions: configSSHOptions,
673-
HostnameSuffix: vals.WorkspaceHostnameSuffix.String(),
674-
},
675-
AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(),
676-
Entitlements: entitlements.New(),
677-
NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled.
669+
SSHConfig: sshConfigResponse,
670+
AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(),
671+
Entitlements: entitlements.New(),
672+
NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled.
678673
}
679674
if httpServers.TLSConfig != nil {
680675
options.TLSCertificates = httpServers.TLSConfig.Certificates

‎cli/server_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1830,6 +1830,56 @@ func TestServer(t *testing.T) {
18301830
})
18311831
}
18321832

1833+
// TestServer_InvalidSSHDeploymentConfig checks that unsafe SSH config flags are
1834+
// rejected at startup, before any database connection, so these invocations
1835+
// fail fast.
1836+
func TestServer_InvalidSSHDeploymentConfig(t *testing.T) {
1837+
t.Parallel()
1838+
1839+
testCases := []struct {
1840+
name string
1841+
flag string
1842+
wantErr string
1843+
}{
1844+
{
1845+
name: "HostnameSuffixLeadingDot",
1846+
flag: "--workspace-hostname-suffix=.coder",
1847+
wantErr: "workspace hostname suffix",
1848+
},
1849+
{
1850+
name: "HostnameSuffixNewline",
1851+
flag: "--workspace-hostname-suffix=coder\nHost *",
1852+
wantErr: "workspace hostname suffix",
1853+
},
1854+
{
1855+
name: "HostnamePrefixNewline",
1856+
flag: "--ssh-hostname-prefix=coder.\nHost *",
1857+
wantErr: "workspace hostname prefix",
1858+
},
1859+
{
1860+
name: "SSHOptionUnparseable",
1861+
flag: "--ssh-config-options=NoSeparatorOption",
1862+
wantErr: "parse ssh config options",
1863+
},
1864+
{
1865+
name: "SSHOptionDisallowedKey",
1866+
flag: "--ssh-config-options=ProxyCommand=ssh -W %h:%p bastion",
1867+
wantErr: `ssh config option "ProxyCommand" is not allowed`,
1868+
},
1869+
}
1870+
1871+
for _, tc := range testCases {
1872+
t.Run(tc.name, func(t *testing.T) {
1873+
t.Parallel()
1874+
ctx := testutil.Context(t, testutil.WaitShort)
1875+
inv, _ := clitest.New(t, "server", tc.flag)
1876+
err := inv.WithContext(ctx).Run()
1877+
require.Error(t, err)
1878+
require.ErrorContains(t, err, tc.wantErr)
1879+
})
1880+
}
1881+
}
1882+
18331883
//nolint:tparallel,paralleltest // This test sets environment variables.
18341884
func TestServer_ExternalAuthGitHubDefaultProvider(t *testing.T) {
18351885
type testCase struct {

‎cli/testdata/coder_server_--help.golden‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,12 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI.
226226
--ssh-config-options string-array, $CODER_SSH_CONFIG_OPTIONS
227227
These SSH config options will override the default SSH config options.
228228
Provide options in "key=value" or "key value" format separated by
229-
commas.Using this incorrectly can break SSH to your deployment, use
230-
cautiously.
229+
commas. Using this incorrectly can break SSH to your deployment, use
230+
cautiously. The following options are not allowed: Host, Match,
231+
Include, ProxyCommand, ProxyJump, LocalCommand, PermitLocalCommand,
232+
RemoteCommand, KnownHostsCommand, PKCS11Provider, SecurityKeyProvider,
233+
SmartcardDevice, XAuthLocation. Option values must not contain
234+
newline, carriage return, or NUL characters.
231235

232236
--web-terminal-renderer string, $CODER_WEB_TERMINAL_RENDERER (default: canvas)
233237
The renderer to use when opening a web terminal. Valid values are
@@ -236,7 +240,8 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI.
236240
--workspace-hostname-suffix string, $CODER_WORKSPACE_HOSTNAME_SUFFIX (default: coder)
237241
Workspace hostnames use this suffix in SSH config and Coder Connect on
238242
Coder Desktop. By default it is coder, resulting in names like
239-
myworkspace.coder.
243+
myworkspace.coder. The suffix must not start with a dot, and must not
244+
contain spaces, newlines, or glob characters (* and ?).
240245

241246
CONFIG OPTIONS:
242247
Use a YAML configuration file when your server launch become unwieldy.

‎cli/testdata/server-config.yaml.golden‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -538,12 +538,18 @@ client:
538538
# (default: coder., type: string)
539539
sshHostnamePrefix: coder.
540540
# Workspace hostnames use this suffix in SSH config and Coder Connect on Coder
541-
# Desktop. By default it is coder, resulting in names like myworkspace.coder.
541+
# Desktop. By default it is coder, resulting in names like myworkspace.coder. The
542+
# suffix must not start with a dot, and must not contain spaces, newlines, or glob
543+
# characters (* and ?).
542544
# (default: coder, type: string)
543545
workspaceHostnameSuffix: coder
544546
# These SSH config options will override the default SSH config options. Provide
545-
# options in "key=value" or "key value" format separated by commas.Using this
546-
# incorrectly can break SSH to your deployment, use cautiously.
547+
# options in "key=value" or "key value" format separated by commas. Using this
548+
# incorrectly can break SSH to your deployment, use cautiously. The following
549+
# options are not allowed: Host, Match, Include, ProxyCommand, ProxyJump,
550+
# LocalCommand, PermitLocalCommand, RemoteCommand, KnownHostsCommand,
551+
# PKCS11Provider, SecurityKeyProvider, SmartcardDevice, XAuthLocation. Option
552+
# values must not contain newline, carriage return, or NUL characters.
547553
# (default: <unset>, type: string-array)
548554
sshConfigOptions: []
549555
# The upgrade message to display to users when a client/server mismatch is

0 commit comments

Comments
 (0)