Merge pull request #1640 from dcermak/handle-invalid-scope

docker_client: Handle "invalid_scope" errors

docker/docker_client.go

@@ -126,8 +126,9 @@ type dockerClient struct {
 }
 
 type authScope struct {
-	remoteName string
-	actions    string
+	resourceType string
+	remoteName   string
+	actions      string
 }
 
 // sendAuth determines whether we need authentication for v2 or v1 endpoint.
@@ -236,6 +237,7 @@ func newDockerClientFromRef(sys *types.SystemContext, ref dockerReference, regis
 	}
 	client.signatureBase = sigBase
 	client.useSigstoreAttachments = registryConfig.useSigstoreAttachments(ref)
+	client.scope.resourceType = "repository"
 	client.scope.actions = actions
 	client.scope.remoteName = reference.Path(ref.ref)
 	return client, nil
@@ -474,6 +476,33 @@ func (c *dockerClient) makeRequest(ctx context.Context, method, path string, hea
 	return c.makeRequestToResolvedURL(ctx, method, url, headers, stream, -1, auth, extraScope)
 }
 
+// Checks if the auth headers in the response contain an indication of a failed
+// authorizdation because of an "insufficient_scope" error. If that's the case,
+// returns the required scope to be used for fetching a new token.
+func needsRetryWithUpdatedScope(err error, res *http.Response) (bool, *authScope) {
+	if err == nil && res.StatusCode == http.StatusUnauthorized {
+		challenges := parseAuthHeader(res.Header)
+		for _, challenge := range challenges {
+			if challenge.Scheme == "bearer" {
+				if errmsg, ok := challenge.Parameters["error"]; ok && errmsg == "insufficient_scope" {
+					if scope, ok := challenge.Parameters["scope"]; ok && scope != "" {
+						if newScope, err := parseAuthScope(scope); err == nil {
+							return true, newScope
+						} else {
+							logrus.WithFields(logrus.Fields{
+								"error":     err,
+								"scope":     scope,
+								"challenge": challenge,
+							}).Error("Failed to parse the authentication scope from the given challenge")
+						}
+					}
+				}
+			}
+		}
+	}
+	return false, nil
+}
+
 // parseRetryAfter determines the delay required by the "Retry-After" header in res and returns it,
 // silently falling back to fallbackDelay if the header is missing or invalid.
 func parseRetryAfter(res *http.Response, fallbackDelay time.Duration) time.Duration {
@@ -513,6 +542,29 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method stri
 	for {
 		res, err := c.makeRequestToResolvedURLOnce(ctx, method, url, headers, stream, streamLen, auth, extraScope)
 		attempts++
+
+		// By default we use pre-defined scopes per operation. In
+		// certain cases, this can fail when our authentication is
+		// insufficient, then we might be getting an error back with a
+		// Www-Authenticate Header indicating an insufficient scope.
+		//
+		// Check for that and update the client challenges to retry after
+		// requesting a new token
+		//
+		// We only try this on the first attempt, to not overload an
+		// already struggling server.
+		// We also cannot retry with a body (stream != nil) as stream
+		// was already read
+		if attempts == 1 && stream == nil && auth != noAuth {
+			if retry, newScope := needsRetryWithUpdatedScope(err, res); retry {
+				logrus.Debug("Detected insufficient_scope error, will retry request with updated scope")
+				// Note: This retry ignores extraScope. That’s, strictly speaking, incorrect, but we don’t currently
+				// expect the insufficient_scope errors to happen for those callers. If that changes, we can add support
+				// for more than one extra scope.
+				res, err = c.makeRequestToResolvedURLOnce(ctx, method, url, headers, stream, streamLen, auth, newScope)
+				extraScope = newScope
+			}
+		}
 		if res == nil || res.StatusCode != http.StatusTooManyRequests || // Only retry on StatusTooManyRequests, success or other failure is returned to caller immediately
 			stream != nil || // We can't retry with a body (which is not restartable in the general case)
 			attempts == backoffNumIterations {
@@ -592,8 +644,18 @@ func (c *dockerClient) setupRequestAuth(req *http.Request, extraScope *authScope
 				cacheKey := ""
 				scopes := []authScope{c.scope}
 				if extraScope != nil {
-					// Using ':' as a separator here is unambiguous because getBearerToken below uses the same separator when formatting a remote request (and because repository names can't contain colons).
-					cacheKey = fmt.Sprintf("%s:%s", extraScope.remoteName, extraScope.actions)
+					// Using ':' as a separator here is unambiguous because getBearerToken below
+					// uses the same separator when formatting a remote request (and because
+					// repository names that we create can't contain colons, and extraScope values
+					// coming from a server come from `parseAuthScope`, which also splits on colons).
+					cacheKey = fmt.Sprintf("%s:%s:%s", extraScope.resourceType, extraScope.remoteName, extraScope.actions)
+					if colonCount := strings.Count(cacheKey, ":"); colonCount != 2 {
+						return fmt.Errorf(
+							"Internal error: there must be exactly 2 colons in the cacheKey ('%s') but got %d",
+							cacheKey,
+							colonCount,
+						)
+					}
 					scopes = append(scopes, *extraScope)
 				}
 				var token bearerToken
@@ -648,9 +710,10 @@ func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge chall
 	if service, ok := challenge.Parameters["service"]; ok && service != "" {
 		params.Add("service", service)
 	}
+
 	for _, scope := range scopes {
-		if scope.remoteName != "" && scope.actions != "" {
-			params.Add("scope", fmt.Sprintf("repository:%s:%s", scope.remoteName, scope.actions))
+		if scope.resourceType != "" && scope.remoteName != "" && scope.actions != "" {
+			params.Add("scope", fmt.Sprintf("%s:%s:%s", scope.resourceType, scope.remoteName, scope.actions))
 		}
 	}
 	params.Add("grant_type", "refresh_token")
@@ -700,8 +763,8 @@ func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge,
 	}
 
 	for _, scope := range scopes {
-		if scope.remoteName != "" && scope.actions != "" {
-			params.Add("scope", fmt.Sprintf("repository:%s:%s", scope.remoteName, scope.actions))
+		if scope.resourceType != "" && scope.remoteName != "" && scope.actions != "" {
+			params.Add("scope", fmt.Sprintf("%s:%s:%s", scope.resourceType, scope.remoteName, scope.actions))
 		}
 	}
 

docker/docker_client_test.go

@@ -2,6 +2,7 @@ package docker
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -187,3 +188,143 @@ func TestUserAgent(t *testing.T) {
 		}
 	}
 }
+
+func TestNeedsRetryOnError(t *testing.T) {
+	needsRetry, _ := needsRetryWithUpdatedScope(errors.New("generic"), nil)
+	if needsRetry {
+		t.Fatal("Got needRetry for a connection that included an error")
+	}
+}
+
+var registrySuseComResp = http.Response{
+	Status:     "401 Unauthorized",
+	StatusCode: http.StatusUnauthorized,
+	Proto:      "HTTP/1.1",
+	ProtoMajor: 1,
+	ProtoMinor: 1,
+	Header: map[string][]string{
+		"Content-Length":                  {"145"},
+		"Content-Type":                    {"application/json"},
+		"Date":                            {"Fri, 26 Aug 2022 08:03:13 GMT"},
+		"Docker-Distribution-Api-Version": {"registry/2.0"},
+		// "Www-Authenticate":                {`Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*",error="insufficient_scope"`},
+		"X-Content-Type-Options": {"nosniff"},
+	},
+	Request: nil,
+}
+
+func TestNeedsRetryOnInsuficientScope(t *testing.T) {
+	resp := registrySuseComResp
+	resp.Header["Www-Authenticate"] = []string{
+		`Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*",error="insufficient_scope"`,
+	}
+	expectedScope := authScope{
+		resourceType: "registry",
+		remoteName:   "catalog",
+		actions:      "*",
+	}
+
+	needsRetry, scope := needsRetryWithUpdatedScope(nil, &resp)
+
+	if !needsRetry {
+		t.Fatal("Expected needing to retry")
+	}
+
+	if expectedScope != *scope {
+		t.Fatalf("Got an invalid scope, expected '%q' but got '%q'", expectedScope, *scope)
+	}
+}
+
+func TestNeedsRetryNoRetryWhenNoAuthHeader(t *testing.T) {
+	resp := registrySuseComResp
+	delete(resp.Header, "Www-Authenticate")
+
+	needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp)
+
+	if needsRetry {
+		t.Fatal("Expected no need to retry, as no Authentication headers are present")
+	}
+}
+
+func TestNeedsRetryNoRetryWhenNoBearerAuthHeader(t *testing.T) {
+	resp := registrySuseComResp
+	resp.Header["Www-Authenticate"] = []string{
+		`OAuth2 realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*"`,
+	}
+
+	needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp)
+
+	if needsRetry {
+		t.Fatal("Expected no need to retry, as no bearer authentication header is present")
+	}
+}
+
+func TestNeedsRetryNoRetryWhenNoErrorInBearer(t *testing.T) {
+	resp := registrySuseComResp
+	resp.Header["Www-Authenticate"] = []string{
+		`Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*"`,
+	}
+
+	needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp)
+
+	if needsRetry {
+		t.Fatal("Expected no need to retry, as no insufficient error is present in the authentication header")
+	}
+}
+
+func TestNeedsRetryNoRetryWhenInvalidErrorInBearer(t *testing.T) {
+	resp := registrySuseComResp
+	resp.Header["Www-Authenticate"] = []string{
+		`Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*,error="random_error"`,
+	}
+
+	needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp)
+
+	if needsRetry {
+		t.Fatal("Expected no need to retry, as no insufficient_error is present in the authentication header")
+	}
+}
+
+func TestNeedsRetryNoRetryWhenInvalidScope(t *testing.T) {
+	resp := registrySuseComResp
+	resp.Header["Www-Authenticate"] = []string{
+		`Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="foo:bar",error="insufficient_scope"`,
+	}
+
+	needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp)
+
+	if needsRetry {
+		t.Fatal("Expected no need to retry, as no insufficient_error is present in the authentication header")
+	}
+}
+
+func TestNeedsNoRetry(t *testing.T) {
+	resp := http.Response{
+		Status:     "200 OK",
+		StatusCode: http.StatusOK,
+		Proto:      "HTTP/1.1",
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header: map[string][]string{"Apptime": {"D=49722"},
+			"Content-Length":                  {"1683"},
+			"Content-Type":                    {"application/json; charset=utf-8"},
+			"Date":                            {"Fri, 26 Aug 2022 09:00:21 GMT"},
+			"Docker-Distribution-Api-Version": {"registry/2.0"},
+			"Link":                            {`</v2/_catalog?last=f35%2Fs2i-base&n=100>; rel="next"`},
+			"Referrer-Policy":                 {"same-origin"},
+			"Server":                          {"Apache"},
+			"Strict-Transport-Security":       {"max-age=31536000; includeSubDomains; preload"},
+			"Vary":                            {"Accept"},
+			"X-Content-Type-Options":          {"nosniff"},
+			"X-Fedora-Proxyserver":            {"proxy10.iad2.fedoraproject.org"},
+			"X-Fedora-Requestid":              {"YwiLpHEhLsbSTugJblBF8QAAAEI"},
+			"X-Frame-Options":                 {"SAMEORIGIN"},
+			"X-Xss-Protection":                {"1; mode=block"},
+		},
+	}
+
+	needsRetry, _ := needsRetryWithUpdatedScope(nil, &resp)
+	if needsRetry {
+		t.Fatal("Got the need to retry, but none should be required")
+	}
+}

