Skip to content

chore: Set permissions for GitHub actions#1942

Closed
neilnaveen wants to merge 3 commits intohome-assistant:devfrom
turrisxyz:Pinned-Dependencies-GitHub
Closed

chore: Set permissions for GitHub actions#1942
neilnaveen wants to merge 3 commits intohome-assistant:devfrom
turrisxyz:Pinned-Dependencies-GitHub

Conversation

@neilnaveen
Copy link
Contributor

 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: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
@homeassistant
Copy link

Hi @neilnaveen,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@agners
Copy link
Member

agners commented Jun 1, 2022

Can you also add global

permissions:
  contents: read

to the release.yml and dev.yml?

@agners
Copy link
Member

agners commented Jun 1, 2022

I guess for dev.yml we also need pull-requests: read.

Comment on lines +9 to +11
permissions:
contents: read

Copy link
Member

Choose a reason for hiding this comment

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

Instead of this default workflow permissions should be set to read
https://github.com/home-assistant/operating-system/settings/actions

Copy link
Member

@agners agners Jun 1, 2022

Choose a reason for hiding this comment

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

Agreed, other than in the dev workflow, let's remove those. I will switch to the restricted setting in the project setting once we've merged this.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree with that.
We do not need global additional scopes, each job should set what they need.

Copy link
Member

Choose a reason for hiding this comment

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

Right, we only need it in the first job afact.

Copy link
Member

@agners agners Jun 7, 2022

Choose a reason for hiding this comment

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

Please delete this, as we will set the default to read, so no explicit permissions are required here.

- cron: "0 * * * *"
workflow_dispatch:

permissions:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this default workflow permissions should be set to read
https://github.com/home-assistant/operating-system/settings/actions

Copy link
Member

Choose a reason for hiding this comment

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

...and this

neilnaveen and others added 2 commits June 1, 2022 11:40
@neilnaveen
Copy link
Contributor Author

I guess for dev.yml we also need pull-requests: read.

I have added global permissions for both release.yml and dev.yml


permissions:
contents: read
pull-requests: read
Copy link
Member

Choose a reason for hiding this comment

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

This one we'll have too keep as it adds an additional global permission.

Copy link
Member

Choose a reason for hiding this comment

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

So we need this in the prepare job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't get it. What do you want me to do? Please let me know. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Essentially, move these three lines into the job description, under

jobs:
  prepare:


permissions:
contents: read
pull-requests: read
Copy link
Member

Choose a reason for hiding this comment

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

Essentially, move these three lines into the job description, under

jobs:
  prepare:

Comment on lines +9 to +11
permissions:
contents: read

Copy link
Member

@agners agners Jun 7, 2022

Choose a reason for hiding this comment

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

Please delete this, as we will set the default to read, so no explicit permissions are required here.

types: [published]

permissions:
contents: read
Copy link
Member

Choose a reason for hiding this comment

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

...and delete this.

- cron: "0 * * * *"
workflow_dispatch:

permissions:
Copy link
Member

Choose a reason for hiding this comment

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

...and this

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.

4 participants