Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Pin GitHub Actions to specific commits for security#103635

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

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 seehttps://github.com/gentoo-ev/www.gentoo-ev.org/pull/5/files .

Looking forward to your review 🍻

arhadthedev reacted with thumbs up emoji
@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 maliciousv3 with the same tag/version?

hartwork reacted with thumbs up emoji

@hartwork
Copy link
ContributorAuthor

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

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

@hugovk
Copy link
Member

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

Is the security concern about a scenario where someone takes over one of the actions repos and replaces e.g.v3 with a maliciousv3 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 ownactions/*, seeing as we're trusting them for GitHub and all the actions infra etc.

@hartwork
Copy link
ContributorAuthor

Hi@hugovk let me address the two at once:

  • CPython will not be stuck at say 3.5.2, e.g. seehttps://github.com/gentoo-ev/www.gentoo-ev.org/pull/5/files for proof.
  • I agree that the risk foractions/* 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 tocontents: read yet, unlike the others. Should I make a pull request for that as well?

ezio-melotti reacted with thumbs up emoji

Copy link
Member

@ezio-melottiezio-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. seehttps://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 fromv3 tov4), but will still use the latestv3 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 thev3.5.2 commit to thev3.5.3 commit) in order to have the latestv3 version. This might also require some tweaks in thedependabot.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
ContributorAuthor

CPython will not be stuck at say 3.5.2, e.g. seehttps://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 fromv3 tov4), but will still use the latestv3 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 thev3.5.2 commit to thev3.5.3 commit) in order to have the latestv3 version. This might also require some tweaks in thedependabot.yml file.

@ezio-melotti I see, true, the new waycould 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 reacted with thumbs up emoji

@ezio-melottiezio-melotti added the type-securityA security issue labelMay 10, 2023
@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch fromda39a99 todde508fCompareMay 10, 2023 11:55
@hartwork
Copy link
ContributorAuthor

@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.

@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch 3 times, most recently from074dca8 to386a5f8CompareMay 15, 2023 20:08
@hartwork
Copy link
ContributorAuthor

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

@hugovk
Copy link
Member

I'd at least like to keep GitHub's ownhttps://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
ContributorAuthor

hartwork commentedMay 16, 2023
edited
Loading

Hi@hugovk

I'd at least like to keep GitHub's ownhttps://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.

TheOpenSSF (Open Source Security Foundation) developed aScorecard that was first brought to my attentionin context of libexpat. One of the checks their checker API does is to look for pinned GitHub Actions. That can be seen athttps://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 doesgit 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?

arhadthedev reacted with thumbs up emoji

@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch from386a5f8 to9496499CompareMay 16, 2023 11:25
@hartwork

This comment was marked as outdated.

@hartwork

This comment was marked as outdated.

@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch from9496499 to8f7fc26CompareMay 31, 2023 13:10
@hartwork

This comment was marked as outdated.

@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch from8f7fc26 to2273824CompareJune 3, 2023 14:17
@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch from2273824 toc13c12aCompareJune 3, 2023 14:19
@hartwork
Copy link
ContributorAuthor

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?

@hartworkhartworkforce-pushed thepin-github-actions-at-commit-level branch fromc13c12a toeaf235dCompareJune 3, 2023 14:24
@hugovk
Copy link
Member

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

@hugovk
Copy link
Member

Thanks again for your patience.

TheOpenSSF (Open Source Security Foundation) developed aScorecard that was first brought to my attentionin context of libexpat. One of the checks their checker API does is to look for pinned GitHub Actions. That can be seen atapi.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 ownactions/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 ownactions/*.

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
ContributorAuthor

Hi@hugovk ,

TheOpenSSF (Open Source Security Foundation) developed aScorecard that was first brought to my attentionin context of libexpat. One of the checks their checker API does is to look for pinned GitHub Actions. That can be seen atapi.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 ownactions/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 ownactions/*.

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ezio-melottiezio-melottiezio-melotti left review comments

@hugovkhugovkAwaiting requested review from hugovkhugovk is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hartwork@ezio-melotti@hugovk@arhadthedev@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp