Skip to content

Hash-pin all actions, apply other fixes from zizmor#829

Open
woodruffw wants to merge 5 commits intoRustCrypto:masterfrom
woodruffw-forks:ww/ci
Open

Hash-pin all actions, apply other fixes from zizmor#829
woodruffw wants to merge 5 commits intoRustCrypto:masterfrom
woodruffw-forks:ww/ci

Conversation

@woodruffw
Copy link
Copy Markdown

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:

  • I've hash-pinned all of your action dependencies with 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.
  • I've enabled cooldowns on your Dependabot ecosystem groups, to reduce the "window of opportunity" for compromise if/when a downstream is compromised.
  • I've disabled actions/checkout's default credential-persistence behavior with persist-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 that persist-credentials: true is explicitly set.
  • I've dropped your default permissions to {}, wherever possible. I've also moved all non-empty permissions into their respective jobs rather than setting them at the entire workspace level.
  • I've removed a minor template injection (github.ref_name can be safely replaced with the built-in GITHUB_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.yml config file, which disables some findings for your own first-party action usage in RustCrypto/*. 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 🙂

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>
@newpavlov
Copy link
Copy Markdown
Member

Except for the publish workflow, we do not use CI for anything sensitive. We set read-only permissions for actions on the repository level, so such hardening is IMO unnecessary in our case.

@tarcieri
WDYT?

@woodruffw
Copy link
Copy Markdown
Author

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 Cargo.lock), and explicitly dropping permissions ensures that any policy changes (e.g. on forks) don't countermand your assumption that the permissions are always minimal (so you can treat it as a defense-in-depth).

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!

@newpavlov
Copy link
Copy Markdown
Member

integrity locking is considered a best practice in all other packaging ecosystems

What you do here is equivalent to using git = "...", commit = "..." in Cargo.toml, not to locking Cargo.lock. It makes it pretty annoying to update workflow dependencies and does not provide any meaningful security improvements.

It may be worth to hash-pin dependencies in the publish workflow due to its sensitivity (e.g. to prevent malicious source code changes before publishing), but I don't think it's worth the trouble for other workflows.

explicitly dropping permissions ensures that any policy changes (e.g. on forks) don't countermand your assumption that the permissions are always minimal

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.

@woodruffw
Copy link
Copy Markdown
Author

What you do here is equivalent to using git = "...", commit = "..." in Cargo.toml, not to locking Cargo.lock. It makes it pretty annoying to update workflow dependencies and does not provide any meaningful security improvements.

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 main does so with a read-write cache token, even if you set all permissions to read-only on the repository level. If you read from caches in your publishing process (I did a scan for this, but it's difficult to fully statically assert), then an attacker who compromises a read only CI workflow can still pivot into compromising your publishing workflow through a poisoned cache.

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.

@newpavlov
Copy link
Copy Markdown
Member

newpavlov commented Apr 13, 2026

Perhaps I'm misunderstanding, but aren't you using Dependabot to manage these workflow dependency updates?

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.

then an attacker who compromises a read only CI workflow can still pivot into compromising your publishing workflow through a poisoned cache.

IIUC we do not use caching in our publish workflow, so attackers can not use this vector.

@woodruffw
Copy link
Copy Markdown
Author

Does it update the hash-pinned workflow dependencies? Then there is no security benefit, since we would just pull the most recent (potentially compromised) commit in the repo. It would only create unnecessary update noise.

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.

IIUC we do not use caching in our publish workflow.

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. actions/checkout doesn't use the GitHub Actions cache at the moment for example, but there's no reason it couldn't in the future.

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.

@newpavlov
Copy link
Copy Markdown
Member

As I wrote above, I think it may be worth to harden the publish workflow, but not everything else. We would have to apply the chosen approach to all our repos and I would prefer to not add additional maintenance headache if there is little to no benefit in it.

I will wait for @tarcieri to chime in before deciding what to do.

@tarcieri
Copy link
Copy Markdown
Member

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.

@newpavlov
Copy link
Copy Markdown
Member

newpavlov commented Apr 13, 2026

Ok, so we are generally in agreement. Let's keep only the publish changes then.

Although, I am not sure whether we need the permissions: {} part considering the repository-wide read-only setting. It also may be worth to exclude publish.yml from the Dependabot config or configure its updates to a longer (1-6 months) period schedule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants