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

Pin GitHub Actions to specific commits for security #103635

Closed

Conversation

hartwork
Copy link
Contributor

I assume this does not need an issue/ticket but I can create one if needed.

For proof that GitHub Dependabot can still keep things up to date for us with the new format see https://github.com/gentoo-ev/www.gentoo-ev.org/pull/5/files .

Looking forward to your review 🍻

@ezio-melotti
Copy link
Member

Is the security concern about a scenario where someone takes over one of the actions repos and replaces e.g. v3 with a malicious v3 with the same tag/version?

@hartwork
Copy link
Contributor Author

hartwork commented May 7, 2023

Is the security concern about a scenario where someone takes over one of the actions repos and replaces e.g. v3 with a malicious v3 with the same tag/version?

@ezio-melotti exactly! I have a few more words on this at karlb/wikdict-web#28 (comment) .

@hugovk
Copy link
Member

hugovk commented May 7, 2023

Because of the convention for the major version to point to the latest available (e.g. v3 points to v3.5.2, and will point to v3.6.0 when it comes out), right now dependabot only updates for major updates, for example:

- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4

With pins we get something like this:

- - uses: actions/checkout@v3
+ - uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab  # v3.5.2

Do we need to change dependabot to update also for minor and patch releases? Otherwise would be stuck on v3.5.2 until v4 comes out, and miss out on v3.6, v3.7 etc?

@hugovk
Copy link
Member

hugovk commented May 7, 2023

Is the security concern about a scenario where someone takes over one of the actions repos and replaces e.g. v3 with a malicious v3 with the same tag/version?

And is this a theoretical concern, or has there been any recorded incidences of it happening yet? I think we can at least trust GitHub's own actions/*, seeing as we're trusting them for GitHub and all the actions infra etc.

@hartwork
Copy link
Contributor Author

hartwork commented May 7, 2023

