search

opencontainers / go-digest

Common digest package used across the container ecosystem
https://www.opencontainers.org/
Other
184 stars 60 forks source link

[SUPERCEDED] Standardize and disambiguate the return from `digest.Parse` #76

Closed nima closed 1 year ago

nima commented 2 years ago

Please see https://github.com/oras-project/oras-go/issues/225 and https://github.com/oras-project/oras-go/pull/237 for background.

Superseded by #77, #78

nima commented 2 years ago

Thank you again for the detailed review here @thaJeztah.

I agree with everything you've pointed out here, and I've reverted the change in verifiers.go. Some of the confusion I've inadvertently introduced is due to my own inability to fully wrap my head around these repositories.

The changes that remain now are:

  1. digest.go: make Parse return an empty string if there's an error, and address the impact that change has in digestset/set.go
  2. The addition of two abstraction functions; namely IsIdenticalTo (which compares this digest to another), and IsEmpty.

What are your thoughts on these?

I left some comments, but I'm a bit confused about the change itself; IIUC from the linked issue, the issue you're trying to resolve is a registry that doesn't return the Docker-Content-Digest header. The code in this repository only provides the tools to perform the validation, but does not handle reading the Docker-Content-Digest specifically (i.e., greping for that string does not produce a result in the source code).

Note that the header is optional but if it's returned by the registry, it should contain a valid digest. From the spec;

https://github.com/opencontainers/distribution-spec/blob/4ab4752c3b86a926d7e5da84de64cbbdcc18d313/spec.md#legacy-docker-support-http-headers

Legacy Docker support HTTP headers

Because of the origins this specification, the client MAY encounter Docker-specific headers, such as Docker-Content-Digest, or Docker-Distribution-API-Version. These headers are OPTIONAL and clients SHOULD NOT depend on them.

However, IF the header is present, and IF the client uses the header, it MUST be validated;

https://github.com/opencontainers/distribution-spec/blob/4ab4752c3b86a926d7e5da84de64cbbdcc18d313/spec.md#pulling-manifests

A GET request to an existing manifest URL MUST provide the expected manifest, with a response code that MUST be 200 OK. A successful response SHOULD contain the digest of the uploaded blob in the header Docker-Content-Digest. The Docker-Content-Digest header, if present on the response, returns the canonical digest of the uploaded blob which MAY differ from the provided digest. If the digest does differ, it MAY be the case that the hashing algorithms used do not match. See Content Digests apdx-3 for information on how to detect the hashing algorithm in use. Most clients MAY ignore the value, but if it is used, the client MUST verify the value against the uploaded blob data.

So if the registry is not returning a Docker-Content-Digest, no validation is performed with it, but if it does return the header, it should be valid; which means that if the registry returns an empty value, there's a bug in that registry implementation, and it doesn't comply with the specification.

nima commented 1 year ago

Hi @thaJeztah, when you get some time could you please take a look at this change and give me your feedback and list of concerns?

AkihiroSuda commented 1 year ago

The PR title and the description text is confusing. Hard to understand what this PR is.

nima commented 1 year ago

I've dropped the dependency on this change in oras-go, but I wonder if it still makes sense to keep this change. a) returning a payload + an error seems like something that could lead to confusion, but probably generally safe (since callers generally first check err). b) Do you think the helpers are adding value, or extra code that's not really worth the added LoC?

And @AkihiroSuda I'm happy to move them, just want to make sure that I'm not the only one that thinks it's a good idea to have these new helper/abstraction methods. Thank you for the review too.

nima commented 1 year ago

The PR title and the description text is confusing. Hard to understand what this PR is.

I'll clean this up now and split it into two separate PRs.

nima commented 1 year ago

This was my first PR, and it was (as pointed out) confusing, poorly documented, and contained more than one target change.

Superseded by #77, #78