security: ci: Fixes for Zizmor GHA security scanning #11475
Conversation
This fixes a Zizmor error where some variables are vulnerable to template injection. https://github.com/kata-containers/kata-containers/security/code-scanning/67 Signed-off-by: Aurélien Bombo <abombo@microsoft.com>
This removes the permission from the workflow since it's already present at the job level. https://github.com/kata-containers/kata-containers/security/code-scanning/111 Signed-off-by: Aurélien Bombo <abombo@microsoft.com>
This adds Zizmor GHA security scanning as a PR gate. Note that this does NOT require that Zizmor returns 0 alerts, but rather that Zizmor's invocation completes successfully (regardless of how many alerts it raises). I will set up the former after this commit is merged (through the GH UI). Signed-off-by: Aurélien Bombo <abombo@microsoft.com>
There was a problem hiding this comment.
Pull Request Overview
A security fix for CI configuration to mitigate alerts from Zizmor scanning and improve environment variable consistency in GitHub Actions workflows.
- Included a new required test for "GHA security analysis / zizmor" in the gatekeeper tests.
- Updated several release and deployment workflows to use environment-defined variables for improved clarity and secure parameter passing.
- Removed the "id-token: write" permission in the CI-on-push workflow to further restrict access.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/testing/gatekeeper/required-tests.yaml | Added required test for GHA security analysis (zizmor) |
| .github/workflows/release.yaml | Refactored helm registry login using environment variables |
| .github/workflows/release-s390x.yaml | Introduced TARGET_ARCH from inputs for tag construction |
| .github/workflows/release-ppc64le.yaml | Introduced TARGET_ARCH from inputs for tag construction |
| .github/workflows/release-arm64.yaml | Introduced TARGET_ARCH from inputs for tag construction |
| .github/workflows/release-amd64.yaml | Introduced TARGET_ARCH from inputs for tag construction |
| .github/workflows/publish-kata-deploy-payload.yaml | Switched from direct inputs to using env variables for registry, repo, and tag |
| .github/workflows/ci-on-push.yaml | Removed the unneeded "id-token: write" permission |
| @@ -6,7 +6,7 @@ required_tests: | |||
| - Shellcheck required / shellcheck-required | |||
| # TODO: cargo-deny-runner.yaml not yet treated as conditional | |||
| - Cargo Crates Check Runner / cargo-deny-runner | |||
|
|
|||
| - GHA security analysis / zizmor | |||
There was a problem hiding this comment.
If the PR isn't a workflow change and therefore the job isn't triggered, how will this interact with the required test?
There was a problem hiding this comment.
Hmm good point, and it seems like the code scanning protection rule is pretty basic, ie. "this tool needs to run or else": https://github.com/kata-containers/kata-containers/settings/rules/6331013
So how about we remove the path restriction from the zizmor action?
There was a problem hiding this comment.
I think the path restriction makes sense, so I wonder if we need to have a category of workflow checks (e.g. zizmor & action lint) that only are required when there is a workflow change?
There was a problem hiding this comment.
My issue is that this would only be supported by gatekeeper required tests, not GH UI required tests. GH would still expect (1) zizmor to run for each PR and (2) (I think) zizmor results to be collected for each PR.
There was a problem hiding this comment.
Yeah, I don't think we can make it required through github in the current state
There was a problem hiding this comment.
Agreed, in that case I vote to remove the path restriction since (1) the Zizmor job is relatively cheap and (2) the gatekeeper job is still not a required check either.
The way GH works, we can only require Zizmor results on ALL PR runs, or none, so remove the path filter. Signed-off-by: Aurélien Bombo <abombo@microsoft.com>
|
Forced-merged as workflow change. |
We still should have two approvals for PRs |
|
Argh sorry, completely overlooked that! |
This provides fixes for the alerts surfaced by our Zizmor scanning.
After this PR, only 2 alerts remain (re:
pull_request_target) which I plan to dismiss as wontfix: https://github.com/kata-containers/kata-containers/security/code-scanning?query=pr%3A11475+is%3Aopen+Test run: https://github.com/kata-containers/kata-containers/actions/runs/15908722729