Hi @hugovk let me address the two at once:

  • CPython will not be stuck at say 3.5.2, e.g. see https://github.com/gentoo-ev/www.gentoo-ev.org/pull/5/files for proof.
  • I agree that the risk for actions/* is likely smaller than for the others, but probably not zero. The fact that there are multiple non-GitHub third-party controlled actions in use in CPython was additional motivation to pin actions down in CPython for me.

PS: Workflow .github/workflows/require-pr-label.yml does not yet declare and drop permissions to contents: read yet, unlike the others. Should I make a pull request for that as well?

Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

CPython will not be stuck at say 3.5.2, e.g. see https://github.com/gentoo-ev/www.gentoo-ev.org/pull/5/files for proof.

IIUC @hugovk means that currently @dependabot will only make a new pull request when a major version is released (e.g. to update from v3 to v4), but will still use the latest v3 available automatically. If we pin a commit id, @dependabot will have to create a new PR for every bugfix release (e.g. to update from the v3.5.2 commit to the v3.5.3 commit) in order to have the latest v3 version. This might also require some tweaks in the dependabot.yml file.

PS: Workflow .github/workflows/require-pr-label.yml does not yet declare and drop permissions to contents: read yet, unlike the others. Should I make a pull request for that as well?

Yes, please!

@hartwork
Copy link
Contributor Author

hartwork commented May 8, 2023

CPython will not be stuck at say 3.5.2, e.g. see https://github.com/gentoo-ev/www.gentoo-ev.org/pull/5/files for proof.

IIUC @hugovk means that currently @dependabot will only make a new pull request when a major version is released (e.g. to update from v3 to v4), but will still use the latest v3 available automatically. If we pin a commit id, @dependabot will have to create a new PR for every bugfix release (e.g. to update from the v3.5.2 commit to the v3.5.3 commit) in order to have the latest v3 version. This might also require some tweaks in the dependabot.yml file.

@ezio-melotti I see, true, the new way could be more verbose in terms of pull requests. From what I see at…

ignore:
- dependency-name: "*"
update-types:
- "version-update:semver-minor"
- "version-update:semver-patch"
…the current config will not allow for patch-level bumps though. How would you like us to continue regarding that aspect?

PS: Workflow .github/workflows/require-pr-label.yml does not yet declare and drop permissions to contents: read yet, unlike the others. Should I make a pull request for that as well?

Yes, please!

@ezio-melotti done, please see #104309

@ezio-melotti ezio-melotti added the type-security A security issue label May 10, 2023
@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch from da39a99 to dde508f Compare May 10, 2023 11:55
@hartwork
Copy link
Contributor Author

@ezio-melotti I have adjusted the Dependabot config now to allow minor and patch level bumps so that we have a chance to receive bugfixes in time as we did prior to pinning. I'm aware that more pull requests will be a bit more noise. It's not perfect but a price that I am personally happy to pay for more security in my projects across the board. If it's not considered acceptable to CPython, I can adjust the Dependabot config to do something else.

@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch 3 times, most recently from 074dca8 to 386a5f8 Compare May 15, 2023 20:08
@hartwork
Copy link
Contributor Author

@ezio-melotti @hugovk I have resolved conflicts now, and also pinned the actions that appeared on main in the mean time after rebase. How would you like to continue?

@hugovk
Copy link
Member

hugovk commented May 16, 2023

I'd at least like to keep GitHub's own https://github.com/actions unpinned. Can we have them only bumped for major releases, and not also minor/patch?

And any idea about:

And is this a theoretical concern, or has there been any recorded incidences of it happening yet?

@hartwork
Copy link
Contributor Author

hartwork commented May 16, 2023

Hi @hugovk

I'd at least like to keep GitHub's own https://github.com/actions unpinned. Can we have them only bumped for major releases, and not also minor/patch?

the motivation being to avoid pull requests?

I can try demo that in a minute… ⏳ EDIT: Done.

And any idea about:

And is this a theoretical concern, or has there been any recorded incidences of it happening yet?

Let me add some context and then address your question.

The OpenSSF (Open Source Security Foundation) developed a Scorecard that was first brought to my attention in context of libexpat. One of the checks their checker API does is to look for pinned GitHub Actions. That can be seen at https://api.securityscorecards.dev/projects/github.com/python/cpython in the JSON response for CPython's case in particular, see results of check 14.

I believe three of the most relevant attack scenarios without pinning are:

  • Write access and an unpinned action are used to sneak new commits into Git
  • Read access and an unpinned action are used to release/deploy/publish things
  • Read access and an unpinned action are used to leak CI credentials

Out of these, in particular the first would try to stay as invisible as possible. So we'd only hear about it if done wrong. If everyone just does git pull in a busy repository, it could go unnoticed. An attacker could test their attacks in their own repositories, and make sure it works down to the last bit.

Asking about known incidents is — while very understandable to ask — not what we should focus on. The transparent answer is that I am personally not aware of a case where any of that happened, but:

  • I may just have missed it.
  • I consider it a real threat,
    especially with the variety of non-actions/* actions in the CPython repository.
  • (I could have had knowledge that I was forbidden to talk about even their existence in theory.)
  • We want security fixed before things go wrong, not after 😉

What do you think?

@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch from 386a5f8 to 9496499 Compare May 16, 2023 11:25
@hartwork

This comment was marked as outdated.

@hartwork

This comment was marked as outdated.

@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch from 9496499 to 8f7fc26 Compare May 31, 2023 13:10
@hartwork

This comment was marked as outdated.

@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch from 8f7fc26 to 2273824 Compare June 3, 2023 14:17
@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch from 2273824 to c13c12a Compare June 3, 2023 14:19
@hartwork
Copy link
Contributor Author

hartwork commented Jun 3, 2023

Conflicts resolved once more, ground below had moved again. Since @hugovk doesn't seem to have time for this, @ezio-melotti do you have time for this, how should we proceed here?

@hartwork hartwork force-pushed the pin-github-actions-at-commit-level branch from c13c12a to eaf235d Compare June 3, 2023 14:24
@hugovk
Copy link
Member

hugovk commented Jun 3, 2023

Hi! Thanks for your patience, I had a draft reply which I've lost, let me try again :)

@hugovk
Copy link
Member

hugovk commented Jun 5, 2023

Thanks again for your patience.

The OpenSSF (Open Source Security Foundation) developed a Scorecard that was first brought to my attention in context of libexpat. One of the checks their checker API does is to look for pinned GitHub Actions. That can be seen at api.securityscorecards.dev/projects/github.com/python/cpython in the JSON response for CPython's case in particular, see results of check 14.

As it happens, OpenSSF are currently working to update their Scorecards not to penalise read-only actions:

Workflows that have no secret and all their permissions set to read/none don't benefit from being pinned, and add burden for users to keep them up to date. We may want to relax the Token-Permission check, making this a "bonus" point rather than flagging it as an problem


I believe three of the most relevant attack scenarios without pinning are:

  • Write access and an unpinned action are used to sneak new commits into Git

We have two workflows with write permissions, and they only have write access to PRs. One is GitHub's own actions/stale, the other is Read the Docs' readthedocs/actions/preview, both providers are well trusted.

  • Read access and an unpinned action are used to release/deploy/publish things

I don't think we have any release/deploy/publish, other than the documentation previews.

  • Read access and an unpinned action are used to leak CI credentials

The secrets -- secrets.ADD_TO_PROJECT_PAT, secrets.GITHUB_TOKEN, secrets.MAILGUN_PYTHON_ORG_MAILGUN_KEY -- are only used with GitHub's own actions/*.

So I'm -1 on these changes, it makes things harder to work with and there are more important things on the security scale.

@hartwork
Copy link
Contributor Author

hartwork commented Jun 5, 2023

Hi @hugovk ,

The OpenSSF (Open Source Security Foundation) developed a Scorecard that was first brought to my attention in context of libexpat. One of the checks their checker API does is to look for pinned GitHub Actions. That can be seen at api.securityscorecards.dev/projects/github.com/python/cpython in the JSON response for CPython's case in particular, see results of check 14.

As it happens, OpenSSF are currently working to update their Scorecards not to penalise read-only actions:

Workflows that have no secret and all their permissions set to read/none don't benefit from being pinned, and add burden for users to keep them up to date. We may want to relax the Token-Permission check, making this a "bonus" point rather than flagging it as an problem

thanks for bringing that issue to my attention. At least access to secrets would need to be clarified in context of de-penalization. I'll get involved in that issue, thanks.

I believe three of the most relevant attack scenarios without pinning are:

  • Write access and an unpinned action are used to sneak new commits into Git

We have two workflows with write permissions, and they only have write access to PRs. One is GitHub's own actions/stale, the other is Read the Docs' readthedocs/actions/preview, both providers are well trusted.

  • Read access and an unpinned action are used to release/deploy/publish things

I don't think we have any release/deploy/publish, other than the documentation previews.

  • Read access and an unpinned action are used to leak CI credentials

The secrets -- secrets.ADD_TO_PROJECT_PAT, secrets.GITHUB_TOKEN, secrets.MAILGUN_PYTHON_ORG_MAILGUN_KEY -- are only used with GitHub's own actions/*.

Regarding the three scenarios I introduced above I should add that these were meant to be general, not specific to CPython.

If you believe you're all safe with CPython's GitHub Actions today, it would be great to have some GitHub Action that prevents that state from degrading through pull requests in the future. Would be trivial to detect with a few lines of shell.

So I'm -1 on these changes, it makes things harder to work with and there are more important things on the security scale.

The harder-to-work-with part is only the potential volume of pull requests: CI and Dependabot do the rest, it's a few clicks of a button by a human every now and then, nothing more. A price that personally I am happily paying in my own projects but that you and others seem to give a lot more weight than me, which I find surprising but which I can accept. I will hence close this pull request as "wontfix".

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.

5 participants