Hash-pin all actions, apply other fixes from zizmor#829
Hash-pin all actions, apply other fixes from zizmor#829woodruffw wants to merge 5 commits intoRustCrypto:masterfrom
Conversation
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Key fixes: - Drop persisted credentials - Eliminate template injections - Add cooldowns to Dependabot updates Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
|
Except for the @tarcieri |
|
FWIW, I think this worth doing even in the presence of repository-level controls: integrity locking is considered a best practice in all other packaging ecosystems (e.g. with With that said, I'm happy to remove anything here (or close outright) if that's what you all prefer. You know what's best for your project! |
What you do here is equivalent to using It may be worth to hash-pin dependencies in the
Firstly, what happens in forks is not our responsibility. Secondly, what is the worst case scenario? Fork authors do not have publishing rights for crates in this repo, so IIUC in the worst case scenario source code in their fork will be compromised which will be visible when they'll try to create PRs based on it. |
Perhaps I'm misunderstanding, but aren't you using Dependabot to manage these workflow dependency updates? I don't think you should be managing these updated by hand; I agree doing so is annoying. And yes, you're right that it isn't literally a separate lockfile, because GitHub Actions lacks that as a platform. Hopefully they'll add that soon, but in the mean time this is the closest thing their platform allows in terms of eliminating mutable pointers to actions. Separately, it's worth noting that there is a security dimension to this: a compromised action that runs on Pinning your action dependencies doesn't stop this from happening, but it eliminates mutable tags as a source for it; notably, it's the vector that we've seen attackers currently exploit. |
Does it update hash-pinned workflow dependencies? If it does, then there is no security benefit, since we would just pull the most recent (potentially compromised) commit in the repo. It would only result in unnecessary noisy update churn. If does not, then we return to the annoyance part.
IIUC we do not use caching in our |
It does, but that's also why I've added a cooldown configuration to it. The idea is that you'd update the hashes (like you'd update a lockfile), but on a pared-back cadence to ensure that the community has a chance to detect compromised actions before you roll onto them.
That's my understanding as well, but the problem (again, with mutable pointers to actions) is that an action can start to cache at any point. It's also difficult to demonstrate that the runner itself doesn't restore from the (networked) cache. Again I want to emphasize that these aren't really vulnerabilities per se; I think they're good defense-in-depth measures that users of GitHub Actions are adopting in general. But if your position is that they don't represent a concern for you, I'm happy to close this. And I apologize if this has caused noise or consternation for you. |
|
As I wrote above, I think it may be worth to harden the I will wait for @tarcieri to chime in before deciding what to do. |
|
I see the benefit for Cargo.lock itself: it keeps the build "bisectable", i.e. at any given point in time anyone who pulls the source code should be able to reproduce builds in its current state. For something like a read-only GitHub Actions workflow, I don't really see the point. We generally want to use the latest versions of all actions there, historical versions are generally not helpful, and the main "threat" would be convincing us to merge code which breaks the build (i.e. false "green") in a way we can't recognize through code review. But this comes at a high cost in terms of the number of PRs opened to update these dependencies, in addition to the ones to update Cargo.lock, which adds a lot of noise to the commit history which isn't ultimately helpful or relevant to actual development of the source code in that repository. The maintainer burden is then multiplied by the number of repos in this organization. I don't think it's worth it. Now, when it comes to hardening our publish workflows, those are very high value, and there I think such hardening is very much worth it. |
|
Ok, so we are generally in agreement. Let's keep only the Although, I am not sure whether we need the |
Hello! Apologies for the cold PR.
I'm opening this in my capacity as one of uv's maintainers; we (at Astral) have a set of downstreams (including the hash crates in this repo!) that we depend on for uv/Ruff/etc, and we'd like to ensure their CI/CD processes are as hermetic and secure as possible (within the limits of GitHub's platform).
To that effect, this PR contains a few different commits that aim to make the CI here more secure. None of these changes fix vulnerabilities; they're purely defense-in-depth changes that will make a future Trivy-style compromise less fruitful for an attacker.
To summarize:
pinact run -v. Dependabot will keep these action references up to date (and will bump the version comments too), so this shouldn't add any additional maintenance burden.actions/checkout's default credential-persistence behavior withpersist-credentials: false, where possible. OPTIONAL: You have a few workflows that intentionally push back to the repo (or create issues/PRs), so for those workflows I've ensured thatpersist-credentials: trueis explicitly set.permissionsto{}, wherever possible. I've also moved all non-empty permissions into their respective jobs rather than setting them at the entire workspace level.github.ref_namecan be safely replaced with the built-inGITHUB_REF_NAME, which goes through normal shell interpolation rather than being injected directly into the shell).Most of the above was detected automatically with zizmor, which you can integrate into GitHub Actions if you'd like. I've left that out of this PR however, since not every project wants another thing running in CI. But let me know if you'd like it and I'd be happy to send a follow-up PR!
(NB: I did add a
zizmor.ymlconfig file, which disables some findings for your own first-party action usage inRustCrypto/*. This is in a discrete commit that can be amended out if you'd prefer not to have it, just LMK.)Last but not least, please let me know if there's any other information I can provide. All of the above was 100% human written and reviewed 🙂