Skip to content

image/docker: retry push when the ping advertised no challenges #1028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
satwiksps wants to merge 1 commit into
base: main
Choose a base branch
from
Open

image/docker: retry push when the ping advertised no challenges #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 78 additions & 29 deletions image/docker/docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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":
Expand Down Expand Up @@ -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
Expand Down
111 changes: 104 additions & 7 deletions image/docker/docker_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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)
}