Support sha512 content

[sudo-bmitch]

Fixes #494.

Pending tasks before removing this from draft:

• Redesign conformance tests in a separate PR to make a matrix of types of content and registry APIs to test
• Add conformance tests
• Get buy-in from implementations of clients and registries

[sudo-bmitch]

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.

[sudo-bmitch]

@rchincha it would be good to get your feedback on this, and whether Zot would be able to support it.

[rchincha]

project-zot/zot#2075
^ on zot side

[andaaron]

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.

[sudo-bmitch]

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.

[rchincha]

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.

[sudo-bmitch]

We use query params in a lot of other parts of the spec. Is there a reason this should be a header?

[rchincha]

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.

[sudo-bmitch]

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.

[mikebrow]

same here... the client may include sha256 to ensure it is supported, this for future use cases presuming there will eventually be replacement(s)

[andaaron]

Hi @sudo-bmitch

project-zot/zot#2075 ^ on zot side

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.

[rchincha]

@sudo-bmitch at least in "draft" state, maybe update zot to point to @andaaron 's tree/commit.
andaaron/zot@6812b5c

[tianon]

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?

[sudo-bmitch]

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.

[mikebrow]

agree with the idea of at least showing both in the examples... explaining it is normally default.. and testing...

[sajayantony]

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.

[sudo-bmitch]

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.

[tianon]

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.

[wking]

...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 PUT request MUST include the <digest> of the whole blob (not the final chunk) as a query parameter.

We could add:

If the session-opening POST set the digest-algorithm query parameter, the closing PUT request'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 code 400 Bad Request for invalid or unsupported algorithms.
If the registry responses with 201 Created or 202 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.

[sudo-bmitch]

@sudo-bmitch at least in "draft" state, maybe update zot to point to @andaaron 's tree/commit. andaaron/zot@6812b5c

@rchincha I need the changes for this:

distribution-spec/Makefile

Lines 95 to 105 in 11b8e3f

	TEST_REGISTRY_CONTAINER ?= ghcr.io/project-zot/zot-minimal-linux-amd64:v2.0.0-rc6@sha256:bf95a94849cd9c6f596fb10e5a2d03b74267e7886d1ba0b3dab33337d9e46e5c
	registry-ci:
	docker rm -f oci-conformance && \
	mkdir -p $(OUTPUT_DIRNAME) && \
	echo '{"distSpecVersion":"1.1.0-dev","storage":{"rootDirectory":"/tmp/zot","gc":false,"dedupe":false},"http":{"address":"0.0.0.0","port":"5000"},"log":{"level":"debug"}}' > $(shell pwd)/$(OUTPUT_DIRNAME)/zot-config.json
	docker run -d \
	-v $(shell pwd)/$(OUTPUT_DIRNAME)/zot-config.json:/etc/zot/config.json \
	--name=oci-conformance \
	-p 5000:5000 \
	$(TEST_REGISTRY_CONTAINER) && \
	sleep 5

Do you have a container image to point to?

[rchincha]

@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?

[sudo-bmitch]

@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?

[rchincha]

@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.
Thoughts?

[sudo-bmitch]

I would focus only on making PUT /v2/...blob/?digest=sha512:... work first.
Thoughts?

@rchincha since that's already in the spec, what would OCI need to do in that scenario? Does that involve closing this PR and doing nothing? Would we leave #494 unresolved?

[rchincha]

I would focus only on making PUT /v2/...blob/?digest=sha512:... work first.
Thoughts?

@rchincha since that's already in the spec, what would OCI need to do in that scenario? Does that involve closing this PR and doing nothing?

