content: add a verifier version field to VSA #905

kpk47 · 2023-07-07T20:10:02Z

Add a verifier version field to the VSA schema.

This change Increases consistency between provenance.builder and vsa.verifier, and it allows VSA producers/consumers to respond to known flaws in verification tools.

fixes #809

Signed-off-by: kpk47 <[email protected]>

netlify · 2023-07-07T20:10:07Z

✅ Deploy Preview for slsa ready!

Name	Link
🔨 Latest commit	`662908d`
🔍 Latest deploy log	https://app.netlify.com/sites/slsa/deploys/6525d58de67cd30008692b90
😎 Deploy Preview	https://deploy-preview-905--slsa.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

MarkLodato

In the PR description, could you explain why this is needed? Issues are often long and meandering, so briefly summarizing the rationale is helpful for reviews and for posterity.

docs/verification_summary/v1.md

Co-authored-by: Mark Lodato <[email protected]> Signed-off-by: kpk47 <[email protected]>

Signed-off-by: kpk47 <[email protected]>

lehors · 2023-07-10T12:17:07Z

Given that this is requiring new info shouldn't that trigger some version change of the format?

laurentsimon · 2023-07-10T19:15:38Z

docs/verification_summary/v1.md

@@ -213,7 +222,11 @@ WARNING: This is just for demonstration purposes.
 "predicateType": "https://slsa.dev/verification_summary/v1",
 "predicate": {
  "verifier": {
-    "id": "https://example.com/publication_verifier"
+    "id": "https://example.com/publication_verifier",
+    "version": {


I still feel that using resource descriptor is more flexible in general, especially if we need to update this structure in the future. In this example, slsa-verifier-linux-amd64 is kinda arbitrary naming. If our goal is to help with vuln management, using more conventional naming, like purl, etc may be beneficial. Resource descriptors offer this flexibility.

I don't think that ResourceDescriptor is the right type for tracking versions since it doesn't appear to have a version field. Also, part of the motivation for this PR is making the verifier VSA more consistent with the builder provenance field. builder uses map(string->string), so I think it's best to keep that type here.

I'm going to keep map(string->string) in this PR, and we can address whether or not to change both fields to ResourceDescriptor separately.

MarkLodato · 2023-07-11T20:03:39Z

Given that this is requiring new info shouldn't that trigger some version change of the format?

There are no required fields. (An earlier version said required, but it was a mistake.)

So this doesn't require a major version bump, but based on this week's spec meeting, all content changes should have a changelog entry and perhaps a minor version bump.

lehors · 2023-07-11T21:13:40Z

Given that this is requiring new info shouldn't that trigger some version change of the format?

There are no required fields. (An earlier version said required, but it was a mistake.)

Why does line # 68 read: "// Required" ?
Shouldn't this comment be deleted then?

MarkLodato · 2023-07-11T21:17:08Z

Why does line # 68 read: "// Required" ? Shouldn't this comment be deleted then?

The verifier (and verifier.id) was and still is required, but the new verifier.version subfield is not. Thus the following was and still is acceptable:

"verifier": { "id": "..." }

But point taken - we should clarify in the docs since the comment can clearly be misread.

Signed-off-by: kpk47 <[email protected]>

kpk47 · 2023-07-11T23:10:23Z

Why does line # 68 read: "// Required" ? Shouldn't this comment be deleted then?

The verifier (and verifier.id) was and still is required, but the new verifier.version subfield is not. Thus the following was and still is acceptable:
"verifier": { "id": "..." }
But point taken - we should clarify in the docs since the comment can clearly be misread.

I don't see a good way to clarify in the text since the fields are already clearly labeled required vs optional. I deleted the comment instead.

Signed-off-by: kpk47 <[email protected]>

kpk47 · 2023-07-11T23:15:05Z

Given that this is requiring new info shouldn't that trigger some version change of the format?

There are no required fields. (An earlier version said required, but it was a mistake.)

So this doesn't require a major version bump, but based on this week's spec meeting, all content changes should have a changelog entry and perhaps a minor version bump.

I added a changelog entry with a minor version bump, as required by the versioning rules (https://slsa.dev/spec-stages#versioning).

Given the version bump, we shouldn't merge this PR until we're ready to have v1.1 on the website.

lehors · 2023-07-12T05:12:22Z

I don't see a good way to clarify in the text since the fields are already clearly labeled required vs optional. I deleted the comment instead.

That works. Thanks!

lehors · 2023-07-12T05:14:01Z

I added a changelog entry with a minor version bump, as required by the versioning rules (https://slsa.dev/spec-stages#versioning).

Given the version bump, we shouldn't merge this PR until we're ready to have v1.1 on the website.

Shouldn't we have this on a branch specific to the next version like we did when we were working on 1.0 before publication?

MarkLodato · 2023-07-21T20:11:46Z

Shouldn't we have this on a branch specific to the next version like we did when we were working on 1.0 before publication?

Yes, this is blocked by #742.

MarkLodato · 2023-08-18T12:11:19Z

The v1.1 directory now exists as of #942.

Signed-off-by: kpk47 <[email protected]>

kpk47 · 2023-08-29T18:12:58Z

I moved the changes into the v1.1 directory, so this PR is ready for re-review.

Signed-off-by: kpk47 <[email protected]>

joshuagl

This LGTM, thanks. However, as this is a content change, could you please add a changelog entry (per contributing)?

docs/spec/v1.1/verification_summary.md

Co-authored-by: Joshua Lock <[email protected]> Signed-off-by: kpk47 <[email protected]>

Signed-off-by: kpk47 <[email protected]>

joshuagl

LGTM, thanks!

lehors

LGTM

specContent: Add a verifier version field to VSA.

19d9aa4

Signed-off-by: kpk47 <[email protected]>

kpk47 marked this pull request as ready for review July 7, 2023 20:11

kpk47 requested review from MarkLodato, mlieberman85 and joshuagl as code owners July 7, 2023 20:11

kpk47 mentioned this pull request Jul 7, 2023

VSA: can we be more consistent with SLSA fields and VSA fields #809

Closed

MarkLodato requested changes Jul 7, 2023

View reviewed changes

docs/verification_summary/v1.md Outdated Show resolved Hide resolved

docs/verification_summary/v1.md Outdated Show resolved Hide resolved

docs/verification_summary/v1.md Outdated Show resolved Hide resolved

kpk47 and others added 2 commits July 7, 2023 13:44

Apply suggestions from code review

d55b1a7

Co-authored-by: Mark Lodato <[email protected]> Signed-off-by: kpk47 <[email protected]>

respond to review comments

eefdedc

Signed-off-by: kpk47 <[email protected]>

kpk47 requested a review from MarkLodato July 7, 2023 20:58

joshuagl mentioned this pull request Jul 10, 2023

Workstream: Release SLSA v1.1 #900

Open

7 tasks

laurentsimon reviewed Jul 10, 2023

View reviewed changes

MarkLodato changed the title ~~specContent: Add a verifier version field to VSA.~~ content: add a verifier version field to VSA Jul 11, 2023

MarkLodato approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'main' of https://github.com/kpk47/slsa into vsa2

636308e

Delete ambiguous comment in the VSA example

9265232

Signed-off-by: kpk47 <[email protected]>

Added changelog entry for this PR

d2fec19

Signed-off-by: kpk47 <[email protected]>

kpk47 added 2 commits August 29, 2023 11:02

Merge branch 'main' of https://github.com/slsa-framework/slsa into vsa2

cc3b15e

move changes to v1.1 directory

251e905

Signed-off-by: kpk47 <[email protected]>

kpk47 force-pushed the vsa2 branch from 0c52950 to 251e905 Compare August 29, 2023 18:12

kpk47 requested a review from MarkLodato August 29, 2023 18:13

kpk47 added 2 commits August 29, 2023 11:14

fix merge error

9c890e0

Signed-off-by: kpk47 <[email protected]>

lint

83b0abc

Signed-off-by: kpk47 <[email protected]>

joshuagl reviewed Sep 11, 2023

View reviewed changes

docs/spec/v1.1/verification_summary.md Outdated Show resolved Hide resolved

kpk47 and others added 4 commits October 10, 2023 15:45

Merge branch 'main' of https://github.com/kpk47/slsa into vsa2

5ec8e7c

Update docs/spec/v1.1/verification_summary.md

fd28a38

Co-authored-by: Joshua Lock <[email protected]> Signed-off-by: kpk47 <[email protected]>

Add changelog

27ccf43

Signed-off-by: kpk47 <[email protected]>

Merge branch 'vsa2' of https://github.com/kpk47/slsa into vsa2

662908d

kpk47 requested a review from joshuagl October 10, 2023 22:52

joshuagl approved these changes Oct 11, 2023

View reviewed changes

MarkLodato approved these changes Oct 11, 2023

View reviewed changes

lehors approved these changes Oct 11, 2023

View reviewed changes

joshuagl merged commit 68ce28b into slsa-framework:main Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

content: add a verifier version field to VSA #905

content: add a verifier version field to VSA #905

kpk47 commented Jul 7, 2023 •

edited

Loading

netlify bot commented Jul 7, 2023 •

edited

Loading

MarkLodato left a comment

lehors commented Jul 10, 2023

laurentsimon Jul 10, 2023

kpk47 Jul 11, 2023

MarkLodato commented Jul 11, 2023

lehors commented Jul 11, 2023 •

edited

Loading

MarkLodato commented Jul 11, 2023

kpk47 commented Jul 11, 2023

kpk47 commented Jul 11, 2023

lehors commented Jul 12, 2023

lehors commented Jul 12, 2023

MarkLodato commented Jul 21, 2023

MarkLodato commented Aug 18, 2023

kpk47 commented Aug 29, 2023

joshuagl left a comment

joshuagl left a comment

lehors left a comment

content: add a verifier version field to VSA #905

content: add a verifier version field to VSA #905

Conversation

kpk47 commented Jul 7, 2023 • edited Loading

netlify bot commented Jul 7, 2023 • edited Loading

✅ Deploy Preview for slsa ready!

MarkLodato left a comment

Choose a reason for hiding this comment

lehors commented Jul 10, 2023

laurentsimon Jul 10, 2023

Choose a reason for hiding this comment

kpk47 Jul 11, 2023

Choose a reason for hiding this comment

MarkLodato commented Jul 11, 2023

lehors commented Jul 11, 2023 • edited Loading

MarkLodato commented Jul 11, 2023

kpk47 commented Jul 11, 2023

kpk47 commented Jul 11, 2023

lehors commented Jul 12, 2023

lehors commented Jul 12, 2023

MarkLodato commented Jul 21, 2023

MarkLodato commented Aug 18, 2023

kpk47 commented Aug 29, 2023

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl left a comment

Choose a reason for hiding this comment

lehors left a comment

Choose a reason for hiding this comment

kpk47 commented Jul 7, 2023 •

edited

Loading

netlify bot commented Jul 7, 2023 •

edited

Loading

lehors commented Jul 11, 2023 •

edited

Loading