diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 2501187f98..03fdb0d20d 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -113,9 +113,12 @@ type dockerClient struct { namespaceProxy string // The following members are detected registry properties: - // They are set after a successful detectProperties(), and never change afterwards. + // They are set after a successful detectProperties(), and never change afterwards, + // with the exception of challenges: if detectProperties() did not see any, a later + // response can still record some, see recordMissingChallenges. client *http.Client scheme string + challengesLock sync.RWMutex // Protects challenges. challenges []challenge supportsSignatures bool @@ -510,32 +513,68 @@ func (c *dockerClient) resolveRequestURL(path string) (*url.URL, error) { return res, nil } -// Checks if the auth headers in the response contain an indication of a failed -// authorization 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(res *http.Response) (bool, *authScope) { - if res.StatusCode == http.StatusUnauthorized { - for challenge := range iterateAuthHeader(res.Header) { - 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") - } +// needsRetryWithUpdatedScope checks whether res indicates that the request should be retried with +// updated authentication, updating c if necessary. It handles two cases: +// - The authorization failed with an "insufficient_scope" error; then the returned scope is the +// one which must be used to fetch a new token. +// - We have not recorded any authentication challenges (detectProperties() did not see any), but +// this response carries some; then they are recorded and a retry can use them. The returned +// scope is nil in that case, i.e. the caller should keep using the scope it already has. +func (c *dockerClient) needsRetryWithUpdatedScope(res *http.Response) (bool, *authScope) { + if res.StatusCode != http.StatusUnauthorized { + return false, nil + } + + // Do this first: if we don’t have any challenges, we sent the request unauthenticated, and + // no updated scope alone would make a retry succeed. + missingChallengesRecorded := c.recordMissingChallenges(res.Header) + + for challenge := range iterateAuthHeader(res.Header) { + 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") } } } } } + + if missingChallengesRecorded { + return true, nil + } return false, nil } +// recordMissingChallenges records the authentication challenges advertised in header if c does not +// have any yet, and reports whether it did. +// +// A registry may allow unauthenticated GET/HEAD but require authentication for writes; then the +// GET /v2/ ping in detectProperties() succeeds without advertising any challenge, we send the write +// unauthenticated, and we would have no way to act on the resulting 401. The challenges in that 401 +// tell us how to authenticate, so record them and let the caller retry. +func (c *dockerClient) recordMissingChallenges(header http.Header) bool { + c.challengesLock.Lock() + defer c.challengesLock.Unlock() + if len(c.challenges) != 0 { + return false + } + newChallenges := slices.Collect(iterateAuthHeader(header)) + if len(newChallenges) == 0 { + return false + } + logrus.Debugf("Registry did not advertise any authentication challenge on ping, recording the %d challenge(s) from a 401 response", len(newChallenges)) + c.challenges = newChallenges + return true +} + // 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 { @@ -590,17 +629,21 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method stri // 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(res); retry { - logrus.Debug("Detected insufficient_scope error, will retry request with updated scope") + if retry, newScope := c.needsRetryWithUpdatedScope(res); retry { + logrus.Debug("Detected insufficient_scope error or missing challenges, will retry request with updated authentication") res.Body.Close() - // 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, requestURL, headers, stream, streamLen, auth, newScope) + if newScope != nil { + // Note: This retry ignores the extraScope we were called with. 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. + extraScope = newScope + } + // If newScope is nil we only recorded challenges we did not have before; the scope we were + // called with is still the right one, so keep it. + res, err = c.makeRequestToResolvedURLOnce(ctx, method, requestURL, headers, stream, streamLen, auth, extraScope) if err != nil { return nil, err } - extraScope = newScope } } @@ -735,11 +778,15 @@ func parseRegistryWarningHeader(header string) string { // // debugging: https://github.com/containers/image/pull/211#issuecomment-273426236 and follows up func (c *dockerClient) setupRequestAuth(req *http.Request, extraScope *authScope) error { - if len(c.challenges) == 0 { + c.challengesLock.RLock() + challenges := c.challenges + c.challengesLock.RUnlock() + + if len(challenges) == 0 { return nil } - schemeNames := make([]string, 0, len(c.challenges)) - for _, challenge := range c.challenges { + schemeNames := make([]string, 0, len(challenges)) + for _, challenge := range challenges { schemeNames = append(schemeNames, challenge.Scheme) switch challenge.Scheme { case "basic": @@ -1010,7 +1057,9 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error { if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized { return registryHTTPResponseToError(resp) } + c.challengesLock.Lock() c.challenges = slices.Collect(iterateAuthHeader(resp.Header)) + c.challengesLock.Unlock() c.scheme = scheme c.supportsSignatures = resp.Header.Get("X-Registry-Supports-Signatures") == "1" return nil diff --git a/image/docker/docker_client_test.go b/image/docker/docker_client_test.go index de94d16837..ff04cc9000 100644 --- a/image/docker/docker_client_test.go +++ b/image/docker/docker_client_test.go @@ -239,6 +239,12 @@ var registrySuseComResp = http.Response{ Request: nil, } +// clientWithRecordedChallenges returns a dockerClient which already knows about an authentication +// challenge, so that needsRetryWithUpdatedScope does not take the recordMissingChallenges path. +func clientWithRecordedChallenges() *dockerClient { + return &dockerClient{challenges: []challenge{{Scheme: "bearer"}}} +} + func TestNeedsRetryOnInsuficientScope(t *testing.T) { resp := registrySuseComResp resp.Header["Www-Authenticate"] = []string{ @@ -250,7 +256,7 @@ func TestNeedsRetryOnInsuficientScope(t *testing.T) { actions: "*", } - needsRetry, scope := needsRetryWithUpdatedScope(&resp) + needsRetry, scope := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if !needsRetry { t.Fatal("Expected needing to retry") @@ -265,7 +271,7 @@ func TestNeedsRetryNoRetryWhenNoAuthHeader(t *testing.T) { resp := registrySuseComResp delete(resp.Header, "Www-Authenticate") - needsRetry, _ := needsRetryWithUpdatedScope(&resp) + needsRetry, _ := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no Authentication headers are present") @@ -278,7 +284,7 @@ func TestNeedsRetryNoRetryWhenNoBearerAuthHeader(t *testing.T) { `OAuth2 realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*"`, } - needsRetry, _ := needsRetryWithUpdatedScope(&resp) + needsRetry, _ := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no bearer authentication header is present") @@ -291,7 +297,7 @@ func TestNeedsRetryNoRetryWhenNoErrorInBearer(t *testing.T) { `Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*"`, } - needsRetry, _ := needsRetryWithUpdatedScope(&resp) + needsRetry, _ := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no insufficient error is present in the authentication header") @@ -304,7 +310,7 @@ func TestNeedsRetryNoRetryWhenInvalidErrorInBearer(t *testing.T) { `Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="registry:catalog:*,error="random_error"`, } - needsRetry, _ := needsRetryWithUpdatedScope(&resp) + needsRetry, _ := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no insufficient_error is present in the authentication header") @@ -317,7 +323,7 @@ func TestNeedsRetryNoRetryWhenInvalidScope(t *testing.T) { `Bearer realm="https://registry.suse.com/auth",service="SUSE Linux Docker Registry",scope="foo:bar",error="insufficient_scope"`, } - needsRetry, _ := needsRetryWithUpdatedScope(&resp) + needsRetry, _ := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Expected no need to retry, as no insufficient_error is present in the authentication header") @@ -350,7 +356,7 @@ func TestNeedsNoRetry(t *testing.T) { }, } - needsRetry, _ := needsRetryWithUpdatedScope(&resp) + needsRetry, _ := clientWithRecordedChallenges().needsRetryWithUpdatedScope(&resp) if needsRetry { t.Fatal("Got the need to retry, but none should be required") } @@ -539,3 +545,94 @@ func TestResolveRequestURLWithNamespaceProxy(t *testing.T) { }) } } + +// TestNeedsRetryWithMissingChallenges covers a registry which allows unauthenticated GET/HEAD, so +// that the detectProperties() ping records no challenges at all, but requires authentication for +// writes. +func TestNeedsRetryWithMissingChallenges(t *testing.T) { + for _, c := range []struct { + name string + // status and wwwAuthenticate describe the response to react on. + status int + wwwAuthenticate []string + expectedRetry bool + expectedScope *authScope + expectedSchemes []string // challenges recorded on the client afterwards + }{ + { + name: "basic challenge is recorded and retried", + status: http.StatusUnauthorized, + wwwAuthenticate: []string{`Basic realm="Password expected here ..."`}, + expectedRetry: true, + expectedSchemes: []string{"basic"}, + }, + { + name: "bearer challenge is recorded and retried", + status: http.StatusUnauthorized, + wwwAuthenticate: []string{`Bearer realm="https://registry.example.com/auth",service="registry.example.com"`}, + expectedRetry: true, + expectedSchemes: []string{"bearer"}, + }, + { + // The scope must still be reported so that the retry can obtain a usable token. + name: "insufficient_scope still returns the scope", + status: http.StatusUnauthorized, + wwwAuthenticate: []string{`Bearer realm="https://registry.example.com/auth",service="registry.example.com",scope="registry:catalog:*",error="insufficient_scope"`}, + expectedRetry: true, + expectedScope: &authScope{resourceType: "registry", remoteName: "catalog", actions: "*"}, + expectedSchemes: []string{"bearer"}, + }, + { + name: "401 without any challenge", + status: http.StatusUnauthorized, + wwwAuthenticate: nil, + expectedRetry: false, + expectedSchemes: nil, + }, + { + name: "challenges on a non-401 response are ignored", + status: http.StatusForbidden, + wwwAuthenticate: []string{`Basic realm="Password expected here ..."`}, + expectedRetry: false, + expectedSchemes: nil, + }, + } { + t.Run(c.name, func(t *testing.T) { + res := http.Response{ + StatusCode: c.status, + Header: http.Header{}, + } + if c.wwwAuthenticate != nil { + res.Header["Www-Authenticate"] = c.wwwAuthenticate + } + + client := &dockerClient{} // No challenges recorded, as after a ping which did not need authentication. + needsRetry, scope := client.needsRetryWithUpdatedScope(&res) + assert.Equal(t, c.expectedRetry, needsRetry) + assert.Equal(t, c.expectedScope, scope) + + schemes := []string(nil) + for _, challenge := range client.challenges { + schemes = append(schemes, challenge.Scheme) + } + assert.Equal(t, c.expectedSchemes, schemes) + }) + } +} + +// TestNeedsRetryDoesNotOverwriteChallenges verifies that challenges recorded by detectProperties() +// are not replaced by the contents of a later 401. +func TestNeedsRetryDoesNotOverwriteChallenges(t *testing.T) { + res := http.Response{ + StatusCode: http.StatusUnauthorized, + Header: http.Header{ + "Www-Authenticate": []string{`Basic realm="Password expected here ..."`}, + }, + } + + client := clientWithRecordedChallenges() + needsRetry, scope := client.needsRetryWithUpdatedScope(&res) + assert.False(t, needsRetry) + assert.Nil(t, scope) + assert.Equal(t, []challenge{{Scheme: "bearer"}}, client.challenges) +}