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

Provenance v1: allow arbitrary JSON for params, refine github-actions-workflow #582

Merged
merged 11 commits into from
Jan 31, 2023
Merged

Provenance v1: allow arbitrary JSON for params, refine github-actions-workflow #582

merged 11 commits into from
Jan 31, 2023

Conversation

MarkLodato
Copy link
Member

@MarkLodato MarkLodato commented Jan 25, 2023 •
edited
Loading

This PR refines the provenance v1 schema and the accompanying github-actions-workflow buildType, based on feedback.

Allow arbitrary JSON for params

Generalize externalParameters and systemParameters to allow arbitrary JSON objects rather than only specific types. This makes it easier to represent real-world systems. It also avoids the needs for the extra layer of artifactRef, scalarValue, etc., which was awkward.

This necessitated the removal of the "artifact reference" type of parameter, which combined the actual parameter (usually represented as a URI) with the resolved digest of the artifact that the parameter pointed to. Now the resolved digest is moved to resolvedDependencies (with the URI repeated there to allow joining with the parameters.) The impetus for the removal was because there was no good way to represent the concept of an artifact reference parameter, but upon further reflection, the removal actually seems to simplify things.

Refine github-actions-workflow

Limit to specific event types for which we have fully analyzed the semantics, and add new external parameters for deployment and release events.

Replace the source and workflow fields with a tuple of repository, ref, and path. This is hopefully easier for everyone to understand and use correctly. It comes at the cost of making it harder to join with resolvedDependencies, but this seems like a worthwhile tradeoff.

Expand the documentation to reduce ambiguity.

Other changes

Expand the guidance for BuildDefinition.

Preview links

https://deploy-preview-582--slsa.netlify.app/provenance/v1/
https://deploy-preview-582--slsa.netlify.app/github-actions-workflow/v0.1/

/cc @feelepxyz @ianlewis @tiziano88 @mlieberman85 @asraa @TomHennen @kommendorkapten @rbehjati @jagathprakash @laurentsimon @marcelamelara @joshuagl @trishankatdatadog

@MarkLodato
Add a horizontal rule before footnotes
331a439 
Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato
Provenance v1: allow arbitrary JSON for params
24ba2f3 
Based on feedback, generalize `externalParameters` and
`systemParameters` to allow arbitrary JSON objects rather than only
specific types. This makes it easier to represent real-world systems. It
also avoids the needs for the extra layer of `artifactRef`,
`scalarValue`, etc., which was awkward.

This necessitated the removal of the "artifact reference" type of
parameter, which combined the actual parameter (usually represented as a
URI) with the resolved digest of the artifact that the parameter pointed
to. Now the resolved digest is moved to `resolvedDependencies` (with the
URI repeated there to allow joining with the parameters.) The impetus
for the removal was because there was no good way to represent the
concept of an artifact reference parameter, but upon further reflection,
the removal actually seems to simplify things.

Also expand the guidance for BuildDefinition.

Signed-off-by: Mark Lodato <lodato@google.com>
@netlify
Copy link

netlify bot commented Jan 25, 2023 •
edited
Loading

✅ Deploy Preview for slsa ready!

Name Link
🔨 Latest commit 58778a4
🔍 Latest deploy log https://app.netlify.com/sites/slsa/deploys/63d9190deb42ed00081b6382
😎 Deploy Preview https://deploy-preview-582--slsa.netlify.app/provenance/v1
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@MarkLodato MarkLodato changed the title provenance refinement Provenance v1: allow arbitrary JSON for params Jan 25, 2023
@rbehjati rbehjati mentioned this pull request Jan 25, 2023
Closed
@rbehjati
Copy link

Thanks for this nice change. Is the colour coding for the digram in docs/provenance/v1/model.svg described anywhere? Are the boxes in green the "trusted" parts that do not require verification?

asraa
asraa reviewed Jan 25, 2023
View reviewed changes
docs/github-actions-workflow/v0.1/index.md Outdated Show resolved Hide resolved
docs/github-actions-workflow/v0.1/example.json Outdated Show resolved Hide resolved
docs/provenance/v1/index.md Outdated Show resolved Hide resolved
@MarkLodato
Copy link
Member Author

Thanks for this nice change. Is the colour coding for the digram in docs/provenance/v1/model.svg described anywhere? Are the boxes in green the "trusted" parts that do not require verification?

No, it's not documented. I was wondering when someone would call me on this. 😄

This is what I was thinking, though I'd like a more rigorous approach if anyone has ideas:

  • Green = trusted, does not require verification
  • Orange = external
  • Grey = logical, doesn't really exist
  • Red = the build process itself, red because it's under the tenant's control

@MarkLodato
github: replace actor with actor_id
16144d5 
Signed-off-by: Mark Lodato <lodato@google.com>
marcelamelara
marcelamelara reviewed Jan 26, 2023
View reviewed changes
Copy link
Contributor

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

I think the changes in this PR are generally going to be quite helpful and agree that they simplify the provenance format.

docs/github-actions-workflow/v0.1/example.json Outdated Show resolved Hide resolved
docs/github-actions-workflow/v0.1/index.md Outdated Show resolved Hide resolved
docs/provenance/v1/index.md Outdated Show resolved Hide resolved
@MarkLodato
Fixes
68cbc30 
Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato
Expand github-actions-workflow based on feedback.
629fe19 
- Restrict to specific event types that we've analyzed as meeting the
  model.
