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

Proposal: set custom config media type on attestation manifests #3610

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Feb 9, 2023

This patch sets the media type for attestation manifests to application/vnd.docker.attestation.config.v1+json instead of faking a application/vnd.oci.image.config.v1+json type.

This updates the attestation manifests inline with the OCI artifact type defined at https://github.com/opencontainers/artifacts. Aside from just making us inline with the spec here, we would hopefully be able to see better integrations in registries that just handle config media types and don't inspect the custom buildkit attestations.

Additionally, this makes it firmly impossible for runtimes to accidentally download and run the attestation manifest, treating it as an image manifest.

@jedevc
Copy link
Member Author

jedevc commented Feb 9, 2023

I did a quick test to confirm how well this works across the different registries we test against: https://github.com/docker/build-push-action/actions/runs/4124641031. Looks like the only failure major failure is on GitLab container registry, which is because they validate the config media type against a list of hardcoded types in the database (for example, see here).

@jedevc jedevc force-pushed the attestation-config-media-type branch from 723216e to de1927b Compare February 9, 2023 10:44
@sudo-bmitch
Copy link

sudo-bmitch commented Feb 9, 2023

I believe quay.io is also filtering on the config media type.

@jedevc
Copy link
Member Author

jedevc commented Feb 9, 2023

Ah shoot, I could have sworn we had e2e tests for quay in docker/build-push-action 😢 cc @crazy-max is it feasible to set something like that up?

@jedevc jedevc force-pushed the attestation-config-media-type branch from de1927b to 9afd5a7 Compare February 9, 2023 11:12
@jedevc
Copy link
Member Author

jedevc commented Feb 10, 2023

Did a test that includes quay as well 👀 https://github.com/docker/build-push-action/actions/runs/4142977994

Yup, as expected this fails, it explicitly fails against a validation check:

MANIFEST_INVALID

failed to parse manifest: manifest data does not match schema: 'application/vnd.docker.attestation.config.v1+json' is not one of ['application/vnd.oci.image.config.v1+json', 'application/vnd.cncf.helm.config.v1+json', 'application/vnd.oci.source.image.config.v1+json', 'application/vnd.unknown.config.v1+json']

@jedevc
Copy link
Member Author

jedevc commented Feb 13, 2023

Marking as do-not-merge until:

  • We can test with more registries to find out who we break against
  • We can notify those upstream registries of our plans so we can avoid breaking pipelines (again 😢)

This patch sets the media type for attestation manifests to
"application/vnd.docker.attestation.config.v1+json" instead of faking a
"application/vnd.oci.image.config.v1+json" type.

This updates the attestation manifests inline with the OCI artifact type
defined at https://github.com/opencontainers/artifacts. Aside from just
making us inline with the spec here, we would hopefully be able to see
better integrations in registries that just handle config media types
and don't inspect the custom buildkit attestations.

Additionally, this makes it *firmly* impossible for runtimes to
accidentally download and run the attestation manifest, treating it as
an image manifest.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
Copy link
Member Author

jedevc commented Mar 3, 2023

We've added a few more tests to the e2e suite for build-push-action, I'm fairly confident that the list includes most of the registries we're interested in.

Out of those, the only ones that break are GitLab and Quay (https://github.com/docker/build-push-action/actions/runs/4321829355/jobs/7543515404), so I think we can try and reach out to them to see about making this change for the next release of BuildKit.

@jedevc jedevc force-pushed the attestation-config-media-type branch from 9afd5a7 to 1c26b6e Compare March 3, 2023 09:02
@deleteriousEffect
Copy link

deleteriousEffect commented Mar 3, 2023

Speaking for the GitLab registry, we don't accept media types we don't recognize. Explicitly adding support for anything new gives us the opportunity to ensure that we can properly validate media types we accept and that they behave with our features, particularly online garbage collection, in expected ways.

Essentially, we're choosing to value stability over flexibility and experimentation.

Please open an issue if you want us to support this media type: https://gitlab.com/gitlab-org/container-registry/-/issues

@tianon
Copy link
Member

tianon commented May 19, 2023

With opencontainers/image-spec#1043, this should maybe be updated to use artifactType instead? 👀

(probably also / separately worth considering setting subject to the original image manifest descriptor so that you're "referrers API ready" 😅)

@sudo-bmitch
Copy link

sudo-bmitch commented Jul 28, 2023

If the config is currently empty and there are no plans to add config blob contents in the future, then artifactType would be a better option. The docs currently say other content may be added later, so this becomes a question to the buildkit devs if that's really needed or if you can put everything inside the SLSA attestation.

For registry support, GitLab is the only one I'm aware of that has an approve list for config.mediaTypes, Quay has removed theirs.

If the subject field is used (which is the OCI way to associate artifacts with an image), then expect issues with Docker Hub and ECR until we release OCI image/distribution 1.1.0.

Personally, I'd like to see the artifactType set to something like application/vnd.in-toto+json with an annotation indicating it was built by docker to allow these attestations to be used by multiple tools.

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

Successfully merging this pull request may close these issues.

4 participants