Merge pull request #9283 from dmcgowan/tls-default-behavior

Avoid TLS fallback when protocol is not ambiguous

remotes/docker/config/hosts.go

@@ -100,12 +100,22 @@ func ConfigureHosts(ctx context.Context, options HostOptions) docker.RegistryHos
 				hosts[len(hosts)-1].host = "registry-1.docker.io"
 			} else if docker.IsLocalhost(host) {
 				hosts[len(hosts)-1].host = host
-				if options.DefaultScheme == "" || options.DefaultScheme == "http" {
+				if options.DefaultScheme == "" {
-					hosts[len(hosts)-1].scheme = "http"
+					_, port, _ := net.SplitHostPort(host)
+					if port == "" || port == "443" {
+						// If port is default or 443, only use https
+						hosts[len(hosts)-1].scheme = "https"
+					} else {
+						// HTTP fallback logic will be used when protocol is ambiguous
+						hosts[len(hosts)-1].scheme = "http"
+					}
 
-					// Skipping TLS verification for localhost
+					// When port is 80, protocol is not ambiguous
-					var skipVerify = true
+					if port != "80" {
-					hosts[len(hosts)-1].skipVerify = &skipVerify
+						// Skipping TLS verification for localhost
+						var skipVerify = true
+						hosts[len(hosts)-1].skipVerify = &skipVerify
+					}
 				} else {
 					hosts[len(hosts)-1].scheme = options.DefaultScheme
 				}
@@ -121,13 +131,13 @@ func ConfigureHosts(ctx context.Context, options HostOptions) docker.RegistryHos
 			hosts[len(hosts)-1].capabilities = docker.HostCapabilityPull | docker.HostCapabilityResolve | docker.HostCapabilityPush
 		}
 
-		// explicitTLS indicates that TLS was explicitly configured and HTTP endpoints should
+		// tlsConfigured indicates that TLS was configured and HTTP endpoints should
 		// attempt to use the TLS configuration before falling back to HTTP
-		var explicitTLS bool
+		var tlsConfigured bool
 
 		var defaultTLSConfig *tls.Config
 		if options.DefaultTLS != nil {
-			explicitTLS = true
+			tlsConfigured = true
 			defaultTLSConfig = options.DefaultTLS
 		} else {
 			defaultTLSConfig = &tls.Config{}
@@ -166,7 +176,7 @@ func ConfigureHosts(ctx context.Context, options HostOptions) docker.RegistryHos
 		rhosts := make([]docker.RegistryHost, len(hosts))
 		for i, host := range hosts {
 			// Allow setting for each host as well
-			explicitTLS := explicitTLS
+			explicitTLS := tlsConfigured
 
 			if host.caCerts != nil || host.clientPairs != nil || host.skipVerify != nil {
 				explicitTLS = true
@@ -232,13 +242,19 @@ func ConfigureHosts(ctx context.Context, options HostOptions) docker.RegistryHos
 				rhosts[i].Authorizer = authorizer
 			}
 
-			// When TLS has been explicitly configured for the operation or host, use the
+			// When TLS has been configured for the operation or host and
-			// docker.HTTPFallback roundtripper to catch TLS errors and re-attempt the request as http.
+			// the protocol from the port number is ambiguous, use the
-			// This allows preference for https when configured but also catches TLS errors early enough
+			// docker.HTTPFallback roundtripper to catch TLS errors and re-attempt the
-			// in the request to avoid sending the request twice or consuming the request body.
+			// request as http. This allows preference for https when configured but
+			// also catches TLS errors early enough in the request to avoid sending
+			// the request twice or consuming the request body.
 			if host.scheme == "http" && explicitTLS {
-				host.scheme = "https"
+				_, port, _ := net.SplitHostPort(host.host)
-				rhosts[i].Client.Transport = docker.HTTPFallback{RoundTripper: rhosts[i].Client.Transport}
+				if port != "" && port != "80" {
+					log.G(ctx).WithField("host", host.host).Info("host will try HTTPS first since it is configured for HTTP with a TLS configuration, consider changing host to HTTPS or removing unused TLS configuration")
+					host.scheme = "https"
+					rhosts[i].Client.Transport = docker.HTTPFallback{RoundTripper: rhosts[i].Client.Transport}
+				}
 			}
 
 			rhosts[i].Scheme = host.scheme

remotes/docker/config/hosts_test.go

@@ -295,27 +295,190 @@ func TestLoadCertFiles(t *testing.T) {
 	}
 }
 
-func TestLocalhostHTTPFallback(t *testing.T) {
+func TestHTTPFallback(t *testing.T) {
-	opts := HostOptions{
+	for _, tc := range []struct {
-		DefaultTLS: &tls.Config{
+		host           string
-			InsecureSkipVerify: true,
+		opts           HostOptions
+		expectedScheme string
+		usesFallback   bool
+	}{
+		{
+			host: "localhost:8080",
+			opts: HostOptions{
+				DefaultScheme: "http",
+				DefaultTLS: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedScheme: "https",
+			usesFallback:   true,
+		},
+		{
+			host: "localhost:8080",
+			opts: HostOptions{
+				DefaultScheme: "https",
+				DefaultTLS: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+		{
+			host:           "localhost:8080",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   true,
+		},
+		{
+			host:           "localhost:80",
+			opts:           HostOptions{},
+			expectedScheme: "http",
+			usesFallback:   false,
+		},
+		{
+			host:           "localhost:443",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+		{
+			host: "localhost:80",
+			opts: HostOptions{
+				DefaultScheme: "http",
+				DefaultTLS: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedScheme: "http",
+			usesFallback:   false,
+		},
+		{
+			host: "localhost",
+			opts: HostOptions{
+				DefaultScheme: "http",
+				DefaultTLS: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedScheme: "http",
+			usesFallback:   false,
+		},
+		{
+			host: "localhost",
+			opts: HostOptions{
+				DefaultScheme: "https",
+				DefaultTLS: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+		{
+			host:           "localhost",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+		{
+			host:           "localhost:5000",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   true,
+		},
+		{
+			host:           "example.com",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   false,
 		},
-	}
-	hosts := ConfigureHosts(context.TODO(), opts)
 
-	testHosts, err := hosts("localhost:8080")
+		{
-	if err != nil {
+			host: "example.com",
-		t.Fatal(err)
+			opts: HostOptions{
-	}
+				DefaultScheme: "http",
-	if len(testHosts) != 1 {
+				DefaultTLS: &tls.Config{
-		t.Fatalf("expected a single host for localhost config, got %d hosts", len(testHosts))
+					InsecureSkipVerify: true,
-	}
+				},
-	if testHosts[0].Scheme != "https" {
+			},
-		t.Fatalf("expected https scheme for localhost with tls config, got %q", testHosts[0].Scheme)
+			expectedScheme: "http",
-	}
+			usesFallback:   false,
-	_, ok := testHosts[0].Client.Transport.(docker.HTTPFallback)
+		},
-	if !ok {
+		{
-		t.Fatal("expected http fallback configured for defaulted localhost endpoint")
+			host: "example.com:5000",
+			opts: HostOptions{
+				DefaultScheme: "http",
+				DefaultTLS: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedScheme: "https",
+			usesFallback:   true,
+		},
+		{
+			host:           "example.com:5000",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+		{
+			host: "example2.com",
+			opts: HostOptions{
+				DefaultScheme: "http",
+			},
+			expectedScheme: "http",
+			usesFallback:   false,
+		},
+		{
+			host:           "127.0.0.254:5000",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   true,
+		},
+		{
+			host:           "127.0.0.254",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+		{
+			host:           "[::1]:5000",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   true,
+		},
+		{
+			host:           "::1",
+			opts:           HostOptions{},
+			expectedScheme: "https",
+			usesFallback:   false,
+		},
+	} {
+		testName := tc.host
+		if tc.opts.DefaultScheme != "" {
+			testName = testName + "-default-" + tc.opts.DefaultScheme
+		}
+		t.Run(testName, func(t *testing.T) {
+			ctx := logtest.WithT(context.TODO(), t)
+			hosts := ConfigureHosts(ctx, tc.opts)
+			testHosts, err := hosts(tc.host)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(testHosts) != 1 {
+				t.Fatalf("expected a single host for localhost config, got %d hosts", len(testHosts))
+			}
+			if testHosts[0].Scheme != tc.expectedScheme {
+				t.Fatalf("expected %s scheme for localhost with tls config, got %q", tc.expectedScheme, testHosts[0].Scheme)
+			}
+			_, ok := testHosts[0].Client.Transport.(docker.HTTPFallback)
+			if tc.usesFallback && !ok {
+				t.Fatal("expected http fallback configured for defaulted localhost endpoint")
+			} else if ok && !tc.usesFallback {
+				t.Fatal("expected no http fallback configured for defaulted localhost endpoint")
+			}
+		})
 	}
 }