-
Notifications
You must be signed in to change notification settings - Fork 509
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
common: harden GitHub Actions #6113
base: master
Are you sure you want to change the base?
Conversation
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.
Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)
a discussion (no related file):
DAOS uses an empty permissions: {}
by default. Can we try if it also works for us, instead of 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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
a discussion (no related file):
Previously, grom72 (Tomasz Gromadzki) wrote…
DAOS uses an empty
permissions: {}
by default. Can we try if it also works for us, instead ofcontents: read
?
Such a submission disables all access, I'm not sure we can do that.
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.
Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)
a discussion (no related file):
Previously, osalyk (Oksana Sałyk) wrote…
Such a submission disables all access, I'm not sure we can do that.
If you do this you will have to add jobs.<job_id>.permissions
basically for all our jobs. The default permissions should be set according to what default makes sense in the project.
.github/workflows/main.yml
line 15 at r1 (raw file):
src_checkers: name: Source checkers runs-on: ubuntu-latest
contents: read
is required by all of the jobs in this file. Whereas issues: read
is required only by src_checkers
. Luckily we can make use of the semantics of this file and specify permissions
on both levels. Where the jobs.<job_id>.permissions
adds or removes access as necessary only for the given job.
Ref:
- https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#permissions
- https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idpermissions
Suggestion:
permissions:
contents: read
jobs:
src_checkers:
name: Source checkers
runs-on: ubuntu-latest
permissions:
issues: read
7978f5b
to
63e8359
Compare
63e8359
to
700ce7e
Compare
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
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.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)
a discussion (no related file):
Previously, janekmi (Jan Michalski) wrote…
If you do this you will have to add
jobs.<job_id>.permissions
basically for all our jobs. The default permissions should be set according to what default makes sense in the project.
I think that read permission is available in all our workflows:
The default permission is the one given by the project settings, and this is what permissions: {}
does - follow default.
https://github.com/pmem/pmdk/settings/actions
Workflow permissions
Choose the default permissions granted to the GITHUB_TOKEN when running workflows in this repository. You can specify more granular permissions in the workflow using YAML. Learn more about managing permissions.
-
Read and write permissions
Workflows have read and write permissions in the repository for all scopes. -
Read repository contents and packages permissions
Workflows have read permissions in the repository for the contents and packages scopes only.
Choose whether GitHub Actions can create pull requests or submit approving pull request reviews.
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
a discussion (no related file):
Previously, grom72 (Tomasz Gromadzki) wrote…
I think that read permission is available in all our workflows:
The default permission is the one given by the project settings, and this is whatpermissions: {}
does - follow default.https://github.com/pmem/pmdk/settings/actions
Workflow permissions
Choose the default permissions granted to the GITHUB_TOKEN when running workflows in this repository. You can specify more granular permissions in the workflow using YAML. Learn more about managing permissions.
Read and write permissions
Workflows have read and write permissions in the repository for all scopes.Read repository contents and packages permissions
Workflows have read permissions in the repository for the contents and packages scopes only.Choose whether GitHub Actions can create pull requests or submit approving pull request reviews.
Following our offline conversation: we realized the actual role of the contents
permission. It seems not to serve the purpose we thought initially. So, permissions: {}
in place of contents: read
is sufficient for our purposes with a few exceptions.
This change is