fix(cli): do not overwrite repeated SSH options in config-ssh (#13819)

mafredri · web-flow · commit 978364bd3e85 · 2024-07-09T09:44:56.000+03:00
Fixes #11593
diff --git 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,20 @@ 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
-		}
+	lowerKey := strings.ToLower(key)
+	if o.removedKeys != nil && o.removedKeys[lowerKey] {
+		// 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[lowerKey] = true
 	}
 	return nil
 }
@@ -440,26 +431,29 @@ 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)
 						if err != nil {
 							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_internal_test.go b/cli/configssh_internal_test.go
@@ -272,32 +272,32 @@ 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 ", // Empty value, means remove all following entries that start with "a", i.e. next line.
 				"A hello",
 				"hello world",
 			},
 			Expect: []string{
 				"foo bar",
 				"buzz bazz",
 				"b c",
-				"A hello",
 				"hello world",
 			},
 		},
diff --git 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
