From a6ce7eff65c7039865d41829fefef9ab9bb3e476 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 31 Aug 2021 14:05:49 +0200 Subject: [PATCH 1/6] daemon: move maskCredentials to config package This allows the utility to be used in other places. Signed-off-by: Sebastiaan van Stijn --- daemon/config/config.go | 11 ++++++++ daemon/config/config_test.go | 46 +++++++++++++++++++++++++++++++ daemon/info.go | 15 ++-------- daemon/info_test.go | 53 ------------------------------------ 4 files changed, 59 insertions(+), 66 deletions(-) delete mode 100644 daemon/info_test.go diff --git a/daemon/config/config.go b/daemon/config/config.go index bcf803edd0..87c4d20ab6 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net" + "net/url" "os" "reflect" "strings" @@ -645,3 +646,13 @@ func (conf *Config) GetDefaultRuntimeName() string { return rt } + +// MaskCredentials masks credentials that are in an URL. +func MaskCredentials(rawURL string) string { + parsedURL, err := url.Parse(rawURL) + if err != nil || parsedURL.User == nil { + return rawURL + } + parsedURL.User = url.UserPassword("xxxxx", "xxxxx") + return parsedURL.String() +} diff --git a/daemon/config/config_test.go b/daemon/config/config_test.go index e020dce748..35ac9a0147 100644 --- a/daemon/config/config_test.go +++ b/daemon/config/config_test.go @@ -578,3 +578,49 @@ func TestReloadWithDuplicateLabels(t *testing.T) { err := Reload(configFile, flags, func(c *Config) {}) assert.Check(t, err) } + +func TestMaskURLCredentials(t *testing.T) { + tests := []struct { + rawURL string + maskedURL string + }{ + { + rawURL: "", + maskedURL: "", + }, { + rawURL: "invalidURL", + maskedURL: "invalidURL", + }, { + rawURL: "http://proxy.example.com:80/", + maskedURL: "http://proxy.example.com:80/", + }, { + rawURL: "http://USER:PASSWORD@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://PASSWORD:PASSWORD@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://USER:@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://:PASSWORD@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://USER@docker:password@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://USER%40docker:password@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://USER%40docker:pa%3Fsword@proxy.example.com:80/", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", + }, { + rawURL: "http://USER%40docker:pa%3Fsword@proxy.example.com:80/hello%20world", + maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/hello%20world", + }, + } + for _, test := range tests { + maskedURL := MaskCredentials(test.rawURL) + assert.Equal(t, maskedURL, test.maskedURL) + } +} diff --git a/daemon/info.go b/daemon/info.go index 1ffa3aeeb4..a0f017c5be 100644 --- a/daemon/info.go +++ b/daemon/info.go @@ -2,7 +2,6 @@ package daemon // import "github.com/docker/docker/daemon" import ( "fmt" - "net/url" "os" "runtime" "strings" @@ -64,8 +63,8 @@ func (daemon *Daemon) SystemInfo() *types.Info { Labels: daemon.configStore.Labels, ExperimentalBuild: daemon.configStore.Experimental, ServerVersion: dockerversion.Version, - HTTPProxy: maskCredentials(getEnvAny("HTTP_PROXY", "http_proxy")), - HTTPSProxy: maskCredentials(getEnvAny("HTTPS_PROXY", "https_proxy")), + HTTPProxy: config.MaskCredentials(getEnvAny("HTTP_PROXY", "http_proxy")), + HTTPSProxy: config.MaskCredentials(getEnvAny("HTTPS_PROXY", "https_proxy")), NoProxy: getEnvAny("NO_PROXY", "no_proxy"), LiveRestoreEnabled: daemon.configStore.LiveRestoreEnabled, Isolation: daemon.defaultIsolation, @@ -289,16 +288,6 @@ func osVersion() (version string) { return version } -func maskCredentials(rawURL string) string { - parsedURL, err := url.Parse(rawURL) - if err != nil || parsedURL.User == nil { - return rawURL - } - parsedURL.User = url.UserPassword("xxxxx", "xxxxx") - maskedURL := parsedURL.String() - return maskedURL -} - func getEnvAny(names ...string) string { for _, n := range names { if val := os.Getenv(n); val != "" { diff --git a/daemon/info_test.go b/daemon/info_test.go deleted file mode 100644 index e9441151a9..0000000000 --- a/daemon/info_test.go +++ /dev/null @@ -1,53 +0,0 @@ -package daemon - -import ( - "testing" - - "gotest.tools/v3/assert" -) - -func TestMaskURLCredentials(t *testing.T) { - tests := []struct { - rawURL string - maskedURL string - }{ - { - rawURL: "", - maskedURL: "", - }, { - rawURL: "invalidURL", - maskedURL: "invalidURL", - }, { - rawURL: "http://proxy.example.com:80/", - maskedURL: "http://proxy.example.com:80/", - }, { - rawURL: "http://USER:PASSWORD@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://PASSWORD:PASSWORD@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://USER:@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://:PASSWORD@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://USER@docker:password@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://USER%40docker:password@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://USER%40docker:pa%3Fsword@proxy.example.com:80/", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/", - }, { - rawURL: "http://USER%40docker:pa%3Fsword@proxy.example.com:80/hello%20world", - maskedURL: "http://xxxxx:xxxxx@proxy.example.com:80/hello%20world", - }, - } - for _, test := range tests { - maskedURL := maskCredentials(test.rawURL) - assert.Equal(t, maskedURL, test.maskedURL) - } -} From 427c7cc5f86364466c7173e8ca59b97c3876471d Mon Sep 17 00:00:00 2001 From: Anca Iordache Date: Fri, 16 Jul 2021 09:33:00 +0200 Subject: [PATCH 2/6] Add http(s) proxy properties to daemon configuration This allows configuring the daemon's proxy server through the daemon.json con- figuration file or command-line flags configuration file, in addition to the existing option (through environment variables). Configuring environment variables on Windows to configure a service is more complicated than on Linux, and adding alternatives for this to the daemon con- figuration makes the configuration more transparent and easier to use. The configuration as set through command-line flags or through the daemon.json configuration file takes precedence over env-vars in the daemon's environment, which allows the daemon to use a different proxy. If both command-line flags and a daemon.json configuration option is set, an error is produced when starting the daemon. Note that this configuration is not "live reloadable" due to Golang's use of `sync.Once()` for proxy configuration, which means that changing the proxy configuration requires a restart of the daemon (reload / SIGHUP will not update the configuration. With this patch: cat /etc/docker/daemon.json { "http-proxy": "http://proxytest.example.com:80", "https-proxy": "https://proxytest.example.com:443" } docker pull busybox Using default tag: latest Error response from daemon: Get "https://registry-1.docker.io/v2/": proxyconnect tcp: dial tcp: lookup proxytest.example.com on 127.0.0.11:53: no such host docker build . Sending build context to Docker daemon 89.28MB Step 1/3 : FROM golang:1.16-alpine AS base Get "https://registry-1.docker.io/v2/": proxyconnect tcp: dial tcp: lookup proxytest.example.com on 127.0.0.11:53: no such host Integration tests were added to test the behavior: - verify that the configuration through all means are used (env-var, command-line flags, damon.json), and used in the expected order of preference. - verify that conflicting options produce an error. Signed-off-by: Anca Iordache Signed-off-by: Sebastiaan van Stijn --- cmd/dockerd/config.go | 4 + cmd/dockerd/daemon.go | 28 ++++++ daemon/config/config.go | 8 ++ daemon/info.go | 13 ++- integration/daemon/daemon_test.go | 153 ++++++++++++++++++++++++++++++ 5 files changed, 203 insertions(+), 3 deletions(-) diff --git a/cmd/dockerd/config.go b/cmd/dockerd/config.go index c53f4f3559..0f1dff4b8e 100644 --- a/cmd/dockerd/config.go +++ b/cmd/dockerd/config.go @@ -101,6 +101,10 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) error { flags.StringVar(&conf.DefaultRuntime, "default-runtime", config.StockRuntimeName, "Default OCI runtime for containers") + flags.StringVar(&conf.HTTPProxy, "http-proxy", "", "HTTP proxy URL to use for outgoing traffic") + flags.StringVar(&conf.HTTPSProxy, "https-proxy", "", "HTTPS proxy URL to use for outgoing traffic") + flags.StringVar(&conf.NoProxy, "no-proxy", "", "Comma-separated list of hosts or IP addresses for which the proxy is skipped") + return nil } diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index b38d4915a8..4603daac41 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -87,6 +87,8 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) { return nil } + configureProxyEnv(cli.Config) + warnOnDeprecatedConfigOptions(cli.Config) if err := configureDaemonLogs(cli.Config); err != nil { @@ -779,3 +781,29 @@ func configureDaemonLogs(conf *config.Config) error { }) return nil } + +func configureProxyEnv(conf *config.Config) { + if p := conf.HTTPProxy; p != "" { + overrideProxyEnv("HTTP_PROXY", p) + overrideProxyEnv("http_proxy", p) + } + if p := conf.HTTPSProxy; p != "" { + overrideProxyEnv("HTTPS_PROXY", p) + overrideProxyEnv("https_proxy", p) + } + if p := conf.NoProxy; p != "" { + overrideProxyEnv("NO_PROXY", p) + overrideProxyEnv("no_proxy", p) + } +} + +func overrideProxyEnv(name, val string) { + if oldVal := os.Getenv(name); oldVal != "" && oldVal != val { + logrus.WithFields(logrus.Fields{ + "name": name, + "old-value": oldVal, + "new-value": val, + }).Warn("overriding existing proxy variable with value from configuration") + } + _ = os.Setenv(name, val) +} diff --git a/daemon/config/config.go b/daemon/config/config.go index 87c4d20ab6..9a05bbcfc5 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -166,6 +166,7 @@ type CommonConfig struct { ExecRoot string `json:"exec-root,omitempty"` SocketGroup string `json:"group,omitempty"` CorsHeaders string `json:"api-cors-header,omitempty"` + ProxyConfig // TrustKeyPath is used to generate the daemon ID and for signing schema 1 manifests // when pushing to a registry which does not support schema 2. This field is marked as @@ -276,6 +277,13 @@ type CommonConfig struct { DefaultRuntime string `json:"default-runtime,omitempty"` } +// ProxyConfig holds the proxy-configuration for the daemon. +type ProxyConfig struct { + HTTPProxy string `json:"http-proxy,omitempty"` + HTTPSProxy string `json:"https-proxy,omitempty"` + NoProxy string `json:"no-proxy,omitempty"` +} + // IsValueSet returns true if a configuration value // was explicitly set in the configuration file. func (conf *Config) IsValueSet(name string) bool { diff --git a/daemon/info.go b/daemon/info.go index a0f017c5be..f6e090063e 100644 --- a/daemon/info.go +++ b/daemon/info.go @@ -63,9 +63,9 @@ func (daemon *Daemon) SystemInfo() *types.Info { Labels: daemon.configStore.Labels, ExperimentalBuild: daemon.configStore.Experimental, ServerVersion: dockerversion.Version, - HTTPProxy: config.MaskCredentials(getEnvAny("HTTP_PROXY", "http_proxy")), - HTTPSProxy: config.MaskCredentials(getEnvAny("HTTPS_PROXY", "https_proxy")), - NoProxy: getEnvAny("NO_PROXY", "no_proxy"), + HTTPProxy: config.MaskCredentials(getConfigOrEnv(daemon.configStore.HTTPProxy, "HTTP_PROXY", "http_proxy")), + HTTPSProxy: config.MaskCredentials(getConfigOrEnv(daemon.configStore.HTTPSProxy, "HTTPS_PROXY", "https_proxy")), + NoProxy: getConfigOrEnv(daemon.configStore.NoProxy, "NO_PROXY", "no_proxy"), LiveRestoreEnabled: daemon.configStore.LiveRestoreEnabled, Isolation: daemon.defaultIsolation, } @@ -296,3 +296,10 @@ func getEnvAny(names ...string) string { } return "" } + +func getConfigOrEnv(config string, env ...string) string { + if config != "" { + return config + } + return getEnvAny(env...) +} diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index d31ad3ba46..b4381b8c85 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -1,16 +1,22 @@ package daemon // import "github.com/docker/docker/integration/daemon" import ( + "context" + "fmt" + "net/http" + "net/http/httptest" "os" "os/exec" "path/filepath" "runtime" "testing" + "github.com/docker/docker/api/types" "github.com/docker/docker/daemon/config" "github.com/docker/docker/testutil/daemon" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" + "gotest.tools/v3/env" "gotest.tools/v3/skip" ) @@ -146,3 +152,150 @@ func TestConfigDaemonSeccompProfiles(t *testing.T) { }) } } + +func TestDaemonProxy(t *testing.T) { + skip.If(t, runtime.GOOS == "windows", "cannot start multiple daemons on windows") + + var received string + proxyServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + received = r.Host + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("OK")) + })) + defer proxyServer.Close() + + // Configure proxy through env-vars + t.Run("environment variables", func(t *testing.T) { + defer env.Patch(t, "HTTP_PROXY", proxyServer.URL)() + defer env.Patch(t, "HTTPS_PROXY", proxyServer.URL)() + defer env.Patch(t, "NO_PROXY", "example.com")() + + d := daemon.New(t) + c := d.NewClientT(t) + defer func() { _ = c.Close() }() + ctx := context.Background() + d.Start(t) + + _, err := c.ImagePull(ctx, "example.org:5000/some/image:latest", types.ImagePullOptions{}) + assert.ErrorContains(t, err, "", "pulling should have failed") + assert.Equal(t, received, "example.org:5000") + + // Test NoProxy: example.com should not hit the proxy, and "received" variable should not be changed. + _, err = c.ImagePull(ctx, "example.com/some/image:latest", types.ImagePullOptions{}) + assert.ErrorContains(t, err, "", "pulling should have failed") + assert.Equal(t, received, "example.org:5000", "should not have used proxy") + + info := d.Info(t) + assert.Equal(t, info.HTTPProxy, proxyServer.URL) + assert.Equal(t, info.HTTPSProxy, proxyServer.URL) + assert.Equal(t, info.NoProxy, "example.com") + d.Stop(t) + }) + + // Configure proxy through command-line flags + t.Run("command-line options", func(t *testing.T) { + defer env.Patch(t, "HTTP_PROXY", "http://from-env-http.invalid")() + defer env.Patch(t, "http_proxy", "http://from-env-http.invalid")() + defer env.Patch(t, "HTTPS_PROXY", "https://from-env-https.invalid")() + defer env.Patch(t, "https_proxy", "https://from-env-http.invalid")() + defer env.Patch(t, "NO_PROXY", "ignore.invalid")() + defer env.Patch(t, "no_proxy", "ignore.invalid")() + + d := daemon.New(t) + d.Start(t, "--http-proxy", proxyServer.URL, "--https-proxy", proxyServer.URL, "--no-proxy", "example.com") + + logs, err := d.ReadLogFile() + assert.NilError(t, err) + assert.Assert(t, is.Contains(string(logs), "overriding existing proxy variable with value from configuration")) + for _, v := range []string{"http_proxy", "HTTP_PROXY", "https_proxy", "HTTPS_PROXY", "no_proxy", "NO_PROXY"} { + assert.Assert(t, is.Contains(string(logs), "name="+v)) + } + + c := d.NewClientT(t) + defer func() { _ = c.Close() }() + ctx := context.Background() + + _, err = c.ImagePull(ctx, "example.org:5001/some/image:latest", types.ImagePullOptions{}) + assert.ErrorContains(t, err, "", "pulling should have failed") + assert.Equal(t, received, "example.org:5001") + + // Test NoProxy: example.com should not hit the proxy, and "received" variable should not be changed. + _, err = c.ImagePull(ctx, "example.com/some/image:latest", types.ImagePullOptions{}) + assert.ErrorContains(t, err, "", "pulling should have failed") + assert.Equal(t, received, "example.org:5001", "should not have used proxy") + + info := d.Info(t) + assert.Equal(t, info.HTTPProxy, proxyServer.URL) + assert.Equal(t, info.HTTPSProxy, proxyServer.URL) + assert.Equal(t, info.NoProxy, "example.com") + + d.Stop(t) + }) + + // Configure proxy through configuration file + t.Run("configuration file", func(t *testing.T) { + defer env.Patch(t, "HTTP_PROXY", "http://from-env-http.invalid")() + defer env.Patch(t, "http_proxy", "http://from-env-http.invalid")() + defer env.Patch(t, "HTTPS_PROXY", "https://from-env-https.invalid")() + defer env.Patch(t, "https_proxy", "https://from-env-http.invalid")() + defer env.Patch(t, "NO_PROXY", "ignore.invalid")() + defer env.Patch(t, "no_proxy", "ignore.invalid")() + + d := daemon.New(t) + c := d.NewClientT(t) + defer func() { _ = c.Close() }() + ctx := context.Background() + + configFile := filepath.Join(d.RootDir(), "daemon.json") + configJSON := fmt.Sprintf(`{"http-proxy":%[1]q, "https-proxy": %[1]q, "no-proxy": "example.com"}`, proxyServer.URL) + assert.NilError(t, os.WriteFile(configFile, []byte(configJSON), 0644)) + + d.Start(t, "--config-file", configFile) + + logs, err := d.ReadLogFile() + assert.NilError(t, err) + assert.Assert(t, is.Contains(string(logs), "overriding existing proxy variable with value from configuration")) + for _, v := range []string{"http_proxy", "HTTP_PROXY", "https_proxy", "HTTPS_PROXY", "no_proxy", "NO_PROXY"} { + assert.Assert(t, is.Contains(string(logs), "name="+v)) + } + + _, err = c.ImagePull(ctx, "example.org:5002/some/image:latest", types.ImagePullOptions{}) + assert.ErrorContains(t, err, "", "pulling should have failed") + assert.Equal(t, received, "example.org:5002") + + // Test NoProxy: example.com should not hit the proxy, and "received" variable should not be changed. + _, err = c.ImagePull(ctx, "example.com/some/image:latest", types.ImagePullOptions{}) + assert.ErrorContains(t, err, "", "pulling should have failed") + assert.Equal(t, received, "example.org:5002", "should not have used proxy") + + info := d.Info(t) + assert.Equal(t, info.HTTPProxy, proxyServer.URL) + assert.Equal(t, info.HTTPSProxy, proxyServer.URL) + assert.Equal(t, info.NoProxy, "example.com") + + d.Stop(t) + }) + + // Conflicting options (passed both through command-line options and config file) + t.Run("conflicting options", func(t *testing.T) { + const ( + proxyRawURL = "https://myuser:mypassword@example.org" + ) + + d := daemon.New(t) + + configFile := filepath.Join(d.RootDir(), "daemon.json") + configJSON := fmt.Sprintf(`{"http-proxy":%[1]q, "https-proxy": %[1]q, "no-proxy": "example.com"}`, proxyRawURL) + assert.NilError(t, os.WriteFile(configFile, []byte(configJSON), 0644)) + + err := d.StartWithError("--http-proxy", proxyRawURL, "--https-proxy", proxyRawURL, "--no-proxy", "example.com", "--config-file", configFile, "--validate") + assert.ErrorContains(t, err, "daemon exited during startup") + logs, err := d.ReadLogFile() + assert.NilError(t, err) + expected := fmt.Sprintf( + `the following directives are specified both as a flag and in the configuration file: http-proxy: (from flag: %[1]s, from file: %[1]s), https-proxy: (from flag: %[1]s, from file: %[1]s), no-proxy: (from flag: example.com, from file: example.com)`, + proxyRawURL, + ) + assert.Assert(t, is.Contains(string(logs), expected)) + }) +} From bad4b30e65cfb484516e580748885b3b9a804442 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 13 Sep 2021 09:40:01 +0200 Subject: [PATCH 3/6] integration: skip TestDaemonProxy on rootless CI The proxy configuration works, but looks like we're unable to connect to the test proxy server as part of our test; level=debug msg="Trying to pull example.org:5000/some/image from https://example.org:5000 v2" level=warning msg="Error getting v2 registry: Get \"https://example.org:5000/v2/\": proxyconnect tcp: dial tcp 127.0.0.1:45999: connect: connection refused" level=info msg="Attempting next endpoint for pull after error: Get \"https://example.org:5000/v2/\": proxyconnect tcp: dial tcp 127.0.0.1:45999: connect: connection refused" level=error msg="Handler for POST /v1.42/images/create returned error: Get \"https://example.org:5000/v2/\": proxyconnect tcp: dial tcp 127.0.0.1:45999: connect: connection refused" Signed-off-by: Sebastiaan van Stijn --- integration/daemon/daemon_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index b4381b8c85..8bd8eaef0b 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -155,6 +155,7 @@ func TestConfigDaemonSeccompProfiles(t *testing.T) { func TestDaemonProxy(t *testing.T) { skip.If(t, runtime.GOOS == "windows", "cannot start multiple daemons on windows") + skip.If(t, os.Getenv("DOCKER_ROOTLESS") != "", "cannot connect to localhost proxy in rootless environment") var received string proxyServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 76016b846de5a0ced3a0c0f9fd2357073500526e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 31 Aug 2021 14:13:30 +0200 Subject: [PATCH 4/6] daemon: make sure proxy settings are sanitized when printing The daemon can print the proxy configuration as part of error-messages, and when reloading the daemon configuration (SIGHUP). Make sure that the configuration is sanitized before printing. Signed-off-by: Sebastiaan van Stijn --- cmd/dockerd/daemon.go | 4 +-- daemon/config/config.go | 5 +++ daemon/reload.go | 16 +++++++-- integration/daemon/daemon_test.go | 57 +++++++++++++++++++++++++------ 4 files changed, 68 insertions(+), 14 deletions(-) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index 4603daac41..2c9ae2951b 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -801,8 +801,8 @@ func overrideProxyEnv(name, val string) { if oldVal := os.Getenv(name); oldVal != "" && oldVal != val { logrus.WithFields(logrus.Fields{ "name": name, - "old-value": oldVal, - "new-value": val, + "old-value": config.MaskCredentials(oldVal), + "new-value": config.MaskCredentials(val), }).Warn("overriding existing proxy variable with value from configuration") } _ = os.Setenv(name, val) diff --git a/daemon/config/config.go b/daemon/config/config.go index 9a05bbcfc5..fa534f3249 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -534,6 +534,11 @@ func findConfigurationConflicts(config map[string]interface{}, flags *pflag.Flag var conflicts []string printConflict := func(name string, flagValue, fileValue interface{}) string { + switch name { + case "http-proxy", "https-proxy": + flagValue = MaskCredentials(flagValue.(string)) + fileValue = MaskCredentials(fileValue.(string)) + } return fmt.Sprintf("%s: (from flag: %v, from file: %v)", name, flagValue, fileValue) } diff --git a/daemon/reload.go b/daemon/reload.go index 72379c054e..661619626e 100644 --- a/daemon/reload.go +++ b/daemon/reload.go @@ -28,14 +28,26 @@ func (daemon *Daemon) Reload(conf *config.Config) (err error) { attributes := map[string]string{} defer func() { - jsonString, _ := json.Marshal(daemon.configStore) + if err == nil { + jsonString, _ := json.Marshal(&struct { + *config.Config + config.ProxyConfig + }{ + Config: daemon.configStore, + ProxyConfig: config.ProxyConfig{ + HTTPProxy: config.MaskCredentials(daemon.configStore.HTTPProxy), + HTTPSProxy: config.MaskCredentials(daemon.configStore.HTTPSProxy), + NoProxy: config.MaskCredentials(daemon.configStore.NoProxy), + }, + }) + logrus.Infof("Reloaded configuration: %s", jsonString) + } // we're unlocking here, because // LogDaemonEventWithAttributes() -> SystemInfo() -> GetAllRuntimes() // holds that lock too. daemon.configStore.Unlock() if err == nil { - logrus.Infof("Reloaded configuration: %s", jsonString) daemon.LogDaemonEventWithAttributes("reload", attributes) } }() diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index 8bd8eaef0b..1a79092b48 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -9,6 +9,8 @@ import ( "os/exec" "path/filepath" "runtime" + "strings" + "syscall" "testing" "github.com/docker/docker/api/types" @@ -165,6 +167,8 @@ func TestDaemonProxy(t *testing.T) { })) defer proxyServer.Close() + const userPass = "myuser:mypassword@" + // Configure proxy through env-vars t.Run("environment variables", func(t *testing.T) { defer env.Patch(t, "HTTP_PROXY", proxyServer.URL)() @@ -195,10 +199,10 @@ func TestDaemonProxy(t *testing.T) { // Configure proxy through command-line flags t.Run("command-line options", func(t *testing.T) { - defer env.Patch(t, "HTTP_PROXY", "http://from-env-http.invalid")() - defer env.Patch(t, "http_proxy", "http://from-env-http.invalid")() - defer env.Patch(t, "HTTPS_PROXY", "https://from-env-https.invalid")() - defer env.Patch(t, "https_proxy", "https://from-env-http.invalid")() + defer env.Patch(t, "HTTP_PROXY", "http://"+userPass+"from-env-http.invalid")() + defer env.Patch(t, "http_proxy", "http://"+userPass+"from-env-http.invalid")() + defer env.Patch(t, "HTTPS_PROXY", "https://"+userPass+"myuser:mypassword@from-env-https.invalid")() + defer env.Patch(t, "https_proxy", "https://"+userPass+"myuser:mypassword@from-env-https.invalid")() defer env.Patch(t, "NO_PROXY", "ignore.invalid")() defer env.Patch(t, "no_proxy", "ignore.invalid")() @@ -210,6 +214,7 @@ func TestDaemonProxy(t *testing.T) { assert.Assert(t, is.Contains(string(logs), "overriding existing proxy variable with value from configuration")) for _, v := range []string{"http_proxy", "HTTP_PROXY", "https_proxy", "HTTPS_PROXY", "no_proxy", "NO_PROXY"} { assert.Assert(t, is.Contains(string(logs), "name="+v)) + assert.Assert(t, !strings.Contains(string(logs), userPass), "logs should not contain the non-sanitized proxy URL: %s", string(logs)) } c := d.NewClientT(t) @@ -235,10 +240,10 @@ func TestDaemonProxy(t *testing.T) { // Configure proxy through configuration file t.Run("configuration file", func(t *testing.T) { - defer env.Patch(t, "HTTP_PROXY", "http://from-env-http.invalid")() - defer env.Patch(t, "http_proxy", "http://from-env-http.invalid")() - defer env.Patch(t, "HTTPS_PROXY", "https://from-env-https.invalid")() - defer env.Patch(t, "https_proxy", "https://from-env-http.invalid")() + defer env.Patch(t, "HTTP_PROXY", "http://"+userPass+"from-env-http.invalid")() + defer env.Patch(t, "http_proxy", "http://"+userPass+"from-env-http.invalid")() + defer env.Patch(t, "HTTPS_PROXY", "https://"+userPass+"myuser:mypassword@from-env-https.invalid")() + defer env.Patch(t, "https_proxy", "https://"+userPass+"myuser:mypassword@from-env-https.invalid")() defer env.Patch(t, "NO_PROXY", "ignore.invalid")() defer env.Patch(t, "no_proxy", "ignore.invalid")() @@ -258,6 +263,7 @@ func TestDaemonProxy(t *testing.T) { assert.Assert(t, is.Contains(string(logs), "overriding existing proxy variable with value from configuration")) for _, v := range []string{"http_proxy", "HTTP_PROXY", "https_proxy", "HTTPS_PROXY", "no_proxy", "NO_PROXY"} { assert.Assert(t, is.Contains(string(logs), "name="+v)) + assert.Assert(t, !strings.Contains(string(logs), userPass), "logs should not contain the non-sanitized proxy URL: %s", string(logs)) } _, err = c.ImagePull(ctx, "example.org:5002/some/image:latest", types.ImagePullOptions{}) @@ -280,7 +286,8 @@ func TestDaemonProxy(t *testing.T) { // Conflicting options (passed both through command-line options and config file) t.Run("conflicting options", func(t *testing.T) { const ( - proxyRawURL = "https://myuser:mypassword@example.org" + proxyRawURL = "https://" + userPass + "example.org" + proxyURL = "https://xxxxx:xxxxx@example.org" ) d := daemon.New(t) @@ -295,8 +302,38 @@ func TestDaemonProxy(t *testing.T) { assert.NilError(t, err) expected := fmt.Sprintf( `the following directives are specified both as a flag and in the configuration file: http-proxy: (from flag: %[1]s, from file: %[1]s), https-proxy: (from flag: %[1]s, from file: %[1]s), no-proxy: (from flag: example.com, from file: example.com)`, - proxyRawURL, + proxyURL, ) assert.Assert(t, is.Contains(string(logs), expected)) }) + + // Make sure values are sanitized when reloading the daemon-config + t.Run("reload sanitized", func(t *testing.T) { + const ( + proxyRawURL = "https://" + userPass + "example.org" + proxyURL = "https://xxxxx:xxxxx@example.org" + ) + + d := daemon.New(t) + d.Start(t, "--http-proxy", proxyRawURL, "--https-proxy", proxyRawURL, "--no-proxy", "example.com") + defer d.Stop(t) + err := d.Signal(syscall.SIGHUP) + assert.NilError(t, err) + + logs, err := d.ReadLogFile() + assert.NilError(t, err) + + // FIXME: there appears to ba a race condition, which causes ReadLogFile + // to not contain the full logs after signaling the daemon to reload, + // causing the test to fail here. As a workaround, check if we + // received the "reloaded" message after signaling, and only then + // check that it's sanitized properly. For more details on this + // issue, see https://github.com/moby/moby/pull/42835/files#r713120315 + if !strings.Contains(string(logs), "Reloaded configuration:") { + t.Skip("Skipping test, because we did not find 'Reloaded configuration' in the logs") + } + + assert.Assert(t, is.Contains(string(logs), proxyURL)) + assert.Assert(t, !strings.Contains(string(logs), userPass), "logs should not contain the non-sanitized proxy URL: %s", string(logs)) + }) } From 040b1d5eebc263437ad4bd69fb73398ee5153d84 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 9 Sep 2021 13:43:26 +0200 Subject: [PATCH 5/6] integration/daemon: use "windows" to skip tests For consistency, and to allow easier grepping for all tests that we skip on windows. Signed-off-by: Sebastiaan van Stijn --- integration/daemon/daemon_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index 1a79092b48..0c4f80f959 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -23,7 +23,7 @@ import ( ) func TestConfigDaemonLibtrustID(t *testing.T) { - skip.If(t, runtime.GOOS != "linux") + skip.If(t, runtime.GOOS == "windows") d := daemon.New(t) defer d.Stop(t) @@ -42,7 +42,7 @@ func TestConfigDaemonLibtrustID(t *testing.T) { } func TestDaemonConfigValidation(t *testing.T) { - skip.If(t, runtime.GOOS != "linux") + skip.If(t, runtime.GOOS == "windows") d := daemon.New(t) dockerBinary, err := d.BinaryPath() @@ -108,7 +108,7 @@ func TestDaemonConfigValidation(t *testing.T) { } func TestConfigDaemonSeccompProfiles(t *testing.T) { - skip.If(t, runtime.GOOS != "linux") + skip.If(t, runtime.GOOS == "windows") d := daemon.New(t) defer d.Stop(t) From e7583ab8593ddfdd1f26ad201dc43edf52896bf2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 21 Sep 2021 14:33:01 +0200 Subject: [PATCH 6/6] testutil/daemon: ReadLogFile() trigger os.Sync() before reading Make sure it's written to disk before we try reading the logs. Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 1 + 1 file changed, 1 insertion(+) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index fe12063169..775db6b4ac 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -262,6 +262,7 @@ func (d *Daemon) LogFileName() string { // ReadLogFile returns the content of the daemon log file func (d *Daemon) ReadLogFile() ([]byte, error) { + _ = d.logFile.Sync() return os.ReadFile(d.logFile.Name()) }