I think we still need to resolve the question with guidelines of mixing and matching digests. Here is a sequence (really #494 (comment) rephrased)

1. Customer one builds an image (say A) with sha256 and signs and pushes to a registry
2. Customer two builds another image (say B) derived from A but with sha512 and push to a registry
   2.1 If the manifest is pushed as sha512, do we then convert everything to sha512?
   2.2 Or do we not? coz signatures and sucks for registries, and I would like to keep the ability to trace back image lineage etc.
   2.3 If registries federate and pull from other registries, do they do 2.1 or 2.2 above?
   2.4 Are clients supposed to do some of the above instead?
3. Nice that we now support multiple digests, but what if one of them is broken, what is the path forward. A bad hash algo will bite us maybe 10 years from now.
   etc, etc ...

We can lean on some lessons from other content-addressable projects:
https://lwn.net/Articles/898522/

My concern (IMHO) is we have not set a high-level direction or rationale yet for above first.

Would we leave #494 unresolved?

No, will resolve shortly after.

[sudo-bmitch]

@rchincha

I think we still need to resolve the question with guidelines of mixing and matching digests. Here is a sequence (really #494 (comment) rephrased)

1. Customer one builds an image (say A) with sha256 and signs and pushes to a registry

2. Customer two builds another image (say B) derived from A but with sha512 and push to a registry
   2.1 If the manifest is pushed as sha512, do we then convert everything to sha512?

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.

2.2 Or do we not? coz signatures and sucks for registries, and I would like to keep the ability to trace back image lineage etc.

This.

2.3 If registries federate and pull from other registries, do they do 2.1 or 2.2 above?

Mirrors and the likes should not be modifying the manifest. They need to retain the content addressable guarantee.

2.4 Are clients supposed to do some of the above instead?

Clients are free to do anything when building. If they modify content and change the digest, it's now their responsibility.

3. Nice that we now support multiple digests, but what if one of them is broken, what is the path forward. A bad hash algo will bite us maybe 10 years from now.

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.

We can lean on some lessons from other content-addressable projects: https://lwn.net/Articles/898522/

My concern (IMHO) is we have not set a high-level direction or rationale yet for above first.

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.

Would we leave #494 unresolved?

No, will resolve shortly after.

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 sudobmitch/demo:sha512 is not referenced with a sha512 digest itself). That also means it's not possible to push referrers to a tagged manifest unless the referrers happen to use that same algorithm that the registry defaults to.

[rchincha]

Can we capture all this (rationale) somewhere?
"This is how OCI is thinking about this" ...

[andaaron]

@sudo-bmitch at least in "draft" state, maybe update zot to point to @andaaron 's tree/commit. andaaron/zot@6812b5c

@rchincha I need the changes for this:

distribution-spec/Makefile

Lines 95 to 105 in 11b8e3f

	TEST_REGISTRY_CONTAINER ?= ghcr.io/project-zot/zot-minimal-linux-amd64:v2.0.0-rc6@sha256:bf95a94849cd9c6f596fb10e5a2d03b74267e7886d1ba0b3dab33337d9e46e5c
	registry-ci:
	docker rm -f oci-conformance && \
	mkdir -p $(OUTPUT_DIRNAME) && \
	echo '{"distSpecVersion":"1.1.0-dev","storage":{"rootDirectory":"/tmp/zot","gc":false,"dedupe":false},"http":{"address":"0.0.0.0","port":"5000"},"log":{"level":"debug"}}' > $(shell pwd)/$(OUTPUT_DIRNAME)/zot-config.json
	docker run -d \
	-v $(shell pwd)/$(OUTPUT_DIRNAME)/zot-config.json:/etc/zot/config.json \
	--name=oci-conformance \
	-p 5000:5000 \
	$(TEST_REGISTRY_CONTAINER) && \
	sleep 5

Do you have a container image to point to?

Hi @sudo-bmitch, can you try out ghcr.io/andaaron/zot-minimal-linux-amd64:v2.1.0-manifest-digest?

[sudo-bmitch]

Can we capture all this (rationale) somewhere? "This is how OCI is thinking about this" ...

@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.

[rchincha]

Can we capture all this (rationale) somewhere? "This is how OCI is thinking about this" ...

@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.
IMO, important that an uninformed reader understands the context of this change and also for our sake.

[rchincha]

#547

[sudo-bmitch]

Hi @sudo-bmitch, can you try out ghcr.io/andaaron/zot-minimal-linux-amd64:v2.1.0-manifest-digest?

@andaaron thanks, I've got it working with that image (along with some other cleanups).

[sudo-bmitch]

Something pointed out by @thaJeztah is this text from image-spec:

Implementations MAY implement SHA-512 digest verification for use in descriptors.

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.

[rchincha]

Without a MUST, that means this conformance test needs to be optional

dist-spec v1.2.0?

[sudo-bmitch]

Without a MUST, that means this conformance test needs to be optional

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.

[andaaron]

I can't help but notice the discussions are too much focused on positives/negatives of SHA256 vs SHA512 vs BLAKE3.

IMHO regardless of the feasibility of SHA512 or BLAKE3, the spec doesn't allow the client to tell the server what specific hash algorithm to use for a manifest pushed as a tag, and this is a serious oversight of the dist spec.

The hash algorithm value is outside of the scope of the disc spec (as stated in some of the related discussions such as #547 (comment)). So the dist spec should not force a specific algorithm like SHA256 by not allowing the client to ask for something else. What those alternatives are is not a dist spec concern, it is an image spec concern.

Note in the description/first comment of #494 the alternatives to SHA256 aren't even mentioned.
SHA512 was brought up as an example, as it is already part of the image spec here https://github.com/opencontainers/image-spec/blob/main/descriptor.md#registered-algorithms.

This being said, I suggest to focus on wrapping up on #494, which is supposed to define the API, and not on SHA256 vs SHA512 vs BLAKE3.

[andaaron]

Having all commands joined by && ensures make executes them in the same shell.

In the new code on the right side they seem to be executed in separate shells since they are different rows in the make target. (I also don't see .ONESHELL being used). I don't think this is intended, even if it is unlikely someone would use make -j

It may be worth it to double check the new behavior of the make target is correct.

[sudo-bmitch]

From my limited testing, make -j ... would still run all steps within a single job in order. Removing the && was intended to let make better report failures when one step in the middle of a long list of steps fails. Without that, users are guessing if it's the mkdir, or which of the two docker commands output some error message. However, I'm far from a make expert, so if there are errors this creates, send a reproducible since I'd like to better understand this.

[andaaron]

Nevermind, all is ok, please resolve

[andaaron]

See my other comment on this file, same applies here.

[andaaron]

It doesn't apply here, these commands are independent. let's mark this as resolved.

[sudo-bmitch]

This being said, I suggest to focus on wrapping up on #494, which is supposed to define the API, and not on SHA256 vs SHA512 vs BLAKE3.

I believe this PR would resolve #494, and my focus is on the pending tasks listed in the description, leaving others to the blake3 work. (Mainly because, until it's part of a released version of go-digest, adding blake3 support to my own projects will be a big lift.)

Rewriting the conformance tests is non-trivial, and I'm chipping away at it in my spare time. That effort is likely to intersect with a lot of other OCI projects, including wg-auth, support for sparse manifests, standardizing the streaming blob push API, and I'm sure a lot of other gaps that I'm forgetting right now.

[andaaron]

Sorry, my impression was the coding part was complete, and this was blocked by disagreements on whether or not SHA512 is useful.

[wking]

Rewriting the conformance tests is non-trivial, and I'm chipping away at it in my spare time.

Your initial description here includes "Redesign conformance tests in a separate PR...". Do you have a link to that separate pull, even if it's still in the early/draft stage? Or some other way for folks to collaborate to grind down that blocker, without reproducing whatever work you've already done locally?

[sudo-bmitch]

Rewriting the conformance tests is non-trivial, and I'm chipping away at it in my spare time.

Your initial description here includes "Redesign conformance tests in a separate PR...". Do you have a link to that separate pull, even if it's still in the early/draft stage? Or some other way for folks to collaborate to grind down that blocker, without reproducing whatever work you've already done locally?

The current progress is up at https://github.com/sudo-bmitch/distribution-spec/tree/pr-conformance-v2 but I don't think collaborating on this at such an early stage would make it go faster. Working through some of the other co-dependent issues would be much more helpful. A couple of important ones are:

• https://github.com/opencontainers/wg-auth
• Streamed Blob Upload not defined by spec #303

[wking]

Working through some of the other co-dependent issues would be much more helpful. A couple of important ones are...

Hmm, I'm not clear on how either of those co-depend on this pull, they seem pretty orthogonal to "which digest algorithm?". #303 also seems pretty sticky (clarifying a distribution-spec-specific chunked upload, when there's already generic HTTP for chunking and pipelining). So both 303 and wg-auth look like they could turn into "survey many registries and clients to figure out what sorts of things folks actually do" research projects. While that's great for anyone with time, it's a lot less bounded than "refactor a test suite to handle flexible digest hash registration", and I don't think I personally have time for large research projects, or the registry/client background to be able to pull that kind of context together quickly. Sorry!

[sudo-bmitch]

Working through some of the other co-dependent issues would be much more helpful. A couple of important ones are...

Hmm, I'm not clear on how either of those co-depend on this pull, they seem pretty orthogonal to "which digest algorithm?".

They aren't codependent on supporting other digest algorithms. They are codependent on a refactor/rewrite of the conformance tests. Both auth and a streaming blob push are included in the conformance tests but not in the spec.

The reason for the conformance test rewrite is because the current design has reached its limits and cannot be easily extended with the directions being taken by the spec. Supporting content with different digest algorithms is just one of the capabilities of a new conformance test. So while this PR is blocked by that work, that work is not only focused on this PR.