add tests for config.go and resolve a few config bugs

Author: cyrilst

cmd/sshproxy/sshproxy.go

@@ -296,7 +296,7 @@ func mainExitCode() int {
 			}
 		}
 	} else {
-		if config.Etcd.Mandatory {
+		if config.Etcd.Mandatory.(bool) {
 			log.Fatal("Etcd is mandatory but unavailable")
 		}
 	}

config/sshproxy.yaml

@@ -204,10 +204,7 @@
 #          ENV1: /tmp/env
 #      ssh:
 #          args: ["-vvv", "-Y"]
-#      # If routes are specified, each specified route is fully overridden, not merged.
-#      routes:
-#          default:
-#              dest: [hostx]
+#      dest: [hostx]
 #    - match:
 #        - groups: [bar]
 #          groups: [baz]

pkg/utils/config.go

@@ -16,6 +16,7 @@ import (
 	"os"
 	"regexp"
 	"slices"
+	"sort"
 	"strings"
 	"time"
 
@@ -82,13 +83,15 @@ type sshConfig struct {
 	Args []string `yaml:",flow,omitempty"`
 }
 
+// We use interface{} instead of real type to check if the option was specified
+// or not.
 type etcdConfig struct {
 	Endpoints []string      `yaml:",flow"`
 	TLS       etcdTLSConfig `yaml:",omitempty"`
 	Username  string        `yaml:",omitempty"`
 	Password  string        `yaml:",omitempty"`
 	KeyTTL    int64         `yaml:",omitempty"`
-	Mandatory bool          `yaml:",omitempty"`
+	Mandatory interface{}   `yaml:",omitempty"`
 }
 
 type etcdTLSConfig struct {
@@ -108,12 +111,12 @@ type subConfig struct {
 	Dump                  interface{}                        `yaml:",omitempty"`
 	DumpLimitSize         interface{}                        `yaml:"dump_limit_size,omitempty"`
 	DumpLimitWindow       interface{}                        `yaml:"dump_limit_window,omitempty"`
-	Etcd                  interface{}                        `yaml:",omitempty"`
+	Etcd                  etcdConfig                         `yaml:",omitempty"`
 	EtcdStatsInterval     interface{}                        `yaml:"etcd_stats_interval,omitempty"`
 	LogStatsInterval      interface{}                        `yaml:"log_stats_interval,omitempty"`
 	BlockingCommand       interface{}                        `yaml:"blocking_command,omitempty"`
 	BgCommand             interface{}                        `yaml:"bg_command,omitempty"`
-	SSH                   interface{}                        `yaml:",omitempty"`
+	SSH                   sshConfig                          `yaml:",omitempty"`
 	TranslateCommands     map[string]*TranslateCommandConfig `yaml:"translate_commands,omitempty"`
 	Environment           map[string]string                  `yaml:",omitempty"`
 	Service               interface{}                        `yaml:",omitempty"`
@@ -143,8 +146,15 @@ func PrintConfig(config *Config, groups map[string]bool) []string {
 	output = append(output, fmt.Sprintf("config.blocking_command = %s", config.BlockingCommand))
 	output = append(output, fmt.Sprintf("config.bg_command = %s", config.BgCommand))
 	output = append(output, fmt.Sprintf("config.ssh = %+v", config.SSH))
-	for k, v := range config.TranslateCommands {
-		output = append(output, fmt.Sprintf("config.TranslateCommands.%s = %+v", k, v))
+	// Internally, we don't care of TranslateCommands's order. But we want to
+	// always display it in the same order
+	keys := make([]string, 0, len(config.TranslateCommands))
+	for k := range config.TranslateCommands {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for _, k := range keys {
+		output = append(output, fmt.Sprintf("config.TranslateCommands.%s = %+v", k, config.TranslateCommands[k]))
 	}
 	output = append(output, fmt.Sprintf("config.environment = %v", config.Environment))
 	output = append(output, fmt.Sprintf("config.service = %s", config.Service))
@@ -168,6 +178,9 @@ func parseSubConfig(config *Config, subconfig *subConfig) error {
 	}
 
 	if subconfig.CheckInterval != nil {
+		if fmt.Sprintf("%T", subconfig.CheckInterval) != "string" {
+			return fmt.Errorf("check_interval: %v is not a string", subconfig.CheckInterval)
+		}
 		var err error
 		config.CheckInterval, err = time.ParseDuration(subconfig.CheckInterval.(string))
 		if err != nil {
@@ -188,18 +201,52 @@ func parseSubConfig(config *Config, subconfig *subConfig) error {
 	}
 
 	if subconfig.DumpLimitWindow != nil {
+		if fmt.Sprintf("%T", subconfig.DumpLimitWindow) != "string" {
+			return fmt.Errorf("dump_limit_window: %v is not a string", subconfig.DumpLimitWindow)
+		}
 		var err error
 		config.DumpLimitWindow, err = time.ParseDuration(subconfig.DumpLimitWindow.(string))
 		if err != nil {
 			return err
 		}
 	}
 
-	if subconfig.Etcd != nil {
-		config.Etcd = subconfig.Etcd.(etcdConfig)
+	if subconfig.Etcd.Endpoints != nil {
+		config.Etcd.Endpoints = subconfig.Etcd.Endpoints
+	}
+
+	if subconfig.Etcd.TLS.CAFile != "" {
+		config.Etcd.TLS.CAFile = subconfig.Etcd.TLS.CAFile
+	}
+
+	if subconfig.Etcd.TLS.KeyFile != "" {
+		config.Etcd.TLS.KeyFile = subconfig.Etcd.TLS.KeyFile
+	}
+
+	if subconfig.Etcd.TLS.CertFile != "" {
+		config.Etcd.TLS.CertFile = subconfig.Etcd.TLS.CertFile
+	}
+
+	if subconfig.Etcd.Username != "" {
+		config.Etcd.Username = subconfig.Etcd.Username
+	}
+
+	if subconfig.Etcd.Password != "" {
+		config.Etcd.Password = subconfig.Etcd.Password
+	}
+
+	if subconfig.Etcd.KeyTTL != 0 {
+		config.Etcd.KeyTTL = subconfig.Etcd.KeyTTL
+	}
+
+	if subconfig.Etcd.Mandatory != nil {
+		config.Etcd.Mandatory = subconfig.Etcd.Mandatory
 	}
 
 	if subconfig.EtcdStatsInterval != nil {
+		if fmt.Sprintf("%T", subconfig.EtcdStatsInterval) != "string" {
+			return fmt.Errorf("etcd_stats_interval: %v is not a string", subconfig.EtcdStatsInterval)
+		}
 		var err error
 		config.EtcdStatsInterval, err = time.ParseDuration(subconfig.EtcdStatsInterval.(string))
 		if err != nil {
@@ -208,6 +255,9 @@ func parseSubConfig(config *Config, subconfig *subConfig) error {
 	}
 
 	if subconfig.LogStatsInterval != nil {
+		if fmt.Sprintf("%T", subconfig.LogStatsInterval) != "string" {
+			return fmt.Errorf("log_stats_interval: %v is not a string", subconfig.LogStatsInterval)
+		}
 		var err error
 		config.LogStatsInterval, err = time.ParseDuration(subconfig.LogStatsInterval.(string))
 		if err != nil {
@@ -223,8 +273,12 @@ func parseSubConfig(config *Config, subconfig *subConfig) error {
 		config.BgCommand = subconfig.BgCommand.(string)
 	}
 
-	if subconfig.SSH != nil {
-		config.SSH = subconfig.SSH.(sshConfig)
+	if subconfig.SSH.Exe != "" {
+		config.SSH.Exe = subconfig.SSH.Exe
+	}
+
+	if subconfig.SSH.Args != nil {
+		config.SSH.Args = subconfig.SSH.Args
 	}
 
 	// merge translate_commands
@@ -296,7 +350,14 @@ func LoadAllDestsFromConfig(filename string) ([]string, error) {
 			config.Dest = append(config.Dest, override.Dest...)
 		}
 	}
-	return config.Dest, nil
+	// expand destination nodesets
+	_, nodesetDlclose, nodesetExpand := nodesets.InitExpander()
+	defer nodesetDlclose()
+	dsts, err := nodesetExpand(strings.Join(config.Dest, ","))
+	if err != nil {
+		return nil, fmt.Errorf("invalid nodeset: %s", err)
+	}
+	return dsts, nil
 }
 
 // LoadConfig load configuration file and adapt it according to specified user/group/sshdHostPort.

pkg/utils/config_test.go

@@ -0,0 +1,190 @@
+// Copyright 2015-2025 CEA/DAM/DIF
+//  Author: Arnaud Guignard <[email protected]>
+//  Contributor: Cyril Servant <[email protected]>
+//
+// This software is governed by the CeCILL-B license under French law and
+// abiding by the rules of distribution of free software.  You can  use,
+// modify and/ or redistribute the software under the terms of the CeCILL-B
+// license as circulated by CEA, CNRS and INRIA at the following URL
+// "http://www.cecill.info".
+
+package utils
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+	"time"
+)
+
+var start time.Time = time.Now()
+
+var loadConfigTests = []struct {
+	filename, username string
+	want               []string
+	err                string
+}{
+	{"nonexistingfile.yaml", "alice", []string{}, "open nonexistingfile.yaml: no such file or directory"},
+	{"../../test/configEmpty.yaml", "alice", []string{}, "no destination defined for service 'default'"},
+	{"../../test/configInvalid.yaml", "alice", []string{}, "yaml: found character that cannot start any token"},
+	{"../../test/configCheckIntervalError.yaml", "alice", []string{}, `time: invalid duration "not a duration"`},
+	{"../../test/configCheckIntervalNotString.yaml", "alice", []string{}, "check_interval: 10 is not a string"},
+	{"../../test/configDumpLimitWindowError.yaml", "alice", []string{}, `time: invalid duration "not a duration"`},
+	{"../../test/configDumpLimitWindowNotString.yaml", "alice", []string{}, "dump_limit_window: 10 is not a string"},
+	{"../../test/configEtcdStatsIntervalError.yaml", "alice", []string{}, `time: invalid duration "not a duration"`},
+	{"../../test/configEtcdStatsIntervalNotString.yaml", "alice", []string{}, "etcd_stats_interval: 10 is not a string"},
+	{"../../test/configLogStatsIntervalError.yaml", "alice", []string{}, `time: invalid duration "not a duration"`},
+	{"../../test/configLogStatsIntervalNotString.yaml", "alice", []string{}, "log_stats_interval: 10 is not a string"},
+	{"../../test/configMatchSourceError.yaml", "alice", []string{}, "source: invalid address: address 127.0.0.1:abcd: invalid port"},
+	{"../../test/configRouteSelectError.yaml", "alice", []string{}, "invalid value for `route_select` option of service 'default': notarouteselect"},
+	{"../../test/configModeError.yaml", "alice", []string{}, "invalid value for `mode` option of service 'default': notamode"},
+	// yes, "cannont" is an upstream typo
+	{"../../test/configDestNodesetError.yaml", "alice", []string{}, "invalid nodeset for service 'default': cannont convert ending range to integer a - rangeset parse error"},
+	{"../../test/configDestError.yaml", "alice", []string{}, "invalid destination '127.0.0.1:abcd' for service 'default': address 127.0.0.1:abcd: invalid port"},
+	{"../../test/configDefault.yaml", "alice", []string{
+		"libnodeset.so not found, falling back to iskylite's implementation",
+		"groups = map[bar:true foo:true]",
+		"config.debug = false",
+		"config.log = ",
+		"config.check_interval = 0s",
+		"config.error_banner = ",
+		"config.dump = ",
+		"config.dump_limit_size = 0",
+		"config.dump_limit_window = 0s",
+		"config.etcd = {Endpoints:[] TLS:{CAFile: KeyFile: CertFile:} Username: Password: KeyTTL:0 Mandatory:<nil>}",
+		"config.etcd_stats_interval = 0s",
+		"config.log_stats_interval = 0s",
+		"config.blocking_command = ",
+		"config.bg_command = ",
+		"config.ssh = {Exe:ssh Args:[-q -Y]}",
+		"config.environment = map[]",
+		"config.service = default",
+		"config.dest = [127.0.0.1:22]",
+		"config.route_select = ordered",
+		"config.mode = sticky",
+		"config.force_command = ",
+		"config.command_must_match = false",
+		"config.etcd_keyttl = 0",
+		"config.max_connections_per_user = 0",
+	}, ""},
+	{"../../test/config.yaml", "alice", []string{
+		"libnodeset.so not found, falling back to iskylite's implementation",
+		"groups = map[bar:true foo:true]",
+		"config.debug = true",
+		"config.log = /tmp/sshproxy-foo/alice.log",
+		"config.check_interval = 2m0s",
+		"config.error_banner = an other error banner",
+		"config.dump = /tmp/sshproxy-alice-" + start.Format(time.RFC3339Nano) + ".dump",
+		"config.dump_limit_size = 20",
+		"config.dump_limit_window = 3m0s",
+		"config.etcd = {Endpoints:[host2] TLS:{CAFile:ca2.pem KeyFile:cert2.key CertFile:cert2.pem} Username:test2 Password:pass2 KeyTTL:2 Mandatory:false}",
+		"config.etcd_stats_interval = 4m0s",
+		"config.log_stats_interval = 5m0s",
+		"config.blocking_command = /a/blocking/command",
+		"config.bg_command = /a/background/command",
+		"config.ssh = {Exe:sshhhhh Args:[-vvv -Y]}",
+		"config.TranslateCommands.acommand = &{SSHArgs:[] Command:something DisableDump:true}",
+		"config.TranslateCommands.internal-sftp = &{SSHArgs:[-s] Command:anothercommand DisableDump:false}",
+		"config.environment = map[ENV1:/tmp/env XAUTHORITY:/dev/shm/.Xauthority_alice]",
+		"config.service = service5",
+		"config.dest = [server1:12345]",
+		"config.route_select = bandwidth",
+		"config.mode = balanced",
+		"config.force_command = acommand",
+		"config.command_must_match = false",
+		"config.etcd_keyttl = 0",
+		"config.max_connections_per_user = 0",
+	}, ""},
+	{"../../test/config.yaml", "notalice", []string{
+		"libnodeset.so not found, falling back to iskylite's implementation",
+		"groups = map[bar:true foo:true]",
+		"config.debug = true",
+		"config.log = /var/log/sshproxy/notalice.log",
+		"config.check_interval = 2m30s",
+		"config.error_banner = an error banner",
+		"config.dump = /var/lib/sshproxy/dumps/notalice/" + start.Format(time.RFC3339Nano) + "-abcd.dump",
+		"config.dump_limit_size = 10",
+		"config.dump_limit_window = 2m31s",
+		"config.etcd = {Endpoints:[host1:port1 host2:port2] TLS:{CAFile:ca.pem KeyFile:cert.key CertFile:cert.pem} Username:test Password:pass KeyTTL:5 Mandatory:true}",
+		"config.etcd_stats_interval = 2m33s",
+		"config.log_stats_interval = 2m32s",
+		"config.blocking_command = ",
+		"config.bg_command = ",
+		"config.ssh = {Exe:ssh Args:[-q -Y]}",
+		"config.TranslateCommands.internal-sftp = &{SSHArgs:[-oForwardX11=no -oForwardAgent=no -oPermitLocalCommand=no -oClearAllForwardings=yes -oProtocol=2 -s] Command:sftp DisableDump:true}",
+		"config.environment = map[XAUTHORITY:/dev/shm/.Xauthority_notalice]",
+		"config.service = default",
+		"config.dest = [host5:4222]",
+		"config.route_select = ordered",
+		"config.mode = sticky",
+		"config.force_command = ",
+		"config.command_must_match = true",
+		"config.etcd_keyttl = 3600",
+		"config.max_connections_per_user = 50",
+	}, ""},
+}
+
+func TestLoadConfig(t *testing.T) {
+	sid := "abcd"
+	groups := map[string]bool{"foo": true, "bar": true}
+	sshdHostPort := "127.0.0.1:22"
+	for _, tt := range loadConfigTests {
+		cachedConfig = Config{}
+		config, err := LoadConfig(tt.filename, tt.username, sid, start, groups, sshdHostPort)
+		if err == nil && tt.err != "" {
+			t.Errorf("got no error, want %s", tt.err)
+		} else if err != nil && err.Error() != tt.err {
+			t.Errorf("ERROR: %s, want %s", err, tt.err)
+		} else if err == nil && !reflect.DeepEqual(PrintConfig(config, groups), tt.want) {
+			t.Errorf("want:\n%v\ngot:\n%v", tt.want, PrintConfig(config, groups))
+		} else if err == nil {
+			cachedConfig, err := LoadConfig(tt.filename, tt.username, sid, start, groups, sshdHostPort)
+			if err != nil {
+				t.Errorf("ERROR: %s", err)
+			} else if config != cachedConfig {
+				t.Error("config and cachedConfig should be the same")
+			}
+		}
+	}
+}
+
+var loadAllDestsFromConfigTests = []struct {
+	filename string
+	want     []string
+	err      string
+}{
+	{"nonexistingfile.yaml", []string{}, "open nonexistingfile.yaml: no such file or directory"},
+	{"../../test/configInvalid.yaml", []string{}, "yaml: found character that cannot start any token"},
+	{"../../test/configDestNodesetError.yaml", []string{}, "invalid nodeset: cannont convert ending range to integer a - rangeset parse error"},
+	{"../../test/configDefault.yaml", []string{"127.0.0.1"}, ""},
+	{"../../test/config.yaml", []string{
+		"192.168.0.1",
+		"192.168.0.2",
+		"192.168.0.3",
+		"192.168.0.4",
+		"192.168.0.5",
+		"192.168.0.6",
+		"192.168.0.7",
+		"192.168.0.8",
+		"192.168.0.9",
+		"192.168.0.10",
+		"host5:4222",
+		"server1:12345",
+	}, ""},
+}
+
+func TestLoadAllDestsFromConfig(t *testing.T) {
+	for _, tt := range loadAllDestsFromConfigTests {
+		config, err := LoadAllDestsFromConfig(tt.filename)
+		if err != nil {
+			sort.Strings(config)
+		}
+		if err == nil && tt.err != "" {
+			t.Errorf("got no error, want %s", tt.err)
+		} else if err != nil && err.Error() != tt.err {
+			t.Errorf("ERROR: %s, want %s", err, tt.err)
+		} else if err == nil && !reflect.DeepEqual(config, tt.want) {
+			t.Errorf("want:\n%v\ngot:\n%v", tt.want, config)
+		}
+	}
+}