- Clarify documentation.
- Rename `workflow` to `workflowPath` to be unambiguous.

Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato
Support deployment event and further clarify docs
e8566aa 
Signed-off-by: Mark Lodato <lodato@google.com>
@rbehjati
Copy link

Thanks for this nice change. Is the colour coding for the digram in docs/provenance/v1/model.svg described anywhere? Are the boxes in green the "trusted" parts that do not require verification?

No, it's not documented. I was wondering when someone would call me on this. 😄

This is what I was thinking, though I'd like a more rigorous approach if anyone has ideas:

  • Green = trusted, does not require verification
  • Orange = external
  • Grey = logical, doesn't really exist
  • Red = the build process itself, red because it's under the tenant's control

Thanks for clarifying. This looks good to me. Perhaps this legend could be added to a corner of the image, and then we can iterate on it later. As an idea for a future improvement, I think it would be nice to clarify how the diagram and particularly the "Build Process" relate to the builder. The description in https://deploy-preview-582--slsa.netlify.app/provenance/v1/ talks about builder, but is not shown in the diagram.

@MarkLodato
Support release event and fix typos
2d07915 
Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato MarkLodato mentioned this pull request Jan 27, 2023
Closed
@MarkLodato
Copy link
Member Author

Thanks for clarifying. This looks good to me. Perhaps this legend could be added to a corner of the image, and then we can iterate on it later. As an idea for a future improvement, I think it would be nice to clarify how the diagram and particularly the "Build Process" relate to the builder. The description in https://deploy-preview-582--slsa.netlify.app/provenance/v1/ talks about builder, but is not shown in the diagram.

Great points! I filed #593 to track since it will take some time and it's a bit orthogonal to the original intent of this PR.

@MarkLodato
No longer force use of <repo>@<ref> for external params.
2029656 
Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato
add TODO for triggering_actor_id
69f6dfd 
Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato MarkLodato mentioned this pull request Jan 27, 2023
Closed
@MarkLodato MarkLodato changed the title Provenance v1: allow arbitrary JSON for params Provenance v1: allow arbitrary JSON for params, refine github-actions-workflow Jan 27, 2023
@MarkLodato
Copy link
Member Author

Hey all, I would like to submit this ASAP so that we can continue to iterate. Any approvals would be appreciated. I'm happy to make small fixes and corrections, but any other significant change I'd like to defer via a TODO so that this PR gets unblocked. Thanks!

@MarkLodato
GitHub Workflow: add numeric IDs
2aa1061 
Also remove the TODOs about base_ref and head_ref - they are now
irrelevant since we no longer support pull request events.

Signed-off-by: Mark Lodato <lodato@google.com>
kommendorkapten
kommendorkapten approved these changes Jan 31, 2023
View reviewed changes
Copy link

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

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

Good work all!

One minor comment (can be resolved later). The section on how to migrate from 0.2 format still relies on externalParameters such as entryPoint and source which is not covered any more here.

joshuagl
joshuagl approved these changes Jan 31, 2023
View reviewed changes
docs/github-actions-workflow/v0.1/index.md Outdated Show resolved Hide resolved
ianlewis
ianlewis approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

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

Sorry for the late comments.

docs/github-actions-workflow/v0.1/index.md Show resolved Hide resolved
docs/provenance/v1/index.md Outdated Show resolved Hide resolved
rbehjati
rbehjati approved these changes Jan 31, 2023
View reviewed changes
Copy link

@rbehjati rbehjati 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 making this change.

@MarkLodato
Provenance v1: more fixes
58778a4 
Signed-off-by: Mark Lodato <lodato@google.com>
@MarkLodato MarkLodato merged commit d9295df into slsa-framework:main Jan 31, 2023
@MarkLodato MarkLodato deleted the provenance-refinement branch January 31, 2023 13:36
@MarkLodato
Copy link
Member Author

Thanks, everyone! Tons of great contributions! I'll send out more PRs to address the TODOs and iterate on the design.

@MarkLodato MarkLodato mentioned this pull request Feb 1, 2023
Merged
MarkLodato added a commit to MarkLodato/slsa that referenced this pull request Feb 1, 2023
@MarkLodato
Provenance v1: fix v0.2->v1 for systemParameters
51f060c 
This should have been changed in slsa-framework#582.

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

@feelepxyz feelepxyz feelepxyz left review comments

@asraa asraa asraa left review comments

@laurentsimon laurentsimon laurentsimon left review comments

@ianlewis ianlewis ianlewis approved these changes

@kommendorkapten kommendorkapten kommendorkapten approved these changes

@rbehjati rbehjati rbehjati approved these changes

@joshuagl joshuagl joshuagl approved these changes

@abacchi abacchi abacchi approved these changes

@marcelamelara marcelamelara marcelamelara approved these changes

@TomHennen TomHennen Awaiting requested review from TomHennen

@mlieberman85 mlieberman85 Awaiting requested review from mlieberman85

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

Successfully merging this pull request may close these issues.
10 participants
@MarkLodato @rbehjati @feelepxyz @ianlewis @asraa @kommendorkapten @joshuagl @laurentsimon @abacchi @marcelamelara