Skip to content

Commit

Permalink
Merge pull request containerd#9299 from dmcgowan/1.7-fix-ambiguous-tl…
Browse files Browse the repository at this point in the history 
…s-fallback

[release/1.7] Fix ambiguous tls fallback
  • Loading branch information
@AkihiroSuda
AkihiroSuda committed Oct 26, 2023
2 parents bb65a3a + 68abc54 commit 6a9a187
Show file tree
Hide file tree
Showing 4 changed files with 270 additions and 40 deletions.
46 changes: 31 additions & 15 deletions remotes/docker/config/hosts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,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
}
Expand All @@ -122,13 +132,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{}
Expand Down Expand Up @@ -167,7 +177,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
Expand Down Expand Up @@ -235,13 +245,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
Expand Down
201 changes: 182 additions & 19 deletions remotes/docker/config/hosts_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,27 +285,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")
}
})
}
}

Expand Down
11 changes: 7 additions & 4 deletions remotes/docker/pusher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,16 @@ func (p dockerPusher) push(ctx context.Context, desc ocispec.Descriptor, ref str
}

if lurl.Host != lhost.Host || lhost.Scheme != lurl.Scheme {

lhost.Scheme = lurl.Scheme
lhost.Host = lurl.Host
log.G(ctx).WithField("host", lhost.Host).WithField("scheme", lhost.Scheme).Debug("upload changed destination")

// Strip authorizer if change to host or scheme
lhost.Authorizer = nil
// Check if different than what was requested, accounting for fallback in the transport layer
requested := resp.Request.URL
if requested.Host != lhost.Host || requested.Scheme != lhost.Scheme {
// Strip authorizer if change to host or scheme
lhost.Authorizer = nil
log.G(ctx).WithField("host", lhost.Host).WithField("scheme", lhost.Scheme).Debug("upload changed destination, authorizer removed")
}
}
}
q := lurl.Query()
Expand Down
52 changes: 50 additions & 2 deletions remotes/docker/pusher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/containerd/containerd/errdefs"
"github.com/containerd/containerd/reference"
"github.com/containerd/containerd/remotes"
"github.com/containerd/log/logtest"
"github.com/opencontainers/go-digest"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -86,6 +87,42 @@ func TestPusherErrClosedRetry(t *testing.T) {
}
}

func TestPusherHTTPFallback(t *testing.T) {
ctx := logtest.WithT(context.Background(), t)

p, reg, _, done := samplePusher(t)
defer done()

reg.uploadable = true
reg.username = "testuser"
reg.secret = "testsecret"
reg.locationPrefix = p.hosts[0].Scheme + "://" + p.hosts[0].Host

p.hosts[0].Scheme = "https"
client := p.hosts[0].Client
if client == nil {
clientC := *http.DefaultClient
client = &clientC
}
if client.Transport == nil {
client.Transport = http.DefaultTransport
}
client.Transport = HTTPFallback{client.Transport}
p.hosts[0].Client = client
phost := p.hosts[0].Host
p.hosts[0].Authorizer = NewDockerAuthorizer(WithAuthCreds(func(host string) (string, string, error) {
if host == phost {
return "testuser", "testsecret", nil
}
return "", "", nil
}))

layerContent := []byte("test")
if err := tryUpload(ctx, t, p, layerContent); err != nil {
t.Errorf("upload failed: %v", err)
}
}

// TestPusherErrReset tests the push method if the request needs to be retried
// i.e when ErrReset occurs
func TestPusherErrReset(t *testing.T) {
Expand Down Expand Up @@ -189,9 +226,20 @@ type uploadableMockRegistry struct {
availableContents []string
uploadable bool
putHandlerFunc func(w http.ResponseWriter, r *http.Request) bool
locationPrefix string
username string
secret string
}

func (u *uploadableMockRegistry) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if u.secret != "" {
user, pass, ok := r.BasicAuth()
if !ok || user != u.username || pass != u.secret {
w.Header().Add("WWW-Authenticate", "basic realm=test")
w.WriteHeader(http.StatusUnauthorized)
return
}
}
if r.Method == http.MethodPut && u.putHandlerFunc != nil {
// if true return the response witout calling default handler
if u.putHandlerFunc(w, r) {
Expand All @@ -205,9 +253,9 @@ func (u *uploadableMockRegistry) defaultHandler(w http.ResponseWriter, r *http.R
if r.Method == http.MethodPost {
if matches := blobUploadRegexp.FindStringSubmatch(r.URL.Path); len(matches) != 0 {
if u.uploadable {
w.Header().Set("Location", "/upload")
w.Header().Set("Location", u.locationPrefix+"/upload")
} else {
w.Header().Set("Location", "/cannotupload")
w.Header().Set("Location", u.locationPrefix+"/cannotupload")
}

dgstr := digest.Canonical.Digester()
Expand Down

0 comments on commit 6a9a187

Please sign in to comment.