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

Branch-Protection: Review/remove scoring based on Tiers #3123

Open
diogoteles08 opened this issue Jun 5, 2023 · 7 comments
Open

Branch-Protection: Review/remove scoring based on Tiers #3123

diogoteles08 opened this issue Jun 5, 2023 · 7 comments

Comments

@diogoteles08
Copy link
Contributor

diogoteles08 commented Jun 5, 2023

Is your feature request related to a problem? Please describe.
Currently, the Branch-Protection check calculates the scores based on Tiers. That can end up causing frustration, because if a repo is placed in an inferior Tier -- limited by one specific requirement of its next tier --, it gets no score rewards if it completes a requirement of a higher tier. I can give two practical examples:

  1. At this current date, the project github.com/firebase/flutterfire is located on the Tier 2 -- earning 6/10 -- because it decided to not comply with the Status checks defined rule. However, the repository uses the Branch Protection rule of required reviewers >= 2, which is a rule with strong security impact, and gets no score reward for that rule, because the rule is on Tier 4. See below the Scorecard Branch-Protection evaluation for this example:
{
  "date": "2023-06-05T11:21:24-03:00",
  "repo": {
    "name": "github.com/firebase/flutterfire",
    "commit": "81f1b80c445cfaab3e7752b8c58f35c92d234d8d"
  },
  "scorecard": {
    "version": "4.10.5",
    "commit": "27cfe92ed356fdb5a398c919ad480817ea907808"
  },
  "score": 6,
  "checks": [
    {
      "details": [
        "Info: 'force pushes' disabled on branch 'master'",
        "Info: 'allow deletion' disabled on branch 'master'",
        "Warn: no status checks found to merge onto branch 'master'",
        "Info: number of required reviewers is 2 on branch 'master'",
        "Warn: codeowner review is not required on branch 'master'"
      ],
      "score": 6,
      "reason": "branch protection is not maximal on development and all release branches",
      "name": "Branch-Protection",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/27cfe92ed356fdb5a398c919ad480817ea907808/docs/checks.md#branch-protection",
        "short": "Determines if the default and release branches are protected with GitHub's branch protection settings."
      }
    }
  ],
  "metadata": null
}
  1. If a repository can't comply with the rule of required reviewers >= 1 (a perfectly understandable use-case is a project of a solo-maintainer), it will be locked on the Tier 1 -- earning 3/10 -- and will not receive any score reward if it implements the Status checks defined, which is also a rule with strong security impact and it's the best a solo-maintainer can do as Branch-Protection effort.

Describe the solution you'd like
Further discussion might be required for the best solution to this, but an initial purpose is to remove the idea of Tiers and define that each Branch Protection rule have an independent value, and your score will be the sum of the scores of the rules you comply.

As an example, for an analysis without using the administrative access token, we could evaluate the score according to a table as this:

Branch Protection Rule Contribution to Branch Protection score
Prevent force push 1.5
Prevent branch deletion 1.5
Required reviewers >=1 3
Status checks defined 2
Required reviewers >= 2 2

With this configurations, the examples I gave above would have the following score changes:

  1. Score 6/10 (Tier 2) would become score 8/10, because the Required reviewers >= 2 rule would be considered.
  2. Score 3/10 (Tier 1) would become score 5/10, because the Status checks defined rule would be considered.
@diogoteles08 diogoteles08 added the kind/enhancement New feature or request label Jun 5, 2023
@spencerschrock
Copy link
Member

It seemed like there was a reason when this leveled scoring was introduced in #1287. Let me check with @laurentsimon regarding any consumers of these tiers.

@laurentsimon
Copy link
Contributor

laurentsimon commented Aug 18, 2023

the tiers were introduced because without them, scores were hard to disambiguate. A score of X could mean you do no code review and use pre-submit; or the other way around, which is different from a security's standpoint. The tiers were designed from a security point of view.

  1. At this current date, the...

They have a title pre-submit https://github.com/firebase/flutterfire/blob/master/.github/workflows/pr_title.yaml. They can make this one a status check and get the points for it. They don't need all the pre-submit to be present. One should suffice to pass

  1. If a repository ...

This one is a bit more unfortunate. Note that Status checks defined does not really provide security per-se, it's more about regression testing (which you could maybe argue in some cases is about security?)... so it's more like CI-Tests. I don't know if this situation would happen in practice: if there's no reviews, the main contributor will be pushing directly to main branch, so there's no branch protection and status check is not used. Have you seen repos in state (2)?

@spencerschrock
Copy link
Member

the tiers were introduced because without them, scores were hard to disambiguate.

So after structured results land, removing the tiers could be a possibility?

@laurentsimon
Copy link
Contributor

I suppose it could. Depends if some users still want to use scores for project comparisons.

@diogoteles08
Copy link
Contributor Author

diogoteles08 commented Aug 31, 2023

They have a title pre-submit https://github.com/firebase/flutterfire/blob/master/.github/workflows/pr_title.yaml. They can make this one a status check and get the points for it. They don't need all the pre-submit to be present. One should suffice to pass

Yes that makes sense, it should be easy for them to get the whole score for Tier 2. But I don't like the idea of requiring a exact predetermined posture or behaviour in order to give a good Scorecard score. I'd rather see Scorecard as a tool that is able to evaluate the security of a project independently of some specific choices. In other words, I don't think a maintainer should need to know the Scorecard rules to get a good score -- if their project follows good security measures, it should receive a good score already.

Requiring 2 reviewers is a very strong security measure per se (and also a huge time and resource investment), I believe Scorecard should value and recognize this effort independent of other requirements.

I don't know if this situation would happen in practice: if there's no reviews, the main contributor will be pushing directly to main branch, so there's no branch protection and status check is not used. Have you seen repos in state (2)?

Yeah... Unfortunately I couldn't find cases like this 😢 , even though I'd love to believe this would be the case of solo-maintainers with huge care on security and/or best practices. EDIT: I think it's may be the case of github.com/nghttp2/nghttp2. Although I couldn't confirm 100%, the maintainer is the only committer and he keeps opening PRs to merge their commits, without approval of anyone else.

@spencerschrock
Copy link
Member

With #3354 merged, I think the tier system (or having admin enforced in tier 1) is going to limit most repo rule scenarios to 2 points for tier 1.

Copy link

This issue is stale because it has been open for 60 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants