Skip to content

Commit 2384e9c

Browse files
EmyrkEmyrk
authored
fix: Do not try and write to malformed ssh configs (coder#6000)
Abort writing the ssh config if it has: - Out of order coder headers - More than 1 of each coder header - Missing 1 or the other coder header
1 parent c16b938 commit 2384e9c

3 files changed

Lines changed: 187 additions & 14 deletions

File tree

‎cli/configssh.go‎

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,11 @@ func configSSH() *cobra.Command {
206206
// Parse the previous configuration only if config-ssh
207207
// has been run previously.
208208
var lastConfig *sshConfigOptions
209-
if section, ok := sshConfigGetCoderSection(configRaw); ok {
209+
section, ok, err := sshConfigGetCoderSection(configRaw)
210+
if err != nil {
211+
return err
212+
}
213+
if ok {
210214
c := sshConfigParseLastOptions(bytes.NewReader(section))
211215
lastConfig = &c
212216
}
@@ -249,7 +253,10 @@ func configSSH() *cobra.Command {
249253
configModified := configRaw
250254

251255
buf := &bytes.Buffer{}
252-
before, after := sshConfigSplitOnCoderSection(configModified)
256+
before, _, after, err := sshConfigSplitOnCoderSection(configModified)
257+
if err != nil {
258+
return err
259+
}
253260
// Write the first half of the users config file to buf.
254261
_, _ = buf.Write(before)
255262

@@ -418,22 +425,39 @@ func sshConfigParseLastOptions(r io.Reader) (o sshConfigOptions) {
418425
return o
419426
}
420427

421-
func sshConfigGetCoderSection(data []byte) (section []byte, ok bool) {
422-
startIndex := bytes.Index(data, []byte(sshStartToken))
423-
endIndex := bytes.Index(data, []byte(sshEndToken))
424-
if startIndex != -1 && endIndex != -1 {
425-
return data[startIndex : endIndex+len(sshEndToken)], true
428+
// sshConfigGetCoderSection is a helper function that only returns the coder
429+
// section of the SSH config and a boolean if it exists.
430+
func sshConfigGetCoderSection(data []byte) (section []byte, ok bool, err error) {
431+
_, section, _, err = sshConfigSplitOnCoderSection(data)
432+
if err != nil {
433+
return nil, false, err
426434
}
427-
return nil, false
435+
436+
return section, len(section) > 0, nil
428437
}
429438

430-
// sshConfigSplitOnCoderSection splits the SSH config into two sections,
431-
// before contains the lines before sshStartToken and after contains the
432-
// lines after sshEndToken.
433-
func sshConfigSplitOnCoderSection(data []byte) (before, after []byte) {
439+
// sshConfigSplitOnCoderSection splits the SSH config into 3 sections.
440+
// All lines before sshStartToken, the coder section, and all lines after
441+
// sshEndToken.
442+
func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after []byte, err error) {
443+
startCount := bytes.Count(data, []byte(sshStartToken))
444+
endCount := bytes.Count(data, []byte(sshEndToken))
445+
if startCount > 1 || endCount > 1 {
446+
return nil, nil, nil, xerrors.New("Malformed config: ssh config has multiple coder sections, please remove all but one")
447+
}
448+
434449
startIndex := bytes.Index(data, []byte(sshStartToken))
435450
endIndex := bytes.Index(data, []byte(sshEndToken))
451+
if startIndex == -1 && endIndex != -1 {
452+
return nil, nil, nil, xerrors.New("Malformed config: ssh config has end header, but missing start header")
453+
}
454+
if startIndex != -1 && endIndex == -1 {
455+
return nil, nil, nil, xerrors.New("Malformed config: ssh config has start header, but missing end header")
456+
}
436457
if startIndex != -1 && endIndex != -1 {
458+
if startIndex > endIndex {
459+
return nil, nil, nil, xerrors.New("Malformed config: ssh config has coder section, but it is malformed and the END header is before the START header")
460+
}
437461
// We use -1 and +1 here to also include the preceding
438462
// and trailing newline, where applicable.
439463
start := startIndex
@@ -444,10 +468,10 @@ func sshConfigSplitOnCoderSection(data []byte) (before, after []byte) {
444468
if end < len(data) {
445469
end++
446470
}
447-
return data[:start], data[end:]
471+
return data[:start], data[start:end], data[end:], nil
448472
}
449473

450-
return data, nil
474+
return data, nil, nil, nil
451475
}
452476

453477
// writeWithTempFileAndMove writes to a temporary file in the same

‎cli/configssh_internal_test.go‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,125 @@ import (
1111
"github.com/stretchr/testify/require"
1212
)
1313

14+
func Test_sshConfigSplitOnCoderSection(t *testing.T) {
15+
t.Parallel()
16+
17+
testCases := []struct {
18+
Name string
19+
Input string
20+
Before string
21+
Section string
22+
After string
23+
Err bool
24+
}{
25+
{
26+
Name: "Empty",
27+
Input: "",
28+
Before: "",
29+
Section: "",
30+
After: "",
31+
Err: false,
32+
},
33+
{
34+
Name: "JustSection",
35+
Input: strings.Join([]string{sshStartToken, sshEndToken}, "\n"),
36+
Before: "",
37+
Section: strings.Join([]string{sshStartToken, sshEndToken}, "\n"),
38+
After: "",
39+
Err: false,
40+
},
41+
{
42+
Name: "NoSection",
43+
Input: strings.Join([]string{"# Some content"}, "\n"),
44+
Before: "# Some content",
45+
Section: "",
46+
After: "",
47+
Err: false,
48+
},
49+
{
50+
Name: "Normal",
51+
Input: strings.Join([]string{
52+
"# Content before the section",
53+
sshStartToken,
54+
sshEndToken,
55+
"# Content after the section",
56+
}, "\n"),
57+
Before: "# Content before the section",
58+
Section: strings.Join([]string{"", sshStartToken, sshEndToken, ""}, "\n"),
59+
After: "# Content after the section",
60+
Err: false,
61+
},
62+
{
63+
Name: "OutOfOrder",
64+
Input: strings.Join([]string{
65+
"# Content before the section",
66+
sshEndToken,
67+
sshStartToken,
68+
"# Content after the section",
69+
}, "\n"),
70+
Err: true,
71+
},
72+
{
73+
Name: "MissingStart",
74+
Input: strings.Join([]string{
75+
"# Content before the section",
76+
sshEndToken,
77+
"# Content after the section",
78+
}, "\n"),
79+
Err: true,
80+
},
81+
{
82+
Name: "MissingEnd",
83+
Input: strings.Join([]string{
84+
"# Content before the section",
85+
sshEndToken,
86+
"# Content after the section",
87+
}, "\n"),
88+
Err: true,
89+
},
90+
{
91+
Name: "ExtraStart",
92+
Input: strings.Join([]string{
93+
"# Content before the section",
94+
sshStartToken,
95+
sshEndToken,
96+
sshStartToken,
97+
"# Content after the section",
98+
}, "\n"),
99+
Err: true,
100+
},
101+
{
102+
Name: "ExtraEnd",
103+
Input: strings.Join([]string{
104+
"# Content before the section",
105+
sshStartToken,
106+
sshEndToken,
107+
sshEndToken,
108+
"# Content after the section",
109+
}, "\n"),
110+
Err: true,
111+
},
112+
}
113+
114+
for _, tc := range testCases {
115+
tc := tc
116+
t.Run(tc.Name, func(t *testing.T) {
117+
t.Parallel()
118+
119+
before, section, after, err := sshConfigSplitOnCoderSection([]byte(tc.Input))
120+
if tc.Err {
121+
require.Error(t, err)
122+
return
123+
}
124+
125+
require.NoError(t, err)
126+
require.Equal(t, tc.Before, string(before), "before")
127+
require.Equal(t, tc.Section, string(section), "section")
128+
require.Equal(t, tc.After, string(after), "after")
129+
})
130+
}
131+
}
132+
14133
// This test tries to mimic the behavior of OpenSSH
15134
// when executing e.g. a ProxyCommand.
16135
func Test_sshConfigExecEscape(t *testing.T) {

‎cli/configssh_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,36 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
529529
"--yes",
530530
},
531531
},
532+
{
533+
name: "Start/End out of order",
534+
matches: []match{
535+
//{match: "Continue?", write: "yes"},
536+
},
537+
writeConfig: writeConfig{
538+
ssh: strings.Join([]string{
539+
"# Content before coder block",
540+
headerEnd,
541+
headerStart,
542+
"# Content after coder block",
543+
}, "\n"),
544+
},
545+
wantErr: true,
546+
},
547+
{
548+
name: "Multiple sections",
549+
matches: []match{
550+
//{match: "Continue?", write: "yes"},
551+
},
552+
writeConfig: writeConfig{
553+
ssh: strings.Join([]string{
554+
headerStart,
555+
headerEnd,
556+
headerStart,
557+
headerEnd,
558+
}, "\n"),
559+
},
560+
wantErr: true,
561+
},
532562
}
533563
for _, tt := range tests {
534564
tt := tt

0 commit comments

Comments
 (0)