slsa-framework / slsa

Supply-chain Levels for Software Artifacts
https://slsa.dev
Other
1.48k stars 212 forks source link

How should the Source track address merge strategies? #1040

Open kpk47 opened 3 months ago

kpk47 commented 3 months ago
          Some communities are very adamant about _never_ squashing commits, instead using commits to describe logical bits of change and then just merging them all in to preserve the blame. While this might introduce some risk (i.e. by creating reachable malicious commits), that risk can be mitigated if all commits are reviewed during the process. 

While squash+merge might be a best practice from a security perspective, I am not sure whether we can be overly prescriptive of it as a requirement for this track/level.

_Originally posted by @arewm in https://github.com/slsa-framework/slsa/pull/1037#discussion_r1542179643_

The Source track currently requires squash+merge, but @arewm points out that not all communities are open to that merge strategy. Should we maintain the requirement for squash+merge and evangelize that strategy's benefits, or should we accommodate others? If the latter, then which ones?

david-a-wheeler commented 3 months ago

I suggest dropping the requirement to mandate squash+merge. It's controversial & you can make arguments in both directions. For example, squash+merge removes the history that lets you see what problems were found & fixed by a review; indeed, it mostly hides the review. I don't think requiring squash+merge is a hill worth dying on :-).

adityasaky commented 3 months ago

+1, I think requiring squash merge is not the best idea. IMO, there's value in preserving not just the history of the feature branch but also the signatures on the commits that are merged in.

MarkLodato commented 2 months ago

+1, it should not require squash merge.

Instead, I think what we want is that it's possible to know which commits meet the requirements (retention, attribution, etc) and which do not. For example, in a GitHub model where reviewed only look at the merge commit but not the intermediate garbage commit, there would be some way for the verifier to know that only the merge commits are "good" but that the intermediate commits have no claims. This could be by a convention that it's always "first parent history" (using git terminology), or that the merge commits are signed, or something else.

joshuagl commented 2 months ago

I'm with the other commenters, trying to mandate merge strategy will make the source track a no-go for many (open source projects and not).

zachariahcox commented 4 days ago

that risk can be mitigated if all commits are reviewed during the process.

This is definitely the case, but not typically what happens. Almost all code review tools default to showing the net diff and per-commit-review status is not recorded. CI tools do not tend to stress test each constituent commit, etc.

The most secure repo revisions will squash approved code reviews to reduce certain classes of risk. As long as the attestations are clear about which process was followed, consumers can just be picky about their required merge strategies.