diff --git a/docker/docker_client.go b/docker/docker_client.go index 5de0767408..a1321149ca 100644 --- a/docker/docker_client.go +++ b/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)) } } diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index 171e5b68f8..a7c085215e 100644 --- a/docker/docker_client_test.go +++ b/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": {`; 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") + } +} diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index 6cd693b6bb..44b45c472c 100644 --- a/docker/docker_image_dest.go +++ b/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. diff --git a/docker/wwwauthenticate.go b/docker/wwwauthenticate.go index d0bbbba8a5..c8f2e8a7a6 100644 --- a/docker/wwwauthenticate.go +++ b/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.