Repository: google/go-containerregistry
Pull request #864 (2020-12-08 18:04:38)

Fix misleading Authorization: <redacted>
This is currently always set, but we should only set it if there are
actually credentials being redacted, so that we can tell if the header
was set or not.

Noticed this in https://github.com/google/go-containerregistry/issues/861


Patch:
diff --git a/pkg/v1/remote/transport/logger.go b/pkg/v1/remote/transport/logger.go
index b7e0c60d..3b024866 100644
--- a/pkg/v1/remote/transport/logger.go
+++ b/pkg/v1/remote/transport/logger.go
@@ -47,7 +47,7 @@ func (t *logTransport) RoundTrip(in *http.Request) (out *http.Response, err erro
 
 	// Save these headers so we can redact Authorization.
 	savedHeaders := in.Header.Clone()
-	if in.Header != nil {
+	if in.Header != nil && in.Header.Get("authorization") != "" {
 		in.Header.Set("authorization", "<redacted>")
 	}
 
diff --git a/pkg/v1/remote/transport/ping.go b/pkg/v1/remote/transport/ping.go
index 6ed1d464..396d4e03 100644
--- a/pkg/v1/remote/transport/ping.go
+++ b/pkg/v1/remote/transport/ping.go
@@ -16,6 +16,7 @@ package transport
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -78,7 +79,7 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 		schemes = append(schemes, "http")
 	}
 
-	var connErr error
+	var errs []string
 	for _, scheme := range schemes {
 		url := fmt.Sprintf("%s://%s/v2/", scheme, reg.Name())
 		req, err := http.NewRequest(http.MethodGet, url, nil)
@@ -87,7 +88,7 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 		}
 		resp, err := client.Do(req.WithContext(ctx))
 		if err != nil {
-			connErr = err
+			errs = append(errs, err.Error())
 			// Potentially retry with http.
 			continue
 		}
@@ -124,5 +125,5 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 			return nil, CheckError(resp, http.StatusOK, http.StatusUnauthorized)
 		}
 	}
-	return nil, connErr
+	return nil, errors.New(strings.Join(errs, "; "))
 }
diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go
index 521b2250..d850203d 100644
--- a/pkg/v1/remote/write.go
+++ b/pkg/v1/remote/write.go
@@ -448,13 +448,13 @@ type withMediaType interface {
 //
 // Use reflection to either pull the v1.Descriptor out of remote.Descriptor or
 // create a descriptor based on the RawManifest and (optionally) MediaType.
-func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
+func unpackTaggable(t Taggable) ([]byte, *v1.Descriptor, error) {
 	if d, ok := t.(*Descriptor); ok {
-		return &d.Descriptor, nil
+		return d.Manifest, &d.Descriptor, nil
 	}
 	b, err := t.RawManifest()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// A reasonable default if Taggable doesn't implement MediaType.
@@ -463,17 +463,17 @@ func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
 	if wmt, ok := t.(withMediaType); ok {
 		m, err := wmt.MediaType()
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		mt = m
 	}
 
 	h, sz, err := v1.SHA256(bytes.NewReader(b))
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return &v1.Descriptor{
+	return b, &v1.Descriptor{
 		MediaType: mt,
 		Size:      sz,
 		Digest:    h,
@@ -482,11 +482,7 @@ func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
 
 // commitManifest does a PUT of the image's manifest.
 func (w *writer) commitManifest(t Taggable, ref name.Reference) error {
-	raw, err := t.RawManifest()
-	if err != nil {
-		return err
-	}
-	desc, err := unpackTaggable(t)
+	raw, desc, err := unpackTaggable(t)
 	if err != nil {
 		return err
 	}
@@ -511,7 +507,7 @@ func (w *writer) commitManifest(t Taggable, ref name.Reference) error {
 	}
 
 	// The image was successfully pushed!
-	logs.Progress.Printf("%v: digest: %v size: %d", ref, desc.Digest, len(raw))
+	logs.Progress.Printf("%v: digest: %v size: %d", ref, desc.Digest, desc.Size)
 	return nil
 }
 
@@ -581,7 +577,17 @@ func WriteLayer(repo name.Repository, layer v1.Layer, options ...Option) error {
 	return w.uploadOne(layer)
 }
 
-// Tag adds a tag to the given Taggable.
+// Tag adds a tag to the given Taggable via PUT /v2/.../manifests/<tag>
+//
+// Notable implementations of Taggable are v1.Image, v1.ImageIndex, and
+// remote.Descriptor.
+//
+// If t implements MediaType, we will use that for the Content-Type, otherwise
+// we will default to types.DockerManifestSchema2.
+//
+// Tag does not attempt to write anything other than the manifest, so callers
+// should ensure that all blobs or manifests that are referenced by t exist
+// in the target registry.
 func Tag(tag name.Tag, t Taggable, options ...Option) error {
 	o, err := makeOptions(tag.Context(), options...)
 	if err != nil {


Test patch:
diff --git a/pkg/v1/remote/transport/ping_test.go b/pkg/v1/remote/transport/ping_test.go
index 6131cf28..aa3a42f3 100644
--- a/pkg/v1/remote/transport/ping_test.go
+++ b/pkg/v1/remote/transport/ping_test.go
@@ -19,6 +19,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -169,22 +170,26 @@ func TestPingHttpFallback(t *testing.T) {
 	tests := []struct {
 		reg       name.Registry
 		wantCount int
+		err       string
+		contains  []string
 	}{{
 		reg:       mustRegistry("gcr.io"),
 		wantCount: 1,
+		err:       `Get "https://gcr.io/v2/": http: server gave HTTP response to HTTPS client`,
 	}, {
 		reg:       mustRegistry("ko.local"),
 		wantCount: 2,
 	}, {
 		reg:       mustInsecureRegistry("us.gcr.io"),
-		wantCount: 2,
+		wantCount: 0,
+		contains:  []string{"https://us.gcr.io/v2/", "http://us.gcr.io/v2/"},
 	}}
 
 	gotCount := 0
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			gotCount++
-			http.Error(w, "Forbidden", http.StatusForbidden)
+			w.WriteHeader(http.StatusOK)
 		}))
 	defer server.Close()
 
@@ -195,15 +200,32 @@ func TestPingHttpFallback(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		pr, err := ping(context.Background(), test.reg, tprt)
-		if err == nil {
-			t.Errorf("ping() = %v", pr)
+		// This is the last one, fatal error it.
+		if strings.Contains(test.reg.String(), "us.gcr.io") {
+			server.Close()
 		}
 
-		if test.wantCount != gotCount {
-			t.Errorf("%s: got %d requests, wanted %d", test.reg.String(), test.wantCount, gotCount)
+		_, err := ping(context.Background(), test.reg, tprt)
+		if got, want := gotCount, test.wantCount; got != want {
+			t.Errorf("%s: got %d requests, wanted %d", test.reg.String(), got, want)
 		}
 		gotCount = 0
+
+		if err == nil {
+			if test.err != "" {
+				t.Error("expected err, got nil")
+			}
+			continue
+		}
+		if len(test.contains) != 0 {
+			for _, c := range test.contains {
+				if !strings.Contains(err.Error(), c) {
+					t.Errorf("expected err to contain %q but did not: %q", c, err)
+				}
+			}
+		} else if got, want := err.Error(), test.err; got != want {
+			t.Errorf("got %q want %q", got, want)
+		}
 	}
 }