From 5f5285a6e20badd366b59c03de0dd1678c0322a8 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 28 Jul 2020 16:01:08 -0700 Subject: [PATCH] Sterner warnings for unathenticated tcp People keep doing this and getting pwned because they accidentally left it exposed to the internet. The warning about doing this has been there forever. This introduces a sleep after warning. To disable the extra sleep users must explicitly specify `--tls=false` or `--tlsverify=false` Warning also specifies this sleep will be removed in the next release where the flag will be required if running unauthenticated. Signed-off-by: Brian Goff --- cmd/dockerd/daemon.go | 96 +++++++++++++++++++++-- cmd/dockerd/daemon_test.go | 6 +- cmd/dockerd/options.go | 13 ++- daemon/config/config.go | 4 +- daemon/info.go | 4 +- integration-cli/docker_cli_daemon_test.go | 15 ++-- 6 files changed, 116 insertions(+), 22 deletions(-) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index 842e7f4eb8..275d9a7262 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "fmt" + "net" "os" "path/filepath" "runtime" @@ -380,8 +381,16 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) { conf.Debug = opts.Debug conf.Hosts = opts.Hosts conf.LogLevel = opts.LogLevel - conf.TLS = opts.TLS - conf.TLSVerify = opts.TLSVerify + + if opts.flags.Changed(FlagTLS) { + conf.TLS = &opts.TLS + } + if opts.flags.Changed(FlagTLSVerify) { + conf.TLSVerify = &opts.TLSVerify + v := true + conf.TLS = &v + } + conf.CommonTLSOptions = config.CommonTLSOptions{} if opts.TLSOptions != nil { @@ -409,6 +418,7 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) { return nil, errors.Wrapf(err, "unable to configure the Docker daemon with file %s", opts.configFile) } } + // the merged configuration can be nil if the config file didn't exist. // leave the current configuration as it is if when that happens. if c != nil { @@ -434,7 +444,12 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) { // Regardless of whether the user sets it to true or false, if they // specify TLSVerify at all then we need to turn on TLS if conf.IsValueSet(FlagTLSVerify) { - conf.TLS = true + v := true + conf.TLS = &v + } + + if conf.TLSVerify == nil && conf.TLS != nil { + conf.TLSVerify = conf.TLS } return conf, nil @@ -548,7 +563,7 @@ func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) { CorsHeaders: cli.Config.CorsHeaders, } - if cli.Config.TLS { + if cli.Config.TLS != nil && *cli.Config.TLS { tlsOptions := tlsconfig.Options{ CAFile: cli.Config.CommonTLSOptions.CAFile, CertFile: cli.Config.CommonTLSOptions.CertFile, @@ -556,7 +571,7 @@ func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) { ExclusiveRootPools: true, } - if cli.Config.TLSVerify { + if cli.Config.TLSVerify == nil || *cli.Config.TLSVerify { // server requires and verifies client's certificate tlsOptions.ClientAuth = tls.RequireAndVerifyClientCert } @@ -574,13 +589,43 @@ func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) { return serverConfig, nil } +// checkTLSAuthOK checks basically for an explicitly disabled TLS/TLSVerify +// Going forward we do not want to support a scenario where dockerd listens +// on TCP without either TLS client auth (or an explicit opt-in to disable it) +func checkTLSAuthOK(c *config.Config) bool { + if c.TLS == nil { + // Either TLS is enabled by default, in which case TLS verification should be enabled by default, or explicitly disabled + // Or TLS is disabled by default... in any of these cases, we can just take the default value as to how to proceed + return DefaultTLSValue + } + + if !*c.TLS { + // TLS is explicitly disabled, which is supported + return true + } + + if c.TLSVerify == nil { + // this actually shouldn't happen since we set TLSVerify on the config object anyway + // But in case it does get here, be cautious and assume this is not supported. + return false + } + + // Either TLSVerify is explicitly enabled or disabled, both cases are supported + return true +} + func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, error) { var hosts []string seen := make(map[string]struct{}, len(cli.Config.Hosts)) + useTLS := DefaultTLSValue + if cli.Config.TLS != nil { + useTLS = *cli.Config.TLS + } + for i := 0; i < len(cli.Config.Hosts); i++ { var err error - if cli.Config.Hosts[i], err = dopts.ParseHost(cli.Config.TLS, honorXDG, cli.Config.Hosts[i]); err != nil { + if cli.Config.Hosts[i], err = dopts.ParseHost(useTLS, honorXDG, cli.Config.Hosts[i]); err != nil { return nil, errors.Wrapf(err, "error parsing -H %s", cli.Config.Hosts[i]) } if _, ok := seen[cli.Config.Hosts[i]]; ok { @@ -598,8 +643,43 @@ func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, er addr := protoAddrParts[1] // It's a bad idea to bind to TCP without tlsverify. - if proto == "tcp" && (serverConfig.TLSConfig == nil || serverConfig.TLSConfig.ClientAuth != tls.RequireAndVerifyClientCert) { - logrus.Warn("[!] DON'T BIND ON ANY IP ADDRESS WITHOUT setting --tlsverify IF YOU DON'T KNOW WHAT YOU'RE DOING [!]") + authEnabled := serverConfig.TLSConfig != nil && serverConfig.TLSConfig.ClientAuth == tls.RequireAndVerifyClientCert + if proto == "tcp" && !authEnabled { + logrus.WithField("host", protoAddr).Warn("Binding to IP address without --tlsverify is insecure and gives root access on this machine to everyone who has access to your network.") + logrus.WithField("host", protoAddr).Warn("Binding to an IP address, even on localhost, can also give access to scripts run in a browser. Be safe out there!") + time.Sleep(time.Second) + + // If TLSVerify is explicitly set to false we'll take that as "Please let me shoot myself in the foot" + // We do not want to continue to support a default mode where tls verification is disabled, so we do some extra warnings here and eventually remove support + if !checkTLSAuthOK(cli.Config) { + ipAddr, _, err := net.SplitHostPort(addr) + if err != nil { + return nil, errors.Wrap(err, "error parsing tcp address") + } + + // shortcut all this extra stuff for literal "localhost" + // -H supports specifying hostnames, since we want to bypass this on loopback interfaces we'll look it up here. + if ipAddr != "localhost" { + ip := net.ParseIP(ipAddr) + if ip == nil { + ipA, err := net.ResolveIPAddr("ip", ipAddr) + if err != nil { + logrus.WithError(err).WithField("host", ipAddr).Error("Error looking up specified host address") + } + if ipA != nil { + ip = ipA.IP + } + } + if ip == nil || !ip.IsLoopback() { + logrus.WithField("host", protoAddr).Warn("Binding to an IP address without --tlsverify is deprecated. Startup is intentionally being slowed down to show this message") + logrus.WithField("host", protoAddr).Warn("Please consider generating tls certificates with client validation to prevent exposing unauthenticated root access to your network") + logrus.WithField("host", protoAddr).Warnf("You can override this by explicitly specifying '--%s=false' or '--%s=false'", FlagTLS, FlagTLSVerify) + logrus.WithField("host", protoAddr).Warnf("Support for listening on TCP without authentication or explicit intent to run without authentication will be removed in the next release") + + time.Sleep(15 * time.Second) + } + } + } } ls, err := listeners.Init(proto, addr, serverConfig.SocketGroup, serverConfig.TLSConfig) if err != nil { diff --git a/cmd/dockerd/daemon_test.go b/cmd/dockerd/daemon_test.go index c4dcda87be..f2fff3728f 100644 --- a/cmd/dockerd/daemon_test.go +++ b/cmd/dockerd/daemon_test.go @@ -112,7 +112,7 @@ func TestLoadDaemonCliConfigWithTLSVerify(t *testing.T) { loadedConfig, err := loadDaemonCliConfig(opts) assert.NilError(t, err) assert.Assert(t, loadedConfig != nil) - assert.Check(t, is.Equal(loadedConfig.TLS, true)) + assert.Check(t, is.Equal(*loadedConfig.TLS, true)) } func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) { @@ -125,7 +125,7 @@ func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) { loadedConfig, err := loadDaemonCliConfig(opts) assert.NilError(t, err) assert.Assert(t, loadedConfig != nil) - assert.Check(t, loadedConfig.TLS) + assert.Check(t, *loadedConfig.TLS) } func TestLoadDaemonCliConfigWithoutTLSVerify(t *testing.T) { @@ -138,7 +138,7 @@ func TestLoadDaemonCliConfigWithoutTLSVerify(t *testing.T) { loadedConfig, err := loadDaemonCliConfig(opts) assert.NilError(t, err) assert.Assert(t, loadedConfig != nil) - assert.Check(t, !loadedConfig.TLS) + assert.Check(t, loadedConfig.TLS == nil) } func TestLoadDaemonCliConfigWithLogLevel(t *testing.T) { diff --git a/cmd/dockerd/options.go b/cmd/dockerd/options.go index f6ea21a6dc..e8e8fd41e8 100644 --- a/cmd/dockerd/options.go +++ b/cmd/dockerd/options.go @@ -20,6 +20,10 @@ const ( DefaultCertFile = "cert.pem" // FlagTLSVerify is the flag name for the TLS verification option FlagTLSVerify = "tlsverify" + // FlagTLS is the flag name for the TLS option + FlagTLS = "tls" + // DefaultTLSValue is the default value used for setting the tls option for tcp connections + DefaultTLSValue = false ) var ( @@ -56,8 +60,8 @@ func (o *daemonOptions) InstallFlags(flags *pflag.FlagSet) { flags.BoolVarP(&o.Debug, "debug", "D", false, "Enable debug mode") flags.StringVarP(&o.LogLevel, "log-level", "l", "info", `Set the logging level ("debug"|"info"|"warn"|"error"|"fatal")`) - flags.BoolVar(&o.TLS, "tls", false, "Use TLS; implied by --tlsverify") - flags.BoolVar(&o.TLSVerify, FlagTLSVerify, dockerTLSVerify, "Use TLS and verify the remote") + flags.BoolVar(&o.TLS, FlagTLS, DefaultTLSValue, "Use TLS; implied by --tlsverify") + flags.BoolVar(&o.TLSVerify, FlagTLSVerify, dockerTLSVerify || DefaultTLSValue, "Use TLS and verify the remote") // TODO use flag flags.String("identity"}, "i", "", "Path to libtrust key file") @@ -86,6 +90,11 @@ func (o *daemonOptions) SetDefaultOptions(flags *pflag.FlagSet) { o.TLS = true } + if o.TLS && !flags.Changed(FlagTLSVerify) { + // Enable tls verification unless explicitly disabled + o.TLSVerify = true + } + if !o.TLS { o.TLSOptions = nil } else { diff --git a/daemon/config/config.go b/daemon/config/config.go index bcecd36356..4990727597 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -205,8 +205,8 @@ type CommonConfig struct { Debug bool `json:"debug,omitempty"` Hosts []string `json:"hosts,omitempty"` LogLevel string `json:"log-level,omitempty"` - TLS bool `json:"tls,omitempty"` - TLSVerify bool `json:"tlsverify,omitempty"` + TLS *bool `json:"tls,omitempty"` + TLSVerify *bool `json:"tlsverify,omitempty"` // Embedded structs that allow config // deserialization without the full struct. diff --git a/daemon/info.go b/daemon/info.go index b47cb7283d..610d7fe2ec 100644 --- a/daemon/info.go +++ b/daemon/info.go @@ -219,11 +219,11 @@ func (daemon *Daemon) fillAPIInfo(v *types.Info) { if proto != "tcp" { continue } - if !cfg.TLS { + if cfg.TLS == nil || !*cfg.TLS { v.Warnings = append(v.Warnings, fmt.Sprintf("WARNING: API is accessible on http://%s without encryption.%s", addr, warn)) continue } - if !cfg.TLSVerify { + if cfg.TLSVerify == nil || !*cfg.TLSVerify { v.Warnings = append(v.Warnings, fmt.Sprintf("WARNING: API is accessible on https://%s without TLS client verification.%s", addr, warn)) continue } diff --git a/integration-cli/docker_cli_daemon_test.go b/integration-cli/docker_cli_daemon_test.go index c3c28b4c3c..d5fa41c29e 100644 --- a/integration-cli/docker_cli_daemon_test.go +++ b/integration-cli/docker_cli_daemon_test.go @@ -528,21 +528,26 @@ func (s *DockerDaemonSuite) TestDaemonFlagDebugLogLevelFatal(c *testing.T) { } func (s *DockerDaemonSuite) TestDaemonAllocatesListeningPort(c *testing.T) { - listeningPorts := [][]string{ + type listener struct { + daemon string + client string + port string + } + listeningPorts := []listener{ {"0.0.0.0", "0.0.0.0", "5678"}, {"127.0.0.1", "127.0.0.1", "1234"}, {"localhost", "127.0.0.1", "1235"}, } cmdArgs := make([]string, 0, len(listeningPorts)*2) - for _, hostDirective := range listeningPorts { - cmdArgs = append(cmdArgs, "--host", fmt.Sprintf("tcp://%s:%s", hostDirective[0], hostDirective[2])) + for _, l := range listeningPorts { + cmdArgs = append(cmdArgs, "--tls=false", "--host", fmt.Sprintf("tcp://%s:%s", l.daemon, l.port)) } s.d.StartWithBusybox(c, cmdArgs...) - for _, hostDirective := range listeningPorts { - output, err := s.d.Cmd("run", "-p", fmt.Sprintf("%s:%s:80", hostDirective[1], hostDirective[2]), "busybox", "true") + for _, l := range listeningPorts { + output, err := s.d.Cmd("run", "-p", fmt.Sprintf("%s:%s:80", l.client, l.port), "busybox", "true") if err == nil { c.Fatalf("Container should not start, expected port already allocated error: %q", output) } else if !strings.Contains(output, "port is already allocated") {