-
Notifications
You must be signed in to change notification settings - Fork 503
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
Token-Permissions check clarification #1128
Comments
I agree with this. @laurentsimon @azeemshaikh38 thoughts?
That would be cool! |
we can definitely do that.
That would be awesome! Long-term, scorecard could provide a diff or new files that users can use to create PRs. A user, in this case, could even be a GitHub app, like https://github.com/ossf/allstar which could submit PRs automatically. For this, we'd need a library or a REST API. CLI/web is a great first step, though! Do you have some idea around false negatives/positives for your app, i.e. permissions needed but not displayed by your app; and permissions not needed but that your app displays? If your app has the right logic with no false positives/false negatives, then we could think of replacing scorecard's logic with yours, if it can work without privileged tokens like Let's start with what you proposed: doc and update of our logic. Submit a PR for the doc update. I've created #1128 for tracking the relaxation of our check's logic. Thanks so much for reaching out! |
The app cannot calculate the minimum permissions for all cases. It is essentially based on a knowledge base of permissions needed by each action. So, it is not perfect, but is fairly accurate, and as the knowledge base grows, it gets better at coverage. I will share preview of the website in few days. It is backed by an API...
I just submitted a PR for the doc/ requirement update. #1130. Do share your feedback.
Thanks for the quick response on this. Looking forward to collaborating in the future! |
Thanks @varunsh-coder for reaching out. As @laurentsimon and @naveensrinivasan have already mentioned, we (Scorecard) needs to update the way |
I've thought a bit more about this. I agree that |
Hi @laurentsimon I could try this out and get some user feedback. Based on the PRs I submitted on various repos, I found developers to be:
Another thing to consider is how GitHub itself adds permissions to default workflows, e.g. I recently added a CodeQL workflow via the GitHub UI and it did not have read-all at the top, but had permissions at the job level. I think this decision is not an urgent one, and as we observe more best practices for token permissions and get feedback, it can help shape the decision. BTW, the Step Security web UI for adding permissions to workflows is available here. I will start to pilot it with few users. Feel free to give it a try. I am also working on an agent for GitHub hosted runner (being piloted here and here). Would love to pilot it on Scorecards repository so I can get feedback from you. Let me know if that is ok. |
valid point. GitHub does ask users to define global permissions for workflows, but currently only suggest the settings (https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/). So they agree that it should fail safe. I would love for them to tell users to use a config file instead of the setting.
how about the following:
This is great, thank you for sharing! cc @josepalafox |
I believe your concern is around the scenario where a new job is added to an existing workflow with no top-level permissions, where all existing jobs had minimum job-level permissions, and that new job does not have permissions defined. In this case if Scorecards had influenced the repo owner to add permissions for the rest of the jobs in the first place, it is likely the developer would run Scorecards again and get a lower score, and this will cause them to address permissions for the new job. I think what is going to influence developers to fix their permissions is not the top-level fail-safe permission, because they can always remove it, and will likely do so if their new job fails due to lack of permissions. What is going to influence them is an external factor, which is to get a good score from Scorecards. So, I think you don't have to worry about your concern. If each job has min permissions or top level has min permissions, the workflow should get a score of 10. If not, it should get a lower score. This lower score will cause the developers to fix the permissions either on top level or job level. This approach also gives developers flexibility on how to address the issue - either at top level or job level - since both are technically valid options. My thinking would have been different if the top-level permission were like a policy that is controlled by the admin, but that is not the case. It is just text in the workflow file. |
Thanks for sharing your thoughts, @varunsh-coder
correct.
correct. it's this risky time window that the top-level permission mitigates. We don't how often users will run scorecard. If they run it on all pull requests (we're hoping some will be able to do that with the GitHub action we're writing), then a missing permission would be caught right away. And I'd 100% agree with you that the top-level permission is redundant. But that may not be the case for everyone.
correct. That's why I was suggesting we can influence developers to add the top-level |
Ok, that's fair. I will take an action item to add top-level permissions for the Step Security feature to auto-add permissions. I can pilot the GitHub action for Scorecards, let me know if it is in a state to pilot it. It might also help to have a badge for Scorecard that developers can add to their repos - I don't know if this is already in the plans. |
awesome. I really appreciate that you forced me to explain my thinking. If you have further thoughts, please don't hesitate to share them.
that's great. What exactly do you mean by piloting?
It's in the plans for next year, yes. We'll share a design via the ossf-scorecard-dev mailing list and in an issue once we have the design better thought of. If you have particular ideas about it, feel free to share them in the |
By piloting, I meant I could try out the GitHub Action for Scorecards when it is ready. You had mentioned it as a way to run Scorecards on Pull request and I was looking for a way to do that. My comment on piloting was not related to this particular check/ issue... |
sounds good. We have a working prototype: follow the steps in #1074 (comment) We will be changing the repo name of the GitHub action to comply with GitHub marketplace convention in 3 weeks or so from now. At this point it will become obvious that the change happens because the runs will fail. Ping me on this thread and I'll tell you the new name. The change should be reflected in this line https://github.com/ossf/scorecard/blob/main/.github/workflows/scorecard-analysis.yml#L25 anyway. FYI, I'll be OOO from Nov 23rd to Dec 1 included. |
FWIW top level permissions aren't redundant at all as it was seemingly implied in this thread. They protect forks where the "token permissions" setting has the default value. For example, the systemd repository set the global setting to "read-only" and ships GH Actions with "permissions: contents: read" at the top level to prevent its forks from running into any issues just because obviously nobody can change all the forks (apart from GitHub but it's unlikely they're going to do that mostly for compatibility reasons) |
I tried the Scorecards GitHub Action here. For some reason, GitHub does not render the SARIF results in the Code scanning alerts section. I was able to download and view the SARIF results from workflow artifacts. Results looked as expected for the checks it did show. Do have a look as I do not have knowledge of all checks that Scorecards does... |
Hi @evverx Sorry I could not understand your comment. Are you saying setting at top level is better relative to job level in case someone forks the repo? Did not understand the threat scenario... |
I think what @evverx means is that if a repo has GitHub settings set to "workflows are by default using read-only"; then someone clones it but does not have the same settings set, the permissions will default to write for the clone repo. Is that correct? |
If I'm not mistaken, you ran scorecard only on PRs and not on push/cron events. GitHub will only add results to the scanning dashboard for the push to a branch. I think you may be able to see the results if you search for the PR branch in the dashboard, but they are not shown by default because it's not the default branch. Can you try run scorecard on a push? |
Ran it on push and see the results in the code scanning dashboard now. Not sure if you will have access to view the results though...It will take me some time to review each result for accuracy. Will spend time later today on it. |
I did a quick analysis and have posted results here. Feel free to comment on that issue to discuss the results. W.r.t token permissions, I am curious what the code scanning alert/ SARIF output would be after the change is made to the logic as discussed in this thread. Let us say a workflow does not have top level permissions, but each job has permissions, will it create a code scanning alert? If so what severity? Or will it not create an alert, but give a score of 9. |
That's a really good question. Today, we use the risk level from the https://github.com/ossf/scorecard/blob/main/docs/checks/internal/checks.yaml#L589 In this case, it would be tagged as We originally did not implement this level of granularity to keep things simple, but you're right it's going to be needed. I'm not sure we'll be able to implement this for the first release, but we can add it in the next iteration. Is that OK? Thanks so much for flagging this! |
I have created a tracking issue for it. Feel free to update/ add comments in case my understanding/ terminology is not correct. Thanks! |
I am the founder of Step Security, a security startup to stop software supply chain attacks. Step security has built an App to automatically set the right token permissions for a given GitHub workflow.
During testing, I created PRs in several repos. Unfortunately, they are all getting a score of 0, even though they meet the least privilege requirement, since permissions are being set at the run (job) level.
Here are some examples:
https://github.com/flutter/flutter/blob/master/.github/workflows/lock.yaml
https://github.com/Azure/template-analyzer/blob/main/.github/workflows/codeql-analysis.yml
https://github.com/mgdm/htmlq/blob/master/.github/workflows/build.yml
The current requirement for the token permissions check in Scorecard is for the top (workflow) level permission to be set.
I would like the fixes done by the Step Security App to be inline with the Scorecard criteria.
Can you please clarify why the token permissions check needs the permission to be set at the top level?
Given that it is possible to write a workflow that follows the principle of least privilege by setting permissions at run (job) level, would you be willing to change the requirement to check if permissions are set at each run (job) level? Otherwise developers may need to add an extra line at the top level to meet the Scorecard criteria.
On a related note, I am planning to open source the Step Security App and create a web/ command line interface to make it easier to use. Would love to have the App be listed as a remediation option for the token permissions check. That way developers will find it easier to get a higher Scorecard score. Thanks!
The text was updated successfully, but these errors were encountered: