Skip to content

Commit 320e549

Browse files
jdomeracki-coderjohnstcnCopilot
authored
fix!: validate HostnameSuffix and SSHConfigOptions' (#26154) (#26226)
Backport of #26154 to `release/2.29`. <details> <summary>Backport notes (cherry-pick conflict resolution)</summary> `codersdk/deployment.go` conflicted because `release/2.29` lacks the AI Gateway options that surround the `workspaceHostnameSuffix` option on `main`. Resolved by keeping the branch's existing structure and applying only the upstream change to that region: the updated `workspaceHostnameSuffix` description. The other four hunks (the `unicode` import, the `ParseSSHConfigOption` rewrite plus `Validate*` helpers, the SSH config options description, and `SSHConfigResponse.Validate`) applied cleanly. Verified on the branch: `go build ./codersdk/... ./cli/... ./enterprise/cli/...`, the new `codersdk` validation tests, and golden/docs regeneration (`cli`/`enterprise/cli` `--help` and `server-config.yaml` golden files plus `docs/reference/cli/server.md`) all matched the cherry-picked content. Net diff is identical to the original PR (11 files, +681/-39). </details> _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 77896dd commit 320e549

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
@@ -584,6 +584,10 @@ func mergeSSHOptions(
584584
) (
585585
sshConfigOptions, error,
586586
) {
587+
if err := coderd.Validate(); err != nil {
588+
return sshConfigOptions{}, xerrors.Errorf("invalid ssh config from coderd: %w", err)
589+
}
590+
587591
// Write agent configuration.
588592
defaultOptions := []string{
589593
"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
@@ -410,6 +410,19 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
410410
logger.Debug(ctx, "tracing closed", slog.Error(traceCloseErr))
411411
}()
412412

413+
configSSHOptions, err := vals.SSHConfig.ParseOptions()
414+
if err != nil {
415+
return xerrors.Errorf("parse ssh config options %q: %w", vals.SSHConfig.SSHConfigOptions.String(), err)
416+
}
417+
sshConfigResponse := codersdk.SSHConfigResponse{
418+
HostnamePrefix: vals.SSHConfig.DeploymentName.String(),
419+
HostnameSuffix: vals.WorkspaceHostnameSuffix.String(),
420+
SSHConfigOptions: configSSHOptions,
421+
}
422+
if err := sshConfigResponse.Validate(); err != nil {
423+
return xerrors.Errorf("invalid ssh config: %w", err)
424+
}
425+
413426
httpServers, err := ConfigureHTTPServers(logger, inv, vals)
414427
if err != nil {
415428
return xerrors.Errorf("configure http(s): %w", err)
@@ -627,20 +640,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
627640
return xerrors.Errorf("parse real ip config: %w", err)
628641
}
629642

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

‎cli/server_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1793,6 +1793,56 @@ func TestServer(t *testing.T) {
17931793
})
17941794
}
17951795

1796+
// TestServer_InvalidSSHDeploymentConfig checks that unsafe SSH config flags are
1797+
// rejected at startup, before any database connection, so these invocations
1798+
// fail fast.
1799+
func TestServer_InvalidSSHDeploymentConfig(t *testing.T) {
1800+
t.Parallel()
1801+
1802+
testCases := []struct {
1803+
name string
1804+
flag string
1805+
wantErr string
1806+
}{
1807+
{
1808+
name: "HostnameSuffixLeadingDot",
1809+
flag: "--workspace-hostname-suffix=.coder",
1810+
wantErr: "workspace hostname suffix",
1811+
},
1812+
{
1813+
name: "HostnameSuffixNewline",
1814+
flag: "--workspace-hostname-suffix=coder\nHost *",
1815+
wantErr: "workspace hostname suffix",
1816+
},
1817+
{
1818+
name: "HostnamePrefixNewline",
1819+
flag: "--ssh-hostname-prefix=coder.\nHost *",
1820+
wantErr: "workspace hostname prefix",
1821+
},
1822+
{
1823+
name: "SSHOptionUnparseable",
1824+
flag: "--ssh-config-options=NoSeparatorOption",
1825+
wantErr: "parse ssh config options",
1826+
},
1827+
{
1828+
name: "SSHOptionDisallowedKey",
1829+
flag: "--ssh-config-options=ProxyCommand=ssh -W %h:%p bastion",
1830+
wantErr: `ssh config option "ProxyCommand" is not allowed`,
1831+
},
1832+
}
1833+
1834+
for _, tc := range testCases {
1835+
t.Run(tc.name, func(t *testing.T) {
1836+
t.Parallel()
1837+
ctx := testutil.Context(t, testutil.WaitShort)
1838+
inv, _ := clitest.New(t, "server", tc.flag)
1839+
err := inv.WithContext(ctx).Run()
1840+
require.Error(t, err)
1841+
require.ErrorContains(t, err, tc.wantErr)
1842+
})
1843+
}
1844+
}
1845+
17961846
//nolint:tparallel,paralleltest // This test sets environment variables.
17971847
func TestServer_Logging_NoParallel(t *testing.T) {
17981848
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

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

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

145149
--ssh-hostname-prefix string, $CODER_SSH_HOSTNAME_PREFIX (default: coder.)
146150
The SSH deployment prefix is used in the Host of the ssh config.
@@ -152,7 +156,8 @@ Clients include the Coder CLI, Coder Desktop, IDE extensions, and the web UI.
152156
--workspace-hostname-suffix string, $CODER_WORKSPACE_HOSTNAME_SUFFIX (default: coder)
153157
Workspace hostnames use this suffix in SSH config and Coder Connect on
154158
Coder Desktop. By default it is coder, resulting in names like
155-
myworkspace.coder.
159+
myworkspace.coder. The suffix must not start with a dot, and must not
160+
contain spaces, newlines, or glob characters (* and ?).
156161

157162
CONFIG OPTIONS:
158163
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
@@ -505,12 +505,18 @@ client:
505505
# (default: coder., type: string)
506506
sshHostnamePrefix: coder.
507507
# Workspace hostnames use this suffix in SSH config and Coder Connect on Coder
508-
# Desktop. By default it is coder, resulting in names like myworkspace.coder.
508+
# Desktop. By default it is coder, resulting in names like myworkspace.coder. The
509+
# suffix must not start with a dot, and must not contain spaces, newlines, or glob
510+
# characters (* and ?).
509511
# (default: coder, type: string)
510512
workspaceHostnameSuffix: coder
511513
# These SSH config options will override the default SSH config options. Provide
512-
# options in "key=value" or "key value" format separated by commas.Using this
513-
# incorrectly can break SSH to your deployment, use cautiously.
514+
# options in "key=value" or "key value" format separated by commas. Using this
515+
# incorrectly can break SSH to your deployment, use cautiously. The following
516+
# options are not allowed: Host, Match, Include, ProxyCommand, ProxyJump,
517+
# LocalCommand, PermitLocalCommand, RemoteCommand, KnownHostsCommand,
518+
# PKCS11Provider, SecurityKeyProvider, SmartcardDevice, XAuthLocation. Option
519+
# values must not contain newline, carriage return, or NUL characters.
514520
# (default: <unset>, type: string-array)
515521
sshConfigOptions: []
516522
# The upgrade message to display to users when a client/server mismatch is

0 commit comments

Comments
 (0)