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

nima · 2022-07-15T07:52:55Z

~~Please see oras-project/oras-go#225 and oras-project/oras-go#237 for background.~~

Superseded by #77, #78

thaJeztah

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.

digest.go

thaJeztah · 2022-07-15T11:59:23Z

digest.go

+	if err := d.Validate(); err != nil {
+		return "", err
+	}


🤔 the existing code looks a bit odd indeed (always returning the digest, even if it didn't validate).

However, it looks like there's some code that uses the returned value even if there's an error;

go-digest/digestset/set.go

Lines 95 to 103 in bde1400

dgst, err := digest.Parse(d)

if err == digest.ErrDigestInvalidFormat {

hex = d

searchFunc = func(i int) bool {

return dst.entries[i].val >= d

}

} else {

hex = dgst.Hex()

alg = dgst.Algorithm()

(i.e., it only considers the error if it's a digest.ErrDigestInvalidFormat). Not sure if that's a bug, or if that's intentional (in either case, the documentation of this function should be updated to explain, and the code I linked to could use some comments as well)

@dmcgowan I see you wrote that code in 77570c9 (#55) perhaps you recall?

I've made a change that I think should address this (valid) issue that you've picked up with the change I made. Let me know if you think it seems reasonable to you.

Why not keep the previous code?

verifiers.go

nima · 2022-07-16T04:08:52Z

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:

digest.go: make Parse return an empty string if there's an error, and address the impact that change has in digestset/set.go
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 · 2022-08-18T06:03:28Z

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?

Specifically, either return (<something>, nil), or (<nothing>, err), but not (<something>, err) which is difficult to reason about. Add two abstraction helper methods: `IsIdenticalTo`, and `IsEmpty` Signed-off-by: Nima Talebi <[email protected]>

AkihiroSuda · 2022-08-18T06:36:59Z

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

AkihiroSuda · 2022-08-18T06:39:27Z

digest.go

+// IsIdenticalTo compares this digest to any other, inclusive of the hashing algorithm used
+func (d Digest) IsIdenticalTo(o Digest) bool {
+	return string(d) == string(o)
+}


This should be probably a separate PR

AkihiroSuda · 2022-08-18T06:39:34Z

digest.go

 func (d Digest) Hex() string {
 	return d.Encoded()
 }

+// IsEmpty does what it says (an empty digest is an empty string)
+func (d Digest) IsEmpty() bool {


This one too

nima · 2022-08-18T15:44:51Z

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 · 2022-08-25T21:22:37Z

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 · 2022-08-26T00:46:12Z

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

nima requested review from AkihiroSuda, dmcgowan, stevvooe, thaJeztah and vbatts as code owners July 15, 2022 07:52

nima marked this pull request as draft July 15, 2022 07:56

nima force-pushed the oras-project/oras-go/issues/225 branch from 1ec2594 to 6c1940b Compare July 15, 2022 07:57

thaJeztah requested changes Jul 15, 2022

View reviewed changes

nima force-pushed the oras-project/oras-go/issues/225 branch from 69926df to 7f14c37 Compare July 16, 2022 04:01

nima force-pushed the oras-project/oras-go/issues/225 branch from 5bc3115 to 774d105 Compare July 16, 2022 04:17

nima mentioned this pull request Aug 18, 2022

fix: oras pull error empty response Docker-Content-Digest oras-project/oras-go#237

Merged

nima force-pushed the oras-project/oras-go/issues/225 branch from 774d105 to 6b741e5 Compare August 18, 2022 04:37

nima marked this pull request as ready for review August 18, 2022 04:47

nima force-pushed the oras-project/oras-go/issues/225 branch 2 times, most recently from 765f894 to 6b741e5 Compare August 18, 2022 05:00

fix: Disambiguate return from Parse

18ee753

Specifically, either return (<something>, nil), or (<nothing>, err), but not (<something>, err) which is difficult to reason about. Add two abstraction helper methods: `IsIdenticalTo`, and `IsEmpty` Signed-off-by: Nima Talebi <[email protected]>

nima force-pushed the oras-project/oras-go/issues/225 branch from 6b741e5 to 18ee753 Compare August 18, 2022 06:09

AkihiroSuda reviewed Aug 18, 2022

View reviewed changes

nima changed the title ~~Oras project/oras go/issues/225~~ Standardize and disambiguate the return from digest.Parse Aug 26, 2022

nima mentioned this pull request Aug 26, 2022

fix: Standarize/disambiguate return from Parse #77

Open

nima changed the title ~~Standardize and disambiguate the return from digest.Parse~~ Standardize and disambiguate the return from digest.Parse [SUPERCEDED] Aug 26, 2022

nima changed the title ~~Standardize and disambiguate the return from digest.Parse [SUPERCEDED]~~ [SUPERCEDED] Standardize and disambiguate the return from digest.Parse Aug 26, 2022

nima closed this Aug 26, 2022

nima deleted the oras-project/oras-go/issues/225 branch August 26, 2022 00:46

nima mentioned this pull request Aug 26, 2022

feat: Add two abstraction helper methods #78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nima commented Jul 15, 2022 •

edited

Loading

thaJeztah left a comment

thaJeztah Jul 15, 2022

nima Jul 16, 2022 •

edited

Loading

AkihiroSuda Aug 18, 2022

nima commented Jul 16, 2022 •

edited

Loading

Legacy Docker support HTTP headers

nima commented Aug 18, 2022

AkihiroSuda commented Aug 18, 2022

AkihiroSuda Aug 18, 2022

AkihiroSuda Aug 18, 2022

nima commented Aug 18, 2022 •

edited

Loading

nima commented Aug 25, 2022

nima commented Aug 26, 2022 •

edited

Loading

	dgst, err := digest.Parse(d)
	if err == digest.ErrDigestInvalidFormat {
	hex = d
	searchFunc = func(i int) bool {
	return dst.entries[i].val >= d
	}
	} else {
	hex = dgst.Hex()
	alg = dgst.Algorithm()

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

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

Conversation

nima commented Jul 15, 2022 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

Legacy Docker support HTTP headers

thaJeztah Jul 15, 2022

Choose a reason for hiding this comment

nima Jul 16, 2022 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda Aug 18, 2022

Choose a reason for hiding this comment

nima commented Jul 16, 2022 • edited Loading

Legacy Docker support HTTP headers

nima commented Aug 18, 2022

AkihiroSuda commented Aug 18, 2022

AkihiroSuda Aug 18, 2022

Choose a reason for hiding this comment

AkihiroSuda Aug 18, 2022

Choose a reason for hiding this comment

nima commented Aug 18, 2022 • edited Loading

nima commented Aug 25, 2022

nima commented Aug 26, 2022 • edited Loading

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

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

nima commented Jul 15, 2022 •

edited

Loading

nima Jul 16, 2022 •

edited

Loading

nima commented Jul 16, 2022 •

edited

Loading

nima commented Aug 18, 2022 •

edited

Loading

nima commented Aug 26, 2022 •

edited

Loading