content: reorganize levels, update requirement text#1483
Conversation
✅ Deploy Preview for slsa ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the SLSA source requirements specification to improve clarity, reorganize structure, and enhance guidance for source control system implementers. The changes focus on refining terminology, consolidating requirements across SLSA levels, and providing more precise technical guidance.
Key changes include:
- Restructured SLSA Level descriptions to better communicate the progression from basic version control to comprehensive code review
- Reorganized requirements tables to emphasize technical controls, provenance, and intent declaration
- Enhanced documentation around technical controls enforcement and provenance generation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/spec/draft/source-requirements.md | Major content updates including level descriptions, requirements reorganization, terminology fixes, and grammar improvements |
| .vscode/settings.json | Added domain-specific words to spell-check dictionary |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
This is meant to fix #1459, right? |
TomHennen
left a comment
There was a problem hiding this comment.
Thanks for this Zach it's looking promising!
TomHennen
left a comment
There was a problem hiding this comment.
Sorry for the long turnaround on these last few comments, I find I really need some time to focus to think about them.
MarkLodato
left a comment
There was a problem hiding this comment.
Huge improvement! Thanks for the hard work here.
In order to get this to land sooner, you might want to consider splitting this PR in two:
- Pure editorial changes --- grammar, typos, rewording, including the updated table at the top. These are uncontroversial and can just get submitted since there is little disagreement here and it's an obvious improvement.
- Content changes. This is where all of the discussion is. Splitting this to a separate PR might make it easier to discuss these changes in isolation.
Note: I didn't review any of the content changes yet in this PR since there is still a lot of back-and-forth. I'll review once those in-flight edits are made.
d499679 to
b8ae06f
Compare
Signed-off-by: Zachariah Cox <zachariahcox@github.com>
Co-authored-by: Tom Hennen <TomHennen@users.noreply.github.com> Signed-off-by: Zachariah Cox <zachariahcox@github.com>
Co-authored-by: Tom Hennen <TomHennen@users.noreply.github.com> Signed-off-by: Zachariah Cox <zachariahcox@github.com>
arewm
left a comment
There was a problem hiding this comment.
Thanks, @zachariahcox for all of this work. Two comments that are relatively minor. We can move them out to issues if we want to handle them outside this PR (I feel like they should be addressed before we publish).
Also, @MarkLodato requested changes so he'll need to re-review.
| If the SCS supports Tags, the SCS MUST be configured to prevent them from | ||
| being moved or deleted. |
There was a problem hiding this comment.
Are these tags intended to be just for the consumable Source Revisions or globally? In RC1, it was more clear that these were just for the protected branches.
There was a problem hiding this comment.
good question. in this case, I think we just mean the definition of "tag" from above, but maybe we should add a hyper link for clarity
If we need another revision on this PR, I'll add this link.
There was a problem hiding this comment.
Agree. It's a bit tautological, but basically, if you intend for something to be immutable then the SCS needs to be configured to prevent them from being moved/deleted.
If it's global that seems fine. If it only applies to configure named references, that seems fine too? A non-branch named reference that isn't configured to prevent moving/updating, won't get an L2+ VSA/provenance associated with it.
| For each technical control claimed in a VSA, continuity MUST be established and | ||
| tracked from a specific start revision. | ||
| If there is a lapse in continuity for a specific control, continuity of that | ||
| control MUST be re-established from a new revision. |
There was a problem hiding this comment.
This is probably more of an implementation detail (i.e. related to source-tool), but should we be stating continuity at a granular level or at a more comprehensive level?
i.e. will continuity need to be established for a specific control or would it need to be re-established for all controls? This probably affects how verification is intended to be worked with.
There was a problem hiding this comment.
IMO doing it for specific controls would be easier so let's start there?
This also makes adding new controls easier since you don't have to reestablish continuity.
But yeah, maybe that can be left to the implementations?
There was a problem hiding this comment.
I like this line of thought.
It seems like there are some controls that are foundational (ie, no force pushing) and might impact the validity of the others?
As mentioned in other comment threads, I think the control continuity "lengh of enforcement" metric is interesting, but probably gameable due to how different the change velocities can be between repos. For example, how long / how many commits do you need to enforce a control before it's "good enough" -- for some repos that might be 2 while for another it's 200. Would you put a control "on probation" if it was broken recently?
I think these are interesting questions and we should come back to them.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
MarkLodato
left a comment
There was a problem hiding this comment.
Overall I think this PR is a strict improvement from the previous version and should be merged. I don't want my comments to hold this PR up any further.
For the record, this PR actually does three separate things:
-
Content: Redefine L2 and L3 by shuffling around requirements. L2 is now about trust in the history and L3 is about enforcing (additional?) technical controls.
I agree with this change. It is largely in line with bullet 2 in #1459. That said, I think L3 could use a bit more refining. "Enforce organizational technical controls" is a bit ambiguous; personally I'd frame it around publication.
-
Content: Change the details of specific requirements. For example, it is now explicit that History requires both server-side reflog and non-fast-forward updates (using git terminology).
Most of these changes LGTM, but I'm not sure I agree 100% with all of the them. For example, there used to be a requirement at what is now L3 around naming specific branches as being "consumable", and I thought that was helpful for giving L3 meaning. That appears to have been dropped?
-
Editorial: Reword a lot of text for clarity.
LGTM. There's still room for further improvement, but let's get this merged and then iterate.
For the future, I highly recommend splitting PRs so that they are either pure content changes (minimizing edits) or pure editorial changes (zero content changes). Doing so would make it much easier to review and would likely speed up the review process.
Two requests for the PR description:
- Update the PR description to summarize what the content changes are.
- Do not mark #1459 as fixed. This PR only addresses the second bullet (out of four) in that issue. (If the group disagrees with the rest of my suggestions, I ask that you say so in that issue rather than implicitly closing it as part of this PR.)
[Using "Request changes" so my request doesn't get lost. Once the description is updated, I'll click Approve.]
|
@MarkLodato @TomHennen @arewm @adityasaky I have updated the description of this PR to summarize what I think ended up happening here. Please feel free to directly edit as you see fit 👍 Thank you! |
Related to points 2 and 3 of #1459
This PR reorganizes the levels to achieve slightly different goals.
There is still work to do here 😅
Major content changes:
Minor content changes: