misc: pin actions to a full-length commit SHA

[naveensrinivasan]

• Pinned actions by SHA https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies
• Included permissions for the action. https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions

Pin actions to a full length commit SHA

Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.

https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions

Also, dependabot supports upgrading based on SHA.

Signed-off-by: naveensrinivasan [email protected]

Motivation

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Related PRs

[facebook-github-bot]

Hi @naveensrinivasan!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[Josh-Cena]

I'm indeed aware of this practice, but I'm slightly skeptical of its usefulness. Do you have examples of other repos using this practice? actions/checkout and actions/setup-node seem so widely used I can hardly imagine them being breached.

[netlify]

✅ [V2]

Name	Link
🔨 Latest commit	f0a405c
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6256f9247a77c5000966c153
😎 Deploy Preview	https://deploy-preview-6984--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	51
🟢 Accessibility	100
🟢 Best practices	92
🟢 SEO	100
🟢 PWA	90

Lighthouse ran on https://deploy-preview-6984--docusaurus-2.netlify.app/

[naveensrinivasan]

I'm indeed aware of this practice, but I'm slightly skeptical of its usefulness. Do you have examples of other repos using this practice? actions/checkout and actions/setup-node seem so widely used I can hardly imagine them being breached.

GitHub's own repository pin's their checkout actions by SHA and doesn't use the version tag https://github.com/github/docs/blob/ea7f218c91ecbae9a700a8702b51a7d2736e0d2c/.github/workflows/docs-review-collect.yml#L23

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[Josh-Cena]

Thanks!

I wish GitHub actions can eventually have lock files so we no longer have to do something like this.

[Josh-Cena]

@naveensrinivasan I do not have write access to your PR. Please format your file, or I can't merge this. You just need to delete the double spaces before the trailing comment in build-perf.yml.

[slorber]

I'm not really willing to make it less convenient for official Github actions, @3 should be fine, very unlikely to cause any trouble (or the whole world will be in a very bad state) and is more convenient to maintain

Ok to pin sha on external actions 👍 please link to the pinned commit.

[Josh-Cena]

@slorber We can't totally overlook the probability of security breaches even from trusted sources like actions/ or github/. There's no concept of locking or checksums on GitHub, so we can't be totally safe, but I think this is the best move we can take. We have dependabot to update the actions anyways.

[slorber]

ok, we can give it a try to pin sha

please explain where you found the actions/checkout hash, because it's not the one I see associated with the v3 release

And it doesn't seem to actually exist: actions/checkout@b0e28b5

[Josh-Cena]

Someone else noticed the wrong SHA as well in juju/juju#13856

@naveensrinivasan How could I get that SHA?

[naveensrinivasan]

Someone else noticed the wrong SHA as well in juju/juju#13856

@naveensrinivasan How could I get that SHA?

I fixed a couple of them which had issues.

The easiest way to get the SHA is using the tag API https://api.github.com/repos/treosh/lighthouse-ci-action/git/refs/tags/9.3.0

{
  "ref": "refs/tags/9.3.0",
  "node_id": "MDM6UmVmMjEwNTUyOTA0OnJlZnMvdGFncy85LjMuMA==",
  "url": "https://api.github.com/repos/treosh/lighthouse-ci-action/git/refs/tags/9.3.0",
  "object": {
    "sha": "b4dfae3eb959c5226e2c5c6afd563d493188bfaf",
    "type": "commit",
    "url": "https://api.github.com/repos/treosh/lighthouse-ci-action/git/commits/b4dfae3eb959c5226e2c5c6afd563d493188bfaf"
  }
}

and then using the SHA b4dfae3eb959c5226e2c5c6afd563d493188bfaf construct an API like this to verify
https://api.github.com/repos/treosh/lighthouse-ci-action/git/commits/b4dfae3eb959c5226e2c5c6afd563d493188bfaf which is what is pinned.

[naveensrinivasan]

Closing this in favor of this one #7028. The rebasing with the comment inline became an issue.

[Josh-Cena]

You don't need to rebase—just add more commits. We squash-merge anyways. Please don't open duplicated PRs because that just adds more noise to our history. (This principle applies to a lot of repos.) Also, that one is lacking the permission changes, which is really the crux.

[Josh-Cena]

The URLs are actually not necessary. @slorber and I do know how to check SHAs, it was only that we couldn't find your previous SHA at all.

[slorber]

@naveensrinivasan still interested to know where the wrong Sha comes from, can you explain please?

Is it from another action that you interverted by mistake?

[naveensrinivasan]

@naveensrinivasan still interested to know where the wrong Sha comes from, can you explain please?

Is it from another action that you interverted by mistake?

The SHA's came from Git Tag instead of the commit SHA.

For some of the repositories the way in which is tagged is different and that is why is my other PR you can see how I managed to add the links to Git tag or the commit with the GitHub URL.

