mirror of
https://github.com/ossf/scorecard.git
synced 2024-09-19 04:57:14 +03:00
📖 Improve explanation about multiple reviewers (and their lack) (#1017)
* Improve explanation about multiple reviewers (and their lack) The current text oversells the value of multiple reviewers, and falsely assumes it's always possible. I'm a *huge* fan of having a second reviewer, but it obviously *can't* be done when there's only 1 active participant. Even projects with multiple active participants find it difficult in practice if there aren't many participants. Also, multiple reviewers guarantee nothing; the other "reviewers" might be sock puppets or other subverted accounts. So yes, encourage review, but let's make it clear that it can't prevent all problems & that some projects cannot currently do it. Put the details in Code-Review, where it best belongs. Also, projects *can* try to remediate the lack of active participants, so give them some practical remediation steps. Finally: "pull request" is a GitHub-specific term. GitLab, SourceForge, and many other forges instead use the term "merge request". So in the interest of not locking into one specific proprietary service, let's include a more generic term. Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com> * Make fixes based on review Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com> * Explain how to get top score in Contributors Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com> Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com>
This commit is contained in:
parent
34b97e37b3
commit
45fb77983b
@ -23,8 +23,10 @@ A low score is therefore considered `High` risk.
|
|||||||
|
|
||||||
## Branch-Protection
|
## Branch-Protection
|
||||||
|
|
||||||
[Branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) allows defining rules to enforce certain workflows for branches, such as requiring a review or passing certain status checks.
|
[Branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) can enable various rules to enforce certain workflows for branches, such as preventing rewriting of public history (e.g., a *force push*), requiring review before acceptance into a main branch, or passing certain status checks before acceptance into a main branch..
|
||||||
Branch protection ensures compromised contributors cannot intentionally inject malicious code. A low score is therefore considered `High` risk.
|
Branch protection can reduce the risk of unintentional or malicious code from entering the "main" branch. Branch protection rules that prevent rewriting of public history (e.g., preventing *force push* of public branches) prevent history from changing without external notice. Branch protection rules that require status checks ensure that at least those checks are met before a change is accepted. Branch protection rules that require at least one other reviewer reviews greatly reduces the risk that a compromise of a contributor's account will lead to injection of malicious code. Review also increases the likelihood that an unintentional vulnerability in a contribution will be detected and fixed before the change is accepted. A low score is therefore considered `High` risk.
|
||||||
|
Note, however, that requiring reviews for every proposed change is impractical for many projects, since many projects simply don't have that many active participants. For more discussion, see [Code Reviews](#code-reviews).
|
||||||
|
In some cases these rules will need to be suspended. For example, if a past commit includes illegal content (such as child pornography), it may be impractical to hide the commit and in such cases the history may need to be rewritten.
|
||||||
This check determines if the default and release branches are protected with GitHub's branch protection settings. The check only works when the token has [Admin access](https://github.community/t/enable-branch-protection-get-api-without-admin/14197) to the repository. This check determines if the default and release branches are protected.
|
This check determines if the default and release branches are protected with GitHub's branch protection settings. The check only works when the token has [Admin access](https://github.community/t/enable-branch-protection-get-api-without-admin/14197) to the repository. This check determines if the default and release branches are protected.
|
||||||
|
|
||||||
**Remediation steps**
|
**Remediation steps**
|
||||||
@ -53,20 +55,25 @@ A low score is considered 'Low' risk. The check uses the URL for the Git repo an
|
|||||||
|
|
||||||
## Code-Review
|
## Code-Review
|
||||||
|
|
||||||
This check tries to determine if the project requires code review before pull requests are merged.
|
This check tries to determine if the project requires code review before pull requests (aka merge requests) are merged.
|
||||||
Reviewing code improves the quality of code in general. In addition, it ensures compromised contributors cannot intentionally inject malicious code. A low score is therefore considered `High` risk.
|
Reviewing code improves the quality of code in general, because reviews may detect various unintentional problems that can be fixed immediately before they are merged. Such problems include unintentional vulnerabilities, so unintentional vulnerabilities can be reduced through review. Reviews also make it more difficult for an attacker to insert malicious code (either as a malicious contributor or as an attacker who has subverted a contributor's account), because a reviewer might detect the subversion. This increased difficulty can even deter attackers. A low score is therefore considered `High` risk.
|
||||||
|
However, requiring that all proposed changes be reviewed by someone else before merging them is impractical for some projects. A project with only one active participant cannot practically enforce multi-person review, and even a project with multiple active participants may not have enough active participation to be able to require review of all proposed changes. Such projects will not be able to practically enforce rules such as requiring a minimum number of reviewers, and administrators in practice will be exempt from reviews. Projects with a small number of active participants, but more than one, instead sometimes aim for a review of a percentage of proposals (e.g., "at least half of all proposed changes are reviewed").
|
||||||
|
In addition, note that requiring review does not eliminate all risks. The other reviewer(s) might fail to notice unintentional vulnerabilities or malicious code. They might be colluding with a malicious developer. The "other" reviewers might even be the same person (aka a "sock puppet").
|
||||||
|
In short, requiring others' review before accepting a change reduces risks to users, as those additional reviewers may detect problems early, but it is not a practical requirement for some projects.
|
||||||
The check first tries to detect if Branch-Protection is enabled on the default branch ,and the number of reviewers is at least 1. If this fails, it checks if the recent (~30) commits have a Github-approved review or if the merger is different from the committer (implicit review). It also performs similar check for reviews using [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels "lgtm" or "approved") and Gerrit ("Reviewed-on" and "Reviewed-by").
|
The check first tries to detect if Branch-Protection is enabled on the default branch ,and the number of reviewers is at least 1. If this fails, it checks if the recent (~30) commits have a Github-approved review or if the merger is different from the committer (implicit review). It also performs similar check for reviews using [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels "lgtm" or "approved") and Gerrit ("Reviewed-on" and "Reviewed-by").
|
||||||
|
|
||||||
**Remediation steps**
|
**Remediation steps**
|
||||||
- Follow security best practices by performing strict code reviews for every new pull request.
|
- If the project has only contributor, or does not have enough reviewers to practically require that all contributions be reviewed, try to recruit more maintainers to the project who will be willing to review others' work. Ideally at least some of these people will be from different organizations. If the project has very limited utility, consider expanding its intended utility so more people will be interested in improving it, and make that larger scope clear to potential contributors.
|
||||||
|
- Follow security best practices by performing strict code reviews for every new pull request / merge request.
|
||||||
- Make "code reviews" mandatory in your repository configuration. E.g. [GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging).
|
- Make "code reviews" mandatory in your repository configuration. E.g. [GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging).
|
||||||
- Enforce the rule for administrators / code owners as well. E.g. [GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#include-administrators)
|
- Enforce the rule for administrators / code owners as well. E.g. [GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#include-administrators)
|
||||||
|
|
||||||
## Contributors
|
## Contributors
|
||||||
|
|
||||||
This check tries to determine if the project has a set of contributors from multiple companies.
|
This check tries to determine if the project has a set of contributors from multiple organizations (e.g., companies).
|
||||||
Low score has 'Low' risk.
|
Low score has 'Low' risk.
|
||||||
The check works by looking at the authors of recent commits and checking the `Company` field on the GitHub user profile. A contributor must have at least 5 commits in the last 30 commits. The check succeeds if all contributors span at least 2 different companies.
|
Some projects cannot meet this requirement. Obviously projects with only one active participant cannot meet it. Many projects have a narrow scope and may not be able to attract the interest of multiple organizations. See [Code Reviews](#code-reviews) for added notes about projects with a small number of participants.
|
||||||
|
The check works by looking at the authors of recent commits and checking the `Company` field on the GitHub user profile. A contributor must have at least 5 commits in the last 30 commits. The check succeeds if all contributors span at least 2 different organizations (companies).
|
||||||
|
|
||||||
**Remediation steps**
|
**Remediation steps**
|
||||||
- There is *NO* remediation work needed here. This is to provide some insights on which organization(s) have contributed to the project and making trust decisions based on that. But you can ask your contributors to join their respective organizations.
|
- There is *NO* remediation work needed here. This is to provide some insights on which organization(s) have contributed to the project and making trust decisions based on that. But you can ask your contributors to join their respective organizations.
|
||||||
|
@ -125,11 +125,35 @@ checks:
|
|||||||
short: Determines if the default and release branches are protected with GitHub's branch protection settings.
|
short: Determines if the default and release branches are protected with GitHub's branch protection settings.
|
||||||
description: >-
|
description: >-
|
||||||
[Branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches)
|
[Branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches)
|
||||||
allows defining rules to enforce certain workflows for
|
can enable various rules to enforce certain workflows for
|
||||||
branches, such as requiring a review or passing certain status checks.
|
branches, such as preventing rewriting of public history
|
||||||
|
(e.g., a *force push*),
|
||||||
|
requiring review before acceptance into a main branch,
|
||||||
|
or passing certain status checks before acceptance into a main branch.
|
||||||
|
|
||||||
Branch protection ensures compromised contributors cannot
|
Branch protection can reduce the risk of unintentional or malicious
|
||||||
intentionally inject malicious code. A low score is therefore considered `High` risk.
|
code from entering the "main" branch.
|
||||||
|
Branch protection rules that prevent rewriting of public history
|
||||||
|
(e.g., preventing *force push* of public branches) prevent history
|
||||||
|
from changing without external notice.
|
||||||
|
Branch protection rules that require status checks ensure that
|
||||||
|
at least those checks are met before a change is accepted.
|
||||||
|
Branch protection rules that require at least one other reviewer
|
||||||
|
reviews greatly reduces the risk that a compromise of a contributor's
|
||||||
|
account will lead to injection of malicious code. Review also increases
|
||||||
|
the likelihood that an unintentional vulnerability in a contribution
|
||||||
|
will be detected and fixed before the change is accepted.
|
||||||
|
A low score is therefore considered `High` risk.
|
||||||
|
|
||||||
|
Note, however, that requiring reviews for every proposed change
|
||||||
|
is impractical for many projects, since many projects simply don't
|
||||||
|
have that many active participants. For more discussion, see
|
||||||
|
[Code Reviews](#code-reviews).
|
||||||
|
|
||||||
|
In some cases these rules will need to be suspended.
|
||||||
|
For example, if a past commit includes illegal content
|
||||||
|
(such as child pornography), it may be impractical to hide the commit
|
||||||
|
and in such cases the history may need to be rewritten.
|
||||||
|
|
||||||
This check determines if the default and release branches are
|
This check determines if the default and release branches are
|
||||||
protected with GitHub's branch protection settings.
|
protected with GitHub's branch protection settings.
|
||||||
@ -219,14 +243,47 @@ checks:
|
|||||||
Code-Review:
|
Code-Review:
|
||||||
risk: High
|
risk: High
|
||||||
tags: supply-chain, security, source-code, code-reviews
|
tags: supply-chain, security, source-code, code-reviews
|
||||||
short: Determines if the project requires code review before pull requests are merged.
|
short: Determines if the project requires code review before pull requests (aka merge requests) are merged.
|
||||||
description: >-
|
description: >-
|
||||||
This check tries to determine if the project requires code review before
|
This check tries to determine if the project requires code review before
|
||||||
pull requests are merged.
|
pull requests (aka merge requests) are merged.
|
||||||
|
|
||||||
Reviewing code improves the quality of code in general. In addition, it ensures
|
Reviewing code improves the quality of code in general,
|
||||||
compromised contributors cannot intentionally inject malicious code. A low
|
because reviews may detect various unintentional
|
||||||
score is therefore considered `High` risk.
|
problems that can be fixed immediately before they are merged.
|
||||||
|
Such problems include unintentional vulnerabilities, so unintentional
|
||||||
|
vulnerabilities can be reduced through review.
|
||||||
|
Reviews also make it
|
||||||
|
more difficult for an attacker to insert malicious code
|
||||||
|
(either as a malicious contributor or as an attacker who has
|
||||||
|
subverted a contributor's account), because a reviewer might detect
|
||||||
|
the subversion.
|
||||||
|
This increased difficulty can even deter attackers.
|
||||||
|
A low score is therefore considered `High` risk.
|
||||||
|
|
||||||
|
However, requiring that all proposed changes be reviewed by someone
|
||||||
|
else before merging them is impractical for some projects.
|
||||||
|
A project with only one active participant cannot practically
|
||||||
|
enforce multi-person review, and even a project with multiple
|
||||||
|
active participants may not have enough active participation to be able
|
||||||
|
to require review of all proposed changes.
|
||||||
|
Such projects will not be able to practically enforce rules such as
|
||||||
|
requiring a minimum number of reviewers, and administrators in
|
||||||
|
practice will be exempt from reviews.
|
||||||
|
Projects with a small number of active participants, but more than one,
|
||||||
|
instead sometimes aim for a review of a percentage of proposals
|
||||||
|
(e.g., "at least half of all proposed changes are reviewed").
|
||||||
|
|
||||||
|
In addition, note that requiring review does not eliminate all risks.
|
||||||
|
The other reviewer(s) might fail to notice unintentional vulnerabilities
|
||||||
|
or malicious code. They might be colluding with a malicious developer.
|
||||||
|
The "other" reviewers might even be the same person
|
||||||
|
(aka a "sock puppet").
|
||||||
|
|
||||||
|
In short, requiring others' review before accepting a change
|
||||||
|
reduces risks to users,
|
||||||
|
as those additional reviewers may detect problems early,
|
||||||
|
but it is not a practical requirement for some projects.
|
||||||
|
|
||||||
The check first tries to detect if Branch-Protection is enabled
|
The check first tries to detect if Branch-Protection is enabled
|
||||||
on the default branch ,and the number of reviewers is at least 1. If this
|
on the default branch ,and the number of reviewers is at least 1. If this
|
||||||
@ -236,9 +293,19 @@ checks:
|
|||||||
[Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme)
|
[Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme)
|
||||||
(labels "lgtm" or "approved") and Gerrit ("Reviewed-on" and "Reviewed-by").
|
(labels "lgtm" or "approved") and Gerrit ("Reviewed-on" and "Reviewed-by").
|
||||||
remediation:
|
remediation:
|
||||||
|
- >-
|
||||||
|
If the project has only one contributor, or does not have enough
|
||||||
|
reviewers to practically require that all contributions be reviewed,
|
||||||
|
try to recruit more maintainers to the project who will be
|
||||||
|
willing to review others' work.
|
||||||
|
Ideally at least some of these people will
|
||||||
|
be from different organizations.
|
||||||
|
If the project has very limited utility, consider expanding its
|
||||||
|
intended utility so more people will be interested in improving
|
||||||
|
it, and make that larger scope clear to potential contributors.
|
||||||
- >-
|
- >-
|
||||||
Follow security best practices by performing strict code reviews for
|
Follow security best practices by performing strict code reviews for
|
||||||
every new pull request.
|
every new pull request / merge request.
|
||||||
- >-
|
- >-
|
||||||
Make "code reviews" mandatory in your repository configuration. E.g.
|
Make "code reviews" mandatory in your repository configuration. E.g.
|
||||||
[GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging).
|
[GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging).
|
||||||
@ -248,17 +315,26 @@ checks:
|
|||||||
Contributors:
|
Contributors:
|
||||||
risk: Low
|
risk: Low
|
||||||
tags: source-code
|
tags: source-code
|
||||||
short: Determines if the project has a set of contributors from multiple companies.
|
short: Determines if the project has a set of contributors from multiple organizations (e.g., companies).
|
||||||
description: >-
|
description: >-
|
||||||
This check tries to determine if the project has a set of contributors from
|
This check tries to determine if the project has a set of contributors from
|
||||||
multiple companies.
|
multiple organizations (e.g., companies).
|
||||||
|
|
||||||
Low score has 'Low' risk.
|
Low score has 'Low' risk.
|
||||||
|
|
||||||
|
Some projects cannot meet this requirement.
|
||||||
|
Obviously projects with only one active participant
|
||||||
|
cannot meet it.
|
||||||
|
Many projects have a narrow scope and may not be able to attract
|
||||||
|
the interest of multiple organizations.
|
||||||
|
See [Code Reviews](#code-reviews) for added notes about
|
||||||
|
projects with a small number of participants.
|
||||||
|
|
||||||
The check works by looking at the authors of recent commits
|
The check works by looking at the authors of recent commits
|
||||||
and checking the `Company` field on the GitHub user profile. A contributor
|
and checking the `Company` field on the GitHub user profile. A contributor
|
||||||
must have at least 5 commits in the last 30 commits. The check succeeds if
|
must have at least 5 commits in the last 30 commits.
|
||||||
all contributors span at least 2 different companies.
|
The highest score is achieved when there are contributors from
|
||||||
|
at least 3 different companies in the last 30 commits.
|
||||||
remediation:
|
remediation:
|
||||||
- >-
|
- >-
|
||||||
There is *NO* remediation work needed here. This is to provide some
|
There is *NO* remediation work needed here. This is to provide some
|
||||||
|
Loading…
Reference in New Issue
Block a user