-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add token permissions for code-scanning/trivy.yml #1348
Conversation
code-scanning/trivy.yml
Outdated
permissions: read-all | ||
|
||
jobs: | ||
build: | ||
permissions: | ||
contents: read # for actions/checkout to fetch code | ||
security-events: write # for github/codeql-action/upload-sarif to upload SARIF results |
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.
Hi, thanks for the PR!
Could you explain what you're aiming for with these changes? It seems that the top-level permissions statement is superfluous given the more specific statement applied to the build
job...?
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.
Hi @nickfyson, @knqyf263 these changes are part of #1299. @Devils-Knight and others from StepSecurity are fixing token permissions across the starter workflow templates, so they are secure by default. We have also setup a website https://app.stepsecurity.io that developers can use to set token permissions for their workflows.
@nickfyson to answer your question, ossf/scorecards recommends adding workflow level permissions, so that if developers add a new job to the workflow, it inherits the read-all
permissions, and is secure-by-default.
@knqyf263 to answer your question, if job level permissions are granted, then any permission not specified is not granted to the job. So, if contents: read
is removed, the build
job will not have contents-read
permission.
CC: @laurentsimon
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.
Ah, thanks for the context. 🙂
How come read-all
though? Would it be even better to just give read to contents by default and nothing else?
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.
@nickfyson yes, we got that feedback from multiple developers, and have updated the logic on https://app.stepsecurity.io/. This PR was created before that change though. @Devils-Knight can you please update this PR so that the workflow level permissions are set to contents: read
?
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.
@varunsh-coder Thanks for explaining the changes 😇. Have updated the workflow level permission as requested by you please check it out.
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.
@varunsh-coder Thanks for explaining the changes 😇. Have updated the workflow level permission as requested by you please check it out.
LGTM
jobs: | ||
build: | ||
permissions: | ||
contents: read # for actions/checkout to fetch code |
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.
Hi, I'm a maintainer of Trivy. I'd like to know why this line is needed. Looks like read-all
is granted in the top-level permissions.
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.
Good question. If you define permissions in the job, those you do not specify default to none
, not to the ones defined at the top level. The top level permissions only serve as a fail-safe mechanism if a developer ever adds a new job and forget to set permissions explicitly... this happens :-)
See the doc in https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idpermissions If you specify the access for any of these scopes, all of those that are not specified are set to none
It was confusing to me a first as well.
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.
Given the additional explanation I'm happy with these changes, and can appreciate the value of setting the global permissions also.
@knqyf263 As one of the tool maintainers, are you happy with these changes? If so I shall 🙂
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.
This PR adds token permissions for
code-scanning/trivy.yml
. The permissions were added using https://app.stepsecurity.io/.Pre-requisites
Please note that at this time we are only accepting new starter workflows for Code Scanning. Updates to existing starter workflows are fine.
Tasks
For all workflows, the workflow:
.yml
file with the language or platform as its filename, in lower, kebab-cased format (for example,docker-image.yml
). Special characters should be removed or replaced with words as appropriate (for example, "dotnet" instead of ".NET").For CI workflows, the workflow:
ci
directory.ci/properties/*.properties.json
file (for example,ci/properties/docker-publish.properties.json
).push
tobranches: [ $default-branch ]
andpull_request
tobranches: [ $default-branch ]
.release
withtypes: [ created ]
.docker-publish.yml
).For Code Scanning workflows, the workflow:
code-scanning
directory.code-scanning/properties/*.properties.json
file (for example,code-scanning/properties/codeql.properties.json
), with properties set as follows:name
: Name of the Code Scanning integration.organization
: Name of the organization producing the Code Scanning integration.description
: Short description of the Code Scanning integration.categories
: Array of languages supported by the Code Scanning integration.iconName
: Name of the SVG logo representing the Code Scanning integration. This SVG logo must be present in theicons
directory.push
tobranches: [ $default-branch, $protected-branches ]
andpull_request
tobranches: [ $default-branch ]
. We also recommend aschedule
trigger ofcron: $cron-weekly
(for example,codeql.yml
).Some general notes:
actions
organization, or