Merge pull request containerd#10109 from dmcgowan/fix-fallback-explicit-tls

Update HTTP fallback to better account for TLS timeout and previous attempts

Author: mxpv

cmd/ctr/commands/resolver.go

@@ -149,6 +149,11 @@ func resolverDefaultTLS(clicontext *cli.Context) (*tls.Config, error) {
 		config.Certificates = []tls.Certificate{keyPair}
 	}
 
+	// If nothing was set, return nil rather than empty config
+	if !config.InsecureSkipVerify && config.RootCAs == nil && config.Certificates == nil {
+		return nil, nil
+	}
+
 	return config, nil
 }
 

core/remotes/docker/config/hosts.go

@@ -244,7 +244,7 @@ func ConfigureHosts(ctx context.Context, options HostOptions) docker.RegistryHos
 
 			// When TLS has been configured for the operation or host and
 			// the protocol from the port number is ambiguous, use the
-			// docker.HTTPFallback roundtripper to catch TLS errors and re-attempt the
+			// docker.NewHTTPFallback roundtripper to catch TLS errors and re-attempt the
 			// 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.
@@ -253,7 +253,7 @@ func ConfigureHosts(ctx context.Context, options HostOptions) docker.RegistryHos
 				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].Client.Transport = docker.NewHTTPFallback(rhosts[i].Client.Transport)
 				}
 			}
 

core/remotes/docker/config/hosts_test.go

@@ -472,11 +472,11 @@ func TestHTTPFallback(t *testing.T) {
 			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 {
+			_, defaultTransport := testHosts[0].Client.Transport.(*http.Transport)
+			if tc.usesFallback && defaultTransport {
 				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")
+			} else if !defaultTransport && !tc.usesFallback {
+				t.Fatalf("expected no http fallback configured for defaulted localhost endpoint")
 			}
 		})
 	}

core/remotes/docker/pusher_test.go

@@ -110,7 +110,7 @@ func TestPusherHTTPFallback(t *testing.T) {
 	if client.Transport == nil {
 		client.Transport = http.DefaultTransport
 	}
-	client.Transport = HTTPFallback{client.Transport}
+	client.Transport = NewHTTPFallback(client.Transport)
 	p.hosts[0].Client = client
 	phost := p.hosts[0].Host
 	p.hosts[0].Authorizer = NewDockerAuthorizer(WithAuthCreds(func(host string) (string, string, error) {

core/remotes/docker/resolver.go

@@ -714,34 +714,62 @@ func IsLocalhost(host string) bool {
 	return ip.IsLoopback()
 }
 
-// HTTPFallback is an http.RoundTripper which allows fallback from https to http
-// for registry endpoints with configurations for both http and TLS, such as
-// defaulted localhost endpoints.
-type HTTPFallback struct {
-	http.RoundTripper
+// NewHTTPFallback returns http.RoundTripper which allows fallback from https to
+// http for registry endpoints with configurations for both http and TLS,
+// such as defaulted localhost endpoints.
+func NewHTTPFallback(transport http.RoundTripper) http.RoundTripper {
+	return &httpFallback{
+		super: transport,
+	}
 }
 
-func (f HTTPFallback) RoundTrip(r *http.Request) (*http.Response, error) {
-	resp, err := f.RoundTripper.RoundTrip(r)
-	var tlsErr tls.RecordHeaderError
-	if errors.As(err, &tlsErr) && string(tlsErr.RecordHeader[:]) == "HTTP/" {
-		// server gave HTTP response to HTTPS client
-		plainHTTPUrl := *r.URL
-		plainHTTPUrl.Scheme = "http"
+type httpFallback struct {
+	super http.RoundTripper
+	host  string
+}
+
+func (f *httpFallback) RoundTrip(r *http.Request) (*http.Response, error) {
+	// only fall back if the same host had previously fell back
+	if f.host != r.URL.Host {
+		resp, err := f.super.RoundTrip(r)
+		if !isTLSError(err) {
+			return resp, err
+		}
+	}
+
+	plainHTTPUrl := *r.URL
+	plainHTTPUrl.Scheme = "http"
 
-		plainHTTPRequest := *r
-		plainHTTPRequest.URL = &plainHTTPUrl
+	plainHTTPRequest := *r
+	plainHTTPRequest.URL = &plainHTTPUrl
 
+	if f.host != r.URL.Host {
+		f.host = r.URL.Host
+
+		// update body on the second attempt
 		if r.Body != nil && r.GetBody != nil {
 			body, err := r.GetBody()
 			if err != nil {
 				return nil, err
 			}
 			plainHTTPRequest.Body = body
 		}
+	}
 
-		return f.RoundTripper.RoundTrip(&plainHTTPRequest)
+	return f.super.RoundTrip(&plainHTTPRequest)
+}
+
+func isTLSError(err error) bool {
+	if err == nil {
+		return false
+	}
+	var tlsErr tls.RecordHeaderError
+	if errors.As(err, &tlsErr) && string(tlsErr.RecordHeader[:]) == "HTTP/" {
+		return true
+	}
+	if strings.Contains(err.Error(), "TLS handshake timeout") {
+		return true
 	}
 
-	return resp, err
+	return false
 }

core/remotes/docker/resolver_test.go

@@ -24,6 +24,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -406,7 +407,7 @@ func TestHTTPFallbackResolver(t *testing.T) {
 		}
 
 		client := &http.Client{
-			Transport: HTTPFallback{http.DefaultTransport},
+			Transport: NewHTTPFallback(http.DefaultTransport),
 		}
 		options := ResolverOptions{
 			Hosts: func(host string) ([]RegistryHost, error) {
@@ -427,6 +428,59 @@ func TestHTTPFallbackResolver(t *testing.T) {
 	runBasicTest(t, "testname", sf)
 }
 
+func TestHTTPFallbackTimeoutResolver(t *testing.T) {
+	sf := func(h http.Handler) (string, ResolverOptions, func()) {
+
+		l, err := net.Listen("tcp", "127.0.0.1:0")
+		if err != nil {
+			t.Fatal(err)
+		}
+		server := &http.Server{
+			Handler:           h,
+			ReadHeaderTimeout: time.Second,
+		}
+		go func() {
+			// Accept first connection but do not do anything with it
+			// to force TLS handshake to timeout. Subsequent connection
+			// will be HTTP and should work.
+			c, err := l.Accept()
+			if err != nil {
+				return
+			}
+			go func() {
+				time.Sleep(time.Second)
+				c.Close()
+			}()
+			server.Serve(l)
+		}()
+		host := l.Addr().String()
+
+		defaultTransport := &http.Transport{
+			TLSHandshakeTimeout: time.Millisecond,
+		}
+		client := &http.Client{
+			Transport: NewHTTPFallback(defaultTransport),
+		}
+
+		options := ResolverOptions{
+			Hosts: func(host string) ([]RegistryHost, error) {
+				return []RegistryHost{
+					{
+						Client:       client,
+						Host:         host,
+						Scheme:       "https",
+						Path:         "/v2",
+						Capabilities: HostCapabilityPull | HostCapabilityResolve | HostCapabilityPush,
+					},
+				}, nil
+			},
+		}
+		return host, options, func() { l.Close() }
+	}
+
+	runBasicTest(t, "testname", sf)
+}
+
 func TestResolveProxy(t *testing.T) {
 	var (
 		ctx  = context.Background()