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
content: source track draft: simplify and clarify level goals #1097
content: source track draft: simplify and clarify level goals #1097
Changes from 3 commits
989aeb8
b4f8b7b
68e8632
5d5df51
b2071c0
cc8a51c
5a456d6
ad62f09
e4050b4
47f8ad3
d212e08
8bf4205
2343b07
a7fd4fe
b003548
09b0c67
6d3e582
99f372a
6b82d6d
a583425
64132b8
9ab53dc
7269b05
1be7b2a
7faa87d
843729c
69f611c
08ec500
6515598
f962ac4
d809349
2f86109
834d64c
9b72a77
006a4c1
febc191
6e3f3e3
6c05dc1
3919bad
cb63de3
f2398a4
d1219c6
5527616
6df6b33
31d63ba
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.
I presume the repository ID is different from the URL to locate the repository on the SCP? For example, the repository ID for this one is
346517502
per https://api.github.com/repos/slsa-framework/slsa.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.
Oh, I had imagined the repository ID is the URL (e.g.
https://github.com/slsa-framework/slsa
).Having it be something that's meaningful to humans would be pretty beneficial.
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.
Yeah, I think that's partly what I was getting at. I wonder if there's an endpoint to translate the numeric ID to the URL or if the current definition for repository (even without the ID) inherently has a URL associated with it because we only consider a repository on the SCP at the moment. This is part of the massaging I wonder if I should take a stab at separately.
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.
IMO we should consider the numeric ID to be an implementation detail and not worry about it?
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.
it's an interesting question -- today we definitely have resurrection and rename attacks (so you need the gh repo numeric id to disambiguate), but I think in combination with the git object id, the url is probably fine.
IE, it doesn't likely matter much if a hijacked repo (with a different numeric id) has the same url and ships the same revision, we can ship the corresponding attestation. I'd want to think through it some more.
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 something that we had been considering for level 2 (at least in #1066).
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 could soften it to "must have a defined change process of any kind", but it seems ok to me to say that "keeping the history of those past revisions" is pretty close to the definition of version control.
Is that what you were thinking?
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.
Yeah we talked about this the other day and I think the resolution was basically "by default all SCPs/VCSs keep the entire revision history and in fact getting rid of some of that history after a short amount of time would be pretty difficult to do safely, so we're better off just requiring it from the start and we can see if anyone complains"
But now I'm also remembering some discussion we had where you said something like "some users want to be able to get rid of 'stuff' after a certain amount of time to save storage costs". I don't remember what 'stuff' was though?
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 parsing this bit as "there must be a mechanism to submit revisions that captures the following information", which I think may be very strict in what systems can meet the requirement, when perhaps we want the spec to focus on the data, which may be gathered in other ways. This is tied to my other comment about definitions / model used, and perhaps best left to a follow on PR I submit after this is merged?
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.
At some level in this track, we may want to set code review requirements, but I think it should be an implementation detail whether this is enforced pre-merge or not. For example, a policy "all changes must be reviewed by two maintainers" can be met even if the SCP in use doesn't provide a way to require approval pre-merge. Enabling two approval as a branch protection rule on an SCP is one way to achieve the two party review requirement. Alternatively, we may have mailing list driven models where approvals / merges look very different. The system doesn't have a way to require pre-approval, but we can tell that the requirement was met. I suggest we just record the review happened, and separately set review requirements here or at some other level.
Separate note that may be part of a requirement from the policy PoV: "specific actors" is a bit open-ended. Does this mean we must be able to require reviews with greater granularity than just "a maintainer of the repository"? More along the lines of requiring reviews from specific developers for specific files that are modified (eg: via CODEOWNERS)?
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.
Not requiring review before merge SGTM (and as you say, I can see why a SCP might still choose to require the review prior to merge). However "Record reviews / votes submitted before a change proposal is accepted." still seems to require the action take place before the change is merged?
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.
Correct, but I think that's generally in line with recording events in the order that they happen in. Perhaps my phrasing is off. I also reckon reviews after it's accepted is a nice-to-have data point but not meaningful in policy decisions about that merge event? I may be missing some use cases though.