From 343380be382afa923ae4847525385db2400e6347 Mon Sep 17 00:00:00 2001 From: Satwik Sai Prakash Sahoo Date: Mon, 27 Jul 2026 21:17:14 +0530 Subject: [PATCH] image/docker: retry push when the ping advertised no challenges A registry may allow unauthenticated GET/HEAD but require authentication for writes. The GET /v2/ ping in detectProperties() then succeeds without advertising any authentication challenge, so the client records none, sends the blob upload POST unauthenticated, and gives up on the resulting 401 even though it does have usable credentials. Record the challenges from such a 401 response and retry the request with them. The retry keeps the scope it already had: only an insufficient_scope error asks for a different one. challenges are no longer written only once from detectProperties(), so protect them with a mutex; concurrent blob uploads share one client. Fixes: #1009 Signed-off-by: Satwik Sai Prakash Sahoo --- image/docker/docker_client.go | 107 +++++++++++++++++++-------- image/docker/docker_client_test.go | 111 +++++++++++++++++++++++++++-- 2 files changed, 182 insertions(+), 36 deletions(-) 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) +}