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
editorial: Clarify language in VSA spec #882
editorial: Clarify language in VSA spec #882
Changes from 23 commits
8b0b25e
1233829
68adac3
956dc5a
b0a877b
60396db
a71652a
0d386cf
f841bcf
46dcb8f
abe634c
b1ef965
7800f4b
ee07742
359c50a
79fdfe1
2f22cf3
4cde255
c0e01a1
d0ef5f4
f2e1d11
614a115
a219938
3c8ee37
1febf58
1fa3fa9
e214c2a
03fe050
ead4e1c
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.
Could you update the PR title and description to be more, er, descriptive?
This is also a breaking change (change to required field) so we should use the
BREAKING CHANGE:
footer as per https://www.conventionalcommits.org/en/v1.0.0/.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.
Done.
There's a lot of content in this PR. Do you think the summary does it justice?
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.
Personally I would go into more detail, particularly to explain the reason for the change. Something like:
policy
andtimeVerified
optional because [...]SLSA_BUILD_LEVEL_UNEVALUATED
entry because [...]resourceUri
is required.Hmm, this PR is getting a bit big. Might make sense to split...
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.
+1 to splitting the PR. I'll keep "how to verify" in this one and move the rest to new ones.
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'm worried that this new version is in a weird limbo where it's half SLSA-specific and half generic. For example, it's not clear if you need to list a SLSA build level even if you're saying nothing about the build level.
Would it make sense to make it fully generic and perhaps move it to the in-toto repo? Moving it to in-toto would have the side benefit of avoiding a SLSA version number bump.
Note that provenance would still remain within the SLSA repo because it's inextricably tied to the SLSA build model and SLSA build levels. VSA, on the other hand, seems actually quite generic.
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've been wondering the same thing. I don't have an answer, but I've got a couple of questions/thoughts that may help shape the decision.
If we do keep the VSA spec under slsa-framework, then the community needs to articulate a clearer vision of its role in the SLSA ecosystem. If nothing else, we should update the "How-to SLSA" pages with more VSA examples.
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.
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.
Throughout the doc, "verifier" is ambiguous. Is it the entity producing the VSA (verifying some other artifact) or the entity checking the VSA (verifying the VSA itself). We need more precise terminology. A simple diagram might also help to give a quick visual.
Maybe "VSA Producer" and "VSA Consumer"? (This was in an earlier comment by @laurentsimon. Raising visibility here.)
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.
We seem to have lost this level of detail, e.g. checking
vsa.resourceUri
againstprovenance.resolvedDependencies.uri
. Should this go somewhere as well? Perhaps a new "How to produce" section?Alternatively should this detail go in the SLSA Provenance "How to verify" section, since it's more about provenance?
/cc @TomHennen
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 vote not to mention those here.
The check of
vsa.resourceUri
andprovenance.resolvedDependencies.uri
really belongs to the topic of policy and verification, and the URI matching is an opinionated way to check whether the short-circuit is permitted.In order for that to work, there are some implicit assumptions, namely 1) the policy being evaluated is completely determined by the URIs; 2) there is a 1:1 mapping between the URIs and policies; 3) both
vsa.resourceUri
andprovenance.resolvedDependencies.uri
follow the same specs.We could either spend a lot of energy fully flesh out all these, and in the end we just described one way it can work; Meanwhile, there are other ways short-circuiting could work, for example the
policy.uri
andpolicy.digest
also contain information that may be used for making decision.Alternatively, I think it is better just leave a high-level description here that "the verifier can use VSA to short-circuit policy verification when appropriate" and leave the details in SLSA Provenance "How to verify".
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.
nit: Why is it recommended? (or alternatively just remove the recommendation if we don't have a good reason)
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.
verifier -> consumer?
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.
Line 175 says required but here it says it's optional. Which is it?
If required, what does that mean? Is one of the
SLSA_
values required? When we have more than one track, is it required to have exactly one entry for each track?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.
Should this div go down near line 352? Should there be a different div id for 'how to verify'?
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 actually think SlsaResult should move up here.
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's fine too, as long as they go together. :)
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.
nit: again, it's unclear if this "verify" means produce or consume.
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.
This piece probably needs more fleshing out, perhaps in a separate PR. There have been many questions about what the resource URI is, e.g. #891.
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.
Instead of
slsaResult
this should beverificationResult
, right?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 the confusion results from overlap in
verificationResult
andverifiedLevels
. The latter also has aFAILED
entry, and the name implies that it's only for PASSED. Should we deprecateverificationResult
and just sayverifiedLevels
are only for things that passed?I'm assuming the original intention was to support saying "I checked at L2 and it failed". If so, is that a foot gun?
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
verificationResult != 'PASSED'
thenverifiedLevels
must be empty? If so, SGTMThere 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.
As above,
verificationResult
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 understand this piece. Why does someone check that the build level was unevaluated? If they don't care, shouldn't they just ignore it?
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.
verificationResult
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.
Q: Should this enum name be changed to something not slsa specific?
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.
What is the purpose of this new field? That's not clear to me.