-
Notifications
You must be signed in to change notification settings - Fork 208
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.
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
Add a header with manifest tag to pull and push #358
base: main
Are you sure you want to change the base?
Add a header with manifest tag to pull and push #358
Conversation
Signed-off-by: Yinon Avraham <[email protected]>
Also opened a PR with the implementation of adding this header: moby/moby#44359 |
If the goal is to achieve tag-based authorization, I'm not following how adding header would enable this. The actual token needs to have it in the scope https://www.rfc-editor.org/rfc/rfc6749#section-4.2.2 Currently the distribution spec also doesn't define authentication in any form and kind of leave it to implementations to define this -e.g - https://docs.docker.com/registry/spec/auth/oauth/#example-refreshing-an-access-token |
Because the distribution spec leaves authentication and authorization out of scope (which IMHO is a good decision), and because this information is helpful in several use cases, the suggestion here is to add it as a new request header. For tag based authorization, having the header with the relevant tag when pushing or pulling a manifest or blob by digest can be very helpful. It gives the registry a hint on the context which points it on which tag to check, which leads the registry to the actual manifest to see if the requested blob is referenced. The auth token only serves as a mean for carrying the granted permissions. The header is used to easily locate the tag's manifest. Without it, the registry needs to get all tagged manifests of the named image, filter only those referencing the blob, and check each one to find the first that the auth token grants permissions on. |
Some stream of consciousness feedback:
|
@sudo-bmitch thanks for the feedback.
Understood. The goal with the suggested header name was to be aligned with similar existing headers like
Right, mentioning the
This is one of the reasons the header is added to each request, not relying on the auth. Anonymous access can be viewed at the same as any other principal. A repository owner might want to open specific tags for anonymous access, but require auth on other tags. Having the header on each request, regardless of the auth, supports it.
True for pull, not sure about push. But anyway - pulling (or pushing) images by digest is a different story, not covered by the suggestion in this PR. I did start to think about it, but decided to not mix the two for now and keep this PR focused on tags.
True, but knowing which manifest list to start with by its tag is a very big hint which can greatly reduce the search area. The common recursion level is 2 (manifest list -> manifests) and the number of leaf manifests is usually small (e.g. number of supported platforms).
Good point. As much as I saw, the reference implementation (moby) takes the tags one-by-one and relies on the
You can verify that the authenticated principal is authorized to push the specific tag and fail the upload of the blobs earlier in the process, instead of only when the manifest is uploaded.
Registries must take such "lies" under consideration. The suggestion says this is not a hard requirement - clients SHOULD send it, registries MAY use it. This header acts as a hint, enabling the registry to improve some actions based on it. |
Here's some context from when the spec was being written: #26 Looking back at the suggested use cases:
Scoping access to tags instead of repos is certainly something that I've seen interest in. But I don't think depending on user hints is a good way to securely implement it. Maybe I'm missing something.
Because of how blobs are reused between manifests, and manifests can have multiple tags, this feels like the wrong place to create that relationship, especially since images sharing layers may be pushed long after the blobs they are sharing were first pushed. But I spend most of my time on the client side, so I want leave space for registry operators to weigh in. For myself, I don't see a clear issue being solved by this that wouldn't be better solved another way. So it may help to work through what problems this solves, and why this is the best way to solve those problems. |
@sudo-bmitch thanks for the link regarding the header naming. As I understand, the suggestion is to name this field: Let's put the discussion on The suggestion is not to rely on the manifest tag in the header for scoping the request, as the header by itself cannot be trusted. The goal of the header is a hint for the registry on the context of the request, i.e. which manifest to check and verify this blob belongs to it. Today, when a registry receives a request for blob, and the request has authorization which is scoped to specific image repositories and tags (because it is explicit in the token, or implied by other means - specific to the auth provider), in order for the registry to check the request can access the requested blob, it needs to:
Having the tag added to the context of the request using a header gives a hint to the registry where to look. If the header is missing - nothing changed, this is the same as before (backward compatible). But if the header exists, the registry can check only that specific tag, avoiding the need to scan all other tags. If the client lies and hints with a wrong tag there are several options:
Except for tag based authorization, there are other viable use cases for adding this header. Another example is registries which collect statistics on downloads and uploads can use this quite simply in order to give more fine-grained reports such as how many bytes downloaded/uploaded per tag. |
X-ref: #328 |
@sajayantony , @sudo-bmitch , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments assume we pass a list of references rather than a correlation ID. But which makes the most sense is best worked out in the linked issue.
It was a quick discussion to raise some visibility, which is when we linked to the existing issue. We should probably work through design discussions in the issue first, before making changes in the PR. |
@sudo-bmitch I started to fix the PR based on the comments, but I have the following dilemma - To resolve that and make the header name more explicit, I suggest to use: |
- Rename header to OCI-Named-Ref - Remove "hint" - Merge to a single sentence, one for pull, one for push - Change SHOULD to MAY in the push process Signed-off-by: Yinon Avraham <[email protected]>
@yinonavraham That hits on #216. We've been rather inconsistent and need to clean that up. I would lean towards reference being the full name (e.g. registry/repo:tag) and coming up with a new name for a tag or descriptor. |
Signed-off-by: Yinon Avraham <[email protected]>
1858c7b
to
a579a51
Compare
@sudo-bmitch how can we push this forward? |
We need to get some reviews from other mods, but I think most of the focus is on getting a 1.1 release out right now. |
Signed-off-by: Yinon Avraham [email protected]
Add an optional request header
Docker-Manifest-Tag
to all requests in the push and pull operations.This request header, when added, shall include the tag value of the image being pushed / pulled.
For example, when pushing or pulling image
foo:1.2
, the following header will be added to all requests:Registries can then use this header, if it exists, for various use cases. For example:
/v2/
(ping) request - to identify the specific requested tag and by that limit the authorization request (auth challenge), making the authorization grant specific to that tag.../blobs/...
requests - optimize locating blobs while they are being pushed or pulled. This is useful in registry implementations which keep a link between tags and their blobs