Skip to content

feat: add zizmor static action checks#351

Closed
gforsyth wants to merge 5 commits intorapidsai:branch-25.08from
gforsyth:zizmor
Closed

feat: add zizmor static action checks#351
gforsyth wants to merge 5 commits intorapidsai:branch-25.08from
gforsyth:zizmor

Conversation

@gforsyth
Copy link
Copy Markdown
Contributor

This is an experiment in using zizmor (https://docs.zizmor.sh/) to run static analysis on all of the shared workflow files. This seems like a nice tool to add, especially in this repository.

Some of the classes of errors/warnings that are fixed here (and any associated configuration):

unpinned-uses

https://docs.zizmor.sh/audits/#unpinned-uses

Any uses: mapping should pin to an exact hash to avoid things getting swapped out underneath.
Since we deliberately make use of some of the uses: to reference our own shared-workflows and nv-gha-runners -- these are allowed to point at refs. Those patterns are configured in .github/zizmor.yml

artipacked

https://docs.zizmor.sh/audits/#artipacked

We should always explicitly set persist-credentials, either to true or false. Everywhere that the checkout action has implicitly set it to true, I have made it an explicit true.

I suspect some of these need to be true, but probably not all of them. I'll have to do some testing downstream to see which actions break when those secrets are disabled.

overprovisioned-secrets

https://docs.zizmor.sh/audits/#overprovisioned-secrets

Looking up secrets like secrets[some_key] injects the entire secrets context into the runner, but I don't see a way around that for our current secret lookup patterns. I've just ignored these instances individually.

unpinned-images

https://docs.zizmor.sh/audits/#unpinned-images

The only unpinned images belong to us and those have been individually allowlisted.

template-injection

https://docs.zizmor.sh/audits/#template-injection

Generally speaking lines like run: ${{ inputs.script }} are susceptible to template injection. Also, that's the entire point of these lines, so they are individually allowlisted.

For other potentially injectable lines, like run: rapids-wheels-anaconda "${{ inputs.package-name }}" "${{ inputs.package-type }}" , I've unpacked the inputs in to the environment and passed through those env-vars instead.

@gforsyth gforsyth added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 15, 2025
@gforsyth gforsyth changed the title fix: pin non-nvidia actions to hashes feat: add zizmor static action checks May 15, 2025
@msarahan
Copy link
Copy Markdown
Contributor

Moving from v4 to sha/hash is a good way to reduce surprises, but it makes it harder to judge at a glance whether a given image should be updated. How does it work with automatic image bump tools? Maybe it would be sufficient to include a comment about what human-readable version the hash represents.

@gforsyth
Copy link
Copy Markdown
Contributor Author

Good points @msarahan - I think dependabot can be configured to update images to the latest hash instead of the latest tag, although finding the documentation for how to do that is proving a bit tricky.

fix up build-in-devcontainer

chore: add more ignores to various expected unpinned images

fix(project-set): stop inheriting blanket secrets

fix(wheels): address zizmor concerns in wheel files

feat: add zizmor pre-commit hook

fix: commit zizmor config file
@jameslamb
Copy link
Copy Markdown
Member

I think dependabot can be configured to update images to the latest hash instead of the latest tag, although finding the documentation for how to do that is proving a bit tricky.

You might find the links and other info at https://github.com/rapidsai/build-infra/issues/204 useful (private issue because security, sorry). I also could not find docs on how to do that with dependabot, but there are some examples there of folks who are doing it.

@gforsyth
Copy link
Copy Markdown
Contributor Author

Thanks, @jameslamb ! From what I've seen, I think dependabot updates actions based on how they're currently specified, so if we switch to using the full SHA, it will update that in-place.

@jameslamb
Copy link
Copy Markdown
Member

Cool, cool, that makes sense.

it makes it harder to judge at a glance whether a given image should be updated

For this point, I believe dependabot will automatically update a trailing same-line comment pointing to a tag. See dependabot/dependabot-core#5951

so e.g. if you have:

- uses: actions/checkout@abc123 # v4.2.1

It'll propose an update like:

- uses: actions/checkout@def456 # v4.2.2

That'll help with readability, I think.

@gforsyth gforsyth changed the base branch from branch-25.06 to branch-25.08 June 30, 2025 19:25
@gforsyth gforsyth closed this Jun 30, 2025
gforsyth added a commit that referenced this pull request Jul 16, 2025
This is an experiment in using `zizmor` (https://docs.zizmor.sh/) to run static analysis on all of the shared workflow files.  This seems like a nice tool to add, especially in this repository.

Some of the classes of errors/warnings that are fixed here (and any associated configuration):

### unpinned-uses
https://docs.zizmor.sh/audits/#unpinned-uses

Any `uses:` mapping should pin to an exact hash to avoid things getting swapped out underneath.
Since we deliberately make use of some of the `uses:` to reference our own `shared-workflows` and `nv-gha-runners` -- these are allowed to point at refs.  Those patterns are configured in `.github/zizmor.yml`

### artipacked
https://docs.zizmor.sh/audits/#artipacked

We should always explicitly set `persist-credentials`, either to `true` or `false`.  Everywhere that the `checkout` action has implicitly set it to `true`, I have made it an explicit `true`.

I suspect some of these need to be true, but probably not all of them.  I'll have to do some testing downstream to see which actions break when those secrets are disabled.

### overprovisioned-secrets
https://docs.zizmor.sh/audits/#overprovisioned-secrets

Looking up secrets like `secrets[some_key]` injects the entire secrets context into the runner, but I don't see a way around that for our current secret lookup patterns. I've just ignored these instances individually.

### unpinned-images
https://docs.zizmor.sh/audits/#unpinned-images

The only unpinned images belong to us and those have been individually allowlisted.


### template-injection
https://docs.zizmor.sh/audits/#template-injection

Generally speaking lines like `run: ${{ inputs.script }}` are susceptible to template injection.  Also, that's the entire point of these lines, so they are individually allowlisted.

For other potentially injectable lines, like `run: rapids-wheels-anaconda "${{ inputs.package-name }}" "${{ inputs.package-type }}" `, I've unpacked the inputs in to the environment and passed through those env-vars instead.

In #351, @msarahan and @jameslamb and I determined that we can use full SHAs to specify the image to use for a given step, but leave a trailing same-line comment that points to the corresponding tag.

That let's us use the more secure exact pin for the images that we don't control, but still keeps it human-readable and compatible with dependabot, e.g.

```
- uses: actions/checkout@def456 # v4.2.2

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

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants