-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add provenance.jq #76
feat: add provenance.jq #76
Conversation
8ce4963
to
6fcc5db
Compare
I think the shape of this is probably roughly OK -- I've got a lot of minor comments, but I don't want the higher-level discussion to get lost in them, so I'll save them for now. 😄 I am concerned by how GitHub-centric it is. Perhaps slightly more accurately, I'm concerned that there aren't any obvious indications or guardrails or places to put a clear TODO for non-GitHub usage of code that is mostly generic. 👀 Do you have any thoughts on how/where this might change if we were to generate it from Jenkins also? Which values would need to change? To be clear, I don't think we need to do all the work for that, but IMO it's worth annotating at least the things we know are GitHub-only and/or somewhere in the file adding a note that makes it really clear this is GitHub specific for now, perhaps even naming the function Do you want me to go through nits here/now, or wait until we're closer to integrating before hitting the low-level/cosmetic stuff? |
6fcc5db
to
50e2707
Compare
Good point! I added
Sure! Whatever is more convenient for you, comments here, pairing or even just pushing changes to my branch (you should have write access). This is mostly a PoC to get things started but I would be delighted if we ended up using it! |
1fec599
to
11c49b5
Compare
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.
One question
11c49b5
to
ff96c7e
Compare
ff96c7e
to
5c73be4
Compare
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 few more specific comments to hopefully generate more discussion to fine-tune a few points before getting into nits like whitespace 🙈 ❤️
83ad7d0
to
2ab144c
Compare
10b5d3a
to
93e0c45
Compare
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 not completely done, and I think there's still some discussion points open (notably the high-level input) so I'm trying to avoid commenting in places that might change or go away based on those discussions, but here's a few more nit-picking notes before I go to lunch 👀
93e0c45
to
c57fefd
Compare
c57fefd
to
24e679e
Compare
Sounds good! @LaurentGoderre and I hit most of these comments and we also refactored the high level input to use the build object for the Remember that we have a working test environment and some of the high-level changes aren't captured here but will be in a future PR with changes to |
…r calculation adjustments The primary justification for collapsing everything back into the main document (aside from there only being a single caller for each function) is that it makes verifying that we didn't miss anything easier -- as you scan the document, for each field that contains a "load-bearing" URL (first https://in-toto.io/Statement/v1, then https://slsa.dev/provenance/v1, and finally https://actions.github.io/buildtypes/workflow/v1), if you open the URL it describes the expected format, fields, and values, and with them all here and in-order, they're *much* easier to match up and validate to be correct and exhaustive. Granted, that will change over time as we shove more and more (optional) data into this document so that it includes a more complete picture, but for now, this is makes it really easy to double check our work (and the end result is no less organized; for example, the `externalParameters` are still all grouped together under a suitable heading describing what their purpose is). I also made some minor changes to the way values were calculated, especially in the `workflow` block, but very related to the above justification: now the way we calculate the values matches the way they're described in https://actions.github.io/buildtypes/workflow/v1 (specifically using the exact fields parsed in the exact ways they suggest). We will probably deviate from that over time (as suggested by a new "TODO" comment I included), but at least this way our baseline matches theirs and the delta will be easier to track. Additionally, I removed the `(env.GITHUB_CONTEXT | fromjson) as $github` line from here, because I think that's more appropriate behavior for the caller (and added back the explicit function arguments). This will be more clearly meaningful in my follow-up commit adding a basic test.
…rate/validate the `inputs` variance)
I had more suggestions, but figured they might be easier to digest as explicit commits instead of a pile of interconnected suggestions (or prose / whatever it is that GitHub does with suggestion blocks that span a large number of lines):
You can also view them via 24e679e...ad28b37 or steal them directly via fetch-by-commit or https://github.com/infosiftr/doi-meta-scripts/tree/tianon-provenance if you find them unobjectionable as-is. (Of course, happy to discuss further, explain more/better, push directly, adjust, etc etc.) |
@tianon 🙇 thanks for making the time to take a pass on this! I pushed your commits since I don't really have any objections and like the way it turned out. I also added the I didn't realize there was a whole test framework here 😆 , appreciate the addition and updated it with the |
I might want to revert the workflow back to what we had before. Specifically, because this really should represent the workflow claims. As implemented in https://github.com/actions/attest-build-provenance for example, they also decided to use the const [workflowPath, workflowRef] = claims.workflow_ref
.replace(`${claims.repository}/`, '')
.split('@'); ... externalParameters: {
workflow: {
ref: workflowRef,
repository: `${serverURL}/${claims.repository}`,
path: workflowPath
}
}, I believe this is more accurate despite the example given in the build type definition. edit: fixed in 03048f2 |
The problem is we don't have a |
hmm, yeah. This is not ideal. The key thing for me here is that Although, if we are limiting this to Having talked through it, I think I'll go back to what you had for this and then we can avoid #76 (comment) |
03048f2
to
d9b5735
Compare
Co-authored-by: Tianon Gravi <[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.
👀
summary
provenance.jq
filter to generate in-toto provenance for GHA buildsusage
example usage:
output
example output: