From 38ffa807c2c974622d19b6a11eed0b2c0af668f1 Mon Sep 17 00:00:00 2001
From: aler9 <46489434+aler9@users.noreply.github.com>
Date: Tue, 22 Oct 2024 21:45:52 +0200
Subject: [PATCH] fix interacting with insecure HTTPS registries

The Docker daemon allows to interact with insecure registries served
through plain HTTP or served through HTTPS with self-signed
certificates, when the target registry is included inside
"insecureRegistries". In this library it should be possible to interact
with insecure registries likewise by using the "name.Insecure" option
when creating references.

Nonetheless it's currently not possible to interact with insecure
registries served with HTTPS and self-signed certificates, since the
TLS certificate is checked anyway and an "invalid certificate" error is
returned.

A common workaround consists into passing a tls.Config with
InsecureSkipVerify set to true, but this disables TLS validation for
every HTTP request, while the desired behavior is disabling TLS
validation only when "name.Insecure" is in use.

This patch changes the default "remote" options in order to provide a
default tls.Config with InsecureSkipVerify set to true if and only if
"name.Insecure" is in use.

This also fixes bugs in dependent tools like Skaffold, that are using
"name.Insecure", are not using InsecureSKipVerify and are expecting to
be able to interact with insecure registries anyway.
---
 pkg/name/registry.go          |  5 ++
 pkg/v1/remote/catalog.go      |  4 +-
 pkg/v1/remote/delete.go       |  2 +-
 pkg/v1/remote/descriptor.go   |  4 +-
 pkg/v1/remote/layer.go        |  2 +-
 pkg/v1/remote/list.go         |  2 +-
 pkg/v1/remote/multi_write.go  |  2 +-
 pkg/v1/remote/options.go      | 34 ++++++++++++-
 pkg/v1/remote/options_test.go | 95 +++++++++++++++++++++++++++++++++++
 pkg/v1/remote/puller.go       |  2 +-
 pkg/v1/remote/pusher.go       |  2 +-
 pkg/v1/remote/referrers.go    |  2 +-
 pkg/v1/remote/write.go        |  6 +--
 13 files changed, 146 insertions(+), 16 deletions(-)
 create mode 100644 pkg/v1/remote/options_test.go

diff --git a/pkg/name/registry.go b/pkg/name/registry.go
index 5b0d01769..f73f1ef0e 100644
--- a/pkg/name/registry.go
+++ b/pkg/name/registry.go
@@ -77,6 +77,11 @@ func (r Registry) isRFC1918() bool {
 	return false
 }
 
+// IsInsecure checks whether the registry is insecure.
+func (r Registry) IsInsecure() bool {
+	return r.insecure
+}
+
 // Scheme returns https scheme for all the endpoints except localhost or when explicitly defined.
 func (r Registry) Scheme() string {
 	if r.insecure {
diff --git a/pkg/v1/remote/catalog.go b/pkg/v1/remote/catalog.go
index a0281b9fd..c51fe4a64 100644
--- a/pkg/v1/remote/catalog.go
+++ b/pkg/v1/remote/catalog.go
@@ -32,7 +32,7 @@ type Catalogs struct {
 
 // CatalogPage calls /_catalog, returning the list of repositories on the registry.
 func CatalogPage(target name.Registry, last string, n int, options ...Option) ([]string, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(target, options...)
 	if err != nil {
 		return nil, err
 	}
@@ -73,7 +73,7 @@ func CatalogPage(target name.Registry, last string, n int, options ...Option) ([
 
 // Catalog calls /_catalog, returning the list of repositories on the registry.
 func Catalog(ctx context.Context, target name.Registry, options ...Option) ([]string, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(target, options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/delete.go b/pkg/v1/remote/delete.go
index 36e1d0816..91da77737 100644
--- a/pkg/v1/remote/delete.go
+++ b/pkg/v1/remote/delete.go
@@ -20,7 +20,7 @@ import (
 
 // Delete removes the specified image reference from the remote registry.
 func Delete(ref name.Reference, options ...Option) error {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/v1/remote/descriptor.go b/pkg/v1/remote/descriptor.go
index fafe910e9..26363e3bd 100644
--- a/pkg/v1/remote/descriptor.go
+++ b/pkg/v1/remote/descriptor.go
@@ -79,7 +79,7 @@ func Get(ref name.Reference, options ...Option) (*Descriptor, error) {
 // Note that the server response will not have a body, so any errors encountered
 // should be retried with Get to get more details.
 func Head(ref name.Reference, options ...Option) (*v1.Descriptor, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return nil, err
 	}
@@ -90,7 +90,7 @@ func Head(ref name.Reference, options ...Option) (*v1.Descriptor, error) {
 // Handle options and fetch the manifest with the acceptable MediaTypes in the
 // Accept header.
 func get(ref name.Reference, acceptable []types.MediaType, options ...Option) (*Descriptor, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/layer.go b/pkg/v1/remote/layer.go
index 39c205950..e10081074 100644
--- a/pkg/v1/remote/layer.go
+++ b/pkg/v1/remote/layer.go
@@ -69,7 +69,7 @@ func (rl *remoteLayer) Exists() (bool, error) {
 // digest of the blob to be read and the repository portion is the repo where
 // that blob lives.
 func Layer(ref name.Digest, options ...Option) (v1.Layer, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/list.go b/pkg/v1/remote/list.go
index 910d2a94c..5b70de81f 100644
--- a/pkg/v1/remote/list.go
+++ b/pkg/v1/remote/list.go
@@ -36,7 +36,7 @@ func ListWithContext(ctx context.Context, repo name.Repository, options ...Optio
 // List calls /tags/list for the given repository, returning the list of tags
 // in the "tags" property.
 func List(repo name.Repository, options ...Option) ([]string, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(repo, options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/multi_write.go b/pkg/v1/remote/multi_write.go
index a6705de89..5077f57cb 100644
--- a/pkg/v1/remote/multi_write.go
+++ b/pkg/v1/remote/multi_write.go
@@ -23,7 +23,7 @@ import (
 // efficiently as possible, by deduping shared layer blobs while uploading them
 // in parallel.
 func MultiWrite(todo map[name.Reference]Taggable, options ...Option) (rerr error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(nil, options...)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/v1/remote/options.go b/pkg/v1/remote/options.go
index 99a2bb2eb..554f969b3 100644
--- a/pkg/v1/remote/options.go
+++ b/pkg/v1/remote/options.go
@@ -16,6 +16,7 @@ package remote
 
 import (
 	"context"
+	"crypto/tls"
 	"errors"
 	"io"
 	"net"
@@ -26,6 +27,7 @@ import (
 	"github.com/google/go-containerregistry/internal/retry"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/logs"
+	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
 )
@@ -125,9 +127,37 @@ var DefaultTransport http.RoundTripper = &http.Transport{
 	MaxIdleConnsPerHost: 50,
 }
 
-func makeOptions(opts ...Option) (*options, error) {
+// create transport once to allow connection sharing.
+var defaultTransportInsecure = func() http.RoundTripper {
+	tr := DefaultTransport.(*http.Transport).Clone()
+	tr.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
+	return tr
+}()
+
+func targetIsInsecure(target resource) bool {
+	switch target := target.(type) {
+	case name.Registry:
+		return target.IsInsecure()
+
+	case name.Repository:
+		return target.Registry.IsInsecure()
+
+	case name.Digest:
+		return target.Registry.IsInsecure()
+	}
+	return false
+}
+
+func makeOptions(target resource, opts ...Option) (*options, error) {
+	var tr http.RoundTripper
+	if target != nil && targetIsInsecure(target) {
+		tr = defaultTransportInsecure
+	} else {
+		tr = DefaultTransport
+	}
+
 	o := &options{
-		transport:        DefaultTransport,
+		transport:        tr,
 		platform:         defaultPlatform,
 		context:          context.Background(),
 		jobs:             defaultJobs,
diff --git a/pkg/v1/remote/options_test.go b/pkg/v1/remote/options_test.go
new file mode 100644
index 000000000..8563805d1
--- /dev/null
+++ b/pkg/v1/remote/options_test.go
@@ -0,0 +1,95 @@
+package remote
+
+import (
+	"crypto/tls"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+
+	"github.com/google/go-containerregistry/pkg/name"
+)
+
+func TestOptionsInsecure(t *testing.T) {
+	for _, targetType := range []string{
+		"registry",
+		"repository",
+		"digest",
+	} {
+		for _, mode := range []string{
+			"secure",
+			"insecure",
+		} {
+			t.Run(targetType+"_"+mode, func(t *testing.T) {
+				server := httptest.NewTLSServer(
+					http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+						w.WriteHeader(http.StatusOK)
+					}))
+				defer server.Close()
+
+				u, err := url.Parse(server.URL)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				options := []name.Option{}
+
+				if mode == "insecure" {
+					options = append(options, name.Insecure)
+				}
+
+				var target resource
+
+				switch targetType {
+				case "registry":
+					reg, err := name.NewRegistry("myregistry", options...)
+					if err != nil {
+						t.Fatal(err)
+					}
+					target = reg
+
+				case "repository":
+					ref, err := name.ParseReference("myregistry/name:tag", options...)
+					if err != nil {
+						t.Fatal(err)
+					}
+					target = ref.Context()
+
+				case "digest":
+					d, err := name.NewDigest("myregistry/name@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", options...)
+					if err != nil {
+						t.Fatal(err)
+					}
+					target = d
+				}
+
+				opts, err := makeOptions(target, []Option{}...)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				c := &http.Client{Transport: opts.transport}
+
+				res, err := c.Get(u.String())
+
+				if mode == "secure" {
+					if ue, ok := err.(*url.Error); !ok {
+						t.Fatal(err)
+					} else if _, ok := ue.Err.(*tls.CertificateVerificationError); !ok {
+						t.Fatal(err)
+					}
+				} else {
+					if err != nil {
+						t.Fatal(err)
+					}
+					defer res.Body.Close()
+
+					if res.StatusCode != http.StatusOK {
+						t.Fatal(fmt.Printf("unexpected status code: %d", res.StatusCode))
+					}
+				}
+			})
+		}
+	}
+}
diff --git a/pkg/v1/remote/puller.go b/pkg/v1/remote/puller.go
index 7da8017ee..5f0a1185a 100644
--- a/pkg/v1/remote/puller.go
+++ b/pkg/v1/remote/puller.go
@@ -32,7 +32,7 @@ type Puller struct {
 }
 
 func NewPuller(options ...Option) (*Puller, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(nil, options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/pusher.go b/pkg/v1/remote/pusher.go
index 332d8ca0a..8b4619823 100644
--- a/pkg/v1/remote/pusher.go
+++ b/pkg/v1/remote/pusher.go
@@ -99,7 +99,7 @@ type Pusher struct {
 }
 
 func NewPusher(options ...Option) (*Pusher, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(nil, options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/referrers.go b/pkg/v1/remote/referrers.go
index 48e3835f9..828786c11 100644
--- a/pkg/v1/remote/referrers.go
+++ b/pkg/v1/remote/referrers.go
@@ -34,7 +34,7 @@ import (
 //
 // The subject manifest doesn't have to exist in the registry for there to be descriptors that refer to it.
 func Referrers(d name.Digest, options ...Option) (v1.ImageIndex, error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(d, options...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go
index 1167cb793..502e66ce6 100644
--- a/pkg/v1/remote/write.go
+++ b/pkg/v1/remote/write.go
@@ -654,7 +654,7 @@ func WriteIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) (rerr e
 
 // WriteLayer uploads the provided Layer to the specified repo.
 func WriteLayer(repo name.Repository, layer v1.Layer, options ...Option) (rerr error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(repo, options...)
 	if err != nil {
 		return err
 	}
@@ -691,7 +691,7 @@ func Tag(tag name.Tag, t Taggable, options ...Option) error {
 // should ensure that all blobs or manifests that are referenced by t exist
 // in the target registry.
 func Put(ref name.Reference, t Taggable, options ...Option) error {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return err
 	}
@@ -700,7 +700,7 @@ func Put(ref name.Reference, t Taggable, options ...Option) error {
 
 // Push uploads the given Taggable to the specified reference.
 func Push(ref name.Reference, t Taggable, options ...Option) (rerr error) {
-	o, err := makeOptions(options...)
+	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return err
 	}