Skip to content

Conversation

@tongpu
Copy link
Member

@tongpu tongpu commented Jul 2, 2025

Description

This PR introduces zizmor to scan the security of our GitHub Actions

Issues

n/a

Checklist

  • This PR contains a description of the changes I'm making
  • I updated the version in Chart.yaml
  • I updated the changelog with an artifacthub.io/changes annotation in Chart.yaml, check the example in the documentation.
  • I updated applicable README.md files using pre-commit run
  • I documented any high-level concepts I'm introducing in docs/
  • CI is currently green and this is ready for review
  • I am ready to test changes after they are applied and released

@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2025
@github-advanced-security
Copy link

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.

@tongpu tongpu removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2025
@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2025
@tongpu tongpu force-pushed the ci/add_zizmor_eaction branch from f2a70bc to e61a004 Compare July 9, 2025 13:51
@tongpu tongpu self-assigned this Jul 9, 2025
@tongpu tongpu force-pushed the ci/add_zizmor_eaction branch from 858ed78 to 659c3eb Compare July 9, 2025 15:01
@tongpu tongpu marked this pull request as ready for review July 9, 2025 15:02
@tongpu tongpu requested a review from a team as a code owner July 9, 2025 15:02
@tongpu tongpu requested review from gianklug, hairmare and vmaillot July 9, 2025 15:02
@hairmare
Copy link
Contributor

hairmare commented Jul 9, 2025

Does this need some change to docs/?

@hairmare
Copy link
Contributor

hairmare commented Jul 9, 2025

we might also want to consider adding this: https://github.com/zizmorcore/zizmor-pre-commit

@tongpu
Copy link
Member Author

tongpu commented Jul 10, 2025

we might also want to consider adding this: https://github.com/zizmorcore/zizmor-pre-commit

Adding a pre-commit action would definitely make it more transparent, but then we would end up with both the zizmor pre-commit hook and the zizmor action in the pipeline.

Does this need some change to docs/?

We could use this as a starting point to improve the documentation of the actions we're using, because right now we don't explicitly documented any of them.

@hairmare
Copy link
Contributor

We could use this as a starting point to improve the documentation of the actions we're using, because right now we don't explicitly documented any of them.

sounds like a plan, right now a lot of it is implicitly covered by us using pre-commit for linting and doxing the actions setup properly would for sure be beneficial

Comment on lines +8 to +12
dangerous-triggers:
ignore:
# We need the pull_request_target trigger here to allow us to add labels
# to PRs. We restrict the permissions within the job to "pull-requests: write"
- .github/workflows/pr-sizing.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the warning you added to .github/workflows/pr-sizing.yaml.

I think we should do some more stringent review of our usage of pull_request_target, it just doesn't feel like a it's false positive enough to ignore without further analysis. We have "Require approval for all external contributors" enabled on the repo, but zizmor makes me question if that is enough.

Do we want to do an "appsec" style review in this thread or would we prefer doing this in a new issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's definitely not a false positive, but I would assume that changing it to pull_request would require us to configure some credentials to allow the pr size action to add labels to PRs.

I'm fine with doing an appsec review in this PR, since my goal was to improve the security of this repository and I'd rather do this all in one swoop. We might want to split out the simple fixes from this PR into another one and focus on the introduction of zizmor in this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

While preparing #1424 I realized that the pr-sizing action is not even checking out the source code, but instead getting the number of changed lines from the files in the PR. But that doesn't actually change the security stance much, since anyone could introduce a change to the workflow that would be executed in the context of this repository.

@tongpu tongpu mentioned this pull request Jul 15, 2025
7 tasks
@tongpu
Copy link
Member Author

tongpu commented Jul 15, 2025

I've opened #1424 with the obvious changes to improve the security of our actions. We can choose to implement the changes in #1424 and continue with the integration of the action and improvements to the pr-sizing action in here.

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

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants