-
Notifications
You must be signed in to change notification settings - Fork 219
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
Provenance v1.0: initial draft #525
Provenance v1.0: initial draft #525
Conversation
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Move all top-level inputs to their own field so that it is clear what must be verified. Move resolvedDependencies outside since it's not top-level. Create new buildDependencies containing resolvedDependencies and environment. This way buildDefinition is complete. Revert field names to their v0.2 names, since the name change isn't worth the switch. Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
✅ Deploy Preview for slsa ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Signed-off-by: Mark Lodato <[email protected]>
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 is looking really good. I did a first pass review and added some minor observations and comments.
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
f87271e
to
5aab59b
Compare
Signed-off-by: Mark Lodato <[email protected]>
Also move to a subdirectory (1) since we now have multiple files and (2) to make relative includes work correclty whether or not the URL has a trailing slash. Signed-off-by: Mark Lodato <[email protected]>
docs/provenance/v1.0.proto
Outdated
// TODO: Flesh out this model more. | ||
// | ||
// OPTIONAL. | ||
repeated ArtifactReference builderDependencies = 3; |
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.
A CI pipeline's binaries could be categorized in 2 ways.
- The list of orchestrator tools like Tekton Pipeline, TektonChains, TektonTriggers...
- The list of actual tools which make the build like gcc.
Should this tools from only from (1) or even (2).
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 should only be (1). (2) should go in resolvedDependencies
. Do you have a suggestion to make that more clear?
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
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 everyone, thanks for the very helpful feedback! I'm hoping to get this merged this week, so if you could review end-of-day Friday (US Eastern), that would be appreciated!
There are still many TODOs and outstanding issues, particularly on the design of the parameters. The intention is that we submit this and iterate. By having it checked in, we can more easily get feedback from potential implementers.
Preview: https://deploy-preview-525--slsa.netlify.app/provenance/v1/
NOTES:
-
I aggressively marked GitHub comment threads as "resolved" in order to reduce noise. I did my best to try to capture the intent either as a TODO or actually implementing it. Feel free to reopen threads, though the UI doesn't make that particularly easy...
-
I reorganized a bit and changed around the documentation format. Please take a look. I'm hoping this new format is now easier to follow. (The reason for moving under a directory is to make it easier to have related files, and the reason for dropping the ".0" from the URL is that is what our documentation says we would do, since minor version bumps are backwards compatible.)
Thanks all!
"digest": { "sha1": "c27d339ee6075c1f744c5d4b200f7901aad2c369" } | ||
} | ||
}, | ||
"workflow_path": { "value": ".github/workflow/release.yml" } |
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.
Is this thread OK to resolve, or is there a specific request here (hard to follow thread)?
docs/provenance/v1.0.proto
Outdated
// TODO: Flesh out this model more. | ||
// | ||
// OPTIONAL. | ||
repeated ArtifactReference builderDependencies = 3; |
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 should only be (1). (2) should go in resolvedDependencies
. Do you have a suggestion to make that more clear?
Signed-off-by: Mark Lodato <[email protected]>
Signed-off-by: Mark Lodato <[email protected]>
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.
Looks good to me, thanks very much! Could we please ask another CI provider (e.g., GitLab) to contribute their own Build Type to make sure that we have designed a general enough Build Provenance?
Signed-off-by: Mark Lodato <[email protected]>
Good point. Added TODO noting that it's a blocker for calling it stable. |
Approved, but shouldn't hang on my opinion alone 🙂 |
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 is a fantastic evolution of the provenance format. Thanks for calling out the bits that would especially benefit from feedback with the "RFC" markers.
I fully support merging this as-is and iterating in tree. I have made a few minor suggestions to fix some of the links, otherwise LGTM.
Signed-off-by: Mark Lodato <[email protected]>
Thanks. I fixed all those issues, I think. I'll do a "squash and merge", but I'll leave this open for a bit for you to verify if you have time. |
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.
LGTM
Signed-off-by: Mark Lodato <[email protected]>
Thanks all! This is now squashed and merged, but please continue to provide feedback and suggestions. |
"id": "https://github.com/slsa-framework/slsa-github-generator/.github/workflows/builder_go_slsa3.yml@refs/tags/v0.0.1" | ||
}, | ||
"metadata": { | ||
"invocationId": "https://github.com/octocat/hello-world/actions/runs/1536140711/attempts/1", |
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.
Love having the full URL in here!
This is an initial draft v1.0 for the provenance format. It attempts to address the major sources of confusion with v0.2, particularly the separation of concerns between top-level external inputs, resolved dependencies, and per-run details. There are still many outstanding TODOs and unresolved design decisions. The idea is to submit this now so that we can more easily get feedback on the schema and substance, then send further PRs to address the remaining TODOs and RFCs. Signed-off-by: Mark Lodato <[email protected]>
This is an initial draft v1.0 for the provenance format. It attempts to address the major sources of confusion with the current format (v0.2), particularly the separation of concerns between top-level external inputs, resolved dependencies, and per-run details.
There are still many TODOs. I hope to get this merged sooner rather than later so that we can get feedback on the schema and substance, then send further PRs to address the remaining TODOs.
(I'll rewrite the history before merging so that it's not full of all my "WIP" commits.)
Preview: https://deploy-preview-525--slsa.netlify.app/provenance/v1/