-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: vsa support #777
Merged
ramonpetgrave64
merged 68 commits into
slsa-framework:main
from
ramonpetgrave64:ramonpetgrave64-vsa
Jul 11, 2024
Merged
feat: vsa support #777
Changes from all commits
Commits
Show all changes
68 commits
Select commit
Hold shift + click to select a range
7980fde
Changed success message to a more general "PASSED: SLSA verification …
ramonpetgrave64 b5eb147
skeletion verify-vsa command
ramonpetgrave64 a25abe2
testdata, sample invocation in README.md
ramonpetgrave64 b90ede0
rename to TrustedProducerID, allow muyltiple --subject-digest flags
ramonpetgrave64 a3a573a
cleanup
ramonpetgrave64 9704c97
parse dsse envelope
ramonpetgrave64 2f76f12
different test example
ramonpetgrave64 2dc64f7
vsa parser
ramonpetgrave64 1f123f3
attempt to verify envelope
ramonpetgrave64 edde0a8
cleanup, more skeleton
ramonpetgrave64 ead4e9b
use utility to parse envelope, docs, use keyID
ramonpetgrave64 13a74b5
embed the google vsa key, match against all signatures, match the sub…
ramonpetgrave64 610ef6f
verify reamining fields, print attestations
ramonpetgrave64 944c9a6
singular print-attestation
ramonpetgrave64 2ef9a40
minify test data
ramonpetgrave64 f0fedec
verify vsa passed message
ramonpetgrave64 ad1b81d
update README
ramonpetgrave64 f5362e5
rename to PublicKeyHashAlgo
ramonpetgrave64 5636d0a
rename to resource URI
ramonpetgrave64 fec61b1
use pointers
ramonpetgrave64 8befbc6
use plain bool
ramonpetgrave64 7fb5bf9
switch wanted, got order
ramonpetgrave64 fbe83fb
change error type
ramonpetgrave64 00fed87
typo
ramonpetgrave64 e47312f
literl hash algo
ramonpetgrave64 cba639f
specific errors and test cases
ramonpetgrave64 ff1cf43
undo regression tag change
ramonpetgrave64 942d8bb
remove accidental binary
ramonpetgrave64 73c9884
lint: no pointer for crypto.publickkey
ramonpetgrave64 0172a12
lint
ramonpetgrave64 e27f99f
no need for sigstoreEnvelope
ramonpetgrave64 968a34d
typo
ramonpetgrave64 519a928
clarify comments
ramonpetgrave64 b9c6de5
flag descriptions, optional --verified-levels
ramonpetgrave64 f3b63b7
reword simple hash
ramonpetgrave64 e0919a8
hash-algo description
ramonpetgrave64 23d8e33
singular attestation path
ramonpetgrave64 bf38fb0
help docs
ramonpetgrave64 1ccec0e
comment doc
ramonpetgrave64 92ce34e
fix capitalization
ramonpetgrave64 f9a4b35
cli help about default options
ramonpetgrave64 9b2554e
cli about print-attestation
ramonpetgrave64 e452493
fix cap
ramonpetgrave64 7813046
remove experimental
ramonpetgrave64 721eee5
singular attestation
ramonpetgrave64 719e118
typo
ramonpetgrave64 311b211
func doc comment
ramonpetgrave64 e8ed9cc
algo help
ramonpetgrave64 3d6e498
caps
ramonpetgrave64 9560d1a
rename
ramonpetgrave64 21f5c3a
lint
ramonpetgrave64 80b4cec
typo
ramonpetgrave64 368e43c
readme: caveats
ramonpetgrave64 d33cbc3
dont use TrustedAttestorID
ramonpetgrave64 a71e44a
rename to public-key-signing-hash-algo
ramonpetgrave64 6aef931
cleanup
ramonpetgrave64 8cf01ea
capitalization
ramonpetgrave64 15e9019
typo
ramonpetgrave64 9cd9553
go.mod conflicts
ramonpetgrave64 891ffff
Merge branch 'main' into ramonpetgrave64-vsa
ramonpetgrave64 b145f36
remove --public-key-hash-algo, make verified-levels an array
ramonpetgrave64 701d13a
Merge branch 'main' into ramonpetgrave64-vsa
ramonpetgrave64 59deb51
fix readme
ramonpetgrave64 4cf2972
typo
ramonpetgrave64 475962b
more keyid tests
ramonpetgrave64 a1068c4
add slsa level inference
ramonpetgrave64 9ad9097
revise link to how-to-verify
ramonpetgrave64 9f25bde
Revert "revise link to how-to-verify"
ramonpetgrave64 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Do we want to warn that this is a dangerous / advanced option that really nobody should be using. In the future we'll have
verify-artifact --vsa-path
that will verify the VSA for an artifact (using digest's hash). I don't know how to best discourage usage of this command. Maybe: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.
Why do you think this is dangerous? If you get to the point where you have a VSA you need to verify, I assume you should understand what the VSA represents. So maybe adding a link to https://slsa.dev/spec/v1.1/verification_summary#purpose to make sure users understand one?
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.
@laurentsimon Instead of
verify-artifact --vsa-path
, I was thinking to continue withverify-vsa path1 path2 ...
, orverify-vsa --artifact-path path1 --artifact-path path2 ...
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 don't think re-using
verify-vsa
is consistent with the existing API. We currently useverify-artifact
andverify-image
, so I think it's the right way to build on existing APIs. You can verify an artifact using provenance, or a VSA, or something else.verify-vsa
is a low-level API for the rare use case that the artifact is not available, and should not be used / advertised otherwise.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.
re: why it's dangerous API. TOCTOU is an example of a vuln https://github.com/slsa-framework/slsa-verifier?tab=readme-ov-file#containers-1. We've been very careful to design the CLI in a way that prevent users from shooting themselves in the foot (well, I suppose they still can :)). Leaving it up to users to calculate a sha is both poor UX (running the CLI now requires computing a hash) and prone to mistakes (not everyone can be an expert)
In general I think of
verify-vsa
andverify-provenance
(which does not exist yet) as low-level APIs that verify signature and match content passed by the caller. The safer APIs users should use are theverify-artifact
,verify-image
. Wdut?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.
OK, I see what you're saying. This seems more like a not ideal UX rather than dangerous, since we are asking for a digest, and the digest should be an immutable reference - if it's not, then it's technically non-compliant with SLSA. If we were to add a comment, I might say that we'd like to refactor this at some point to move VSA verification under
verify-artifact
.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 think of the CLI options more like
verify-artifact
actually means "verify-provenance-with-artifact", or "verify-artifact-provenance"With this thinking, a VSA is not actually a provenance (build provenance). And I'm open to making a new command "verify-artifact-vsa". To avoid the danger of the user not realizing that they're not enjoying the benefits of an actual build provenance.
re: TOCU, you're referring to how
verify-image
relies on the user using external tools to calculate digests (docs incorrectly says "tarball")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.
That was not the original intention, and that's why the term 'provenance' is not in the command's name. It's intended to verify artifacts with whatever attestation. It so happens the only one currently supported is provenance, but it need not be. If you ever need to verify an artifact with several attestations, you'll be able to do
verify-artifact --provenance-path --source-path etc
... unless they are packed into a bundle and you'll likely use--attestations-path
like in npm command.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.
just have to be careful that users don't use the non-immutable image for pulling