docker_client: Handle "invalid_scope" errors #1079
Conversation
There was a problem hiding this comment.
Thanks!
There’s an immediate concurrency concern. Other than that, based on a very quick initial look, this seems useful and reasonable at a first glance.
(Note that Parameters["scope"] is not used at all, because Docker has historically ignored it, and some implementations just don’t return useful values. Using that only on a failure where the previous code would give up is almost certainly fine, but it will be necessary for me to return to the old research.)
docker/docker_client.go
Outdated
| @@ -627,6 +660,11 @@ func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge chall | |||
| if service, ok := challenge.Parameters["service"]; ok && service != "" { | |||
| params.Add("service", service) | |||
| } | |||
|
|
|||
| if scope, ok := challenge.Parameters["scope"]; ok && scope != "" { | |||
| params.Add("scope", scope) | |||
There was a problem hiding this comment.
Note to self: Should this be added or replace the original?
docker/docker_client.go
Outdated
| // check for that and update the client challenges to retry with requesting a new token | ||
| if retry, challenges := needsRetryWithUpdatedChallenges(err, resp); retry { | ||
| logrus.Debug("Detected insufficent_scope error, retrying request with updated challenges") | ||
| c.challenges = challenges |
There was a problem hiding this comment.
dockerClient, once initialized, can be used concurrently (HasThreadSafe…), so overwriting a property which is intentionally in the “… and never change afterwards” section of dockerClient is not safe. (This is also why extraScope is a parameter and not a property.)
There was a problem hiding this comment.
Ah, ok. Must have overlooked that :(. I had initially thought to put the updated scope into the extraScope argument, but as that would have required to rework the authScope struct a bit (as the current code always assumes the resourcetype part of it to be repository I went of the above approach in my first try. I guess I need to rethink that again :).
What do you think, does it make sense to pass the updated scope via the extraScope argument?
rhafer
force-pushed
the
search
branch
2 times, most recently
from
12dfcc2 to
0b97e06
Compare
|
@mtrmac @vrothberg PTAL |
By default docker_client just uses the auth challenges from the /v2/ ping request to request a Bearer Token. For some requests (e.g. for /v2/_catalog on some registries) this might not be sufficient and return a a HTTP Unauthorized Error with the "www-authenticate" header including an "insufficient_scope" error. In that case the client will now retry the request and fetch a new token with updated challenges to have the "scope" matching for what the endpoint needs. Signed-off-by: Ralf Haferkamp <[email protected]> Signed-off-by: Ralf Haferkamp <[email protected]>
|
This would need another rebase. But I am not able to spend any time on this. So I'll just close it. |
|
Sorry about that, not getting back to the PR is totally our fault. I have at least filed #1478 so that we track this action item. |
|
@rhafer Could I take over this code? I have some free time I'd like to invest in this. |
|
@josegomezr Sure, go ahead. As stated I won't be able to spent any time on this. (Neither does this bug affect me personally anymore) |
|
@josegomezr Did you ever work on this? |
podman 2.1.1 (which uses containers/images 5.7.0) fails to search some registries. E.g.
podman --log-level=debug search registry.suse.com/gives:
Looking into the details of the requests and response and trying to repeat it with
curlI can see that the request to/v2/_catalogfailes with Status 401 and the www-authenticate containing this error:This can be fixed by simply fetching a new token with the
scopevalue set to the value return in that header.This PR tries to implement a fix for the above behavior by checking for the error and retrying the request with and updated token.