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

editorial: Clarify language in VSA spec #882

Closed
wants to merge 29 commits into from
Closed

editorial: Clarify language in VSA spec #882

Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8b0b25e
Update VSA for SLSA v1.1
kpk47 Jun 9, 2023
1233829
Address review comments
kpk47 Jun 23, 2023
68adac3
clarify that you can set verifiedLevels without setting policy
kpk47 Jun 26, 2023
956dc5a
Clarify that verifiedLevels can contain custom values.
kpk47 Jun 28, 2023
b0a877b
reworked How to Verify section
kpk47 Jun 28, 2023
60396db
fix formatting
kpk47 Jun 29, 2023
a71652a
fix formatting
kpk47 Jun 29, 2023
0d386cf
review comments
kpk47 Jun 29, 2023
f841bcf
Apply suggestions from code review
kpk47 Jun 30, 2023
46dcb8f
resolve issues with verifiedLevels
kpk47 Jun 30, 2023
abe634c
Clarify wording around roles; update introduction
kpk47 Jul 5, 2023
b1ef965
Merge branch 'main' of https://github.com/kpk47/slsa into vsa
kpk47 Jul 5, 2023
7800f4b
Apply suggestions from code review
kpk47 Jul 11, 2023
ee07742
impl: update README to explain how to use Netlify (#898)
MarkLodato Jul 6, 2023
359c50a
impl: add missing links in relatedwork doc (#912)
joshuagl Jul 10, 2023
79fdfe1
impl: update renovate config for conventional commits (#911)
joshuagl Jul 10, 2023
2f22cf3
impl: Update github-actions (#909)
renovate-bot Jul 11, 2023
4cde255
impl: Remove dashes from types in PR name lint (#903)
arewm Jul 11, 2023
c0e01a1
nonspec: community meeting is no longer biweekly (#906)
MarkLodato Jul 11, 2023
d0ef5f4
grammar/clarity edits
kpk47 Jul 11, 2023
f2e1d11
Added changelog for this PR
kpk47 Jul 11, 2023
614a115
Merge branch 'main' of https://github.com/kpk47/slsa into vsa
kpk47 Jul 11, 2023
a219938
lint
kpk47 Jul 11, 2023
3c8ee37
Merge branch 'main' of https://github.com/slsa-framework/slsa into vsa
kpk47 Sep 5, 2023
1febf58
move changes to v1.1 directory
kpk47 Sep 5, 2023
1fa3fa9
backport editorial changes to v1.0
kpk47 Sep 6, 2023
e214c2a
move ediorial changes to v1.1 directory
kpk47 Sep 6, 2023
03fe050
fix link refs
kpk47 Sep 6, 2023
ead4e1c
fix link caps
kpk47 Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions docs/verification_summary/v1.md
View file
Open in desktop
Copy link
Member

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?

  • title: summarize the change rather than just "update for v1.1"
  • desc: Could you go into more detail? "Update the introduction" doesn't really explain how the intro was changed or why, and similarly for why the fields are now optional or why the section on verifying VSAs is warranted.

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

Copy link
Contributor Author

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?

Copy link
Member

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:

  • Clarify that VSA can be used to express verification of things other than the SLSA Level. Previously this was technically possible, but it was only mentioned in one sentence (under SlsaResult). Now the purpose, model, and field documentation make this explicit.
  • Make fields policy and timeVerified optional because [...]
  • Add new "How to verify" section, including examples. Previously it was unclear to many readers exactly how VSAs were intended to be used, leading to possible mistakes. Now we make this explicit. This also brings the VSA spec up to parity with Provenance, which already has such a section.
  • Add a SLSA_BUILD_LEVEL_UNEVALUATED entry because [...]
  • Explain why resourceUri is required.
  • Other edits for clarity.

Hmm, this PR is getting a bit big. Might make sense to split...

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

  • Do we expect VSAs that communicate strictly non-SLSA properties (e.g. an attestation for a property explicitly out of SLSA's scope)? If so, we should try to move the VSA spec out of slsa-framework.
  • Does SLSA expect to support any attestations that would be alternatives to VSA? If so, then we should update the spec with a more general description of a summary attestation and its minimum requirements to convey SLSA provenance faithfully.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,13 @@ Understanding of SLSA [Software Attestations](/attestation-model),

A Verification Summary Attestation (VSA) is an attestation that some entity
(`verifier`) verified one or more software artifacts (the `subject` of an
in-toto attestation [Statement]) by evaluating the artifact and a `bundle`
of attestations against some `policy`. Users who trust the `verifier` may
assume that the artifacts met the indicated SLSA level without themselves
needing to evaluate the artifact or to have access to the attestations the
`verifier` used to make its determination.
in-toto attestation [Statement]) by evaluating the artifact and an
attestation against some `policy`. Specifically, a VSA is a claim by
`verifier` that `subject` passed the policy for `resourceUri`. Users who trust
the `verifier` may assume that the artifacts identified by the
`(subject, resourceUri)` pair met the indicated SLSA level without
themselves needing to evaluate the artifact or to have access to the
attestations the `verifier` used to make its determination.

The VSA also allows consumers to determine the verified levels of
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section is saying: if the transitive dependencies have VSAs themselves, then this VSA summarizes them also. And if there's provenance but no VSA for a dependency, this top-level VSA will summarize the provenance, essentially creating transitive VSAs within a top-level VSA. And if there's no provenance for dependencies, the VSA does nothing for them.

Is that correct?

Copy link
Contributor

@AdamZWu AdamZWu Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The basic idea is that, the dependencyLevels in a VSA summarizes the policy verification results for all transitive dependencies.

The policy verification result of a dependency could either come from an "actual" verification, i.e. evaluation the provenance of the dependency against a policy for the dependency Uri; or from a VSA that summarizes a prior policy verification on this dependency.

Effectively, VSA functions as a verification "shortcut" -- if a matching and valid VSA is found, the verifier no longer needs to evaluate the artifact provenance and its entire dependency sub-tree.

If a dependency has no VSA nor provenance, what happens really depends on the policy for its Uri.

  • If the policy says "evaluate at L0" (effectively bypasses the provenance check), then the recursive verification will pass at L0;
  • Otherwise, since a provenance is required but missing, the recursive verification will fail.

The dependencyLevels in the dependent's VSA will record the recursive verification results accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting, thanks! @kpk47 would you be open to including some of this language? This explanation makes sense to me, but I'm not sure I'd have gleaned that from the current paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a go, but went slightly higher level and removed the decision points. @olivekl @AdamZWu does my summary make sense to you and match your understanding of the chained verification process?

all of an artifact’s _transitive_ dependencies. The verifier does this by
Expand Down Expand Up @@ -122,7 +124,7 @@ of the other top-level fields, such as `subject`, see [Statement]._
> can sign provenance for the "Google Cloud Deploy" verifier, but "GitHub" cannot
> sign for the "Google Cloud Deploy" verifier.
>
> The field is required, even if it is implicit from the signer, to aid readability and
> This field is required, even if it is implicit from the signer, to aid readability and
> debugging. It is an object to allow additional fields in the future, in case one
> URI is not sufficient.

Expand All @@ -132,21 +134,25 @@ of the other top-level fields, such as `subject`, see [Statement]._
> URI indicating the verifier’s identity.

<a id="timeVerified"></a>
`timeVerified` _string ([Timestamp]), required_
`timeVerified` _string ([Timestamp]), optional_

> Timestamp indicating what time the verification occurred.

<a id="resourceUri"></a>
`resourceUri` _string ([ResourceURI]), required_

> URI that identifies the resource associated with the artifact being verified.
>
> This field is required to prevent confusion attacks. E.g., a VSA indicating
> that a package can be published as `foo` should not be useable to publish
> the package as `bar`.

<a id="policy"></a>
`policy` _object ([ResourceDescriptor]), required_
`policy` _object ([ResourceDescriptor]), optional_

> Describes the policy that the `subject` was verified against.
>
> The entry MUST contain a `uri`.
> This field is RECOMMENDED.
Copy link
Member

@MarkLodato MarkLodato Jul 13, 2023 •
edited
Loading

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)


<a id="inputAttestations"></a>
`inputAttestations` _array ([ResourceDescriptor]), optional_
Expand All @@ -167,7 +173,7 @@ of the other top-level fields, such as `subject`, see [Statement]._
> Either “PASSED” or “FAILED” to indicate if the artifact passed or failed the policy verification.

<a id="verifiedLevels"></a>
`verifiedLevels` _array ([SlsaResult]), required_
`verifiedLevels` _array ([SlsaResult]), optional_

> Indicates the highest level of each track verified for the artifact (and not
> its dependencies), or "FAILED" if policy verification failed.
Expand Down Expand Up @@ -240,6 +246,24 @@ WARNING: This is just for demonstration purposes.

<div id="slsaresult">
Copy link
Contributor

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'?

Copy link
Member

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.

Copy link
Contributor

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


## How to Verify

Verification SHOULD include the following steps

1. Verify the signature on the VSA envelope using the preconfigured roots of trust.
2. Verify the statemet's `subject` matches the digest of the artifact in question.
3. Verify that the `predicateType` is `https://slsa.dev/verification_summary/v1`.
4. Verify the `verifier` matches the public key (or equivalent) used to verify the signature in step 1.
5. Verify that the value for `resourceUri` in the VSA matches the expected value.

Resulting threat mitigation: See
[Verifying artifacts](/spec/v1.0/verifying-artifacts) for details about which
threats are addressed by verifying each SLSA level.

IMPORTANT: A VSA does not protect against compromise of the verifier, such as by
a malicious insider. Instead, VSA users SHOULD carefully consider which
verifiers they add to their roots of trust.

## _SlsaResult (String)_
Copy link
Member

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?


</div>
Expand Down