docker/docker_image_dest.go

@@ -358,8 +358,9 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 		// Checking candidateRepo, and mounting from it, requires an
 		// expanded token scope.
 		extraScope := &authScope{
-			remoteName: reference.Path(candidateRepo),
-			actions:    "pull",
+			resourceType: "repository",
+			remoteName:   reference.Path(candidateRepo),
+			actions:      "pull",
 		}
 		// This existence check is not, strictly speaking, necessary: We only _really_ need it to get the blob size, and we could record that in the cache instead.
 		// But a "failed" d.mountBlob currently leaves around an unterminated server-side upload, which we would try to cancel.

docker/wwwauthenticate.go

@@ -3,6 +3,7 @@ package docker
 // Based on github.com/docker/distribution/registry/client/auth/authchallenge.go, primarily stripping unnecessary dependencies.
 
 import (
+	"fmt"
 	"net/http"
 	"strings"
 )
@@ -70,6 +71,18 @@ func parseAuthHeader(header http.Header) []challenge {
 	return challenges
 }
 
+/// parses an authentication scope string of the form `$resource:$remote:$actions`
+func parseAuthScope(scopeStr string) (*authScope, error) {
+	if parts := strings.Split(scopeStr, ":"); len(parts) == 3 {
+		return &authScope{
+			resourceType: parts[0],
+			remoteName:   parts[1],
+			actions:      parts[2],
+		}, nil
+	}
+	return nil, fmt.Errorf("error parsing auth scope: '%s'", scopeStr)
+}
+
 // NOTE: This is not a fully compliant parser per RFC 7235:
 // Most notably it does not support more than one challenge within a single header
 // Some of the whitespace parsing also seems noncompliant.