Skip to content
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

📖 Update Branch-Protection admin and non-admin requirements #2772

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
95f4f59
docs: Branch protection admin-only requirements
gabibguti Mar 20, 2023
1c001b4
docs: Branch protection requirements by tier
gabibguti Mar 20, 2023
65d38cc
docs: How get a perfect score in branch protection
gabibguti Mar 20, 2023
bf5fcce
docs: Fix local images ref in doc
gabibguti Mar 20, 2023
c32e1e6
docs: Fix typo
gabibguti Mar 20, 2023
f37fdc7
docs: Fix check specific table of contents
gabibguti Mar 20, 2023
408c179
fix: Code owners setting is non admin
gabibguti Mar 20, 2023
66609ae
docs: Fix branch protection applied not only to main branch
gabibguti Mar 20, 2023
34f20b5
docs: Add alt text for images
gabibguti Mar 21, 2023
0225297
docs: You can get a perfect score with non admin access
gabibguti Mar 21, 2023
4491ab6
Merge branch 'main' into docs/update-branch-protection-requirements
gabibguti Mar 21, 2023
0adfa73
docs: update max tier scores
gabibguti Mar 27, 2023
e0c9f69
docs: update tier 1 max points explanation
gabibguti Mar 28, 2023
62f893b
docs: Move changes to internal checks doc
gabibguti Jun 23, 2023
363049d
docs: Revert changes on checks doc
gabibguti Jun 23, 2023
cb4c982
docs: Fix admin settings evaluated on branch protection
gabibguti Jul 6, 2023
1b90890
docs: Change branch protection model status checks
gabibguti Jul 6, 2023
3e4bb83
docs: Change tiers score to expected score
gabibguti Jul 7, 2023
2e3c0cc
Merge branch 'main' into docs/update-branch-protection-requirements
gabibguti Jul 7, 2023
78b3987
docs: Fix Tier 3 score
gabibguti Jul 7, 2023
2a9d56f
Merge branch 'main' into docs/update-branch-protection-requirements
spencerschrock Jul 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions docs/checks.md
gabibguti marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ certain workflows for branches, such as requiring review or passing certain
status checks before acceptance into a main branch, or preventing rewriting of
public history.

Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, `StrictStatusCheck` and `RequireCodeownerReview`. If
Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, `RequireLastPushApproval` and `RequiresStatusChecks`. If
gabibguti marked this conversation as resolved.
Show resolved Hide resolved
the provided token does not have admin access, the check will query the branch
settings accessible to non-admins and provide results based only on these settings.
Even so, we recommend using a non-admin token, which provides a thorough enough
Expand Down Expand Up @@ -97,28 +97,27 @@ commit.

This test has tiered scoring. Each tier must be fully satisfied to achieve points at the next tier. For example, if you fulfill the Tier 3 checks but do not fulfill all the Tier 2 checks, you will not receive any points for Tier 3.

Note: If Scorecard is run without an administrative access token, the requirements that specify “For administrators” are ignored.
Note: If Scorecard is run without an administrative access token, the requirements that specify “For administrators” can be safely ignored, and scores will be determined as if all such requirements have been met.

Tier 1 Requirements (3/10 points):
- Prevent force push
- Prevent branch deletion
- For administrators: Do not allow bypassing the above settings

Tier 2 Requirements (6/10 points):
- Required reviewers >=1
- For administrators: Last push review
- For administrators: Strict status checks (require branches to be up-to-date before merging)

Tier 3 Requirements (8/10 points):
- Status checks defined
- Require at least 1 reviewer for approval before merging
- For administrators: Require branch to be up to date before merging
- For administrators: Require approval of the most recent reviewable push

Tier 3 Requirements (7/10 points):
gabibguti marked this conversation as resolved.
Show resolved Hide resolved
- Require branch to pass at least 1 status check before merging

Tier 4 Requirements (9/10 points):
- Required reviewers >= 2
- Require at least 2 reviewers for approval before merging
- Require review from code owners
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved

Tier 5 Requirements (10/10 points):
- For administrators: Dismiss stale reviews
- For administrators: Require CODEOWNER review

- For administrators: Dismiss stale reviews and approvals when new commits are pushed
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved

**Remediation steps**
- Enable branch protection settings in your source hosting provider to avoid force pushes or deletion of your important branches.
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
26 changes: 26 additions & 0 deletions docs/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ This page answers frequently asked questions about Scorecard, including its purp
- [Pinned-Dependencies: Will Scorecard detect unpinned dependencies in tests with Dockerfiles?](#pinned-dependencies-will-scorecard-detect-unpinned-dependencies-in-tests-with-dockerfiles)
- [Pinned-Dependencies: Can I use version pinning instead of hash pinning?](#pinned-dependencies-can-i-use-version-pinning-instead-of-hash-pinning)
- [Signed-Releases: Why sign releases?](#signed-releases-why-sign-releases)
- [Branch-Protection: How to setup a 10/10 branch protection on GitHub?](#branch-protection-how-to-setup-a-1010-branch-protection-on-github)

---

Expand Down Expand Up @@ -80,3 +81,28 @@ Currently, the main benefit of [signed releases](/checks.md#signed-releases) is
However, there are already moves to make it even more relevant. For example, the OpenSSF is working on [implementing signature verification for NPM packages](https://github.blog/2022-08-08-new-request-for-comments-on-improving-npm-security-with-sigstore-is-now-open/) which would allow a consumer to automatically verify if the package they are downloading was generated through a reliable builder and if it is correctly signed.

Signing releases already has some relevance and it will soon offer even more security benefits for both consumers and maintainers.

### Branch-Protection: How to setup a 10/10 branch protection on GitHub?

To get a 10/10 score for Branch-Protection check using a non-admin token, you should have the following settings for your branches:
gabibguti marked this conversation as resolved.
Show resolved Hide resolved

![GitHub's branch protection settings with the following options selected: "Require a pull request before merging", "Require approvals" with 1 approver, "Require review from Code Owners", "Require status checks to pass before merging", "Require branches to be up to date before merging", and have at least one Status Check chosen. All other options are unchecked.](/docs/design/images/branch-protection-settings-non-admin-token.png)

When using an admin token, Scorecard can verify if a few other important settings are ensured:
gabibguti marked this conversation as resolved.
Show resolved Hide resolved

![GitHub's branch protection settings with the following options selected: "Require a pull request before merging", "Require approvals" with 2 approvers, "Dismiss stale pull request approvals when new commits are pushed", "Require review from Code Owners", "Require approval of the most recent reviewable push", "Require status checks to pass before merging", "Require branches to be up to date before merging", have at least one Status Check chosen, and "Do not allow bypassing the above settings". All other options are unchecked.](/docs/design/images/branch-protection-settings-admin-token.png)

It's important to reiterate that Branch-Protection score is Tier-based. If a setting from Tier 1 is not satisfied, it does not matter that all other settings are met, the score will be truncated up the Tier's maximum. In this case, 3/10. The following table shows the relation between branch protection settings on GitHub and the score Tier:

| Name | Status | Required only for admin token | Tier |
| -------------------------------------------------------------------------------------------------------- | ------------------------------- | ----------------------------- | ---- |
| Allow force pushes | Disabled | - | 1 |
| Allow deletions | Disabled | - | 1 |
| Do not allow bypassing the above settings | Enabled | Yes | 1 |
| Require a pull request before merging > Require Approvals | Enabled with at least 1 | - | 2 |
| Require status checks to pass before merging > Require branches to be up to date before merging | Enabled | Yes | 2 |
| Require a pull request before merging > Require approval of the most recent reviewable push | Enabled | Yes | 2 |
| Require status checks to pass before merging > Status Checks | At least 1 | - | 3 |
| Require a pull request before merging > Require Approvals | Enabled with at least 2 | - | 4 |
| Require a pull request before merging > Require review from Code Owners | Enabled and has CODEOWNERS file | - | 4 |
| Require a pull request before merging > Dismiss stale pull request approvals when new commits are pushed | Enabled | Yes | 5 |
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved