laurentsimon
commented
3 years ago Describe the bug Currently scorecard requires linear history and disabling stale review dismissal as part of Branch-Protection, and I believe that's a mistake. Remove these:
"Warn: linear history disabled on branch 'main'", "Warn: Stale review dismissal disabled on branch 'main'",Reproduction steps Steps to reproduce the behavior:
- Run on any repo that doesn't do this, e.g., https://github.com/coreinfrastructure/best-practices-badge - note that we once required squashing, but we stopped that.
Expected behavior No complaints if you don't require linear history or if stale review dismissal is enabled.
Additional context
Scorecard should only report problems if there is clear evidence that the problem will lead to security problems. It's fine to include approaches that reduce defects in general, since a subset of defects are unintentional vulnerabilities, so I can accept a broad rationale.
However, requiring linear history does not improve security. It's an aesthetic, like 2-space vs. 4-space indentation. Some people like linear histories because they seem "clean" to them, but that is a debatable aesthetic that is not the purpose of scorecard. Scorecard shouldn't require everyone implement projects according to some house/company policy, but instead should focus on criteria that improve security.
Linear histories have pros and cons. Here are a few cons:
- It falsifies history, which means analysis of commits does not analyze the actual project history. "Linear history" could be more accurately described as "history falsification". That can lead to misleading results from later analysis.
- It does not work well with cryptographically signed commits of many people. Many projects are far more distributed and have many branches that contain the work of many people. If each commit is signed and there are multiple commits, you can't really rebase and keep the cryptographic signatures. "Solutions" include dropping cryptographic signatures of commits (ouch!) or falsifying who wrote what (possibly illegal under the license + copyright law, and also interfering with the measurement of how many organizations are contributing).
good feedback, thanks
Similarly, there's a reason for dismissing stale reviews - they are stale. Some people want them dismissed because they're false, others don't. If anything, for security you don't want to consider a stale review as valid, so there's a better argument for enabling stale review if anything. I think it's better to let projects choose what works for them.
The GitHub configuration name "stale" is mis-leading. AFAIK, a review is considered stale if it's reviewed and then modified by the author, requiring the review to need approval again. This helps prevents innocuous-looking reviews from being reviewed-then-modified to be malicious. There is a security gain to this. There are usability concerns with this, obviously: it slows down the dev/review process and I would buy the argument that it may have the opposite effect: review fatigue which would lead to less through reviews. We should at least log the config as Info instead of Warn.
Anyone else has opinion on this?
Added this for discussion for next meeting, we can go over the entire branch protection check.
You may also have an opinion on https://github.com/ossf/scorecard/blob/main/checks/branch_protection.go#L256, which warns if the number of reviews is less than 2 (2 is a magical number that helps against sock puppet attacks by forcing a collision to perform it)
github-actions[bot]
Describe the bug Currently scorecard requires linear history and disabling stale review dismissal as part of Branch-Protection, and I believe that's a mistake. Remove these:
Reproduction steps Steps to reproduce the behavior:
Expected behavior No complaints if you don't require linear history or if stale review dismissal is enabled.
Additional context
Scorecard should only report problems if there is clear evidence that the problem will lead to security problems. It's fine to include approaches that reduce defects in general, since a subset of defects are unintentional vulnerabilities, so I can accept a broad rationale.
However, requiring linear history does not improve security. It's an aesthetic, like 2-space vs. 4-space indentation. Some people like linear histories because they seem "clean" to them, but that is a debatable aesthetic that is not the purpose of scorecard. Scorecard shouldn't require everyone implement projects according to some house/company policy, but instead should focus on criteria that improve security.
Linear histories have pros and cons. Here are a few cons:
Similarly, there's a reason for dismissing stale reviews - they are stale. Some people want them dismissed because they're false, others don't. If anything, for security you don't want to consider a stale review as valid, so there's a better argument for enabling stale review if anything. I think it's better to let projects choose what works for them.
Anyway, scorecard needs to keep focused. It should only require criteria that are clearly improvements for security, and nothing else.