rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
273 stars 178 forks source link

Hash pin github workflow dependencies #366

Closed joycebrum closed 1 year ago

joycebrum commented 1 year ago

Description

Hi again (related issue #360), I'd like to suggest another security practice recommended by the OpenSSF Scorecard and the GitHub itself which is to hash pin the CI dependencies to prevent tag renaming attacks. This means hash pinning GitHub Workflow actions.

Along with hash-pinning dependencies, I also recommend adopting dependabot or renovatebot to help keep the dependencies up to date. Both tools can update hashes and associated semantic version comments.

Together with this issue I'll also suggest a PR with the changes since they are quite simple.

Any questions or concerns just let me know. Thanks!

Additional Context

A tag renaming attack is a type of attack whereby an attacker:

For more informations about the dependency-update tools:

newpavlov commented 1 year ago

I am not sure we need hash pinning, considering that we use read-only workflow permissions.

As for dependabot, number of our dependencies is quite small, so it will not be that helpful. Also, dependabot has an annoying behavior of bumping patch versions in Cargo.toml, which we certainly do not need.

joycebrum commented 1 year ago

I am not sure we need hash pinning, considering that we use read-only workflow permissions.

That's not wrong actually. The workflows are already considered safe enough because no much harm could be done with "read only" permission.

Hash pinning dependencies is much more like an extra precaution to make CI workflows even more reliable. It is currently the only way of using GitHub Actions as immutable releases, which increases CI stability --- avoiding breaking changes, for example.

If you still rather not hash pin, we can at least minor version pin the actions, to prevent blind updates to potentially break changes (or even malicious releases, although they won't be able to do much compromising a read only workflow).

newpavlov commented 1 year ago

we can at least minor version pin the actions, to prevent blind updates to potentially break changes (or even malicious releases, although they won't be able to do much compromising a read only workflow).

Potential breaking changes in actions is not an issue in practice, at least in my experience. And some actions (such as dtolnay/rust-toolchain) don't even use release tags.

I don't think that pinning non-major version will help against a malicious actor, the releases are based on tags and such actor can always release a compromised version under an older tag.

I am interested in hearing @josephlr's opinion, but otherwise I am inclined to leave the CI config as-is.

josephlr commented 1 year ago

I also think leaving the CI configuration as-is is fine for now. I think that for the workflows which don't have any write/edit permissions (like our tests.yml) we don't need to hash dependencies for security.

Then, the only question is one of reliability. Which setup makes the CI break the least? Which setup causes the least toil for maintainers? I think the current setup is fine here. If we encounter CI breaks due to actions not respecting Semver, we can revisit.