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

Resolve several open issues #206

Merged
merged 5 commits into from
Dec 1, 2020

Conversation

pmengelbert
Copy link
Contributor

@pmengelbert pmengelbert commented Oct 28, 2020

Built on top of PR #204

Resolves #193 #26 #60 #68 #79 #126 #132 #183 #186 #195 #201 #202

@pmengelbert pmengelbert force-pushed the mini-updates branch 2 times, most recently from ffa8fcc to 1c4acb3 Compare October 28, 2020 20:51
@pmengelbert pmengelbert marked this pull request as ready for review November 10, 2020 16:20
spec.md Outdated
@@ -54,7 +58,7 @@ Several terms are used frequently in this document and warrant basic definitions
- **Pull**: the act of downloading Blobs and Manifests from a Registry
- **Blob**: the binary form of content that is stored by a Registry, addressable by a Digest
- **Manifest**: a JSON document which defines an Artifact. Manifests are defined under the OCI Image Spec <sup>[apdx-2](#appendix)</sup>
- **Config**: a section in the Manifest (and associated Blob) which contains Artifact metadata
- **Config**: a blob referenced in the Manifest (and associated Blob) which contains Artifact metadata and describes the Manifest
Copy link
Contributor

Choose a reason for hiding this comment

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

The (and associated Blob) here no longer makes sense.

and describes the Manifest

Does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the language here, thanks for the suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have an update to push with the updated language?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops 🥵 . Pushed now

spec.md Outdated Show resolved Hide resolved
spec.md Outdated
Location: <blob-location>
```

The digest contained in the `Location` header MAY be different from that of the blob that was mounted. As such, a client
Copy link
Member

Choose a reason for hiding this comment

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

Where did this wording come from related to using the digest?

The previous language was here https://github.com/opencontainers/distribution-spec/blob/f67bc11ba3a083a9c62f8fa53ad14c5bcf2116af/spec.md#cross-repository-blob-mount and is more clear. A client should NEVER use a digest from the registry without verifying it matches. Normally the client will just ignore this, I don't think it is correct to say the client should use it and verifying is expensive. Most registries use sha256 anyway, the case where it would differ is if the registry was using a different hashing algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most registries use sha256 anyway, the case where it would differ is if the registry was using a different hashing algorithm.

It would be great to be more explicit about this in the spec. The digest canonicalization section in the original spec seems mostly to do with signed schema 1 manifests:

To provide verification of http content, any response MAY include a Docker-Content-Digest header. This will include the digest of the target entity returned in the response. For blobs, this is the entire blob content. For manifests, this is the manifest body without the signature content, also known as the JWS payload. Note that the commonly used canonicalization for digest calculation MAY be dependent on the mediatype of the content, such as with manifests.

Though the section about different algorithms is good and maybe we should copy that over:

The client MAY choose to ignore the header or MAY verify it to ensure content integrity and transport security. This is most important when fetching by a digest. To ensure security, the content SHOULD be verified against the digest used to fetch the content. At times, the returned digest MAY differ from that used to initiate a request. Such digests are considered to be from different domains, meaning they have different values for algorithm. In such a case, the client MAY choose to verify the digests in both domains or ignore the server's digest. To maintain security, the client MUST always verify the content against the digest used to fetch the content.

IMPORTANT: If a digest is used to fetch content, the client SHOULD use the same digest used to fetch the content to verify it. The header Docker-Content-Digest SHOULD NOT be trusted over the "local" digest.

We already link out to https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md#digests but that links to https://github.com/opencontainers/image-spec/blob/v1.0.1/canonicalization.md#canonicalization which is a 404 🤷 Looks like it should be https://github.com/opencontainers/image-spec/blob/v1.0.1/considerations.md#canonicalization (fixed at HEAD) which links to https://github.com/opencontainers/image-spec/blob/v1.0.1/considerations.md#canonicalization but I don't see anything here that would make me think "oh, I should make sure everything is using the same algorithm", especially in the context of a client-registry interaction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a commit that reflects these suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, there may be a little bit of gap in specification around using sha256. There are a few spots where there is an assumption made and if the registry were to use a different algorithm, it could cause some issues. When starting an upload, the client should know the desired digest (but there are cases it doesn't). If it doesn't, and the client/registry are using different algorithms, the finalization would require the registry to either recalculate the digest, calculate it using multiple algorithms, or reject. In the case of cross repository mount, the digest is likely referenced in a manifest and it is not an option to use a different digest that the registry returns.

spec.md Outdated
@@ -54,7 +58,7 @@ Several terms are used frequently in this document and warrant basic definitions
- **Pull**: the act of downloading Blobs and Manifests from a Registry
- **Blob**: the binary form of content that is stored by a Registry, addressable by a Digest
- **Manifest**: a JSON document which defines an Artifact. Manifests are defined under the OCI Image Spec <sup>[apdx-2](#appendix)</sup>
- **Config**: a section in the Manifest (and associated Blob) which contains Artifact metadata
- **Config**: a blob referenced in the Manifest which contains Artifact metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also described in image-spec if you want to link to that (similar to how manifest links to apdx-2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again, just pushed changes addressing this and the above

spec.md Outdated Show resolved Hide resolved
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
wy65701436 added a commit to wy65701436/distribution-spec that referenced this pull request Feb 2, 2021
As PR opencontainers#206 has been merged, the testing code should be updated accordingly.
Signed-off-by: Wang Yan <wangyan@vmware.com>
wy65701436 added a commit to wy65701436/distribution-spec that referenced this pull request Feb 2, 2021
As PR opencontainers#206 has been merged, the testing code should be updated accordingly.
Signed-off-by: Wang Yan <wangyan@vmware.com>
@jdolitsky jdolitsky mentioned this pull request Mar 24, 2021
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 17, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 17, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 17, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 17, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 27, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 27, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added
jwhb added a commit to jwhb/krustlet that referenced this pull request Jul 27, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added

Signed-off-by: jwhb <jwhy@jwhy.de>
nitishm pushed a commit to nitishm/krustlet that referenced this pull request Oct 8, 2021
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added

Signed-off-by: jwhb <jwhy@jwhy.de>
flavianmissi added a commit to flavianmissi/distribution-spec that referenced this pull request May 12, 2022
Mentions `TAG_INVALID` and `MANIFEST_UNVERIFIED` as legacy error codes.

Also adjusts error codes' numbering - `code-11` was removed by
opencontainers#206.

Closes opencontainers#319

Signed-off-by: Flavian Missi <fmissi@redhat.com>
thomastaylor312 pushed a commit to krustlet/oci-distribution that referenced this pull request Mar 2, 2023
oci errors updated to be even with https://github.com/opencontainers/distribution-spec/blob/main/spec.md#errors-2
* TAG_INVALID and MANIFEST_UNVERIFIED were removed from OCI spec: opencontainers/distribution-spec#206
* TOO_MANY_REQUESTS was added

Signed-off-by: jwhb <jwhy@jwhy.de>
sudo-bmitch pushed a commit to sudo-bmitch/distribution-spec that referenced this pull request Aug 18, 2023
Mentions `TAG_INVALID` and `MANIFEST_UNVERIFIED` as legacy error codes.

Also adjusts error codes' numbering - `code-11` was removed by
opencontainers#206.

Closes opencontainers#319

Signed-off-by: Flavian Missi <fmissi@redhat.com>
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.

Update "conformance" section
4 participants