Add zizmor to pre-commit

[hugovk]

https://github.com/woodruffw/zizmor is a static analysis tool for GitHub Actions and can find potential security issues.

The one found here are all straightforward:

       - uses: actions/checkout@v4
+        with:
+          persist-credentials: false

[bwoodsend]

Meh, seems a little blunt. Given that the only dangerous permission those SSH keys have is write access to this repository, surely it's more meaningful just to turn that permission off in settings? Beyond that, I can only see persist-credentials: false making sense for private repositories.

[hugovk]

Write access to the repo means you can deploy to PyPI, because we're using Trusted Publishing without a dedicated "environment" for manual approvals: https://docs.pypi.org/trusted-publishers/security-model/

Perhaps we should add an environment?

My thinking about fixing these was that when the tool is ready to be used in pre-commit, we can add it there, because it would have helped against some of the problems that led to the ongoing Ultralytics vulnerability (note this was written by the author of zizmor):

https://blog.yossarian.net/2024/12/06/zizmor-ultralytics-injection

And after I opened this, zizmor is now ready for pre-commit (https://github.com/woodruffw/zizmor-pre-commit), so I've added it to this PR.

---

Given that the only dangerous permission those SSH keys have is write access to this repository, surely it's more meaningful just to turn that permission off in settings?

Yeah, can do. As those settings are only visible to admins, I think it's also good to have safe values in the code as well.

Beyond that, I can only see persist-credentials: false making sense for private repositories.

We could revert these workflow changes, and ignore that rule in the config?

https://woodruffw.github.io/zizmor/configuration/#rulesidignore

Or we can also just close this PR if you prefer?

[bwoodsend]

We could revert these workflow changes, and ignore that rule in the config?

Yeah, that would appease my nose wrinkling.

Perhaps we should add an environment?

Hmm, maybe. I don't know what that would block. Would Renovate still be able to verify PRs for example.

TBH, the more I read about trusted publishing, the more I feel that we should just go back to downloading the wheels as artifacts then uploading them locally via twine. Not being able to tell when a workflow can invisibly use $GITHUB_TOKEN to gain an OIDC to log into PyPI is horrible. There's no transparency. The rules are complex and we appear to be plugging loopholes using indirect workarounds. Even the old way of taking the token from ${{ secrets.pypi_password }} felt more secure since at least then I knew that the secret is undefined for pull requests from forks.

[woodruffw]

Even the old way of taking the token from ${{ secrets.pypi_password }} felt more secure since at least then I knew that the secret is undefined for pull requests from forks.

FWIW: the absence of a secret in a workflow definition doesn't mean it's not present in that workflow's runner's environment. GitHub's runner agent doesn't distinguish between "used" and "unused" secrets when loading them into a runner: they all get loaded, and any code on that runner can dump them all from memory. That's why the Ultralytics attacker was able to pivot and steal a PyPI API token, despite not compromising a workflow that had one listed.

The thing that keeps pull requests from getting access to secrets.* is that the pull_request event causes a different set of secrets (ones for the fork, not the upstream) to get loaded. This is why pull_request_target is so dangerous: it undoes that upstream/fork split and gives the fork access to the upstream's secrets.

TL;DR: every workflow that runs in your upstream repo context has access to your secrets, whether you explicitly include them or not. That's why Trusted Publishing isn't based on secrets, and it's also why the docs encourage people to configure an (actions, not process) environment for the Trusted Publishing workflow 🙂

[woodruffw]

(And to answer the question: adding an environment wouldn't block anything, except whichever workflow you want it to. If your Renovate job isn't in the same workflow as your release job, the two won't interact.)

[hugovk]

Updated to ignore this rule.

[hugovk]

I've reviewed the tokens we have at https://github.com/ultrajson/ultrajson/settings/secrets/actions

• CODECOV_TOKEN can stay.

• There was also PYPI_PASSWORD which I have deleted; its corresponding API token had already been deleted from PyPI some time ago.

(Here's an idea, leave fake credentials in the GH settings to waste the time of attackers! 🙃)