Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add GitHub Artifact Attestations post #56
Add GitHub Artifact Attestations post #56
Changes from 3 commits
f44933e
530eb7c
c0ad5f7
f861586
730e380
3d08a1a
cbad725
1ed7357
bbc8e6c
7fec3c7
92db426
933edb0
2432242
568eed8
0af6aa3
01dfee7
2c273f6
620f039
f48c7fb
4f03d89
55f62d1
d89effe
8b73a5c
dd5c3a6
85fea8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, does the GH CLI check against the OID claims rather than the SLSA provenance currently? Does slsa-verifier do the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should do both. slsa-verifier does both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should do both. Otherwise the verification would pass but the provenance predicate that the CLI prints out with
--format json
(and thus the data that potentially gets fed to policy engines) could be different from the real values.It appears that the
sigstore-go
library will check against the OID claims using a policy which the GitHub CLI creates and send to sigstore-go's verifier.https://github.com/cli/cli/blob/ff3a59802dfe0957fba65584551783ece554a03a/pkg/cmd/attestation/verification/sigstore.go#L121
However, I haven't yet been able to verify that they also check these against the values in the predicate. Pretty sure that sigstore-go doesn't do that so the CLI should be doing it but I haven't found it in the code anywhere (I had a hidden pending comment about this that I just published now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe run some tests against the server by writing a nice tool.. which other package registries will be able to tests their impl with :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still haven't been able to verify that this actually happens. I've looked at the code and they use
sigstore-go
to verify the attestations with a policy that checks the owner/repo.https://github.com/cli/cli/blob/f9722d8df4161d66c8207bbe77e316c63c6d3a7c/pkg/cmd/attestation/verification/sigstore.go#L96-L144
I'm pretty sure sigstore's validation code doesn't look into the SLSA predicate to verify it matches the certificate but I haven't got that far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigstore-go verifies the subject only - https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/signature.go#L136-L184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitHub CLI also provides a
github.com/sigstore/sigstore-go/pkg/verify.PolicyBuilder
https://github.com/cli/cli/blob/ff3a59802dfe0957fba65584551783ece554a03a/pkg/cmd/attestation/verification/sigstore.go#L121The
PolicyBuilder
has checks on the repo owner and/or repo name depending on the user options givenhttps://github.com/cli/cli/blob/ff3a59802dfe0957fba65584551783ece554a03a/pkg/cmd/attestation/verify/policy.go#L31-L59
So far this is all I've been able to verify that they do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the extensions are checked by sigstore-go here
https://github.com/sigstore/sigstore-go/blob/817509999e0de65d795fa099010ea1fbdd7b0828/pkg/verify/certificate_identity.go#L133
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that's correct, so this should be sufficient assuming you setup the policy to check those extension values. This would ignore the predicate values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the attestations API does check them and will refuse to persist the attestation in the store if the values don't match. So it's possible the
gh
CLI is relying on this check.I get this error if I use my fork and generate a predicate with a different workflow ref.
https://github.com/ianlewis/gha-artifact-attestations-test/actions/runs/9170942023/job/25214201468
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if there's ever a bug that allows writing to the attestation store, it'd allow an attacker to put anything in the intoto payload. They should verify in the client too, otherwise what you're really verifying client-side is the equivalent of a VSA (ie a pre-verified attestation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can test their verification code with the attestation store mocked out to verify my assumptions.