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

BUG: High-risk write permissions only punished at top-level, low-risk permissions punished everywhere #3045

Closed
pnacht opened this issue May 22, 2023 · 3 comments · Fixed by #3162
Assignees
Labels

Comments

@pnacht
Copy link
Contributor

pnacht commented May 22, 2023

Describe the bug
Scorecard currently only punishes the high-risk permissions contents/packages/actions: write if they are at the top-level. At the job-level, a warning is shown but no score penalty is applied.

However, all the other lower-risk permissions (i.e. checks) apply a score penalty wherever they are.

Reproduction steps
Steps to reproduce the behavior:

  1. See https://github.com/ossf/scorecard/blob/main/checks/evaluation/permissions/permissions.go#L325-L374
  2. Note that all low-risk permissions are checked with permissionIsPresent while high-risk permissions use permissionIsPresentInTopLevel.

Expected behavior
The current situation is the opposite of what'd be expected: high-risk permissions should be punished more severely than low-risk permissions (or they should be punished equally).

So, I'd expect either:

  1. all write permissions are always punished, regardless of where they are defined. However, this goes against the conclusion in Explicitly allowed action permissions? #2338.
  2. all write permissions should be forgiven at the job-level. This seems to be closest to the spirit of Explicitly allowed action permissions? #2338.
  3. all permissions have two levels of punishment: a high value if set at the top-level, a low value if set at the job-level. (The values don't have to be the same for all permissions).

(2) seems closest to the spirit of #2338, but I'd also see value in (3), with something like:

permission Penalty
@ top-level
Penalty
@ job-level
High-risk (contents,
packages, actions)
-10 -1
Low-risk (other) -1 0

This would somewhat capture the risk of adding high-risk permissions even if only at the job-level (which could be further reduced to no loss if hash-pinned, as suggested by #3022).

@pnacht pnacht added the kind/bug Something isn't working label May 22, 2023
@pnacht
Copy link
Contributor Author

pnacht commented Jun 12, 2023

If there's interest in this, feel free to assign it to me. Unless someone suggests otherwise, I'd move forward with (2), forgiving all write permissions set at job level.

@spencerschrock
Copy link
Member

Scorecard currently only punishes the high-risk permissions contents/packages/actions: write if they are at the top-level. At the job-level, a warning is shown but no score penalty is applied.

However, all the other lower-risk permissions (i.e. checks) apply a score penalty wherever they are.

Agree this is paradoxical, and came about because the higher-risk permissions were more broadly used (and therefore got more complaints about the penalty), leading to #2338 .

I have no problem with 2.

@heitorlessa
Copy link

Just hit this when implementing scorecard in our project: https://github.com/aws-powertools/powertools-lambda-python/blob/3f2498545a5f207bff7fc283956235d99858b306/.github/workflows/release-drafter.yml#L28

In this instance, we need contents: write only to create a release in draft mode and keep it up to date.

In other cases, we use contents: write to create temporary branches + create a PR from trusted workflows like building a changelog on every merged PR -- this allows us to keep a protected branch (trunk) and yet allow automation to keep us safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants