-
Notifications
You must be signed in to change notification settings - Fork 503
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
cosign download attestation
cannot filter by attestation predicate type
#3030
Comments
It looks like the Lines 153 to 163 in 83c08ce
Sure enough, the SLSA Provenance attestation doesn't have that annotation, but the SBOM one does. There's disparity in how the cosign verify-attestation $IMAGE --certificate-identity-regexp '.*' --certificate-oidc-issuer-regexp '.*' --type 'https://slsa.dev/provenance/v0.2'
Verification for quay.io/lucarval/festoji@sha256:61ac53fe9316734ed8a8bc6418b2c371730a2c122b5df1d7f76b960d381f5457 --
The following checks were performed on each of these signatures:
- The cosign claims were validated
- Existence of the claims in the transparency log was verified offline
- The code-signing certificate was verified using trusted certificate authority certificates
Certificate subject: https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_container_slsa3.yml@refs/tags/v1.4.0
Certificate issuer URL: https://token.actions.githubusercontent.com
GitHub Workflow Trigger: push
GitHub Workflow SHA: d9f2616186b454700a4c196e696b9a60fdcecfdb
GitHub Workflow Name: Package
GitHub Workflow Repository: lcarva/festoji
GitHub Workflow Ref: refs/heads/master
{"payloadType":"application/vnd.in-toto+json","payload":" That's because the cosign/pkg/policy/attestation.go Lines 84 to 87 in 83c08ce
|
The problem seems to be that older cosign versions do not add the I don't see anything in the in-toto spec about using annotations to specify the predicate type: https://github.com/in-toto/attestation/blob/main/spec/README.md#statement It does seem like the approach taken by Proposal: stop caring about the |
That sounds right to me—it does the right thing in more circumstances (including better backwards compat). CC @chaospuppy would love your thoughts here! |
Fixes sigstore#3030 Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
Fixes sigstore#3030 Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
Fixes sigstore#3030 Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
Fixes sigstore#3030 Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
Thanks for the ping @znewman01! So the motivation for adding the annotation to the attestation layer of the manifest (which is not part of the DSSE envelope or a diversion from the in-toto spec) was two-fold:
I do think it's confusing to have started adding the annotations at the same time they became usable, but that's why the default behavior matches what it was previously, where all the attestations are returned if a Maybe it would be best to update the fallback behavior if a |
@chaospuppy, thanks for the prompt reply! See inline comments below.
I'd be overly cautious of using a field that is not part of the spec.
Good point. I do think we should favor correctness over performance. IMO the performance hit here is negligible.
The issue is that the
I think this is an improvement over the current approach. However, I take issue in it being different than what I made a PR which makes the logic the same for both commands: #3031 Happy to hear what you think! |
Just to make sure we're thinking about this in the same way, this is what the manifest for an attestation looks like, where the annotations are being added in a standard location:
We are definitely in agreement about this, there is no standard that will mention I have no issue with making |
Ultimately I'm good with the PR being made because the annotations will still be getting created, but there's probably some followup work on my end to get the |
At the risk of complicating things even further, how does this play with OCI 1.1 reference types which support a filter by CC @sudo-bmitch Overall I defer to you all. My priorities are consistency between the different commands and correctness (no surprise missing data), and performance is a distant third. So I think I'd be okay with either:
IIUC #3031 does (2). Please let me know if I'm misunderstanding anything here, the OCI side of Cosign is not my strength :) |
I'm not too familiar with
If we do this in I think we should go with option 2, which is what the PR does, at least for now. There's a longer term feature request to push the predicate type annotation into the in-toto standard(?) to facilitate querying. Having cosign continuing adding those annotation makes sense to me and brings us closer to this goal. |
For an artifact, you do not want to use "application/vnd.oci.image.config.v1+json" for the config. That's a sign that this is a container image (though runtimes will throw errors when they see the layer media type). Instead you either need a dedicated config blob with your own type and content, or if you have no separate config, you would use the artifactType field. We are finishing up what the config descriptor would contain in opencontainers/image-spec#1068, but most likely an artifact that is just a {
"schemaVersion": 2,
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"artifactType": "application/vnd.dsse.envelope.v1+json",
"config": {
"mediaType": "application/vnd.oci.empty.v1+json",
"digest": "sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a",
"size": 2
},
"layers": [
{
"mediaType": "application/vnd.dsse.envelope.v1+json",
"size": 8216,
"digest": "sha256:90e4d184b4a6635ec6f3b88e244de0b7fe0a078569bbec710ea9109db29d8712",
"annotations": {
"dev.cosignproject.cosign/signature": "...",
"dev.sigstore.cosign/bundle": "...",
"dev.sigstore.cosign/certificate": "....",
"dev.sigstore.cosign/chain": "....",
"predicateType": "http://example.mytype"
}
}
]
} You'll likely want to feature gate this behind the oci-1-1 options in cosign since we haven't GA'd this yet. The one registry that I know you'll want to work with is GitLab who has an allow list on their config mediaType and may do the same for the artifactType. |
So sounds like you're on the money @lcarva, we will still need to set the annotation if we wish to identify which layer contains the desired predicate type, even after reference types are introduced. |
Assuming you only have a single layer, you would want to define any annotations useful for filtering on the manifest itself, rather than the layer. Annotations on the manifest are pulled up in the referrers response, allowing you to pick the dsse envelope artifact from a list of artifacts all created for the same image. This is a bit different from the current cosign design where there's one attestation manifest with multiple layers. The goal of the OCI design is to make it easier to add and remove entries, especially when the registry is managing the referrers response. |
Fixes #3030 Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
Description
Given a test image:
$ IMAGE='quay.io/lucarval/festoji@sha256:61ac53fe9316734ed8a8bc6418b2c371730a2c122b5df1d7f76b960d381f5457'
I can download all of its attestations:
However, I cannot use the
--predicate-type
parameter to select the SLSA provenance attestation:However, it does work for the SBOM attestation:
Version
I can also reproduce the issue on the current HEAD of main (83c08ce).
The text was updated successfully, but these errors were encountered: