Avoid TLS fallback when protocol is not ambiguous

The TLS fallback should only be used when the protocol is ambiguous due
to provided TLS configurations and defaulting to http. Do not add TLS
configurations when defaulting to http. When the port is 80 or will be
defaulted to 80, there is no protocol ambiguity and TLS fallback should
not be used.

Signed-off-by: Derek McGowan <derek@mcg.dev>

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")
+			}
+		})
 	}
 }