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

ci: Add minimum GitHub token permissions for workflows #22786

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

varunsh-coder
Copy link
Contributor

Description

This PR adds minimum token permissions for the GITHUB_TOKEN in GitHub Actions workflows using https://github.com/step-security/secure-workflows.
All GitHub Actions workflows have a GITHUB_TOKEN with write access to multiple scopes.
Here is an example of the permissions in one of the workflows:
https://github.com/zulip/zulip/runs/8006109927?check_suite_focus=true#step:1:19

After this change, the scopes will be reduced to the minimum needed for the workflows.

Motivation

Signed-off-by: Varun Sharma varunsh@stepsecurity.io

Self-review checklist

  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@PIG208
Copy link
Member

PIG208 commented Aug 26, 2022

@varunsh-coder thank you for submitting the PR! Could you squash these two commits and removed the commit message in the second one?

Signed-off-by: Varun Sharma <varunsh@stepsecurity.io>
@varunsh-coder
Copy link
Contributor Author

@varunsh-coder thank you for submitting the PR! Could you squash these two commits and removed the commit message in the second one?

Done!

@PIG208

This comment was marked as resolved.

@zulipbot zulipbot added the integration review Added by maintainers when a PR may be ready for integration. label Aug 27, 2022
@timabbott
Copy link
Member

timabbott commented Aug 29, 2022

Thanks for the explanation and pull request @varunsh-coder! This is clearly an improvement, but I'm curious whether it'd be better to just set the default token permissions to "restrictive" for the repository, as documented here, rather than doing overrides to reduce permissions in individual workflows:

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

It seems like that would protect us better if future workflows are added. (I think we'd still need the "security events" hunk as that's an increase in permissions?)

@timabbott
Copy link
Member

The Open Source Security Foundation (OpenSSF) Scorecards also treats not setting token permissions as a high-risk issue. This change will help increase the Scorecard score for this repository.

Is there a way to run this scorecard without installing their GitHub Action?

@varunsh-coder
Copy link
Contributor Author

Thanks for the explanation and pull request @varunsh-coder! This is clearly an improvement, but I'm curious whether it'd be better to just set the default token permissions to "restrictive" for the repository, as documented here, rather than doing overrides to reduce permissions in individual workflows:

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

It seems like that would protect us better if future workflows are added. (I think we'd still need the "security events" hunk as that's an increase in permissions?)

Hi @timabbott, you can also set the default token permissions to "restrictive" for the repository, and yes that will help with future workflows as well.

Setting in the workflow file has couple of advantages:

  1. Tools like Scorecards cannot access the setting in the repository (there is no API for it), so assume that the workflows have not set minimum permissions.
  2. If someone forks the repo, or ends up re-using the workflow in a different repo, the permissions go along if present in the workflow file. The repo settings are not copied over when forked.

Yes, the security-events: write will need to be set explicitly.

Please let me know whatever you decide and if you want me to update the PR. Thanks!

@varunsh-coder
Copy link
Contributor Author

The Open Source Security Foundation (OpenSSF) Scorecards also treats not setting token permissions as a high-risk issue. This change will help increase the Scorecard score for this repository.

Is there a way to run this scorecard without installing their GitHub Action?

Yes, you can run it using the CLI. https://github.com/ossf/scorecard#installation

For some repositories (what publish a package), the scores are also available at https://deps.dev

@timabbott timabbott merged commit 6cdf285 into zulip:main Aug 30, 2022
@timabbott
Copy link
Member

Merged, thanks for the contribution and information @varunsh-coder!

@timabbott
Copy link
Member

Also posted https://chat.zulip.org/#narrow/stream/43-automated-testing/topic/GitHub.20token.20permissions/near/1427444; we'll want to do something similar for our other major repositories like zulip/zulip-mobile.

timabbott pushed a commit that referenced this pull request Aug 30, 2022
This limits the ability for an Action to do mischief with this token.

Fixes #22786.

Signed-off-by: Varun Sharma <varunsh@stepsecurity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when a PR may be ready for integration. size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants