From d48ceb6065ceb3fe2a7aa53bf97322d182a3838e Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Sun, 22 Oct 2023 22:09:16 -0700 Subject: [PATCH] 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 --- remotes/docker/config/hosts.go | 46 ++++--- remotes/docker/config/hosts_test.go | 201 +++++++++++++++++++++++++--- 2 files changed, 213 insertions(+), 34 deletions(-) diff --git a/remotes/docker/config/hosts.go b/remotes/docker/config/hosts.go index b43ea0aa1..0ee1fd869 100644 --- a/remotes/docker/config/hosts.go +++ b/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" { - hosts[len(hosts)-1].scheme = "http" + if options.DefaultScheme == "" { + _, 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 - var skipVerify = true - hosts[len(hosts)-1].skipVerify = &skipVerify + // When port is 80, protocol is not ambiguous + if port != "80" { + // 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 - // docker.HTTPFallback 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. + // 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 + // 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" - rhosts[i].Client.Transport = docker.HTTPFallback{RoundTripper: rhosts[i].Client.Transport} + _, port, _ := net.SplitHostPort(host.host) + 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 diff --git a/remotes/docker/config/hosts_test.go b/remotes/docker/config/hosts_test.go index 28ab41249..2641c6dac 100644 --- a/remotes/docker/config/hosts_test.go +++ b/remotes/docker/config/hosts_test.go @@ -295,27 +295,190 @@ func TestLoadCertFiles(t *testing.T) { } } -func TestLocalhostHTTPFallback(t *testing.T) { - opts := HostOptions{ - DefaultTLS: &tls.Config{ - InsecureSkipVerify: true, +func TestHTTPFallback(t *testing.T) { + for _, tc := range []struct { + host string + 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 { - 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 != "https" { - t.Fatalf("expected https scheme for localhost with tls config, got %q", testHosts[0].Scheme) - } - _, ok := testHosts[0].Client.Transport.(docker.HTTPFallback) - if !ok { - t.Fatal("expected http fallback configured for defaulted localhost endpoint") + { + host: "example.com", + opts: HostOptions{ + DefaultScheme: "http", + DefaultTLS: &tls.Config{ + InsecureSkipVerify: true, + }, + }, + expectedScheme: "http", + usesFallback: false, + }, + { + 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") + } + }) } }