Support sha512 content #543
Conversation
sudo-bmitch
force-pushed
the
pr-digest-on-tag-push
branch
from
b347347 to
811619c
Compare
sudo-bmitch
changed the title
|
CI is failing on this because Zot will need to be updated for the changed APIs. I've been testing against olareg so I know this can work. This gives me more reasons to want to rewrite the conformance tests to focus on a matrix of content and APIs to test. The current tests are getting difficult to follow and manage. |
|
@rchincha it would be good to get your feedback on this, and whether Zot would be able to support it. |
|
project-zot/zot#2075 |
| @@ -329,6 +331,12 @@ The process remains unchanged for chunked upload, except that the post request M | |||
| Content-Length: 0 | |||
| ``` | |||
|
|
|||
| When pushing a blob with a digest algorithm other than `sha256`, the post request SHOULD include the `digest-algorithm` parameter: | |||
There was a problem hiding this comment.
Why do we need this new query param? We already send the expected digest (including the algorithm) as a mandatory parameter when closing the blob upload.
There was a problem hiding this comment.
Two reasons I can think of. It's a courtesy to registries that compute the digest as the blob is received and don't want to reread the blob to recompute it with a new algorithm. It's also a way for clients to get a fast failure if they request an algorithm not supported by the registry, before spending the time and bandwidth to upload the blob.
There was a problem hiding this comment.
Any particular reason this is a query param instead of a header? so API endpoint defs don't change (?)
What are our guidelines regarding what gets set as a header and what as a query param.
There was a problem hiding this comment.
We use query params in a lot of other parts of the spec. Is there a reason this should be a header?
There was a problem hiding this comment.
In the POST, PATCH, PUT sequence, we could have alternatively modeled this as 'Content-Type'/'Accept' type header exchange.
'OCI-Digest-Type'/'OCI-Accept-Digest-Type'? in the POST step itself.
^ just a thought!
In general, older registries will reject in the PUT step, and we cannot assume that registries are implementing streamed hashing.
There was a problem hiding this comment.
In general, older registries will reject in the PUT step, and we cannot assume that registries are implementing streamed hashing.
Correct, the failure condition on older registries requires the blob to be pushed first. This gives newer registries the ability to improve performance of the blob push.
There was a problem hiding this comment.
same here... the client may include sha256 to ensure it is supported, this for future use cases presuming there will eventually be replacement(s)
|
Hi @sudo-bmitch
This one was an intermediate step to replace hardcoded sha256 logic. This one project-zot/zot#2554 passes for me locally with the conformance tests in this PR. It implements the digest query parameter for pushing manifests. |
|
@sudo-bmitch at least in "draft" state, maybe update zot to point to @andaaron 's tree/commit. |
|
|
||
| `/v2/<name>/blobs/uploads/?digest-algorithm=<algorithm>` <sup>[end-4c](#endpoints)</sup> | ||
|
|
||
| Here, `<digest-algorithm>` is the algorithm the registry should use for the blob, e.g. `digest-algorithm=sha512`. |
There was a problem hiding this comment.
It's a minor nit, but shouldn't the example here still be sha256 since we don't want to accidentally encourage anyone to actually use sha512?
There was a problem hiding this comment.
The parameter isn't needed for sha256. That's specified on line 334. I think sha512 is the only other valid digest algorithm today that can be specified here.
There was a problem hiding this comment.
agree with the idea of at least showing both in the examples... explaining it is normally default.. and testing...
There was a problem hiding this comment.
Deprecating 256 at some point would be hard if the default for this new parameter is not specified. I general leaning towards explicit even for sha256.
There was a problem hiding this comment.
If a registry sees a request without a digest-algorithm, are we saying the registry can pick any blob algorithm it wants to, and if the client specifies a different one on the PUT then the request could fail after the blow was uploaded? I'd feel a lot safer if a missing algorithm is treated the same as a legacy client that is using sha256. In which case there won't be a difference between not specifying the parameter and specifying the parameter with sha256.
I'll still add it if that's the community desire, but I worry there may be registries that don't recognize the parameter and reject the request.
There was a problem hiding this comment.
The spec doesn't seem to officially say that's the expected behavior, so it really doesn't seem unreasonable for it to do something different than sha256 if it has a reason to.
There was a problem hiding this comment.
...the registry should use for the blob...
Tiptoeing into new space is tricky, because we don't want to break existing implementations, but we do want to set the stage for reliable future interactions. I think we can pin safely pin this down with some MUST, because the odds that an existing registry attaches some meaning to a digest-algorithm query parameter that's not "the digest that the client is expecting to use" seems low. But "use" is ambiguous, e.g. there's no problem if the registry decides to store a digest-algorithm=sha512 under a sha256 hash internally, all you need for internal storage is a unique identifier. What clients will care about is that future retrieval under their chosen digest works. So maybe, down where the closing PUT is discussed:
The closing
PUTrequest MUST include the<digest>of the whole blob (not the final chunk) as a query parameter.
We could add:
If the session-opening
POSTset thedigest-algorithmquery parameter, the closingPUTrequest's<digest>MUST use the requested<digest-algorithm>.
That would give registries grounds to reject crazy clients doing things like "haha, I said it was going to be sha256, but it's actually sha512:..., hash it again!". And we can require a MUST, because odds that existing clients set <digest-algorithm> for other purposes on the opening POST are very low.
And then, to protect clients from registries, we could replace the line this thread comments on with something like:
Here,
<digest-algorithm>is the algorithm the client intends to use for the blob, e.g.digest-algorithm=sha512.
The registry SHOULD respond with a response code400 Bad Requestfor invalid or unsupported algorithms.
If the registry responses with201 Createdor202 Accepted, the registry SHOULD support pulling the blob by<digest-algorithm>digests.
Ideally, we'd be able to MUST those, because a client requesting digest-algorithm=sha512, uploading a giant blob in chunks, and then getting a 400 "what? sha512:... is not a digest algorithm I recognize" on the closing PUT would be a waste of resources. But existing registries will likely ignore the digest-algorithm query parameter, and we don't want them to all be non-compliant with the distribution-spec as a whole. We want them to be compliant, but just not up to this new efficiency. Clients saddened by the innefficiency of only having their digest algorithm rejected on the closing PUT will be able to point at the SHOULD in the spec and advocate with the registry maintainers to save both sides the wasted traffic by implementing at least the 400 fast-fail guard on unsupported algorithms in the opening POST.
@rchincha I need the changes for this: Lines 95 to 105 in 11b8e3f Do you have a container image to point to? |
|
@sudo-bmitch Thinking about this some more ... Maybe split this work into two parts - 1) first the ability to handle a non-sha256 hash so no further API changes, and 2) the ability to optimize via streaming hints. It may turn out that 2) may not be needed at all. Thoughts? |
@rchincha I'm not sure I follow. What would you include in the first part, the part that has no API changes? |
I would focus only on making PUT /v2/...blob/?digest=sha512:... work first. |
I think we still need to resolve the question with guidelines of mixing and matching digests. Here is a sequence (really #494 (comment) rephrased)
We can lean on some lessons from other content-addressable projects: My concern (IMHO) is we have not set a high-level direction or rationale yet for above first.
No, will resolve shortly after. |
Once the manifest is pushed to the registry, the registry cannot modify it. Doing so would break pinned references and violate the content addressable guarantee. Before the push, it's a client decision of how they generate the manifest. I'd recommend against changing the digest of layers from a base image. Changing the digest of blobs implies foregoing cross-repository blob mounts, increases the number of copies of content on a registry and that need to be pulled to the client, and breaks tools that support rebasing images.
This.
Mirrors and the likes should not be modifying the manifest. They need to retain the content addressable guarantee.
Clients are free to do anything when building. If they modify content and change the digest, it's now their responsibility.
What if the broken algorithm is sha256 and we don't have a way to push content with other digest algorithms? What if registries respond to a broken algorithm by changing their default, breaking the ability to sign images and other uses of referrers? Will clients have to be configured differently for each registry they talk to? My goal is to solve this so we aren't stuck in one of these bad scenarios.
My goal is just to make it possible to push content with other algorithms, focusing on the currently supported algorithms first. Once that is done, I think we have a path forward to add new algorithms (keeping in mind that content built with a new algorithm is not portable to older registries or usable by older downstream consumers). I'm not focused on which algorithms we add in the future (that's a question for image-spec). And I think any security questions should get lumped into that discussion, which we can have later.
It can't be after. Without resolving #494 we'll never get to 2, because it's not possible to push a tagged manifest with anything other than the registry desired digest algorithm (hence the reason |
|
Can we capture all this (rationale) somewhere? |
Hi @sudo-bmitch, can you try out ghcr.io/andaaron/zot-minimal-linux-amd64:v2.1.0-manifest-digest? |
@rchincha I think most of this is covered by "The registry MUST store the manifest in the exact byte representation provided by the client" but feel free to open a PR and we'll discuss there. |
Let me take a stab at the text/content. Not sure where it should land yet. Maybe in the commit msg of this PR itself. |
sudo-bmitch
force-pushed
the
pr-digest-on-tag-push
branch
from
811619c to
04c6343
Compare
sudo-bmitch
force-pushed
the
pr-digest-on-tag-push
branch
from
04c6343 to
26643ef
Compare
Signed-off-by: Brandon Mitchell <[email protected]>
sudo-bmitch
force-pushed
the
pr-digest-on-tag-push
branch
from
26643ef to
063de3f
Compare
@andaaron thanks, I've got it working with that image (along with some other cleanups). |
|
Something pointed out by @thaJeztah is this text from image-spec:
Without a MUST, that means this conformance test needs to be optional, and any content generated with a different digest should be considered non-portable, not just in practice, but also according to the spec. |
dist-spec v1.2.0? |
It would have to be image-spec v1.2 changing the MAY to MUST language, semi-independent from distribution-spec. Clients would still need to be prepared for registries and other clients on older releases of each. For distribution-spec, I think the conformance test really needs to be a matrix style of test for the different types of content running across the various APIs. And an early failure to push the content should skip the remaining tests. I think we should also reorganize for a single path through the APIs, push, query, pull, delete, instead of the setup/teardown per test we have now. I'd want to do that as a separate PR from this, and then make this PR add a new entry to the types of content to test. |
| @@ -483,6 +497,7 @@ Location: <location> | |||
| The `<location>` is a pullable manifest URL. | |||
| The Docker-Content-Digest header returns the canonical digest of the uploaded blob, and MUST be equal to the client provided digest. | |||
| Clients MAY ignore the value but if it is used, the client SHOULD verify the value against the uploaded blob data. | |||
| If the client provided digest is invalid or uses an unsupported algorithm, the registry SHOULD respond with a response code `400 Bad Request`. | |||
|
|
|||
| An attempt to pull a nonexistent repository MUST return response code `404 Not Found`. | |||
|
|
|||
There was a problem hiding this comment.
These lines in the Referrers Tag Schema section seem problematic:
<alg>: the digest algorithm (e.g.sha256orsha512)<ref>: the digest from thesubjectfield (limit of 64 characters)
Hex-encoded sha512 hashes are 128 characters long, so you can't reference them without overrunning the 64 char limit. I guess on the upside, these lines don't use MUST or SHOULD, so altering them isn't technically breaking any spec constraints 😂 We could truncate at 64 characters, to avoid overrunning tag-length limits? Or we could drop limit considerations, and registries that supported the referrer API could figure out how to handle tags of that length before they started accepting the associate digest algorithm?
It seems like we could also reference the image-spec's digest section, and then suggest replacing the : (which is only allowed as the algorithm/encoded separator with a tag-compatible -, without having to have local details about either side? Unfortunately the image spec allows - in both algorithm-separator and encoded. But luckily you don't need to work backwards from referrer tag to digest, only forward from digest to referrer tag.
There was a problem hiding this comment.
The intention is to truncate the hash in the tag schema because tags have a length limit in the spec.
There was a problem hiding this comment.
Why does the tag length matter? Only <reference> as a tag` has a defined limit. Ref in this case would always be a digest, not a tag.
Would this digest truncating also be needed in the URL, not just in the new query parameter?
If someone pushed a blob today (ignoring this manifest endpoint related discussion in this PR) using the full sha512 digest in the URL, is he compliant with the latest published OCI spec?
There was a problem hiding this comment.
Why does the tag length matter? Only
<reference>as a tag` has a defined limit. Ref in this case would always be a digest, not a tag.
The "Referrers Tag Scheme" is a fallback tag, not a digest. That digest the tag refers to may already be pushed to the repository, and is the manifest that the referrers response is extending. These tags are not the digest of the manifest being pushed, they are the digest in the nested manifest's "subject" field.
There was a problem hiding this comment.
I imagine most registries that don't support the referrers API also won't support required changes for new digest algorithm support, negating any value in trying to change the spec here.
There was a problem hiding this comment.
I imagine most registries that don't support the referrers API also won't support required changes for new digest algorithm support, negating any value in trying to change the spec here.
The goal isn't forcing folks who don't want to be in this space into this space. The goal is to help registries and clients that want to be in this space be more likely to share the same expectations. For example, if client A assumes the tag uses 64-char-truncated encoded sections, but client B assumes the tag uses the full encoded section, they won't be able to find eachother's tags. And obviously, clients are free to do their own thing, regardless of what this spec says. What we want to do is be as clear as we can about what spec-compliant behavior looks like, so folks who are trying to be spec compliant don't accidentally diverge.
Maybe something like:
Referrers Tag Schema
The Referrers Tag associated with an Open Container Initiative Image Format Specification
digestMUST match the following grammar:referrers-tag ::= algorithm "-" truncated-encoded truncated-encoded ::= *64([a-zA-Z0-9=_-])where
truncated-encodedis the first section of thedigest'sencodedsection, truncated if necessary to 64 characters.
Clients who do that will have created a 1.whatever OCI digest spec Referrers Tag. Clients who do something else will have created... something else. Which maybe they have good uses for? All we're doing is being clear what we mean when we talk about Referrers Tags in this spec.
There was a problem hiding this comment.
For example, if client A assumes the tag uses 64-char-truncated encoded sections, but client B assumes the tag uses the full encoded section, they won't be able to find eachother's tags.
I'm fine with a clarification of the spec if you want to submit that PR. My point above is that B should always fail and A will often fail so I wouldn't put too much effort into it.
B will fail because the tag is invalid (exceeding the 128 character limit, with 128 characters for the hash, and 7 characters for the sha512- prefix):
Lines 164 to 166 in 7872490
A will likely fail because the manifest it references cannot be pushed by tag to any registry today (the reason for this PR). Registries are also free to reject sha512 content since support for that is only listed as a "MAY" in the spec.
Registries that want to implement sha512 support, including this PR for tagged content, will not be distribution-spec v1.1.0+ conformant without also implementing the referrers API, which eliminates the need to use the fallback tag on those registries.
There was a problem hiding this comment.
I'm fine with a clarification of the spec if you want to submit that PR
I've opened #563 with this clarification, so we can continue this discussion there, without distracting from this pull request. Feel free to mark this thread resolved here :)
Fixes #494.
Pending tasks before removing this from draft: