Skip to content
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

Filter empty DOCKER_DIGEST_HEADER #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Feb 25, 2025

The spec says

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 matches the returned manifest. If the part of a manifest request is a digest, clients SHOULD verify the returned manifest matches this digest.

Specifically the spec says the header MUST be used if "if present on the response", but it does not define what present means. If present means "the header is simply there", then the current behavior is correct and the server is not spec compliant. However, if it means "the key is there and not empty", then the client needs to be more forgiving than it currently is.

The Go client seems to take the definition that the header must be present and non-empty.

Note: I discovered this when working against a Harbor registry using a proxy cache.

The spec says the header MUST be used if "if present on the response",
but does not define what present means. If present means "the key is
there", then the current behavior is correct and the server is not spec
compliant. However, if it means "the key is there and not empty", then
the client needs to be more forgiving than it currently is.

Signed-off-by: Ryan Levick <[email protected]>
Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on this @rylev. Thank you!

Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, could you add an additional test case for this in the digest validation tests?

@rylev
Copy link
Contributor Author

rylev commented Feb 28, 2025

@thomastaylor312 the change is a layer above the digest validation. The digest validation code hasn't changed at all. Is there a place where this code will be exercised in a test. If so, can you point me to it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants