ci: add zizmor to static check GitHub workflows

[tonyandrewmeyer]

Changes:

• Adds zizmor to pre-commit (for those using it) and as a workflow.
• Adjust the default permissions for the workflows to none - when more are needed, each job adds those explicitly (we don't need content:read because the repo is public).
• Set persist-credentials to false for all the checkouts, because we don't need to save those - we never do follow-up git commands.
• Put the charmcraft GitHub variables into environment variables and use them from there, which protects against bad expansion.

Many of the workflows are tested as part of this PR. I've tested most of the others in my fork - the exceptions are TIOBE, publishing (and test publishing), and updating charm pins. The changes are consistent across the workflows, so I don't expect any failures with those.

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[tonyandrewmeyer]

FYI, if you want to just run zizmor locally, you can do uvx zizmor followed by the workflow(s) you want to check. The docs obviously have more details.

[benhoyt]

I admit to being a little skeptical about adding a 3rd party tool for this, but I'm willing to try it. What do you think about the alternative of changing the repo Workflow Permissions to read-only instead?

[benhoyt]

It seems to me that if we set the top-level or repo-level permissions to none (read-only) we don't need this, right? Because even if they're persisted they won't be able to do anything but read. If that's the case, it seems like it adds a fair bit of noise.

[tonyandrewmeyer]

Security is best when layered. Admittedly, there's quite a bit of overlap in the layers here, but it doesn't seem like a lot of noise to me (when you look at the files in total, rather than a PR that's adding this line in several places). This is another case where it seems clear that this would have been the better default, but we can't fix that.

In terms of the actual risk, I believe you're correct, though - it doesn't seem like read-only credentials to a public repository are useful in any way to an attacker.

[benhoyt]

Fair. I've just changed the repo Workflow permissions to read-only as well.

[benhoyt]

Should we pin the version of zizmor, for stability/security?

[tonyandrewmeyer]

@james-garner-canonical is going to pin all the workflows, I believe, so I figured I'd leave it for then.

[benhoyt]

Where does this upload appear, and what does it include?

[tonyandrewmeyer]

That's mostly explained in this comment. It looks like this example. The GitHub docs have more information about SARIF and what's included in the file.

[tonyandrewmeyer]

I admit to being a little skeptical about adding a 3rd party tool for this,

I figured that would be the case, which is why I picked you as a reviewer :)

but I'm willing to try it.

For what it's worth, I've seen a lot of positive mentions of the tool over the last ~6 months, including recommendations from people whose opinion I trust, like Seth Larson (Python Security Developer in Residence), Mike Fiedler (PyPI maintainer), Ned Batchelder, Alex Gaynor, and Paul Kehrer. It's not an Astral tool but it's sponsored by Astral, which has some weight, I think.

I'm also reluctant to add new dependencies, even CI ones, but it seems like there's an active focus on attacking CI and that this is the most highly recommended static analysis tool. It can't do much damage (false security reports would be the only thing I think), and uses minimal resources (yet another Rust tool run with uvx).

What do you think about the alternative of changing the repo Workflow Permissions to read-only instead?

I think it's definitely worth addressing the risks that the tool identified. Switching the repo permissions would cover most of that. I think we should also do the recommended template injection remediation and change the trigger for the conventional commit check.

If we go that route instead then what about keeping it as a pre-commit tool (for those using pre-commit) and considering the CI another time instead? It'd be the .pre-commit-config.yaml file adjustment and probably a .github/zizmor.yaml config file (to include some exclusions, if we're going to rely solely on the repo permissions).

[benhoyt]

Thanks for the details. Let's give it a shot!

[dimaqq]

Suggested change

	pull_request_target: # zizmor: ignore[dangerous-triggers] Doesn't touch code - pull_request results in cancellation errors.
	# This trigger allows the action to access the PR content
	pull_request_target: # zizmor: ignore[dangerous-triggers] Doesn't touch code.

IMO the cancellation errors are a red herring, and I thought I fixed those by using the PR number in the concurrency.group

If the action only really checks the PR title in the github context, then either pull_request or pull_request_target should work.

Is the action uses the GitHub API to look into the PR, then it must run on ..._target.

Your test may be misleading, because you are a member of this repo.
We'd need a fresh, unconnected github account to test.