From d7f59cec053ba2b0d189e5e815dadd7791a57aa8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 28 Nov 2024 12:05:18 +0100 Subject: [PATCH] daemon/config: add basic validation of exec-opt options Validate if options are passed in the right format and if the given option is supported on the current platform. Before this patch, no validation would happen until the daemon was started, and unknown options as well as incorrectly formatted options would be silently ignored on Linux; dockerd --exec-opt =value-only --validate configuration OK dockerd --exec-opt unknown-opt=unknown-value --validate configuration OK dockerd --exec-opt unknown-opt=unknown-value --validate ... INFO[2024-11-28T12:07:44.255942174Z] Daemon has completed initialization INFO[2024-11-28T12:07:44.361412049Z] API listen on /var/run/docker.sock With this patch, exec-opts are included in the validation before the daemon is started/created, and errors are produced when trying to use an option that's either unknown or not supported by the platform; dockerd --exec-opt =value-only --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid exec-opt (=value-only): must be formatted 'opt=value' dockerd --exec-opt isolation=default --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid exec-opt (isolation=default): 'isolation' option is only supported on windows dockerd --exec-opt unknown-opt=unknown-value --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid exec-opt (unknown-opt=unknown-value): unknown option: 'unknown-opt' Signed-off-by: Sebastiaan van Stijn --- daemon/config/config.go | 37 +++++++++++++++++++ daemon/config/config_linux.go | 14 ++++++++ daemon/config/config_test.go | 64 ++++++++++++++++++++++++++++++++- daemon/config/config_windows.go | 15 ++++++++ daemon/daemon_unix.go | 27 ++++++-------- daemon/daemon_windows.go | 33 +++++++---------- 6 files changed, 152 insertions(+), 38 deletions(-) diff --git a/daemon/config/config.go b/daemon/config/config.go index 8c1a56024c..2bafd74659 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -343,6 +343,17 @@ func New() (*Config, error) { return cfg, nil } +// GetExecOpt looks up a user-configured exec-opt. It returns a boolean +// if found, and an error if the configuration has invalid options set. +func (conf *Config) GetExecOpt(name string) (val string, found bool, _ error) { + o, err := parseExecOptions(conf.ExecOptions) + if err != nil { + return "", false, err + } + val, found = o[name] + return val, found, nil +} + // GetConflictFreeLabels validates Labels for conflict // In swarm the duplicates for labels are removed // so we only take same values here, no conflict values @@ -739,10 +750,36 @@ func Validate(config *Config) error { return errors.New(`DEPRECATED: The "api-cors-header" config parameter and the dockerd "--api-cors-header" option have been removed; use a reverse proxy if you need CORS headers`) } + if _, err := parseExecOptions(config.ExecOptions); err != nil { + return err + } + // validate platform-specific settings return validatePlatformConfig(config) } +// parseExecOptions parses the given exec-options into a map. It returns an +// error if the exec-options are formatted incorrectly, or when options are +// used that are not supported on this platform. +// +// TODO(thaJeztah): consider making this more strict: make options case-sensitive and disallow whitespace around "=". +func parseExecOptions(execOptions []string) (map[string]string, error) { + o := make(map[string]string) + for _, keyValue := range execOptions { + k, v, ok := strings.Cut(keyValue, "=") + k = strings.ToLower(strings.TrimSpace(k)) + v = strings.TrimSpace(v) + if !ok || k == "" || v == "" { + return nil, fmt.Errorf("invalid exec-opt (%s): must be formatted 'opt=value'", keyValue) + } + if err := validatePlatformExecOpt(k, v); err != nil { + return nil, fmt.Errorf("invalid exec-opt (%s): %w", keyValue, err) + } + o[k] = v + } + return o, nil +} + // MaskCredentials masks credentials that are in an URL. func MaskCredentials(rawURL string) string { parsedURL, err := url.Parse(rawURL) diff --git a/daemon/config/config_linux.go b/daemon/config/config_linux.go index de2e64de1b..99c2e3d910 100644 --- a/daemon/config/config_linux.go +++ b/daemon/config/config_linux.go @@ -245,6 +245,20 @@ func validatePlatformConfig(conf *Config) error { return verifyDefaultCgroupNsMode(conf.CgroupNamespaceMode) } +// validatePlatformExecOpt validates if the given exec-opt and value are valid +// for the current platform. +func validatePlatformExecOpt(opt, value string) error { + switch opt { + case "isolation": + return fmt.Errorf("option '%s' is only supported on windows", opt) + case "native.cgroupdriver": + // TODO(thaJeztah): add validation that's currently in daemon.verifyCgroupDriver + return nil + default: + return fmt.Errorf("unknown option: '%s'", opt) + } +} + // verifyUserlandProxyConfig verifies if a valid userland-proxy path // is configured if userland-proxy is enabled. func verifyUserlandProxyConfig(conf *Config) error { diff --git a/daemon/config/config_test.go b/daemon/config/config_test.go index 26896382fa..6b3909aa9e 100644 --- a/daemon/config/config_test.go +++ b/daemon/config/config_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "reflect" + "runtime" "strings" "testing" @@ -221,6 +222,7 @@ func TestValidateConfigurationErrors(t *testing.T) { name string field string config *Config + platform string expectedErr string }{ { @@ -352,6 +354,62 @@ func TestValidateConfigurationErrors(t *testing.T) { }, expectedErr: "invalid logging level: foobar", }, + { + name: "exec-opt without value", + config: &Config{ + CommonConfig: CommonConfig{ + ExecOptions: []string{"no-value"}, + }, + }, + expectedErr: "invalid exec-opt (no-value): must be formatted 'opt=value'", + }, + { + name: "exec-opt with empty value", + config: &Config{ + CommonConfig: CommonConfig{ + ExecOptions: []string{"empty-value="}, + }, + }, + expectedErr: "invalid exec-opt (empty-value=): must be formatted 'opt=value'", + }, + { + name: "exec-opt without key", + config: &Config{ + CommonConfig: CommonConfig{ + ExecOptions: []string{"=empty-key"}, + }, + }, + expectedErr: "invalid exec-opt (=empty-key): must be formatted 'opt=value'", + }, + { + name: "exec-opt unknown option", + config: &Config{ + CommonConfig: CommonConfig{ + ExecOptions: []string{"unknown-option=any-value"}, + }, + }, + expectedErr: "invalid exec-opt (unknown-option=any-value): unknown option: 'unknown-option'", + }, + { + name: "exec-opt invalid on linux", + config: &Config{ + CommonConfig: CommonConfig{ + ExecOptions: []string{"isolation=default"}, + }, + }, + platform: "linux", + expectedErr: "invalid exec-opt (isolation=default): option 'isolation' is only supported on windows", + }, + { + name: "exec-opt invalid on windows", + config: &Config{ + CommonConfig: CommonConfig{ + ExecOptions: []string{"native.cgroupdriver=systemd"}, + }, + }, + platform: "windows", + expectedErr: "invalid exec-opt (native.cgroupdriver=systemd): option 'native.cgroupdriver' is only supported on linux", + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -363,7 +421,11 @@ func TestValidateConfigurationErrors(t *testing.T) { assert.Check(t, mergo.Merge(cfg, tc.config, mergo.WithOverride)) } err = Validate(cfg) - assert.Error(t, err, tc.expectedErr) + if tc.platform != "" && tc.platform != runtime.GOOS { + assert.NilError(t, err) + } else { + assert.Error(t, err, tc.expectedErr) + } }) } } diff --git a/daemon/config/config_windows.go b/daemon/config/config_windows.go index 15295b7562..43e22f7d8d 100644 --- a/daemon/config/config_windows.go +++ b/daemon/config/config_windows.go @@ -2,6 +2,7 @@ package config // import "github.com/docker/docker/daemon/config" import ( "context" + "fmt" "os" "path/filepath" @@ -89,3 +90,17 @@ func validatePlatformConfig(conf *Config) error { } return nil } + +// validatePlatformExecOpt validates if the given exec-opt and value are valid +// for the current platform. +func validatePlatformExecOpt(opt, value string) error { + switch opt { + case "isolation": + // TODO(thaJeztah): add validation that's currently in Daemon.setDefaultIsolation() + return nil + case "native.cgroupdriver": + return fmt.Errorf("option '%s' is only supported on linux", opt) + default: + return fmt.Errorf("unknown option: '%s'", opt) + } +} diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 8a323f9940..1e7e3fde43 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -585,32 +585,25 @@ func cgroupDriver(cfg *config.Config) string { return cgroupFsDriver } -// getCD gets the raw value of the native.cgroupdriver option, if set. -func getCD(config *config.Config) string { - for _, option := range config.ExecOptions { - key, val, ok := strings.Cut(option, "=") - if ok && strings.EqualFold(strings.TrimSpace(key), "native.cgroupdriver") { - return strings.TrimSpace(val) - } - } - return "" -} - // verifyCgroupDriver validates native.cgroupdriver func verifyCgroupDriver(config *config.Config) error { - cd := getCD(config) - if cd == "" || cd == cgroupFsDriver || cd == cgroupSystemdDriver { + cd, _, err := config.GetExecOpt("native.cgroupdriver") + if err != nil { + return err + } + switch cd { + case "", cgroupFsDriver, cgroupSystemdDriver: return nil - } - if cd == cgroupNoneDriver { + case cgroupNoneDriver: return fmt.Errorf("native.cgroupdriver option %s is internally used and cannot be specified manually", cd) + default: + return fmt.Errorf("native.cgroupdriver option %s not supported", cd) } - return fmt.Errorf("native.cgroupdriver option %s not supported", cd) } // UsingSystemd returns true if cli option includes native.cgroupdriver=systemd func UsingSystemd(config *config.Config) bool { - cd := getCD(config) + cd, _, _ := config.GetExecOpt("native.cgroupdriver") if cd == cgroupSystemdDriver { return true diff --git a/daemon/daemon_windows.go b/daemon/daemon_windows.go index db5fb0daf5..b0379fcf0f 100644 --- a/daemon/daemon_windows.go +++ b/daemon/daemon_windows.go @@ -25,7 +25,6 @@ import ( "github.com/docker/docker/libnetwork/options" "github.com/docker/docker/libnetwork/scope" "github.com/docker/docker/pkg/idtools" - "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/parsers/operatingsystem" "github.com/docker/docker/pkg/sysinfo" "github.com/docker/docker/pkg/system" @@ -523,26 +522,20 @@ func (daemon *Daemon) setDefaultIsolation(config *config.Config) error { } else { daemon.defaultIsolation = containertypes.IsolationProcess } - for _, option := range config.ExecOptions { - key, val, err := parsers.ParseKeyValueOpt(option) - if err != nil { - return err + val, ok, err := config.GetExecOpt("isolation") + if err != nil { + return err + } + if ok { + isolation := containertypes.Isolation(strings.ToLower(val)) + if !isolation.IsValid() { + return fmt.Errorf("invalid exec-opt value for 'isolation':'%s'", val) } - key = strings.ToLower(key) - switch key { - - case "isolation": - if !containertypes.Isolation(val).IsValid() { - return fmt.Errorf("Invalid exec-opt value for 'isolation':'%s'", val) - } - if containertypes.Isolation(val).IsHyperV() { - daemon.defaultIsolation = containertypes.IsolationHyperV - } - if containertypes.Isolation(val).IsProcess() { - daemon.defaultIsolation = containertypes.IsolationProcess - } - default: - return fmt.Errorf("Unrecognised exec-opt '%s'\n", key) + if isolation.IsHyperV() { + daemon.defaultIsolation = containertypes.IsolationHyperV + } + if isolation.IsProcess() { + daemon.defaultIsolation = containertypes.IsolationProcess } }