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

Add GitHub Artifact Attestations post #56

Merged

Conversation

ianlewis
Copy link
Owner

@ianlewis ianlewis commented May 20, 2024

Blog post on GitHub Artifact Attestations for personal blog.

Copy link

netlify bot commented May 20, 2024

Deploy Preview for www-ianlewis-org ready!

Name Link
🔨 Latest commit 85fea8e
🔍 Latest deploy log https://app.netlify.com/sites/www-ianlewis-org/deploys/6657dbdeed04cf0008700902
😎 Deploy Preview https://deploy-preview-56--www-ianlewis-org.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ianlewis
Copy link
Owner Author

provider are compromised.

So while the SLSA provenance itself might be modified the certificate OID
claims cannot. So GitHub can check the OID claims against the expected values

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?

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

Copy link
Owner Author

@ianlewis ianlewis May 20, 2024

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).

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 :)

Comment on lines 198 to 200
to have. GitHub ensures that the JSON printed by the `gh` client’s `--format
json` option is trustworthy simply by omitting any values from the SLSA
provenance JSON that are not included in the certificate’s OID claims. This
Copy link
Owner Author

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.

Choose a reason for hiding this comment

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

Copy link
Owner Author

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#L121

The PolicyBuilder has checks on the repo owner and/or repo name depending on the user options given
https://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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

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.

Error: Error: Failed to persist attestation: Invalid Argument - values do not match: .github/workflows/artifact-attestations.basic.yml != .github/workflows/artifact-attestations.malicious.yml - https://docs.github.com/rest/repos/repos#create-an-attestation

https://github.com/ianlewis/gha-artifact-attestations-test/actions/runs/9170942023/job/25214201468

Copy link

@laurentsimon laurentsimon May 21, 2024

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).

Copy link
Owner Author

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.

Copy link

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

LGTM. Left a few comments

provider are compromised.

So while the SLSA provenance itself might be modified the certificate OID
claims cannot. So GitHub can check the OID claims against the expected values

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 :)

ianlewis added 13 commits May 22, 2024 08:47
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
ianlewis added 4 commits May 30, 2024 01:34
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
Signed-off-by: Ian Lewis <[email protected]>
@ianlewis ianlewis merged commit 5d8905b into main May 30, 2024
8 checks passed
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.

4 participants