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

Explicitly allowed action permissions? #2338

Open
justinabrahms opened this issue Oct 7, 2022 · 22 comments · Fixed by #2355
Open

Explicitly allowed action permissions? #2338

justinabrahms opened this issue Oct 7, 2022 · 22 comments · Fixed by #2355
Assignees
Labels
kind/enhancement New feature or request Stale

Comments

@justinabrahms
Copy link

Hello.

I'm trying to max out my score for the scorecard, especially in the security section. One of the issues I'm running into is that I use the google release-please action to cut releases and then push those tags back into the repo. Currently, there is no mechanism to explicitly mark that action as "safe".

I would like to check in a yaml file into my repository which says "I really do trust the following actions" and not have to specify a sha / version.

Alternatively, we can update your tool to also say "release-please is safe".

@justinabrahms justinabrahms added the kind/enhancement New feature or request label Oct 7, 2022
@justinabrahms justinabrahms changed the title Explicitly allowed actions? Explicitly allowed action permissions? Oct 7, 2022
@eddie-knight
Copy link
Contributor

Facing the same issue, though I have two actions that were caught by the same check: one from helm, and one from a community creator.

Perhaps we can publish a vetting policy to add trusted actions to the allowlist? This policy should clearly articulate what standard an action is being held to, so that PRs can be made by folks such as Justin & myself to add such actions to the allowlist.

@varunsh-coder
Copy link
Contributor

varunsh-coder commented Oct 7, 2022

I think the token permission check should not ding for using explicit write permission at job level.

If one uses write-all then it is fine to ding the score.

But the use of a specific write permission at job level means there is atleast some effort gone into deciding the permission.

I have similar issue for some of our workflows, where the token permission score is 0, even though the permissions are least privilege.

Given that there are over 10K GitHub Actions in the marketplace, I don't think it is feasible to create a "safe" list and keep it updated. Instead my recommendation is that if there is read at top level and explicit write at job level, it should be a 10.

@naveensrinivasan
Copy link
Member

I think the token permission check should not ding for using explicit write permission at job level.

If one uses write-all then it is fine to ding the score.

But the use of a specific write permission at job level means there is atleast some effort gone into deciding the permission.

I have similar issue for some of our workflows, where the token permission score is 0, even though the permissions are least privilege.

Given that there are over 10K GitHub Actions in the marketplace, I don't think it is feasible to create a "safe" list and keep it updated. Instead my recommendation is that if there is read at top level and explicit write at job level, it should be a 10.

I like this idea. @ossf/scorecard-maintainers Thoughts?

@spencerschrock
Copy link
Member

I don't think it is feasible to create a "safe" list and keep it updated

+1 maintaining an allowlist doesn't scale. And if the only criterion to being added to the list is sending a PR, then the score difference ends up measuring that effort instead of any security posture.

I can see the original intent, as some write permissions are more dangerous than others. If we remove the score penalty, I think there is still value to having a warning in the output.

@eddie-knight
Copy link
Contributor

The code is in place right now for whitelisting. We can scrap that code and move to a wholesale "check it yourself" if we want, but I'm not in a position to make that decision.

Whatever we do, it's somewhat time sensitive because we have several CNCF project working on the Security Slam now through next week. It would be really nice to allow some of these projects to get to 100% without scrapping their existing workflows (unless we do find something insecure).

My proposal is that we create a simple docs entry outlining the process for getting an action approved (or just a link to this doc), then encourage Scorecard users to create an issue and/or PR with evidence to accelerate vetting from a code reviewer.

@azeemshaikh38
Copy link
Contributor

I'm ok with scrapping the allowlist and simply showing a warn without a deduction in points. I think that's majority votes (me, Spencer and Naveen) to scrap this. So ok to proceed here.

If anyone wants to send a PR for this happy to get this reviewed and merged.

@eddie-knight
Copy link
Contributor

I'm looking into this today, with an aim to add a check warning to jobs that contain write permission without deducting points.

@eddie-knight
Copy link
Contributor

eddie-knight commented Oct 15, 2022

Created the PR linked above to specifically address jobs using contents:write, since that is the more widespread usage in jobs.

I noticed that Chaos Mesh is using packages: write and there are likely more cases I haven't noticed. Do we want to remove or reduce the penalty for that as well? Do we want to do this for all job-level activities?

naveensrinivasan added a commit to ossf/scorecard-action that referenced this issue Oct 17, 2022
- New release to address the issue ossf/scorecard#2338

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan added a commit to ossf/scorecard-action that referenced this issue Oct 17, 2022
- New release to address the issue ossf/scorecard#2338

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan added a commit to ossf/scorecard-action that referenced this issue Oct 17, 2022
- New release to address the issue ossf/scorecard#2338

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan added a commit to ossf/scorecard-action that referenced this issue Oct 18, 2022
- New release to address the issue ossf/scorecard#2338

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
@ytsarev
Copy link

ytsarev commented Oct 18, 2022

We also encountered the need for explicit actions:write here in context of k8gb-io/k8gb#970

We need this as a requirement for https://github.com/fkirc/skip-duplicate-actions#usage-examples

@eddie-knight
Copy link
Contributor

Added a PR to address Chaos Mesh & K8gb requirements... recommend reopening this issue if we need more discussion on the topic.

@varunsh-coder
Copy link
Contributor

I think the same change should be made for all other permission scopes as well e.g security-events, checks etc.

@ytsarev
Copy link

ytsarev commented Oct 19, 2022

Looks like we also need pull-requests: write on a job level https://github.com/k8gb-io/k8gb/blob/master/.github/workflows/changelog_pr.yaml#L15

@evverx
Copy link
Contributor

evverx commented May 7, 2023

Looks like this issue hasn't been fully addressed: systemd/systemd#25042 (comment)

@spencerschrock
Copy link
Member

Thanks for the updates, it seems only the contents and packaging had scoring changes despite some talk among the maintainers for any job level permission. I can take a look

@spencerschrock spencerschrock reopened this May 8, 2023
@spencerschrock spencerschrock self-assigned this May 8, 2023
@evverx
Copy link
Contributor

evverx commented May 8, 2023

it seems only the contents and packaging had scoring changes despite

I'm not sure "contents" works. As far as I can see scorecard flags the explicit "contents: write" part in https://github.com/systemd/systemd/blob/main/.github/workflows/make_release.yml (with its top-level read permissions backed by the global read-only setting):
Screenshot 2023-05-08 at 21 30 39.

@spencerschrock
Copy link
Member

I will need to double check how the code-scanning reports are generated then. As we still Warn on things like this, which probably makes it to the code-scanning alerts.

@evverx
Copy link
Contributor

evverx commented May 8, 2023

As we still Warn on things like this, which probably makes it to the code-scanning alerts

It seems to be the case. What's weird though is that scorecard says that their severity is medium:

"Warn: Medium severity: jobLevel 'security-events' permission set to 'write': .github/workflows/differential-shellcheck.yml:22: Verify which permissions are needed and consider whether you can reduce them. (High effort)",
"Warn: Medium severity: jobLevel 'contents' permission set to 'write': .github/workflows/make_release.yml:17: Verify which permissions are needed and consider whether you can reduce them. (High effort)",

but the alerts end up being "high severity" in the dashboard but it's probably a separate issue.

@evverx
Copy link
Contributor

evverx commented May 11, 2023

@spencerschrock looking at #2991 I think it would be really nice if the scorecard project can decide what it's trying to accomplish. If scorecard has the resources to actually continuously audit stuff and decide what is safe I think it should go with #2991.

@evverx
Copy link
Contributor

evverx commented May 23, 2023

As we still Warn on things like this, which probably makes it to the code-scanning alerts.

I guess once it's decided where dashboards should officially live it should be possible to get rid of those alerts by just cutting off the SARIF part of the scorecard action. Without dashboards alerts kind of help to make sense of the scorecard output but with dashboards where the results are beautifully displayed those alerts popping up over and over again are pointless. (The SARIF part would make sense though if the action could be run on PRs but I don't think it's ever going to happen).

@github-actions
Copy link

Stale issue message - this issue will be closed in 7 days

Copy link

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

Copy link

github-actions bot commented May 3, 2024

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

@github-actions github-actions bot added the Stale label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request Stale
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

9 participants