Parse WWW-Authenticate challenges on retry (#846)

* Parse WWW-Authenticate challenges on retry

Use docker/distribution challenge library

* linter stuff

pkg/v1/remote/transport/bearer.go

@@ -24,6 +24,7 @@ import (
 	"net/url"
 	"strings"
 
+	authchallenge "github.com/docker/distribution/registry/client/auth/challenge"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/internal/redact"
 	"github.com/google/go-containerregistry/pkg/name"
@@ -54,6 +55,14 @@ var portMap = map[string]string{
 	"https": "443",
 }
 
+func stringSet(ss []string) map[string]struct{} {
+	set := make(map[string]struct{})
+	for _, s := range ss {
+		set[s] = struct{}{}
+	}
+	return set
+}
+
 // RoundTrip implements http.RoundTripper
 func (bt *bearerTransport) RoundTrip(in *http.Request) (*http.Response, error) {
 	sendRequest := func() (*http.Response, error) {
@@ -74,8 +83,31 @@ func (bt *bearerTransport) RoundTrip(in *http.Request) (*http.Response, error) {
 		return nil, err
 	}
 
-	// Perform a token refresh() and retry the request in case the token has expired
-	if res.StatusCode == http.StatusUnauthorized {
+	// If we hit a WWW-Authenticate challenge, it might be due to expired tokens or insufficient scope.
+	if challenges := authchallenge.ResponseChallenges(res); len(challenges) != 0 {
+		for _, wac := range challenges {
+			// TODO(jonjohnsonjr): Should we also update "realm" or "service"?
+			if scope, ok := wac.Parameters["scope"]; ok {
+				// From https://tools.ietf.org/html/rfc6750#section-3
+				// The "scope" attribute is defined in Section 3.3 of [RFC6749].  The
+				// "scope" attribute is a space-delimited list of case-sensitive scope
+				// values indicating the required scope of the access token for
+				// accessing the requested resource.
+				scopes := strings.Split(scope, " ")
+
+				// Add any scopes that we don't already request.
+				got := stringSet(bt.scopes)
+				for _, want := range scopes {
+					if _, ok := got[want]; !ok {
+						bt.scopes = append(bt.scopes, want)
+					}
+				}
+			}
+		}
+
+		// TODO(jonjohnsonjr): Teach transport.Error about "error" and "error_description" from challenge.
+
+		// Retry the request to attempt to get a valid token.
 		if err = bt.refresh(in.Context()); err != nil {
 			return nil, err
 		}

pkg/v1/remote/transport/bearer_test.go

@@ -159,6 +159,7 @@ func TestBearerTransportTokenRefresh(t *testing.T) {
 				w.Write([]byte(fmt.Sprintf(`{"token": %q}`, refreshedToken)))
 			}
 
+			w.Header().Set("WWW-Authenticate", "scope=foo")
 			w.WriteHeader(http.StatusUnauthorized)
 		}))
 	defer server.Close()
@@ -238,6 +239,7 @@ func TestBearerTransportOauthRefresh(t *testing.T) {
 				return
 			}
 
+			w.Header().Set("WWW-Authenticate", "scope=foo")
 			w.WriteHeader(http.StatusUnauthorized)
 		}))
 	defer server.Close()
@@ -302,6 +304,7 @@ func TestBearerTransportOauth404Fallback(t *testing.T) {
 				return
 			}
 
+			w.Header().Set("WWW-Authenticate", "scope=foo")
 			w.WriteHeader(http.StatusUnauthorized)
 		}))
 	defer server.Close()
@@ -491,3 +494,66 @@ func TestCanonicalAddressResolution(t *testing.T) {
 		}
 	}
 }
+
+func TestInsufficientScope(t *testing.T) {
+	wrong := "the-wrong-scope"
+	right := "the-right-scope"
+	realm := ""
+	expectedService := "my-service.io"
+	passed := false
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			query := r.URL.Query()
+
+			if scopes := query["scope"]; len(scopes) == 0 {
+				if !passed {
+					w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer realm=%q,scope=%q", realm, right))
+					w.WriteHeader(http.StatusUnauthorized)
+				}
+			} else if len(scopes) == 1 {
+				w.Write([]byte(`{"token": "arbitrary-token"}`))
+			} else if len(scopes) == 2 && scopes[1] == right {
+				passed = true
+				w.Write([]byte(`{"token": "arbitrary-token-2"}`))
+			}
+		}))
+	defer server.Close()
+
+	basic := &authn.Basic{Username: "foo", Password: "bar"}
+	u, err := url.Parse(server.URL)
+	if err != nil {
+		t.Error("Unexpected error during url.Parse: ", err)
+	}
+	realm = u.Host
+
+	registry, err := name.NewRegistry(expectedService, name.WeakValidation)
+	if err != nil {
+		t.Error("Unexpected error during NewRegistry: ", err)
+	}
+
+	bt := &bearerTransport{
+		inner:    http.DefaultTransport,
+		basic:    basic,
+		registry: registry,
+		realm:    server.URL,
+		scopes:   []string{wrong},
+		service:  expectedService,
+		scheme:   "http",
+	}
+
+	client := http.Client{Transport: bt}
+
+	res, err := client.Get(fmt.Sprintf("http://%s/v2/foo/bar/blobs/blah", u.Host))
+	if err != nil {
+		t.Error("Unexpected error during client.Get: ", err)
+		return
+	}
+	if res.StatusCode != http.StatusOK {
+		t.Errorf("client.Get final StatusCode got %v, want: %v", res.StatusCode, http.StatusOK)
+	}
+
+	if !passed {
+		t.Error("didn't refresh insufficient scope")
+	}
+}

pkg/v1/remote/transport/ping.go

@@ -22,6 +22,7 @@ import (
 	"net/http"
 	"strings"
 
+	authchallenge "github.com/docker/distribution/registry/client/auth/challenge"
 	"github.com/google/go-containerregistry/pkg/name"
 )
 
@@ -105,18 +106,18 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 				scheme:    scheme,
 			}, nil
 		case http.StatusUnauthorized:
-			wac := resp.Header.Get("WWW-Authenticate")
-			if parts := strings.SplitN(wac, " ", 2); len(parts) == 2 {
-				// If there are two parts, then parse the challenge parameters.
+			if challenges := authchallenge.ResponseChallenges(resp); len(challenges) != 0 {
+				// If we hit more than one, I'm not even sure what to do.
+				wac := challenges[0]
 				return &pingResp{
-					challenge:  challenge(parts[0]).Canonical(),
-					parameters: parseChallenge(parts[1]),
+					challenge:  challenge(wac.Scheme).Canonical(),
+					parameters: wac.Parameters,
 					scheme:     scheme,
 				}, nil
 			}
 			// Otherwise, just return the challenge without parameters.
 			return &pingResp{
-				challenge: challenge(wac).Canonical(),
+				challenge: challenge(resp.Header.Get("WWW-Authenticate")).Canonical(),
 				scheme:    scheme,
 			}, nil
 		default:

pkg/v1/remote/transport/ping_test.go

@@ -124,7 +124,7 @@ func TestPingBasicChallengeNoParams(t *testing.T) {
 func TestPingBearerChallengeWithParams(t *testing.T) {
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.Header().Set("WWW-Authenticate", `Bearer realm="http://auth.foo.io/token`)
+			w.Header().Set("WWW-Authenticate", `Bearer realm="http://auth.example.com/token"`)
 			http.Error(w, "Unauthorized", http.StatusUnauthorized)
 		}))
 	defer server.Close()
@@ -149,7 +149,7 @@ func TestPingBearerChallengeWithParams(t *testing.T) {
 func TestUnsupportedStatus(t *testing.T) {
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.Header().Set("WWW-Authenticate", `Bearer realm="http://auth.foo.io/token`)
+			w.Header().Set("WWW-Authenticate", `Bearer realm="http://auth.example.com/token`)
 			http.Error(w, "Forbidden", http.StatusForbidden)
 		}))
 	defer server.Close()