Docker Registry - Fix Authorizer logic#3052
Docker Registry - Fix Authorizer logic#3052simonferquel wants to merge 3 commits intocontainerd:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3052 +/- ##
==========================================
- Coverage 47.57% 40.81% -6.77%
==========================================
Files 93 75 -18
Lines 8590 9989 +1399
==========================================
- Hits 4087 4077 -10
- Misses 3776 5343 +1567
+ Partials 727 569 -158
Continue to review full report at Codecov.
|
|
The docs are a bit confusing, but the "specified multiple times if there is more than one scope" is part of the deprecated "GET" approach. Refer to https://docs.docker.com/registry/spec/auth/oauth/ for the POST approach which is designed to be compatible with the OAuth2 specification. |
|
From a previous discussion with people in the hub team, I think the doc is not aligned with what is actually working... because my manual tests with both oauth and basic auth fails if scopes are not split in multiple form entries: |
remotes/docker/scope.go
Outdated
There was a problem hiding this comment.
I think you should treat empty and nil map the same way, using len(other) instead.
remotes/docker/scope.go
Outdated
Change the token scope merging to be grammar-aware. This features: - smarter deduplication (repository:foo/bar:pull,push+repository:foo/bar:push,pull -> repository:foo/bar:pull,push) - sort-based normalization (actions on a resources are always sorted alphabethically, returned string slice is always sorted alphabethically) - merging of scopes concerning the same resource (repository:foo/bar:pull+repository:foo/bar:push => repository:foo/bar:pull,push) Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
2dbe9cd to
aa5636a
Compare
|
@dmcgowan seems like you are write (at least on hub, I managed to mount blobs from one repo to the other with a single "scope" form value). I removed the incriminated commit |
2ecc949 to
f9ceda0
Compare
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
f9ceda0 to
5cbfa8b
Compare
Also, extracted token caching logic for better testability Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
5cbfa8b to
4a232ed
Compare
|
Given the merge of #3218, there are now conflicts and the scope/token code has significant changes. Can you validate if there are still deficiencies that need to be addressed? |
|
No longer necessary after #3218 |
This PR fixes two issues in dockerAuthorizer logic:
First, the scope deduplication was a bit too naive. This brings a scope-gramar aware deduplication logic, featuring:
repository:foo/bar:pull,push+repository:foo/bar:push,pull=>repository:foo/bar:pull,push)alphabethically, returned string slice is always sorted
alphabethically)
repository:foo/bar:pull+repository:foo/bar:push=>repository:foo/bar:pull,push)The Deduplication code comes with a suite of test cases checking this behavior.
Second, the token cache takes into account token expiration, and associated scopes. Previously, as soon as a token was present in cache for a given host, it was used unconditionally for subsequent requests to this host. In some cases, if the subsequent requests required a broader scope than the original one, it would fail, To fix that, the PR adds: