From 62ada2136bf97402cfefbca43a19d1980b6cf1b9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 8 Jul 2024 18:57:46 +0300 Subject: [PATCH 1/3] fix(cli): do not overwrite repeated SSH options in config-ssh Fixes #11593 --- cli/configssh.go | 57 +++++++++++++++++++------------------------ cli/configssh_test.go | 15 +++++++++++- 2 files changed, 39 insertions(+), 33 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 26465bf75fe83..82ce525c5b640 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -54,6 +54,7 @@ type sshConfigOptions struct { disableAutostart bool header []string headerCommand string + removedKeys map[string]bool } // addOptions expects options in the form of "option=value" or "option value". @@ -74,30 +75,19 @@ func (o *sshConfigOptions) addOption(option string) error { if err != nil { return err } - for i, existing := range o.sshOptions { - // Override existing option if they share the same key. - // This is case-insensitive. Parsing each time might be a little slow, - // but it is ok. - existingKey, _, err := codersdk.ParseSSHConfigOption(existing) - if err != nil { - // Don't mess with original values if there is an error. - // This could have come from the user's manual edits. - continue - } - if strings.EqualFold(existingKey, key) { - if value == "" { - // Delete existing option. - o.sshOptions = append(o.sshOptions[:i], o.sshOptions[i+1:]...) - } else { - // Override existing option. - o.sshOptions[i] = option - } - return nil - } + if o.removedKeys != nil && o.removedKeys[key] { + // Key marked as removed, skip. + return nil } - // Only append the option if it is not empty. + // Only append the option if it is not empty + // (we interpret empty as removal). if value != "" { o.sshOptions = append(o.sshOptions, option) + } else { + if o.removedKeys == nil { + o.removedKeys = make(map[string]bool) + } + o.removedKeys[key] = true } return nil } @@ -440,13 +430,17 @@ func (r *RootCmd) configSSH() *serpent.Command { configOptions := sshConfigOpts configOptions.sshOptions = nil - // Add standard options. - err := configOptions.addOptions(defaultOptions...) - if err != nil { - return err + // User options first (SSH only uses the first + // option unless it can be given multiple times) + for _, opt := range sshConfigOpts.sshOptions { + err := configOptions.addOptions(opt) + if err != nil { + return xerrors.Errorf("add flag config option %q: %w", opt, err) + } } - // Override with deployment options + // Deployment options second, allow them to + // override standard options. for k, v := range coderdConfig.SSHConfigOptions { opt := fmt.Sprintf("%s %s", k, v) err := configOptions.addOptions(opt) @@ -454,12 +448,11 @@ func (r *RootCmd) configSSH() *serpent.Command { return xerrors.Errorf("add coderd config option %q: %w", opt, err) } } - // Override with flag options - for _, opt := range sshConfigOpts.sshOptions { - err := configOptions.addOptions(opt) - if err != nil { - return xerrors.Errorf("add flag config option %q: %w", opt, err) - } + + // Finally, add the standard options. + err := configOptions.addOptions(defaultOptions...) + if err != nil { + return err } hostBlock := []string{ diff --git a/cli/configssh_test.go b/cli/configssh_test.go index f1be8abe8b4b9..81eceb1b8c971 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -65,7 +65,7 @@ func TestConfigSSH(t *testing.T) { const hostname = "test-coder." const expectedKey = "ConnectionAttempts" - const removeKey = "ConnectionTimeout" + const removeKey = "ConnectTimeout" client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ ConfigSSH: codersdk.SSHConfigResponse{ HostnamePrefix: hostname, @@ -620,6 +620,19 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { regexMatch: `ProxyCommand .* --header-command "printf h1=v1 h2='v2'" ssh`, }, }, + { + name: "Multiple remote forwards", + args: []string{ + "--yes", + "--ssh-option", "RemoteForward 2222 192.168.11.1:2222", + "--ssh-option", "RemoteForward 2223 192.168.11.1:2223", + }, + wantErr: false, + hasAgent: true, + wantConfig: wantConfig{ + regexMatch: "RemoteForward 2222 192.168.11.1:2222.*\n.*RemoteForward 2223 192.168.11.1:2223", + }, + }, } for _, tt := range tests { tt := tt From 8e131ebc5eb0eae380e732c10bd5fd2ac940a704 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 8 Jul 2024 19:11:18 +0300 Subject: [PATCH 2/3] fix test and case insensitivity --- cli/configssh.go | 5 +++-- cli/configssh_internal_test.go | 8 ++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 82ce525c5b640..76b9332a69659 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -75,7 +75,8 @@ func (o *sshConfigOptions) addOption(option string) error { if err != nil { return err } - if o.removedKeys != nil && o.removedKeys[key] { + lowerKey := strings.ToLower(key) + if o.removedKeys != nil && o.removedKeys[lowerKey] { // Key marked as removed, skip. return nil } @@ -87,7 +88,7 @@ func (o *sshConfigOptions) addOption(option string) error { if o.removedKeys == nil { o.removedKeys = make(map[string]bool) } - o.removedKeys[key] = true + o.removedKeys[lowerKey] = true } return nil } diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index 732452a761447..eed5788778e20 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -272,24 +272,25 @@ func Test_sshConfigOptions_addOption(t *testing.T) { }, }, { - Name: "Replace", + Name: "AddTwo", Start: []string{ "foo bar", }, Add: []string{"Foo baz"}, Expect: []string{ + "foo bar", "Foo baz", }, }, { - Name: "AddAndReplace", + Name: "AddAndRemove", Start: []string{ - "a b", "foo bar", "buzz bazz", }, Add: []string{ "b c", + "a=", // Remove all entries that start with "a", i.e. next line. "A hello", "hello world", }, @@ -297,7 +298,6 @@ func Test_sshConfigOptions_addOption(t *testing.T) { "foo bar", "buzz bazz", "b c", - "A hello", "hello world", }, }, From bec1f01e3a5bb06958f357f95686507529cdf879 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 8 Jul 2024 19:15:22 +0300 Subject: [PATCH 3/3] improve comment --- cli/configssh_internal_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index eed5788778e20..16c950af0fd02 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -290,7 +290,7 @@ func Test_sshConfigOptions_addOption(t *testing.T) { }, Add: []string{ "b c", - "a=", // Remove all entries that start with "a", i.e. next line. + "a ", // Empty value, means remove all following entries that start with "a", i.e. next line. "A hello", "hello world", },