Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint our GitHub Actions workflows with zizmor #2331

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Lint our GitHub Actions workflows with zizmor #2331

merged 1 commit into from
Dec 19, 2024

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Dec 12, 2024

Status

Ready for review

Description

zizmor is a new tool to lint GitHub Actions workflows. For the most part our workflows are pretty low risk since we don't give it a bunch of credentials, but we can avoid issues in the future by locking them down now.

Two overall issues needed to be fixed:

  • setting persist-credentials: false for actions/checkout, which we do everywhere except in the workflows that need to push.
  • Don't use template expansion when we can use a normal bash variable.

While zizmor is written in Rust, it is also shipped as a prebuilt binary via PyPI, so we can set it as a poetry dependency and run it as part of our normal lint CI.

Refs freedomofpress/securedrop-tooling#18.

Test Plan

  • visual review
  • CI passes
  • team informed about new tool, etc.

zizmor is a new tool to lint GitHub Actions workflows. For the most part
our workflows are pretty low risk since we don't give it a bunch of
credentials, but we can avoid issues in the future by locking them down
now.

Two overall issues needed to be fixed:
* setting persist-credentials: false for actions/checkout, which we do
everywhere except in the workflows that need to push.
* Don't use template expansion when we can use a normal bash variable.

While zizmor is written in Rust, it is also shipped as a prebuilt binary
via PyPI, so we can set it as a poetry dependency and run it as part of
our normal lint CI.

Refs <freedomofpress/securedrop-tooling#18>.
@legoktm legoktm added the CI label Dec 12, 2024
@legoktm legoktm requested a review from a team as a code owner December 12, 2024 01:36
@rocodes rocodes self-assigned this Dec 18, 2024
@rocodes
Copy link
Contributor

rocodes commented Dec 18, 2024

Thanks for preparing this. Since we'll have to do this in so many places/repos, I went down a little bit of a rabbithole (which I am sure you did too) about whether there was any way to parameterize + reuse a sane default configuration instead of making the change everywhere we use the checkout action.

Did you already test out + reject the idea of creating a custom github action that wraps checkout@v4 and has persist-credentials: false as an input, and then using that action everywhere across all our repos? (and then deprecating it if/when github eventually moves to a different default in actions@v5?)

This is kind of pseudocode so apologies if you already know it won't work, just trying to think about maintenance pain and standardizing across repos. But otherwise lgtm and fine to merge and help with the cross repo effort.

# reusable.yml

on:
  workflow_call:
    inputs:
      persist-credentials:
        type: string
        required: true
        default: false

jobs:
  no_creds_persist_action:
    [...]
    uses: actions/checkout@v4
    with: ${{ inputs.persist-credentials }}

and then call our "no persist checkout" action instead of directly calling actions/checkout@v4.

@legoktm
Copy link
Member Author

legoktm commented Dec 18, 2024 via email

Copy link
Contributor

@rocodes rocodes left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for preparing this.

@rocodes rocodes added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit b59ef3d Dec 19, 2024
58 checks passed
@rocodes rocodes deleted the zizmor branch December 19, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants