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.

Sign up for GitHub

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

Jump to bottom

📖 Update Token-Permissions requirement in checks.yaml #1130

Closed
wants to merge 3 commits into from
Closed

📖 Update Token-Permissions requirement in checks.yaml #1130

wants to merge 3 commits into from

Conversation

varunsh-coder
Copy link
Contributor

@varunsh-coder varunsh-coder commented Oct 13, 2021 •
edited
Loading

  • Please check if the PR fulfills these requirements

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Docs update - to change the requirement for the token permissions check.

  • What is the current behavior? (You can also link to an open issue here)
    Relax token permission check or not #1129

  • What is the new behavior (if this is a feature change)?
    New logic is in the PR

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    It will cause the score to increase for some repos.

  • Other information:
    The logic for this change is the following:

  1. If a developer has set explicit permissions for the token either at the workflow (top-level) or for each of the jobs (run-level), and it is not set to write-all, then at least some amount of effort has gone into deciding what the permissions should be.
  2. Calculating the exact permissions that the token should have is a hard problem and depends on the various different commands and actions being used in the workflow. Therefore, it may be best to check if a best effort has been put into deciding the permissions, rather than checking if the permissions are the exact minimum needed for each job. Otherwise it may happen that the assessment tool (Scorecards) miscalculates the minimum permissions, and as a result the developer may get an unfair score.

The proposed change is a draft, and I expect it to evolve based on discussions...

@varunsh-coder varunsh-coder mentioned this pull request Oct 13, 2021
Closed
@laurentsimon laurentsimon self-requested a review October 13, 2021 22:43
@laurentsimon
Copy link
Contributor

@olivekl any comment?

laurentsimon
laurentsimon reviewed Oct 13, 2021
View reviewed changes
docs/checks/internal/checks.yaml
GitHub, and does not support other source hosting repositories (i.e., Forges).

Setting token permissions to read-only follows the principle of least privilege.
Setting token permissions to the minimum needed follows the principle of least privilege.
This is important because attackers may use a compromised token with write
access to push malicious code into the project.

The highest score is awarded when the permissions definitions in each workflow's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is actually not correct @olivekl
Currently we remove points from the score if permissions are "too broad". This is prone to false positive, though #1099. Maybe we'll need to change the score calculation in the future

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this is not in the scope of this PR? If so, let's discuss this in a different PR or issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this. The new description LGTM.

azeemshaikh38
azeemshaikh38 reviewed Oct 14, 2021
View reviewed changes
docs/checks/internal/checks.yaml
@@ -579,19 +579,20 @@ checks:
description: |
Risk: `High` (vulnerable to malicious code additions)

This check determines whether the project's automated workflows tokens are set
to read-only by default. It is currently limited to repositories hosted on
This check determines whether permissions for the project's automated workflows tokens are set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: change to whether token permissions for the project's automated workflows are set ... for better readability.

azeemshaikh38
azeemshaikh38 approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@azeemshaikh38 azeemshaikh38 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@laurentsimon
Copy link
Contributor

laurentsimon commented Dec 8, 2021 •
edited
Loading

Closing this PR: the code calculation based on the top-level vs run-level permission will be merged in soon thru #1356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivekl olivekl olivekl left review comments

@laurentsimon laurentsimon laurentsimon left review comments

@azeemshaikh38 azeemshaikh38 azeemshaikh38 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@varunsh-coder @laurentsimon @azeemshaikh38 @olivekl