Skip to content

chore: Set permissions for GitHub actions#2400

Merged
bsipocz merged 2 commits into
astropy:mainfrom
turrisxyz:setup-permissions
May 23, 2022
Merged

chore: Set permissions for GitHub actions#2400
bsipocz merged 2 commits into
astropy:mainfrom
turrisxyz:setup-permissions

Conversation

@naveensrinivasan

Copy link
Copy Markdown
Member

@codecov

codecov Bot commented May 8, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2400 (0a145f3) into main (2922473) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2400   +/-   ##
=======================================
  Coverage   62.96%   62.96%           
=======================================
  Files         133      133           
  Lines       17287    17287           
=======================================
  Hits        10885    10885           
  Misses       6402     6402           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

permissions:
actions: read # for github/codeql-action/init to get workflow details
contents: read # for actions/checkout to fetch code
security-events: write # for github/codeql-action/autobuild to send a status report

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Based on the log the security events still seem to have read permissions only. Though the rest was indeed set to none. Is this an upstream bug or I misunderstand something?

GITHUB_TOKEN Permissions
  Actions: read
  Contents: read
  Metadata: read
  SecurityEvents: read

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AFAIK, the SecurityEvents: write is required for writing events. Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that's the problem here, I still see it as "read" rather than "write" in the logs for this PR:

https://github.com/astropy/astroquery/runs/6469748769?check_suite_focus=true#step:1:20

@bsipocz

bsipocz commented May 17, 2022

Copy link
Copy Markdown
Member

Thank you @naveensrinivasan. The PR is certainly an improvement of the current situation, I'll go ahead and merge, but first would like to understand one minor point.

 Restrict the GitHub token permissions only to the required ones; this way, even if the attackers will succeed in compromising your workflow, they won’t be able to do much.

- Included permissions for the action. https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions

https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

[Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/)

Signed-off-by: naveen <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
@naveensrinivasan

Copy link
Copy Markdown
Member Author

Thank you @naveensrinivasan. The PR is certainly an improvement of the current situation, I'll go ahead and merge, but first would like to understand one minor point.

Thanks! Took care of the conflicts!

@bsipocz

bsipocz commented May 23, 2022

Copy link
Copy Markdown
Member

OK, so let's go ahead with this. I suppose that write access issue is an upstream bug anyway, as what you did is following the docs, and that it's not being pickedup properly is a GHA issue than an astroquery.

Thank you!

@bsipocz bsipocz merged commit 7b289e7 into astropy:main May 23, 2022
@bsipocz bsipocz added this to the v0.4.7 milestone May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants