Skip to content

ci: Run zizmor for GHA security analysis - #11392

Merged
sprt merged 1 commit into
mainfrom
sprt/zizmor
Jun 26, 2025
Merged

ci: Run zizmor for GHA security analysis#11392
sprt merged 1 commit into
mainfrom
sprt/zizmor

Conversation

@sprt

@sprt sprt commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

This runs the zizmor security lint [1] on our GH Actions.
The initial workflow uses [2] as a base.

[1] https://docs.zizmor.sh/
[2] https://docs.zizmor.sh/usage/#use-in-github-actions

@sprt
sprt marked this pull request as ready for review June 9, 2025 15:23
@sprt
sprt requested review from Copilot and stevenhorsman June 9, 2025 15:26
Comment thread .github/workflows/zizmor.yaml Outdated
jobs:
zizmor:
runs-on: ubuntu-22.04
permissions: {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if we need the job-level explicit permissions block as well as the top-level, but it doesn't do any harm

@stevenhorsman stevenhorsman Jun 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interestingly this might be the reason that the report isn't showing up in fact given the copilot review which says it need more permissions to report?

@sprt sprt Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMO the workflow should still show up. I think Copilot is confused - per zizmor's docs, we only need security-events: write when Advanced Security is enabled. I had disabled it because I thought it'd would fail on our repo, but I just tried enabling it again - let's see.

https://docs.zizmor.sh/usage/#use-in-github-actions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah ok - thanks!

This comment was marked as outdated.

@stevenhorsman

stevenhorsman commented Jun 9, 2025

Copy link
Copy Markdown
Member

Note that since this is a new workflow, it won't appear in this initial PR (even with the pull_request event).

Do you know the reason for this as when I've added pull_request workflows before they have shown up e.g. #10631 and #10958 ? My understanding was that with the pull_request target the workflows are obtained from the source branch?

@sprt

sprt commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

@stevenhorsman Hmm good point, I'm not exactly sure. Maybe because you had workflow_dispatch? Maybe you ran those manually before submitting the PRs? Some people are reporting the same behavior as here in [1], and [2] says "Some events also require the workflow file to be present on the default branch of the repository in order to run."

[1] https://github.com/orgs/community/discussions/25746
[2] https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/triggering-a-workflow#about-workflow-triggers

@sprt
sprt force-pushed the sprt/zizmor branch 2 times, most recently from a8294ba to 1f3607f Compare June 9, 2025 16:51
@sprt
sprt requested a review from Copilot June 9, 2025 16:54

This comment was marked as outdated.

@sprt sprt mentioned this pull request Jun 9, 2025
25 tasks
@github-advanced-security

Copy link
Copy Markdown

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.

@sprt
sprt force-pushed the sprt/zizmor branch 2 times, most recently from 051b052 to e278bab Compare June 12, 2025 14:21
@sprt
sprt requested review from Copilot and stevenhorsman June 12, 2025 14:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new GitHub Actions workflow to run Zizmor security analysis on pull requests that change workflow files.

  • Introduces .github/workflows/zizmor.yaml to trigger on PR changes to workflow definitions
  • Configures scoped permissions and concurrency for the Zizmor job
  • Adds steps to check out the repo and invoke the zizmor-action
Comments suppressed due to low confidence (2)

.github/workflows/zizmor.yaml:1

  • [nitpick] The workflow name is generic; consider renaming to something like Zizmor Security Analysis for clearer identification in the Actions UI.
name: GHA security analysis

.github/workflows/zizmor.yaml:23

  • [nitpick] Pinning to a commit SHA is stable but hampers readability and upgrades. Consider using a semver tag such as actions/checkout@v4 or the specific v4.2.2 tag instead.
        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

Comment thread .github/workflows/zizmor.yaml Outdated
@stevenhorsman

Copy link
Copy Markdown
Member

Is there anyway we can flag this as failing if there is any issues, or even better if there are issues above a certain threshold, so we can spot issues in PRs before they are merged?

@sprt

sprt commented Jun 12, 2025

Copy link
Copy Markdown
Contributor Author

Is there anyway we can flag this as failing if there is any issues, or even better if there are issues above a certain threshold, so we can spot issues in PRs before they are merged?

Yes! That will be my next step - see the second Important block here: https://github.com/zizmorcore/zizmor-action?tab=readme-ov-file#usage-with-github-advanced-security-recommended

@stevenhorsman stevenhorsman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks! It will be good to see if we can get the ruleset to give us more value as we work to squash these problems and stop them coming back.

@sprt
sprt force-pushed the sprt/zizmor branch 2 times, most recently from cf81127 to 9b2d50f Compare June 13, 2025 18:26
@stevenhorsman stevenhorsman added the security Potential or actual security issue label Jun 20, 2025

@gkurz gkurz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

Thanks @sprt !

@sprt
sprt force-pushed the sprt/zizmor branch 2 times, most recently from 7951570 to f372955 Compare June 20, 2025 19:53
This runs the zizmor security lint [1] on our GH Actions.
The initial workflow uses [2] as a base.

[1] https://docs.zizmor.sh/
[2] https://docs.zizmor.sh/usage/#use-in-github-actions

Signed-off-by: Aurélien Bombo <abombo@microsoft.com>
@stevenhorsman

Copy link
Copy Markdown
Member

I think my suggestion in the AC meeting was actually wrong - I thought that we using if: ${{ inputs.skip-test != 'yes' }} to check this and hence it was skipped, but that's not the case. Maybe it's related to the actions allowlist?

@stevenhorsman

Copy link
Copy Markdown
Member

Maybe it's related to the actions allowlist?

That looks fine to me 😕

@stevenhorsman

stevenhorsman commented Jun 26, 2025

Copy link
Copy Markdown
Member

Ok, I'm confused now as I realized that this workflow is running: https://github.com/kata-containers/kata-containers/actions/runs/15898700799/job/44836322438?pr=11392. What was the problem you were facing @sprt as I guess I misunderstood?

@sprt

sprt commented Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

@stevenhorsman No worries we but misunderstood each other, was early for me! My issue is there's a bunch of required tests that are just not running (and not pending on anything either):
image

@sprt

sprt commented Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

So maybe indeed those aren't running because the gatekeeper is seeing a workflow-only change?

@stevenhorsman

Copy link
Copy Markdown
Member

So maybe indeed those aren't running because the gatekeeper is seeing a workflow-only change?

Yeah, that's right. Gatekeeper is green: https://github.com/kata-containers/kata-containers/actions/runs/15898700802/job/44836322457?pr=11392, so I say use admin rights to merge it 😄

@sprt
sprt merged commit a1aa3e7 into main Jun 26, 2025
@sprt
sprt deleted the sprt/zizmor branch June 26, 2025 13:55
@sprt

sprt commented Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

Force-merged as discussed above.

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

Labels

ok-to-test security Potential or actual security issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants