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.

Sign up for GitHub

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

Jump to bottom

feat: Verify all artifacts passed in cmdline #419

Merged
merged 18 commits into from
Dec 29, 2022
Merged

feat: Verify all artifacts passed in cmdline #419

merged 18 commits into from
Dec 29, 2022

Conversation

mihaimaruseac
Copy link
Contributor

This should close #91.

Turns out that even if we pass an artifact that does not match provenance (same name, invalid hash) or an artifact that is not covered by the provenance (not in subject), the verifier reports the same error: hash not matching provenance. See below:

[...]$ go run ./cli/slsa-verifier/{main,verify}.go verify-artifact --provenance-path /tmp/demo/multiple.intoto.jsonl --source-uri github.com/mihaimaruseac/slsa-lvl3-generic-provenance-with-bazel-example /tmp/demo/*
Verifying artifact /tmp/demo/fib
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verified build using builder https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@refs/tags/v1.4.0 at commit 11fab87c5ee6f46c6f5e68f6c5378c62ce1ca77c
Verifying artifact /tmp/demo/fib: PASSED

Verifying artifact /tmp/demo/hello
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verified build using builder https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@refs/tags/v1.4.0 at commit 11fab87c5ee6f46c6f5e68f6c5378c62ce1ca77c
Verifying artifact /tmp/demo/hello: PASSED

Verifying artifact /tmp/demo/multiple.intoto.jsonl
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verifying artifact /tmp/demo/multiple.intoto.jsonl: FAILED: expected hash 'f846eb812add7699a757e3a91df0a1c3a7b083fb2d907ae4c00e1e186f8641fe' not found: artifact hash does not match provenance subject

FAILED: SLSA verification failed: expected hash 'f846eb812add7699a757e3a91df0a1c3a7b083fb2d907ae4c00e1e186f8641fe' not found: artifact hash does not match provenance subject
Usage:
...

In this case, I used the artifacts from the test repo and passed the provenance as an additional artifact to verify. Observe how verification fails for this additional "artifact".

If I only pass fib and hello, all passes:

[...]$ go run ./cli/slsa-verifier/{main,verify}.go verify-artifact --provenance-path /tmp/demo/multiple.intoto.jsonl --source-uri github.com/mihaimaruseac/slsa-lvl3-generic-provenance-with-bazel-example /tmp/demo/{fib,hello}
Verifying artifact /tmp/demo/fib
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verified build using builder https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@refs/tags/v1.4.0 at commit 11fab87c5ee6f46c6f5e68f6c5378c62ce1ca77c
Verifying artifact /tmp/demo/fib: PASSED

Verifying artifact /tmp/demo/hello
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verified build using builder https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@refs/tags/v1.4.0 at commit 11fab87c5ee6f46c6f5e68f6c5378c62ce1ca77c
Verifying artifact /tmp/demo/hello: PASSED

PASSED: Verified SLSA provenance

Verification still passes if passing one single blob for a correct artifact:

[...]$ go run ./cli/slsa-verifier/{main,verify}.go verify-artifact --provenance-path /tmp/demo/multiple.intoto.jsonl --source-uri github.com/mihaimaruseac/slsa-lvl3-generic-provenance-with-bazel-example /tmp/demo/fib
Verifying artifact /tmp/demo/fib
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verified build using builder https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@refs/tags/v1.4.0 at commit 11fab87c5ee6f46c6f5e68f6c5378c62ce1ca77c
Verifying artifact /tmp/demo/fib: PASSED

PASSED: Verified SLSA provenance

And it fails if the artifact cannot be verified:

[...]$ cp /bin/ls /tmp/demo/fib
[...]$ go run ./cli/slsa-verifier/{main,verify}.go verify-artifact --provenance-path /tmp/demo/multiple.intoto.jsonl --source-uri github.com/mihaimaruseac/slsa-lvl3-generic-provenance-with-bazel-example /tmp/demo/fib
Verifying artifact /tmp/demo/fib
Verified signature against tlog entry index 9712459 at URL: https://rekor.sigstore.dev/api/v1/log/entries/24296fb24b8ad77a1544828b67bb5a2335f7e0d01c504a32ceb6f3a8814ed12c8f1b222d308bd9e8
Verified build using builder https://github.com/slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@refs/tags/v1.4.0 at commit 11fab87c5ee6f46c6f5e68f6c5378c62ce1ca77c
Verifying artifact /tmp/bbss/fib: FAILED: expected hash 'd9408a51bd2846813db391cd58a4f5d2dba116fdd1be31968652cd8ca282ed6f' not found: artifact hash does not match provenance subject

FAILED: SLSA verification failed: expected hash 'd9408a51bd2846813db391cd58a4f5d2dba116fdd1be31968652cd8ca282ed6f' not found: artifact hash does not match provenance subject
Usage:
...

I think maybe we should not print the usage if validation fails. Also, should we do the same for image verification or allow only one argument there?

At the moment, validation fails on the first error. If needed, I'll send a different PR that tries to do as much work as possible and report all errors at the end.

Signed-off-by: Mihai Maruseac mihaimaruseac@google.com

@mihaimaruseac mihaimaruseac mentioned this pull request Dec 28, 2022
Closed
@mihaimaruseac mihaimaruseac changed the title Verify all artifacts passed in cmdline feat: Verify all artifacts passed in cmdline Dec 28, 2022
@laurentsimon
Copy link
Contributor

laurentsimon commented Dec 28, 2022 •
edited
Loading

+1 on not showing the Usage when it fails. I think there's a tracking issue for it. tracks it, IIUC

Turns out that even if we pass an artifact that does not match provenance (same name, invalid hash) or an artifact that is not covered by the provenance (not in subject), the verifier reports the same error: hash not matching provenance

We don't check the subject name, so this seems correct. We only check for a matching hash.

I am a little worried that printing PASSED: Verified SLSA provenance may lead users to conclude that it passed, even if there's another error. But so long as the return error is not 0, that's probably OK (in particular when used in a script)... wdut?

Gathering all errors in a proper "log" structure is something that would be useful #161. It would allow us to also report everything in a JSON format (if users want to parse the results or if it's the response to a REST API).

@mihaimaruseac
Copy link
Contributor Author

We only print PASSED: Verified SLSA provenance if all artifacts passed in the command line have been verified. In all other cases, the last lines are the FAILED:... line and the usage.

laurentsimon
laurentsimon approved these changes Dec 28, 2022
View reviewed changes
Copy link
Contributor

@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. Would you be able to add some tests in main_test.go?

(We can add some e2e in the slsa-framework/example-package repo later)

@mihaimaruseac
Copy link
Contributor Author

Let me look into it

laurentsimon
laurentsimon reviewed Dec 28, 2022
View reviewed changes
Copy link
Contributor

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

Could you also update the documentation in the README.md?

@laurentsimon laurentsimon self-requested a review December 28, 2022 19:58
@laurentsimon
Copy link
Contributor

laurentsimon commented Dec 28, 2022 •
edited
Loading

We only print PASSED: Verified SLSA provenance if all artifacts passed in the command line have been verified. In all other cases, the last lines are the FAILED:... line and the usage.

Awesome, that sounds great! Maybe we can point that out in the README.md too.

asraa
asraa reviewed Dec 28, 2022
View reviewed changes
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

LGTM! Seems like just a few lint/build failures

cli/slsa-verifier/verify/verify_artifact.go Outdated Show resolved Hide resolved
@mihaimaruseac
Remove limit of number of artifacts to verify
0b14fd7 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Update short description
9ea0651 
Mention that we support multiple artifacts as long as they come from the
same provenance.

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Verify all artifacts passed in cmdline
ec54e9f 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Remove header of verify step output
17ace04 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Format the cobra.Command.Use argument
756f312 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Don't return a slice of builderIDs.
f0896d8 
Since all images should come from the same provenance file, there's an
invariant that we will always get exactly one builderID. So, no need to
return a slice of them.

Just to preempt the case when the invariant would be broken, add a
specific check.

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Change existing tests to support multiple artifacts.
962a9b6 
No new tests added, just changing table test data type.

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
[nfc] Format test file
1865ffa 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Change test to allow passing multiple artifacts.
00e571d 
For now, just allow passing the entire array of artifacts to command
line / arguments. The functionality should still be the same.

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Fix typo
7fb8c3e 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Fix path to test artifacts
dabd517 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Allow different provenance path
8f9a6d1 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Try 2 artifacts from existing testdata
a0c552a 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Add more tests for multiple artifacts
393f64b 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Add failing test
7543a87 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Fix artifact and error
7b8b682 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Add final test: no artifact match
abb7444 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Copy link
Contributor Author

Ok, got it. The issue was that there are a lot of occurrences of FAIL in the log, so if a test fail it gets confusing to see what's the culprit.

Updating the README.md and it should all be done in the next 5-10 mins

@mihaimaruseac
Update README.md
0b5bec1 
Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac
Copy link
Contributor Author

All ready for a new review, PTAL and sorry it took so long to fix

asraa
asraa approved these changes Dec 29, 2022
View reviewed changes
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks for the README!

cli/slsa-verifier/verify/verify_artifact.go
provenanceOpts := &options.ProvenanceOpts{
ExpectedSourceURI: c.SourceURI,
ExpectedBranch: c.SourceBranch,
ExpectedDigest: artifactHash,
Copy link
Contributor

Choose a reason for hiding this comment

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

apologies for only seeing this now!

likely it probably makes the most sense to migate ExpectedDigest to ExpectedDigests so that we don't have to run signature verification more than once. I realize that's a breaking change -- maybe we should file a cleanup issue before the next major release?

doing it in a backwards compatible way with a new field would i guess be possible, and we could check for both ExpectedDigest/ExpectedDigests. It's not urgent to me though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Created #422 to track

Copy link
Contributor

Choose a reason for hiding this comment

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

is it breaking for CLI or just the API? If just for the API, we could add a section to the README saying that we respect semver for the CLI only at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it would be just for the API -- the CLI would still have the versioning.

Interesting about CLI semver -- in the past I've experienced only API being the versioning importance, but actually here CLI may be the only thing. No one is using as a library yet..

Copy link
Contributor

Choose a reason for hiding this comment

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

I like what OSV does: the call out exactly what follows semver https://github.com/google/osv-scanner#semver-adherence. Created slsa-framework/slsa-github-generator#1427 for tracking.

@mihaimaruseac mihaimaruseac mentioned this pull request Dec 29, 2022
Open
@laurentsimon
Copy link
Contributor

@mihaimaruseac in a follow-up PR, we also need to update the REST service. See https://github.com/slsa-framework/slsa-verifier/blob/main/experimental/rest/service.go#L21 used by https://github.com/slsa-framework/slsa-verifier/blob/main/cli/experimental/service/main.go

@laurentsimon
Copy link
Contributor

Ready to merge?

@mihaimaruseac
Copy link
Contributor Author

Ready to merge on my side. I'll look into upgrading the REST service and the e2e test in the example repo as well as #422 (shouldn't break anything, afaik, but I'll check to be sure)

@laurentsimon
Copy link
Contributor

Great, thanks @mihaimaruseac Maybe also we need to update the verify-image to do something similar. Can you create a tracking issue for it too?

@laurentsimon laurentsimon merged commit e20f3cc into slsa-framework:main Dec 29, 2022
@mihaimaruseac mihaimaruseac deleted the multiple-artifacts branch December 29, 2022 17:57
@mihaimaruseac mihaimaruseac mentioned this pull request Dec 29, 2022
Open
@mihaimaruseac
Copy link
Contributor Author

MAde #423 to track that. Thanks for review and merging!

@mihaimaruseac
Copy link
Contributor Author

@mihaimaruseac in a follow-up PR, we also need to update the REST service. See https://github.com/slsa-framework/slsa-verifier/blob/main/experimental/rest/service.go#L21 used by https://github.com/slsa-framework/slsa-verifier/blob/main/cli/experimental/service/main.go

So I look at these and this is just the verification, it will need to be updated only after #422, when we migrate to do a single verification for multiple artifacts. I could be mistaken though?

@laurentsimon
Copy link
Contributor

+1, you're correct. Just wanted to be sure we have a tracking issue for it

@mihaimaruseac
Copy link
Contributor Author

Added a mention of this in #422

@mihaimaruseac mihaimaruseac mentioned this pull request Dec 29, 2022
Closed
mihaimaruseac added a commit to mihaimaruseac/slsa-verifier that referenced this pull request Dec 29, 2022
@mihaimaruseac
fix: Expect at least one artifact in verification
8d3f6c6 
Fixes slsa-framework#425 introduces by slsa-framework#419

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
@mihaimaruseac mihaimaruseac mentioned this pull request Dec 29, 2022
Merged
laurentsimon pushed a commit that referenced this pull request Dec 29, 2022
@mihaimaruseac
fix: Expect at least one artifact in verification (#426)
17463c0 
* fix: Expect at least one artifact in verification

Fixes #425 introduces by #419

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>

* go fmt

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>

Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asraa asraa asraa approved these changes

@laurentsimon laurentsimon laurentsimon approved these changes

@ianlewis ianlewis Awaiting requested review from ianlewis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiple artifact
3 participants
@mihaimaruseac @laurentsimon @asraa