[naveensrinivasan]

The URLs are actually not necessary. @slorber and I do know how to check SHAs, it was only that we couldn't find your previous SHA at all.

OK. That means this PR should be good without the URL.

Now based of my other PR you should be able to validate each of the tags and commit SHA.

Let me know if you have any questions.

[Josh-Cena]

👍 Thanks, that makes sense to me. We'll just get some re-confirmation from Facebook that this is the right direction to go (you can tell that I'm personally in favor of this). In the meantime, please check the Prettier error and re-format the files, since I don't have write permissions to your fork.

[naveensrinivasan]

👍 Thanks, that makes sense to me. We'll just get some re-confirmation from Facebook that this is the right direction to go (you can tell that I'm personally in favor of this). In the meantime, please check the Prettier error and re-format the files, since I don't have write permissions to your fork.

Thanks, I have given you write access to the org fork. Would you be able to push the changes that is an issue with the linter?

[zpao]

My opinion: this is overkill. I recognize the threat being guarded against but I wouldn't advise any team here at Meta to go through this process today.

Conceptually I can see an argument being made that this is no different than lockfiles. Those are a good idea! But tooling is built up around them to tell use when there are compatible updates and when there are vulnerabilities in the versions we're using. There's no such infra here, and historically CI configs are very much "set and forget" - nobody is going to come along and update these SHAs. It's bad enough that nobody will update the version refs either, but at least with the named refs we'll get the bug fixes and performance improvements.

So my recommendation: don't do this for first-party or GitHub owned actions (actions/*, github/*). Do it for other actions if you want. The permission scoping and associated comments are great.

[naveensrinivasan]

My opinion: this is overkill. I recognize the threat being guarded against but I wouldn't advise any team here at Meta to go through this process today.

Conceptually I can see an argument being made that this is no different than lockfiles. Those are a good idea! But tooling is built up around them to tell use when there are compatible updates and when there are vulnerabilities in the versions we're using. There's no such infra here, and historically CI configs are very much "set and forget" - nobody is going to come along and update these SHAs. It's bad enough that nobody will update the version refs either, but at least with the named refs we'll get the bug fixes and performance improvements.

So my recommendation: don't do this for first-party or GitHub owned actions (actions/*, github/*). Do it for other actions if you want. The permission scoping and associated comments are great.

The right way to solve this is to use dependabot to get updates on the GitHub actions so they aren't stale. Dependabot is aware of SHA's so it shouldn't be an issue.

[zpao]

The right way to solve this is to use dependabot to get updates on the GitHub actions so they aren't stale. Dependabot is aware of SHA's so it shouldn't be an issue.

Ideally, yes. However, it doesn't actually work. SHAs can be a part of multiple named refs - actions/checkout@fd47087 for example is an ancestor of v2 and v3. Without some kind of static version declaration there's no way to safely suggest a newer SHA. It's possible the information could be used to say you're out of date or the SHA being used is part of some vulnerable range, but that's pretty much it.

From GitHub's own docs at https://docs.github.com/en/code-security/supply-chain-security/end-to-end-supply-chain/securing-builds:

Note: GitHub Actions workflow dependencies are displayed in the dependency graph for informational purposes. Dependabot alerts are not currently supported for GitHub Actions workflows.

If that changes and we can rely on dependabot to automate updates, then I'm onboard.

[Josh-Cena]

@zpao Dependabot does update actions. See https://github.com/facebook/docusaurus/issues?q=label%3Agithub_actions+is%3Aclosed, for example. However, I'm not aware if it's able to update across SHAs—@naveensrinivasan do you have examples?

[naveensrinivasan]

@zpao Dependabot does update actions. See https://github.com/facebook/docusaurus/issues?q=label%3Agithub_actions+is%3Aclosed, for example. However, I'm not aware if it's able to update across SHAs—@naveensrinivasan do you have examples?

ossf/scorecard#1755

[zpao]

Alright, I guess I stand corrected. That's awesome. Are there docs on this? Does it require the # v3 bit at the end to know what ref to follow? If so, is there a way to automate requiring that or will y'all just rely on manual checking?

[naveensrinivasan]

The dependabot doesn't add the additional comment of v3 on PR's.

[slorber]

Dependabot seems to be able to update SHA so I guess it means we can move on and merge this @zpao ?

Regarding the comment, as far as I understand

• it seems not mandatory for Dependabot to work
• Dependabot preserves it (but I guess it doesn't "bump" the comment)
• The comment might become "stale" and should be edited/bumped manually => a bit annoying but ok

That seems more complicated than before but more secure 🤷‍♂️ no strong opinion

[slorber]

Thanks, we decided to move on and try this

Double-checked all SHAs, only one was incorrect

[naveensrinivasan]

Thanks, we decided to move on and try this

Double-checked all SHAs, only one was incorrect

Thanks!