Repository: google/go-containerregistry
Pull request #575 (2019-10-16 20:02:28)

Always wrap transports for scheme and useragent
These were only set for the bearer transport, but we care about basic
and anonymous as well.

Context: https://github.com/GoogleContainerTools/kaniko/pull/817

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=h1) Report
> Merging [#575](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=desc) into [master](https://codecov.io/gh/google/go-containerregistry/commit/d30598c4fd374f6b1b56e70dfa52c06a90a8ce75?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `92%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/575/graphs/tree.svg?width=650&token=wG7iiDCdZ4&height=150&src=pr)](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master    #575      +/-   ##
=========================================
+ Coverage   73.02%   73.1%   +0.07%     
=========================================
  Files          97      99       +2     
  Lines        4389    4402      +13     
=========================================
+ Hits         3205    3218      +13     
  Misses        777     777              
  Partials      407     407
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/transport/schemer.go](https://codecov.io/gh/google/go-containerregistry/pull/575/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvc2NoZW1lci5nbw==) | `100% <100%> (ø)` | |
| [pkg/v1/remote/transport/transport.go](https://codecov.io/gh/google/go-containerregistry/pull/575/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvdHJhbnNwb3J0Lmdv) | `94.28% <100%> (+1.18%)` | :arrow_up: |
| [pkg/v1/remote/transport/useragent.go](https://codecov.io/gh/google/go-containerregistry/pull/575/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvdXNlcmFnZW50Lmdv) | `100% <100%> (ø)` | |
| [pkg/v1/remote/transport/bearer.go](https://codecov.io/gh/google/go-containerregistry/pull/575/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvYmVhcmVyLmdv) | `75.43% <83.33%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=footer). Last update [d30598c...20a7580](https://codecov.io/gh/google/go-containerregistry/pull/575?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/v1/remote/transport/bearer.go b/pkg/v1/remote/transport/bearer.go
index 74e59706..bff2ece0 100644
--- a/pkg/v1/remote/transport/bearer.go
+++ b/pkg/v1/remote/transport/bearer.go
@@ -55,30 +55,19 @@ var portMap = map[string]string{
 // RoundTrip implements http.RoundTripper
 func (bt *bearerTransport) RoundTrip(in *http.Request) (*http.Response, error) {
 	sendRequest := func() (*http.Response, error) {
-		auth, err := bt.bearer.Authorization()
-		if err != nil {
-			return nil, err
-		}
-
 		// http.Client handles redirects at a layer above the http.RoundTripper
 		// abstraction, so to avoid forwarding Authorization headers to places
 		// we are redirected, only set it when the authorization header matches
 		// the registry with which we are interacting.
 		// In case of redirect http.Client can use an empty Host, check URL too.
-		canonicalHeaderHost := bt.canonicalAddress(in.Host)
-		canonicalURLHost := bt.canonicalAddress(in.URL.Host)
-		canonicalRegistryHost := bt.canonicalAddress(bt.registry.RegistryStr())
-		if canonicalHeaderHost == canonicalRegistryHost || canonicalURLHost == canonicalRegistryHost {
+		if matchesHost(bt.registry, in, bt.scheme) {
+			auth, err := bt.bearer.Authorization()
+			if err != nil {
+				return nil, err
+			}
 			hdr := fmt.Sprintf("Bearer %s", auth.RegistryToken)
 			in.Header.Set("Authorization", hdr)
-
-			// When we ping() the registry, we determine whether to use http or https
-			// based on which scheme was successful. That is only valid for the
-			// registry server and not e.g. a separate token server or blob storage,
-			// so we should only override the scheme if the host is the registry.
-			in.URL.Scheme = bt.scheme
 		}
-		in.Header.Set("User-Agent", transportName)
 		return bt.inner.RoundTrip(in)
 	}
 
@@ -168,7 +157,14 @@ func (bt *bearerTransport) refresh() error {
 	return nil
 }
 
-func (bt *bearerTransport) canonicalAddress(host string) (address string) {
+func matchesHost(reg name.Registry, in *http.Request, scheme string) bool {
+	canonicalHeaderHost := canonicalAddress(in.Host, scheme)
+	canonicalURLHost := canonicalAddress(in.URL.Host, scheme)
+	canonicalRegistryHost := canonicalAddress(reg.RegistryStr(), scheme)
+	return canonicalHeaderHost == canonicalRegistryHost || canonicalURLHost == canonicalRegistryHost
+}
+
+func canonicalAddress(host, scheme string) (address string) {
 	// The host may be any one of:
 	// - hostname
 	// - hostname:port
@@ -184,13 +180,13 @@ func (bt *bearerTransport) canonicalAddress(host string) (address string) {
 			return host
 		}
 		if port == "" {
-			port = portMap[bt.scheme]
+			port = portMap[scheme]
 		}
 
 		return net.JoinHostPort(hostname, port)
 	}
 
-	return net.JoinHostPort(host, portMap[bt.scheme])
+	return net.JoinHostPort(host, portMap[scheme])
 }
 
 // https://docs.docker.com/registry/spec/auth/oauth/
diff --git a/pkg/v1/remote/transport/schemer.go b/pkg/v1/remote/transport/schemer.go
new file mode 100644
index 00000000..d70b6a85
--- /dev/null
+++ b/pkg/v1/remote/transport/schemer.go
@@ -0,0 +1,44 @@
+// Copyright 2019 Google LLC All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package transport
+
+import (
+	"net/http"
+
+	"github.com/google/go-containerregistry/pkg/name"
+)
+
+type schemeTransport struct {
+	// Scheme we should use, determined by ping response.
+	scheme string
+
+	// Registry we're talking to.
+	registry name.Registry
+
+	// Wrapped by schemeTransport.
+	inner http.RoundTripper
+}
+
+// RoundTrip implements http.RoundTripper
+func (st *schemeTransport) RoundTrip(in *http.Request) (*http.Response, error) {
+	// When we ping() the registry, we determine whether to use http or https
+	// based on which scheme was successful. That is only valid for the
+	// registry server and not e.g. a separate token server or blob storage,
+	// so we should only override the scheme if the host is the registry.
+	if matchesHost(st.registry, in, st.scheme) {
+		in.URL.Scheme = st.scheme
+	}
+	return st.inner.RoundTrip(in)
+}
diff --git a/pkg/v1/remote/transport/transport.go b/pkg/v1/remote/transport/transport.go
index 61b695d7..1bea604a 100644
--- a/pkg/v1/remote/transport/transport.go
+++ b/pkg/v1/remote/transport/transport.go
@@ -22,10 +22,6 @@ import (
 	"github.com/google/go-containerregistry/pkg/name"
 )
 
-const (
-	transportName = "go-containerregistry"
-)
-
 // New returns a new RoundTripper based on the provided RoundTripper that has been
 // setup to authenticate with the remote registry "reg", in the capacity
 // laid out by the specified scopes.
@@ -49,6 +45,16 @@ func New(reg name.Registry, auth authn.Authenticator, t http.RoundTripper, scope
 		return nil, err
 	}
 
+	// Wrap the given transport in transports that use an appropriate scheme,
+	// (based on the ping response) and set the user agent.
+	t = &useragentTransport{
+		inner: &schemeTransport{
+			scheme:   pr.scheme,
+			registry: reg,
+			inner:    t,
+		},
+	}
+
 	switch pr.challenge.Canonical() {
 	case anonymous:
 		return t, nil
diff --git a/pkg/v1/remote/transport/useragent.go b/pkg/v1/remote/transport/useragent.go
new file mode 100644
index 00000000..8d59ab76
--- /dev/null
+++ b/pkg/v1/remote/transport/useragent.go
@@ -0,0 +1,32 @@
+// Copyright 2019 Google LLC All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package transport
+
+import "net/http"
+
+const (
+	transportName = "go-containerregistry"
+)
+
+type useragentTransport struct {
+	// Wrapped by useragentTransport.
+	inner http.RoundTripper
+}
+
+// RoundTrip implements http.RoundTripper
+func (ut *useragentTransport) RoundTrip(in *http.Request) (*http.Response, error) {
+	in.Header.Set("User-Agent", transportName)
+	return ut.inner.RoundTrip(in)
+}


Test patch:
diff --git a/pkg/v1/remote/transport/bearer_test.go b/pkg/v1/remote/transport/bearer_test.go
index ba575024..01afa1e3 100644
--- a/pkg/v1/remote/transport/bearer_test.go
+++ b/pkg/v1/remote/transport/bearer_test.go
@@ -353,13 +353,9 @@ func TestSchemeOverride(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Unexpected error during NewRegistry: %v", err)
 	}
-	bearer := &authn.Bearer{Token: "does-not-matter"}
-	bt := &bearerTransport{
+	st := &schemeTransport{
 		inner:    rec,
-		bearer:   bearer,
-		basic:    &authn.Basic{},
 		registry: registry,
-		realm:    "token.example.com",
 		scheme:   "http",
 	}
 
@@ -382,7 +378,7 @@ func TestSchemeOverride(t *testing.T) {
 			t.Fatalf("Unexpected error during NewRequest: %v", err)
 		}
 
-		if _, err := bt.RoundTrip(req); err != nil {
+		if _, err := st.RoundTrip(req); err != nil {
 			t.Fatalf("Unexpected error during RoundTrip: %v", err)
 		}
 
@@ -393,129 +389,80 @@ func TestSchemeOverride(t *testing.T) {
 }
 
 func TestCanonicalAddressResolution(t *testing.T) {
-	bearer := &authn.Bearer{Token: "does-not-matter"}
-
 	registry, err := name.NewRegistry("does-not-matter", name.WeakValidation)
 	if err != nil {
 		t.Errorf("Unexpected error during NewRegistry: %v", err)
 	}
 
 	tests := []struct {
-		bt      *bearerTransport
-		address string
-		want    string
+		registry name.Registry
+		scheme   string
+		address  string
+		want     string
 	}{{
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "http",
-		},
-		address: "registry.example.com",
-		want:    "registry.example.com:80",
+		registry: registry,
+		scheme:   "http",
+		address:  "registry.example.com",
+		want:     "registry.example.com:80",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "http",
-		},
-		address: "registry.example.com:12345",
-		want:    "registry.example.com:12345",
+		registry: registry,
+		scheme:   "http",
+		address:  "registry.example.com:12345",
+		want:     "registry.example.com:12345",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "https",
-		},
-		address: "registry.example.com",
-		want:    "registry.example.com:443",
+		registry: registry,
+		scheme:   "https",
+		address:  "registry.example.com",
+		want:     "registry.example.com:443",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "https",
-		},
-		address: "registry.example.com:12345",
-		want:    "registry.example.com:12345",
+		registry: registry,
+		scheme:   "https",
+		address:  "registry.example.com:12345",
+		want:     "registry.example.com:12345",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "http",
-		},
-		address: "registry.example.com:",
-		want:    "registry.example.com:80",
+		registry: registry,
+		scheme:   "http",
+		address:  "registry.example.com:",
+		want:     "registry.example.com:80",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "https",
-		},
-		address: "registry.example.com:",
-		want:    "registry.example.com:443",
+		registry: registry,
+		scheme:   "https",
+		address:  "registry.example.com:",
+		want:     "registry.example.com:443",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "http",
-		},
-		address: "2001:db8::1",
-		want:    "[2001:db8::1]:80",
+		registry: registry,
+		scheme:   "http",
+		address:  "2001:db8::1",
+		want:     "[2001:db8::1]:80",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "https",
-		},
-		address: "2001:db8::1",
-		want:    "[2001:db8::1]:443",
+		registry: registry,
+		scheme:   "https",
+		address:  "2001:db8::1",
+		want:     "[2001:db8::1]:443",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "http",
-		},
-		address: "[2001:db8::1]:12345",
-		want:    "[2001:db8::1]:12345",
+		registry: registry,
+		scheme:   "http",
+		address:  "[2001:db8::1]:12345",
+		want:     "[2001:db8::1]:12345",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "https",
-		},
-		address: "[2001:db8::1]:12345",
-		want:    "[2001:db8::1]:12345",
+		registry: registry,
+		scheme:   "https",
+		address:  "[2001:db8::1]:12345",
+		want:     "[2001:db8::1]:12345",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "http",
-		},
-		address: "[2001:db8::1]:",
-		want:    "[2001:db8::1]:80",
+		registry: registry,
+		scheme:   "http",
+		address:  "[2001:db8::1]:",
+		want:     "[2001:db8::1]:80",
 	}, {
-		bt: &bearerTransport{
-			inner:    &http.Transport{},
-			bearer:   bearer,
-			registry: registry,
-			scheme:   "https",
-		},
-		address: "[2001:db8::1]:",
-		want:    "[2001:db8::1]:443",
+		registry: registry,
+		scheme:   "https",
+		address:  "[2001:db8::1]:",
+		want:     "[2001:db8::1]:443",
 	}}
 
 	for _, tt := range tests {
-		got := tt.bt.canonicalAddress(tt.address)
+		got := canonicalAddress(tt.address, tt.scheme)
 		if got != tt.want {
 			t.Errorf("Wrong canonical host: wanted %v got %v", tt.want, got)
 		}
diff --git a/pkg/v1/remote/transport/transport_test.go b/pkg/v1/remote/transport/transport_test.go
index bf009129..e9f74769 100644
--- a/pkg/v1/remote/transport/transport_test.go
+++ b/pkg/v1/remote/transport/transport_test.go
@@ -15,6 +15,8 @@
 package transport
 
 import (
+	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -30,26 +32,39 @@ var (
 )
 
 func TestTransportSelectionAnonymous(t *testing.T) {
-	server := httptest.NewServer(
-		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		}))
-	defer server.Close()
-	tprt := &http.Transport{
-		Proxy: func(req *http.Request) (*url.URL, error) {
-			return url.Parse(server.URL)
-		},
+	// Record the requests we get in the inner transport.
+	cannedResponse := http.Response{
+		Status:     http.StatusText(http.StatusOK),
+		StatusCode: http.StatusOK,
+		Body:       ioutil.NopCloser(strings.NewReader("")),
 	}
+	recorder := newRecorder(&cannedResponse, nil)
 
 	basic := &authn.Basic{Username: "foo", Password: "bar"}
+	reg := testReference.Context().Registry
 
-	tp, err := New(testReference.Context().Registry, basic, tprt, []string{testReference.Scope(PullScope)})
+	tp, err := New(reg, basic, recorder, []string{testReference.Scope(PullScope)})
 	if err != nil {
 		t.Errorf("New() = %v", err)
 	}
-	// We should get back an unmodified transport
-	if tp != tprt {
-		t.Errorf("New(); got %v, want %v", tp, tprt)
+
+	req, err := http.NewRequest("GET", fmt.Sprintf("http://%s/v2/anything", reg), nil)
+	if err != nil {
+		t.Fatalf("Unexpected error during NewRequest: %v", err)
+	}
+	if _, err := tp.RoundTrip(req); err != nil {
+		t.Fatalf("Unexpected error during RoundTrip: %v", err)
+	}
+
+	if got, want := len(recorder.reqs), 2; got != want {
+		t.Fatalf("expected %d requests, got %d", want, got)
+	}
+	recorded := recorder.reqs[1]
+	if got, want := recorded.URL.Scheme, "https"; got != want {
+		t.Errorf("wrong scheme, want %s got %s", want, got)
+	}
+	if want, got := recorded.Header.Get("User-Agent"), transportName; want != got {
+		t.Errorf("wrong useragent, want %s got %s", want, got)
 	}
 }