Browse code

Merge pull request #41285 from cpuguy83/no_more_pwns

Sterner warnings and deprecation notice for unauthenticated tcp access

Sebastiaan van Stijn authored on 2020/09/25 22:40:41
Showing 6 changed files
... ...
@@ -4,6 +4,7 @@ import (
4 4
 	"context"
5 5
 	"crypto/tls"
6 6
 	"fmt"
7
+	"net"
7 8
 	"os"
8 9
 	"path/filepath"
9 10
 	"runtime"
... ...
@@ -380,8 +381,16 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
380 380
 	conf.Debug = opts.Debug
381 381
 	conf.Hosts = opts.Hosts
382 382
 	conf.LogLevel = opts.LogLevel
383
-	conf.TLS = opts.TLS
384
-	conf.TLSVerify = opts.TLSVerify
383
+
384
+	if opts.flags.Changed(FlagTLS) {
385
+		conf.TLS = &opts.TLS
386
+	}
387
+	if opts.flags.Changed(FlagTLSVerify) {
388
+		conf.TLSVerify = &opts.TLSVerify
389
+		v := true
390
+		conf.TLS = &v
391
+	}
392
+
385 393
 	conf.CommonTLSOptions = config.CommonTLSOptions{}
386 394
 
387 395
 	if opts.TLSOptions != nil {
... ...
@@ -409,6 +418,7 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
409 409
 				return nil, errors.Wrapf(err, "unable to configure the Docker daemon with file %s", opts.configFile)
410 410
 			}
411 411
 		}
412
+
412 413
 		// the merged configuration can be nil if the config file didn't exist.
413 414
 		// leave the current configuration as it is if when that happens.
414 415
 		if c != nil {
... ...
@@ -434,7 +444,12 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
434 434
 	// Regardless of whether the user sets it to true or false, if they
435 435
 	// specify TLSVerify at all then we need to turn on TLS
436 436
 	if conf.IsValueSet(FlagTLSVerify) {
437
-		conf.TLS = true
437
+		v := true
438
+		conf.TLS = &v
439
+	}
440
+
441
+	if conf.TLSVerify == nil && conf.TLS != nil {
442
+		conf.TLSVerify = conf.TLS
438 443
 	}
439 444
 
440 445
 	return conf, nil
... ...
@@ -548,7 +563,7 @@ func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) {
548 548
 		CorsHeaders: cli.Config.CorsHeaders,
549 549
 	}
550 550
 
