Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SLSA v1.0: Add "Verifying Build Systems" #568
SLSA v1.0: Add "Verifying Build Systems" #568
Changes from 5 commits
a55d28b
b0a97c8
1b21bb6
bfd1592
0a3ae1d
5c59c75
bc531ca
e404195
d08a449
0f48461
1c3b95a
444944a
10a8d97
790b67a
b3e67a7
7aafd0d
e2e8c80
2866e69
f6af2be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: use hyphens to separate words, not underscores (e.g. use-cases.md, get-started.md, also that's Google's style guide). I suggest waiting until right before merging so that we don't lose comments.
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.
Done
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 like this list of attacker profiles, but the rest of the doc doesn't seem to use it. That seems like a missing piece, though I can't figure out exactly what's missing.
Maybe it's that the SLSA Build requirements are designed to protect against Low and Medium attackers, while protecting against High attackers is very complex and we need this "verifying systems" piece to convince consumers that they've done a good enough job?
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.
Right now this section is a bit unclear on the meaning of terms like "the source repo", "their", and "the package". Maybe explain that there is a "target" repo/project? Then in low privilege, "their" can be used to describe the attacker creating a separate project unrelated to the target?
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 tried to clarify this section by drawing a distinction between the "target build" and "controlled builds." It may be a bit clunky.
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.
Have you thought about just calling these:
It seems like the rest of the doc does not use the low/medium/high profile terms anyway. Perhaps the alternates are easier to work in?
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.
+1 to the alternatives
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.
Considering private projects, I'd recommend to not use "Everyone else" rather something like "Developer" or "Contributor". I feel this could help apply for both public and private projects.
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.
Done
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.
New wording is "Project contributors". (But it should be singular.)
But that seems overly narrow. An attacker could be someone who is not a contributor at all, no?
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.
Fair point, we are talking about someone who is able to execute the capabilities:
I suppose they are an "Authenticated User" with those abilities. That would satisfy my concern about private / public projects.
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.
We should probably expand on " All provenance generated by L3+ builders must contain a non-forgeable attestation of the builder's L3+ capabilities with a limited validity period." – what attestation? Why? What period?
This AI could be a TODO
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 like the idea of this - maybe for SLSA 1.1 we could create a verified build system attestation spec :)
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.
Added as a TODO. I'm hoping to squeeze it into 1.0 if possible.