-
Notifications
You must be signed in to change notification settings - Fork 504
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
✨ Structured results for permissions #2584
Conversation
Still need to go through this, but WDUT about using the new extended-json format to implement #2577. Rules could also have numeric IDs? A new results format would be a good time to add it since backwards compatibility isn't a concern. |
The rule names are essentially ruleID that are more explicit than numbers. I can definitely add a number, but we have to be sure we don't get collision, ie we need some proper pre-submit to verify this over time. (at least in the current implementation because rule files are stored in a folder named after the check, here |
Regarding the ...
"remediation": {
"text": "Update your workflow using https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions",
"markdown": "Update your workflow using [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions).",
"effort": "Low"
}
... Is it possible to also suggest a direct remediation instruction? It has happened that a maintainer didn't like the tool and got confused about which options to enable to fix the check and though the remediation was also saying to enable Harden Runner which he didn't seem to want. Maybe with a remediation like "Update your workflow by adding In this case, for example, the https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions does not fix the In the print above I've selected the pin dependencies just to show that the changes should be applied. |
Thanks for the feedback. One issue of asking users to fix the top-level themselves is that some of the steps may require write permissions, so their workflow would break after an update. If I update the remediation to explicitly ask to users to untick harden runner and pinned deps, would it be OK? |
qq:
|
@varunsh-coder there seems to be a bug in the webpage: the |
ae53e6e
to
907ed19
Compare
I think it would still be fully dependent of the tool, but it would be much better because it will be clear to the user what he was supposed to use in the tool to fix the check. Something like Update your workflow using the "Restrict permissions for GITHUB_TOKEN" at <link> Or maybe adding the "fix it yourself as a turn around" with something like: Update your workflow using <link> or set the permission default to read (`contents: read`) giving write permissions only to jobs Both options I think would be great. |
That's very helpful, thanks. I realize I did not explain well. Can you take a look at the changes I made? You can edit the PR yourself as well by using the branch on my repo, so feel free to make additional changes you think would be meaningful. Let me know once you've made the changes. Thanks! |
checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml
Outdated
Show resolved
Hide resolved
checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml
Outdated
Show resolved
Hide resolved
@laurentsimon as of now, if a workflow already has permissions defined at the top level, we do not change it. We just give a message that it already has permissions. I can take an action item to re-evaluate the permissions if they are present and suggest better permissions, if possible. Also, regarding checkboxes, we can come up with a way to only show the option as per the query string, e.g. for token permissions, only show that and not the other boxes. That might be better than adding documentation to not select other options. We have also added more remediations via pull-request, e.g. to add dependabot config and CodeQL. Should we discuss this remediation experience in one of the upcoming Scorecard community meetings? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some improvements/fixes to the steps write permission response.
checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml
Outdated
Show resolved
Hide resolved
checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml
Outdated
Show resolved
Hide resolved
checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml
Outdated
Show resolved
Hide resolved
ca0a1e0
to
da13991
Compare
Integration tests success for |
Integration tests success for |
* update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> --------- Signed-off-by: laurentsimon <laurentsimon@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Co-authored-by: Joyce <joycebrumu.u@gmail.com>
* update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> --------- Signed-off-by: laurentsimon <laurentsimon@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: Shofiya2003 <shofiyabootwala@gmail.com>
* update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> --------- Signed-off-by: laurentsimon <laurentsimon@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: Shofiya2003 <shofiyabootwala@gmail.com>
* update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * Update checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml Co-authored-by: Joyce <joycebrumu.u@gmail.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> --------- Signed-off-by: laurentsimon <laurentsimon@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Co-authored-by: Joyce <joycebrumu.u@gmail.com>
This PR is the first of a series to implement the structured results. It starts with the permission check.
This PR is very large because it includes not just the permission check's changes, but also all the needed changes to incorporate the structured results. Fortunately, the majority of files changes are small fixes to change package names.
The main changes to review:
rule/
package. This takes a directory with a list of *.yml files. Each yml file contains the information for a rule.finding
is a finding. It uses therule
to create a finding, and additional information such as a location, snippet, etc. provided by a check.This is eventually displayed to userspkg
is updated to use the structured resultsextended-json
for the--format
option is used. TheSCORECARD_EXPERIMENTAL=1
must be set to enable it.An example result looks like the following:
SCORECARD_EXPERIMENTAL=1 go run . --repo=GoogleCloudPlatform/rad-lab --format extended-json --show-details --checks Token-Permissions | jq
There are several TODOS in the code which I will create a tracking issue for once the review is complete.