Skip to content

ci: add Zizmor workflow#379

Merged
james-garner-canonical merged 7 commits intocanonical:mainfrom
tonyandrewmeyer:ci-add-zizmor
Mar 23, 2026
Merged

ci: add Zizmor workflow#379
james-garner-canonical merged 7 commits intocanonical:mainfrom
tonyandrewmeyer:ci-add-zizmor

Conversation

@tonyandrewmeyer
Copy link
Copy Markdown
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Mar 21, 2026

This PR adds static workflow analysis using Zizmor. It uses the existing configuration file.

Also: injection attacks are avoided by adjusting the workflows to use environment variables.

Fixes #240

Drive-by: allow any version of Go, so that it doesn't need to be kept in sync with pebble@master (or other things in the future?). Pebble just bumped to 1.26, which is why it fails now.

Copy link
Copy Markdown
Collaborator

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds static workflow analysis using Zizmor. It uses the existing configuration file.

Thanks for this. I think we should also add zizmor as a a required check via the repo automation, WDYT?

Also: injection attacks are avoided by adjusting the workflows to use environment variables.

Fixes #240

Nice to finally fix all of these, thanks!

Drive-by: allow any version of Go, so that it doesn't need to be kept in sync with pebble@master (or other things in the future?). Pebble just bumped to 1.26, which is why it fails now.

Great fix, thanks!

@james-garner-canonical james-garner-canonical merged commit b38b735 into canonical:main Mar 23, 2026
312 checks passed
@tonyandrewmeyer
Copy link
Copy Markdown
Contributor Author

Thanks for this. I think we should also add zizmor as a a required check via the repo automation, WDYT?

Yes, that sounds reasonable. In the other Charm Tech repos I don't think it's that important because we have a team practice of not merging unless everything is green or there is some known issue (like the current microk8s one in Concierge). But here where there's CODEOWNERS and a lot of different people, it probably ought to be enforced, even though most of the groups wouldn't be able to cause failures, I suspect.

@tonyandrewmeyer tonyandrewmeyer deleted the ci-add-zizmor branch March 23, 2026 02:13
@james-garner-canonical
Copy link
Copy Markdown
Collaborator

Thanks for this. I think we should also add zizmor as a a required check via the repo automation, WDYT?

Yes, that sounds reasonable. In the other Charm Tech repos I don't think it's that important because we have a team practice of not merging unless everything is green or there is some known issue (like the current microk8s one in Concierge). But here where there's CODEOWNERS and a lot of different people, it probably ought to be enforced, even though most of the groups wouldn't be able to cause failures, I suspect.

Follow up question I was meaning to ask: does the action actually fail if the checks aren't satisfied, or does it just upload the results to be surfaced through the security checks UI? (Which did create (I think blocking) items on PRs already just via having the security checks turned on, without having added this action.)

@tonyandrewmeyer
Copy link
Copy Markdown
Contributor Author

Follow up question I was meaning to ask: does the action actually fail if the checks aren't satisfied, or does it just upload the results to be surfaced through the security checks UI? (Which did create (I think blocking) items on PRs already just via having the security checks turned on, without having added this action.)

See the second "important" box in this section of the docs. The short answer is that it'll not block, but you can (and I think should) configure a ruleset to block on security findings (any, not just zizmor). That looks like this in ops:

IMG_5361

Although it probably needs to be done by the repo automation, I think.

MichaelThamm pushed a commit to MichaelThamm/charmlibs that referenced this pull request Mar 23, 2026
This PR adds static workflow analysis using Zizmor. It uses the existing
configuration file.

Also: injection attacks are avoided by adjusting the workflows to use
environment variables.

Fixes canonical#240

Drive-by: allow any version of Go, so that it doesn't need to be kept in
sync with pebble@master (or other things in the future?). Pebble just
bumped to 1.26, which is why it fails now.

---------

Co-authored-by: James Garner <[email protected]>
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.

Fix remaining zizmor warnings

2 participants