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.

Sign up for GitHub

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

ci: Add GitHub token permissions for workflows #74331

Closed
wants to merge 3 commits into from
Closed

ci: Add GitHub token permissions for workflows #74331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5484237
ci: Add GitHub token permissions for workflows
varunsh-coder Jul 2, 2022
85f430d
Fix formatting to comply with prettier
varunsh-coder Jul 2, 2022
d600ba7
Remove explicit job level read perms
varunsh-coder Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/builder.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ env:
BUILD_TYPE: core
DEFAULT_PYTHON: 3.9

permissions:
contents: read

Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

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

We should set read on repository settings instead

Copy link
Author

Choose a reason for hiding this comment

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

Hi @ludeeus, I think it would be good to set read on repo settings in addition to this change. This way, since the permissions are set explicitly in the workflow, even if one forks this repo, the workflow will still be secure in the forked repo. Please let me know your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

I think that if that is required to secure forks, GitHub has an issue that we should take up with them. Basically, you are saying that the repository settings don't work?

Copy link
Author

Choose a reason for hiding this comment

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

I think that if that is required to secure forks, GitHub has an issue that we should take up with them. Basically, you are saying that the repository settings don't work?

Setting the token permission using repository setting will work for this repository. But if the repository is forked or if someone copies the same workflow and uses it in a different repository, the repository settings will not go along. This is why it is a best practice to keep the permissions along with the workflow file.

Copy link
Member

Choose a reason for hiding this comment

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

So, you are saying the repository permissions settings are not working / causing a security issue?

Since you guys are on a mission fixing / improving security for open source project, I assume you have reported/contacted GitHub about this as well? I am interested in their response actually?

Copy link
Author

Choose a reason for hiding this comment

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

So, you are saying the repository permissions settings are not working / causing a security issue?

Since you guys are on a mission fixing / improving security for open source project, I assume you have reported/contacted GitHub about this as well? I am interested in their response actually?

Hi @frenck I have not reported/ contacted GitHub about this, since they offer multiple solutions (repo setting + workflow file setting) and developers can choose to use both of them to cover all scenarios. I did work with GitHub to set token permissions in starter workflows. For starter workflow, permissions are required to be set in the workflow file. This is the issue for it: actions/starter-workflows#1299

For your repo, if you prefer to just use repo settings for cases where only contents: read is needed, I can remove the explicit workflow level permissions. The only downside is that forks of this repo will not automatically be secure, but the maintainers of those forks can implement the fix on their own. Please let me know. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Hi @frenck, please let me know about this. Thanks!

jobs:
init:
name: Initialize build
Expand Down Expand Up @@ -268,6 +271,8 @@ jobs:
channel: beta

publish_container:
permissions:
packages: write # to push container image
name: Publish meta container for ${{ matrix.registry }}
if: github.repository_owner == 'home-assistant'
needs: ["init", "build_base"]
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

permissions:
contents: read

Comment on lines +37 to +39
Copy link
Member

Choose a reason for hiding this comment

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

We should set read on repository settings instead

jobs:
changes:
name: Determine what has changed
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/lock.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,14 @@ on:
schedule:
- cron: "0 * * * *"

permissions:
contents: read

jobs:
lock:
permissions:
issues: write # for dessant/lock-threads to lock issues
pull-requests: write # for dessant/lock-threads to lock PRs
if: github.repository_owner == 'home-assistant'
runs-on: ubuntu-latest
steps:
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/stale.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@ on:
- cron: "0 * * * *"
workflow_dispatch:

permissions:
contents: read

Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

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

We should set read on repository settings instead

jobs:
stale:
permissions:
issues: write # for actions/stale to close stale issues
pull-requests: write # for actions/stale to close stale PRs
if: github.repository_owner == 'home-assistant'
runs-on: ubuntu-latest
steps:
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/translations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ on:
env:
DEFAULT_PYTHON: 3.9

permissions:
contents: read

Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

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

We should set read on repository settings instead

jobs:
upload:
name: Upload
Expand All @@ -34,6 +37,8 @@ jobs:
python3 -m script.translations upload

download:
permissions:
contents: write # for Git to git push
name: Download
needs: upload
if: github.repository_owner == 'home-assistant' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch')
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/wheels.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ on:
- "requirements.txt"
- "requirements_all.txt"

permissions:
contents: read

Comment on lines +16 to +18
Copy link
Member

Choose a reason for hiding this comment

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

We should set read on repository settings instead

jobs:
init:
name: Initialize wheels builder
Expand Down