551
-	if cli.Config.TLS {
551
+	if cli.Config.TLS != nil && *cli.Config.TLS {
552 552
 		tlsOptions := tlsconfig.Options{
553 553
 			CAFile:             cli.Config.CommonTLSOptions.CAFile,
554 554
 			CertFile:           cli.Config.CommonTLSOptions.CertFile,
... ...
@@ -556,7 +571,7 @@ func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) {
556 556
 			ExclusiveRootPools: true,
557 557
 		}
558 558
 
559
-		if cli.Config.TLSVerify {
559
+		if cli.Config.TLSVerify == nil || *cli.Config.TLSVerify {
560 560
 			// server requires and verifies client's certificate
561 561
 			tlsOptions.ClientAuth = tls.RequireAndVerifyClientCert
562 562
 		}
... ...
@@ -574,13 +589,43 @@ func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) {
574 574
 	return serverConfig, nil
575 575
 }
576 576
 
577
+// checkTLSAuthOK checks basically for an explicitly disabled TLS/TLSVerify
578
+// Going forward we do not want to support a scenario where dockerd listens
579
+//   on TCP without either TLS client auth (or an explicit opt-in to disable it)
580
+func checkTLSAuthOK(c *config.Config) bool {
581
+	if c.TLS == nil {
582
+		// Either TLS is enabled by default, in which case TLS verification should be enabled by default, or explicitly disabled
583
+		// Or TLS is disabled by default... in any of these cases, we can just take the default value as to how to proceed
584
+		return DefaultTLSValue
585
+	}
586
+
587
+	if !*c.TLS {
588
+		// TLS is explicitly disabled, which is supported
589
+		return true
590
+	}
591
+
592
+	if c.TLSVerify == nil {
593
+		// this actually shouldn't happen since we set TLSVerify on the config object anyway
594
+		// But in case it does get here, be cautious and assume this is not supported.
595
+		return false
596
+	}
597
+
598
+	// Either TLSVerify is explicitly enabled or disabled, both cases are supported
599
+	return true
600
+}
601
+
577 602
 func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, error) {
578 603
 	var hosts []string
579 604
 	seen := make(map[string]struct{}, len(cli.Config.Hosts))
580 605
 
606
+	useTLS := DefaultTLSValue
607
+	if cli.Config.TLS != nil {
608
+		useTLS = *cli.Config.TLS
609
+	}
610
+
581 611
 	for i := 0; i < len(cli.Config.Hosts); i++ {
582 612
 		var err error
583
-		if cli.Config.Hosts[i], err = dopts.ParseHost(cli.Config.TLS, honorXDG, cli.Config.Hosts[i]); err != nil {
613
+		if cli.Config.Hosts[i], err = dopts.ParseHost(useTLS, honorXDG, cli.Config.Hosts[i]); err != nil {
584 614
 			return nil, errors.Wrapf(err, "error parsing -H %s", cli.Config.Hosts[i])
585 615
 		}
586 616
 		if _, ok := seen[cli.Config.Hosts[i]]; ok {
... ...
@@ -598,8 +643,43 @@ func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, er
598 598
 		addr := protoAddrParts[1]
599 599
 
600 600
 		// It's a bad idea to bind to TCP without tlsverify.
601
-		if proto == "tcp" && (serverConfig.TLSConfig == nil || serverConfig.TLSConfig.ClientAuth != tls.RequireAndVerifyClientCert) {
602
-			logrus.Warn("[!] DON'T BIND ON ANY IP ADDRESS WITHOUT setting --tlsverify IF YOU DON'T KNOW WHAT YOU'RE DOING [!]")
601
+		authEnabled := serverConfig.TLSConfig != nil && serverConfig.TLSConfig.ClientAuth == tls.RequireAndVerifyClientCert
602
+		if proto == "tcp" && !authEnabled {
603
+			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.")
604
+			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!")
605
+			time.Sleep(time.Second)
606
+
607
+			// If TLSVerify is explicitly set to false we'll take that as "Please let me shoot myself in the foot"
608
+			// 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
609
+			if !checkTLSAuthOK(cli.Config) {
610
+				ipAddr, _, err := net.SplitHostPort(addr)
611
+				if err != nil {
612
+					return nil, errors.Wrap(err, "error parsing tcp address")
613
+				}
614
+
615
+				// shortcut all this extra stuff for literal "localhost"
616
+				// -H supports specifying hostnames, since we want to bypass this on loopback interfaces we'll look it up here.
617
+				if ipAddr != "localhost" {
618
+					ip := net.ParseIP(ipAddr)
619
+					if ip == nil {
620
+						ipA, err := net.ResolveIPAddr("ip", ipAddr)
621
+						if err != nil {
622
+							logrus.WithError(err).WithField("host", ipAddr).Error("Error looking up specified host address")
623
+						}
624
+						if ipA != nil {
625
+							ip = ipA.IP
626
+						}
627
+					}
628
+					if ip == nil || !ip.IsLoopback() {
629
+						logrus.WithField("host", protoAddr).Warn("Binding to an IP address without --tlsverify is deprecated. Startup is intentionally being slowed down to show this message")
630
+						logrus.WithField("host", protoAddr).Warn("Please consider generating tls certificates with client validation to prevent exposing unauthenticated root access to your network")
631
+						logrus.WithField("host", protoAddr).Warnf("You can override this by explicitly specifying '--%s=false' or '--%s=false'", FlagTLS, FlagTLSVerify)
632
+						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")
633
+
634
+						time.Sleep(15 * time.Second)
635
+					}
636
+				}
637
+			}
603 638
 		}
604 639
 		ls, err := listeners.Init(proto, addr, serverConfig.SocketGroup, serverConfig.TLSConfig)
605 640
 		if err != nil {
... ...
@@ -112,7 +112,7 @@ func TestLoadDaemonCliConfigWithTLSVerify(t *testing.T) {
112 112
 	loadedConfig, err := loadDaemonCliConfig(opts)
113 113
 	assert.NilError(t, err)
114 114
 	assert.Assert(t, loadedConfig != nil)
115
-	assert.Check(t, is.Equal(loadedConfig.TLS, true))
115
+	assert.Check(t, is.Equal(*loadedConfig.TLS, true))
116 116
 }
117 117
 
118 118
 func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) {
... ...
@@ -125,7 +125,7 @@ func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) {
125 125
 	loadedConfig, err := loadDaemonCliConfig(opts)
126 126
 	assert.NilError(t, err)
127 127
 	assert.Assert(t, loadedConfig != nil)
128
-	assert.Check(t, loadedConfig.TLS)
128
+	assert.Check(t, *loadedConfig.TLS)
129 129
 }
130 130
 
131 131
 func TestLoadDaemonCliConfigWithoutTLSVerify(t *testing.T) {
... ...
@@ -138,7 +138,7 @@ func TestLoadDaemonCliConfigWithoutTLSVerify(t *testing.T) {
138 138
 	loadedConfig, err := loadDaemonCliConfig(opts)
139 139
 	assert.NilError(t, err)
140 140
 	assert.Assert(t, loadedConfig != nil)
141
-	assert.Check(t, !loadedConfig.TLS)
141
+	assert.Check(t, loadedConfig.TLS == nil)
142 142
 }
143 143
 
144 144
 func TestLoadDaemonCliConfigWithLogLevel(t *testing.T) {
... ...
@@ -20,6 +20,10 @@ const (
20 20
 	DefaultCertFile = "cert.pem"
21 21
 	// FlagTLSVerify is the flag name for the TLS verification option
22 22
 	FlagTLSVerify = "tlsverify"
23
+	// FlagTLS is the flag name for the TLS option
24
+	FlagTLS = "tls"
25
+	// DefaultTLSValue is the default value used for setting the tls option for tcp connections
26
+	DefaultTLSValue = false
23 27
 )
24 28
 
25 29
 var (
... ...
@@ -56,8 +60,8 @@ func (o *daemonOptions) InstallFlags(flags *pflag.FlagSet) {
56 56
 
57 57
 	flags.BoolVarP(&o.Debug, "debug", "D", false, "Enable debug mode")
58 58
 	flags.StringVarP(&o.LogLevel, "log-level", "l", "info", `Set the logging level ("debug"|"info"|"warn"|"error"|"fatal")`)
59
-	flags.BoolVar(&o.TLS, "tls", false, "Use TLS; implied by --tlsverify")
60
-	flags.BoolVar(&o.TLSVerify, FlagTLSVerify, dockerTLSVerify, "Use TLS and verify the remote")
59
+	flags.BoolVar(&o.TLS, FlagTLS, DefaultTLSValue, "Use TLS; implied by --tlsverify")
60
+	flags.BoolVar(&o.TLSVerify, FlagTLSVerify, dockerTLSVerify || DefaultTLSValue, "Use TLS and verify the remote")
61 61
 
62 62
 	// TODO use flag flags.String("identity"}, "i", "", "Path to libtrust key file")
63 63
 
... ...
@@ -86,6 +90,11 @@ func (o *daemonOptions) SetDefaultOptions(flags *pflag.FlagSet) {
86 86
 		o.TLS = true
87 87
 	}
88 88
 
89
+	if o.TLS && !flags.Changed(FlagTLSVerify) {
90
+		// Enable tls verification unless explicitly disabled
91
+		o.TLSVerify = true
92
+	}
93
+
89 94
 	if !o.TLS {
90 95
 		o.TLSOptions = nil
91 96
 	} else {
... ...
@@ -205,8 +205,8 @@ type CommonConfig struct {
205 205
 	Debug     bool     `json:"debug,omitempty"`
206 206
 	Hosts     []string `json:"hosts,omitempty"`
207 207
 	LogLevel  string   `json:"log-level,omitempty"`
208
-	TLS       bool     `json:"tls,omitempty"`
209
-	TLSVerify bool     `json:"tlsverify,omitempty"`
208
+	TLS       *bool    `json:"tls,omitempty"`
209
+	TLSVerify *bool    `json:"tlsverify,omitempty"`
210 210
 
211 211
 	// Embedded structs that allow config
212 212
 	// deserialization without the full struct.
... ...
@@ -219,11 +219,11 @@ func (daemon *Daemon) fillAPIInfo(v *types.Info) {
219 219
 		if proto != "tcp" {
220 220
 			continue
221 221
 		}
222
-		if !cfg.TLS {
222
+		if cfg.TLS == nil || !*cfg.TLS {
223 223
 			v.Warnings = append(v.Warnings, fmt.Sprintf("WARNING: API is accessible on http://%s without encryption.%s", addr, warn))
224 224
 			continue
225 225
 		}
226
-		if !cfg.TLSVerify {
226
+		if cfg.TLSVerify == nil || !*cfg.TLSVerify {
227 227
 			v.Warnings = append(v.Warnings, fmt.Sprintf("WARNING: API is accessible on https://%s without TLS client verification.%s", addr, warn))
228 228
 			continue
229 229
 		}
... ...
@@ -528,21 +528,26 @@ func (s *DockerDaemonSuite) TestDaemonFlagDebugLogLevelFatal(c *testing.T) {
528 528
 }
529 529
 
530 530
 func (s *DockerDaemonSuite) TestDaemonAllocatesListeningPort(c *testing.T) {
531
-	listeningPorts := [][]string{
531
+	type listener struct {
532
+		daemon string
533
+		client string
534
+		port   string
535
+	}
536
+	listeningPorts := []listener{
532 537
 		{"0.0.0.0", "0.0.0.0", "5678"},
533 538
 		{"127.0.0.1", "127.0.0.1", "1234"},
534 539
 		{"localhost", "127.0.0.1", "1235"},
535 540
 	}
536 541
 
537 542
 	cmdArgs := make([]string, 0, len(listeningPorts)*2)
538
-	for _, hostDirective := range listeningPorts {
539
-		cmdArgs = append(cmdArgs, "--host", fmt.Sprintf("tcp://%s:%s", hostDirective[0], hostDirective[2]))
543
+	for _, l := range listeningPorts {
544
+		cmdArgs = append(cmdArgs, "--tls=false", "--host", fmt.Sprintf("tcp://%s:%s", l.daemon, l.port))
540 545
 	}
541 546
 
542 547
 	s.d.StartWithBusybox(c, cmdArgs...)
543 548
 
544
-	for _, hostDirective := range listeningPorts {
545
-		output, err := s.d.Cmd("run", "-p", fmt.Sprintf("%s:%s:80", hostDirective[1], hostDirective[2]), "busybox", "true")
549
+	for _, l := range listeningPorts {
550
+		output, err := s.d.Cmd("run", "-p", fmt.Sprintf("%s:%s:80", l.client, l.port), "busybox", "true")
546 551
 		if err == nil {
547 552
 			c.Fatalf("Container should not start, expected port already allocated error: %q", output)
548 553
 		} else if !strings.Contains(output, "port is already allocated") {