search

facebook / pyre-check

Performant type-checking for python.
https://pyre-check.org/
MIT License
6.79k stars 432 forks source link

Apply Github Actions security best practices - pinned hashes #850

Closed Nick2bad4u closed 3 months ago

Nick2bad4u commented 3 months ago

Security Fixes

Pinned Dependencies

GitHub Action tags and Docker tags are mutable. This poses a security risk. GitHub's Security Hardening guide recommends pinning actions to full length commit.

Example in this repo: publish_website.yml uses the following which could be modified:

uses: JamesIves/github-pages-deploy-action@releases/v4

This PR makes this immutable by pinning it to a full length hash.

uses: JamesIves/github-pages-deploy-action@ec9c88baef04b842ca6f0a132fd61c762aa6c1b0 # releases/v4

Github says directly that:

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. When selecting a SHA, you should verify it is from the action's repository and not a repository fork.

This is especially important when using third-party actions.

I plan to submit another PR (assuming this one is merged) that implements the dependabot actions dependency review that creates PRs automatically to update actions to their newest versions to make this easier to manage. (For example, if a new version of uses: JamesIves/github-pages-deploy-action comes out.) Although it looks like you may already have this setup/partially setup.

Secure Dockerfiles

Pin image tags to digests in Dockerfiles. With the Docker v2 API release, it became possible to use digests in place of tags when pulling images or to use them in FROM lines in Dockerfiles.

This is the same thing as above, only for dockerfiles.

Lastly, if needed, I can break this down into file by file PRs.

stroxler commented 3 months ago

Test failures are pre-existing (I'm working on fixing them now); this looks good to me so I'll try to get it reviewed.

The process may be a little slower than usual because the team is traveling this week for PyCon.

Nick2bad4u commented 3 months ago

Test failures are pre-existing (I'm working on fixing them now); this looks good to me so I'll try to get it reviewed.

The process may be a little slower than usual because the team is traveling this week for PyCon.

No worries. Once this is merged, I will move onto the next few security fixes.

facebook-github-bot commented 3 months ago

@stroxler has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 3 months ago

This pull request has been merged in facebook/pyre-check@a2ae015c837bca78dddd35063a39867d01764f64.