Skip to content
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

Updates the permissions block to be minimal #689

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Aug 9, 2021

And adds a permissions block to the README.

Fixes #464.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@aeisenberg aeisenberg requested a review from a team as a code owner August 9, 2021 18:41
@aeisenberg aeisenberg force-pushed the aeisenberg/update-permissions branch from 9473576 to f374474 Compare August 9, 2021 18:42
@adityasharad
Copy link
Contributor

From our last internal discussion about this, I believe that actions: read is (only) needed if you are analyzing a private repository, and security-events: write is (only) needed if you are not analyzing a pull request. And I think we need at least contents: read (or pull-requests: read for PRs). The overlapping scopes for PRs make this a bit confusing.

So perhaps let's go with the recommendation of actions: read, contents: read, security-events: write? If it turns out that's not entirely minimal, we can refine it again later.

@aeisenberg
Copy link
Contributor Author

Sure, and I can add some comments on when different scopes may not be needed.

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Aug 9, 2021

Here are the API calls we are making (just doing a code search for .request:

  • GET /repos/:owner/:repo/actions/runs/:run_id (actions: read)
  • GET /repos/:owner/:repo/actions/workflows (actions: read)
  • PUT /repos/:owner/:repo/code-scanning/analysis/status (security-events: write)
  • GET /enterprise/code-scanning/codeql-bundle/find/{tag} (security-events: read)
  • GET /enterprise/code-scanning/codeql-bundle/download/{asset_id} (security-events: read)
  • GET /repos/:owner/:repo/code-scanning/codeql/databases (security-events: read)
  • PUT /repos/:owner/:repo/code-scanning/codeql/databases/:language (security-events: write)
  • PUT /repos/:owner/:repo/code-scanning/analysis (security-events: write)

Unless I've missed something, we only need security-events: write and actions: read. And the latter is only for private repos.

@aeisenberg aeisenberg force-pushed the aeisenberg/update-permissions branch from f374474 to 1f9cee9 Compare August 9, 2021 19:56
@adityasharad
Copy link
Contributor

Plus you need enough permission to check out the repo itself in the workflow (but not in the Action), which I believe is either contents: read or pull_requests: read.

@aeisenberg
Copy link
Contributor Author

Ah..yes. Again, only for private repos, it seems. So, contents: read guards the GET /repos/:owner/:repo/contents/:path endpoint. I don't see anything about pull requests, though. This would only be for things like editing the PR itself, or adding comments, etc.

 And adds a permissions block to the README.
@aeisenberg aeisenberg force-pushed the aeisenberg/update-permissions branch from 1f9cee9 to 2175328 Compare August 9, 2021 20:30
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

LGTM once you resolve the changelog conflict. Thanks!

@aeisenberg aeisenberg enabled auto-merge August 9, 2021 21:56
@aeisenberg aeisenberg merged commit 07fa17d into main Aug 9, 2021
@aeisenberg aeisenberg deleted the aeisenberg/update-permissions branch August 9, 2021 22:12
@github-actions github-actions bot mentioned this pull request Aug 16, 2021
5 tasks
@naveensrinivasan
Copy link

Here are the API calls we are making (just doing a code search for .request:

  • GET /repos/:owner/:repo/actions/runs/:run_id (actions: read)
  • GET /repos/:owner/:repo/actions/workflows (actions: read)
  • PUT /repos/:owner/:repo/code-scanning/analysis/status (security-events: write)
  • GET /enterprise/code-scanning/codeql-bundle/find/{tag} (security-events: read)
  • GET /enterprise/code-scanning/codeql-bundle/download/{asset_id} (security-events: read)
  • GET /repos/:owner/:repo/code-scanning/codeql/databases (security-events: read)
  • PUT /repos/:owner/:repo/code-scanning/codeql/databases/:language (security-events: write)
  • PUT /repos/:owner/:repo/code-scanning/analysis (security-events: write)

Unless I've missed something, we only need security-events: write and actions: read. And the latter is only for private repos.

@aeisenberg I am trying to figure out the required permissions for any GitHub Action. What is the process to get all the necessary permission? Thanks

@aeisenberg
Copy link
Contributor Author

There's no principled way of doing this that I can think of. An action can make any arbitrary request to the github api. And it can make requests in many ways (since they are just fundamentally REST requests): curl, gh, octokit, etc.

You could try setting very restrictive permissions, and slowly loosen them until you get your workflow passing. Alternatively, you can try to eyeball the code, which could be tricky if the action is doing non-standard things.

@naveensrinivasan
Copy link

There's no principled way of doing this that I can think of. An action can make any arbitrary request to the github api. And it can make requests in many ways (since they are just fundamentally REST requests): curl, gh, octokit, etc.

You could try setting very restrictive permissions, and slowly loosen them until you get your workflow passing. Alternatively, you can try to eyeball the code, which could be tricky if the action is doing non-standard things.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document what permissions are required
3 participants