Conversation
There was a problem hiding this comment.
Pull request overview
Adds Zizmor static analysis to the CI pipeline and applies a set of GitHub Actions hardening changes (least-privilege permissions, non-persisted checkout credentials, and reduced expression interpolation in shell) to address Zizmor findings.
Changes:
- Add a
make zizmortarget (with optional local install) and a dedicated.github/workflows/zizmor.ymlworkflow to run Zizmor on workflow changes. - Harden multiple workflows by setting minimal
permissionsandactions/checkoutwithpersist-credentials: false. - Refactor some workflow
run:scripts to use environment variables (andjq) instead of direct${{ ... }}interpolation where applicable.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds ZIZMOR_VERSION and a zizmor target to run the linter locally. |
| .github/workflows/zizmor.yml | New workflow to run Zizmor on .github/** workflow changes with minimal permissions. |
| .github/workflows/update-dependencies.yaml | Adds least-privilege permissions, disables persisted checkout credentials, and shifts to GitHub App token usage for Updatecli. |
| .github/workflows/security-audit-reactive.yml | Adds least-privilege permissions and disables persisted checkout credentials. |
| .github/workflows/security-audit-cron.yml | Adds least-privilege permissions and disables persisted checkout credentials. |
| .github/workflows/release.yml | Hardens permissions/checkout and refactors some scripting (env vars + jq) in release/sync steps. |
| .github/workflows/open-release-pr.yml | Adds minimal permissions, disables persisted checkout credentials, and keeps GitHub App token usage. |
| .github/workflows/fossa.yml | Switches to default permissions: {} + job contents: read, disables persisted checkout credentials. |
| .github/workflows/e2e-tests.yml | Adds job permissions, disables persisted checkout credentials, pins an action ref, and moves some expressions into env vars. |
| .github/workflows/ci.yml | Switches default permissions away from read-all, disables persisted checkout credentials across jobs, and moves expressions into env vars for the change-detection script. |
| .github/workflows/build-kwctl.yml | Adds job permissions and disables persisted checkout credentials in relevant jobs. |
| .github/workflows/build-containers.yml | Disables persisted checkout credentials and refactors tag propagation using env vars. |
| .github/workflows/attestation.yml | Refactors to use env vars for inputs/context (to reduce expression interpolation) in attestation retrieval/signing logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/attestation.yml
Outdated
| run: | | ||
| set -e | ||
| DIGEST=$(cat ${{ inputs.component }}-${{ inputs.arch }}.txt) | ||
| DIGEST=$(cat $COMPONENT-$ARCH.txt) |
There was a problem hiding this comment.
DIGEST=$(cat $COMPONENT-$ARCH.txt) is vulnerable to word-splitting/globbing if inputs ever contain unexpected characters, and can also break on components with -/spaces. Quote the path (and generally quote $COMPONENT/$ARCH expansions used in filenames) to make the script robust.
| DIGEST=$(cat $COMPONENT-$ARCH.txt) | |
| DIGEST="$(cat -- "$COMPONENT-$ARCH.txt")" |
| run: | | ||
| set -e | ||
| DIGEST=$(crane manifest ghcr.io/${{ github.repository_owner }}/${{ inputs.component }}@${{ env.DIGEST }} \ | ||
| DIGEST=$(crane manifest ghcr.io/$REPOSITORY_OWNER/$COMPONENT@$DIGEST \ | ||
| | jq -r '.manifests[] | ||
| | select(.annotations["vnd.docker.reference.type"] == "attestation-manifest") | ||
| | .digest') | ||
| if [[ -z "${DIGEST}" ]]; then |
There was a problem hiding this comment.
Several steps rely on set -e but use pipelines (e.g., crane manifest ... | jq ...). In bash, set -e alone won’t fail the step when the first command in a pipeline fails, which can mask network/registry errors. Add set -o pipefail (or set -euo pipefail) in these run blocks so failures in crane correctly fail the job.
Adds a new CI to run Zizmor linter. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com> Assisted-by: Github Copilot
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| INPUT_VERSION: ${{ inputs.version }} | ||
| run: | | ||
| echo TAG_NAME=${{ inputs.version }} >> $GITHUB_ENV | ||
| echo TAG_NAME=$INPUT_VERSION >> $GITHUB_ENV |
There was a problem hiding this comment.
echo TAG_NAME=$INPUT_VERSION >> $GITHUB_ENV writes a workflow input directly into $GITHUB_ENV without escaping. If inputs.version ever contains a newline (or other unexpected characters), it can inject additional environment entries. Prefer using a safe write pattern (e.g., printf for single-line values and/or validating inputs.version against an allowed tag/version regex before exporting).
| echo TAG_NAME=$INPUT_VERSION >> $GITHUB_ENV | |
| if ! printf '%s' "$INPUT_VERSION" | grep -Eq '^[0-9A-Za-z._-]+$'; then | |
| echo "Invalid version format: $INPUT_VERSION" >&2 | |
| exit 1 | |
| fi | |
| printf 'TAG_NAME=%s\n' "$INPUT_VERSION" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Hmm, I'm not sure about this. It make sense, but I think this is not required on this PR.
Makefile
Outdated
| zizmor: ## Run zizmor static analysis on GitHub Actions workflows | ||
| @which zizmor >/dev/null 2>&1 || cargo install --locked zizmor@$(ZIZMOR_VERSION) | ||
| zizmor .github/ |
There was a problem hiding this comment.
The zizmor target relies on which and a PATH side effect from cargo install. On systems without which, or where ~/.cargo/bin isn’t already in PATH, the install can succeed but the subsequent zizmor .github/ invocation still fails. Prefer command -v zizmor and install to a deterministic location (e.g., --root $(LOCALBIN) and run $(LOCALBIN)/bin/zizmor) or document/ensure the expected PATH.
.github/workflows/attestation.yml
Outdated
| DIGEST=$(cat $COMPONENT-$ARCH.txt) | ||
| echo "DIGEST=${DIGEST}" >> "$GITHUB_ENV" | ||
| - name: Find attestation digest | ||
| run: | | ||
| set -e | ||
| DIGEST=$(crane manifest ghcr.io/${{ github.repository_owner }}/${{ inputs.component }}@${{ env.DIGEST }} \ | ||
| DIGEST=$(crane manifest ghcr.io/$REPOSITORY_OWNER/$COMPONENT@$DIGEST \ | ||
| | jq -r '.manifests[] | ||
| | select(.annotations["vnd.docker.reference.type"] == "attestation-manifest") | ||
| | .digest') | ||
| if [[ -z "${DIGEST}" ]]; then |
There was a problem hiding this comment.
This workflow reuses the shell variable name DIGEST for multiple different values (image digest, attestation manifest digest, provenance digest, SBOM digest). It’s easy to accidentally reference the wrong one in future edits. Consider using distinct variable names per step (e.g., IMAGE_DIGEST, ATTESTATION_DIGEST, etc.) and/or quoting variable expansions when building image references to improve readability and reduce foot-guns.
There was a problem hiding this comment.
I think this is a good thing to have. But I do not see the need to do that in this PR
.github/workflows/release.yml
Outdated
| CARGO_VERSION=$(sed -n 's/^version[[:space:]]*=[[:space:]]*"\(.*\)"/\1/p' crates/kwctl/Cargo.toml) | ||
| POLICY_SERVER_VERSION=$(sed -n 's/^version[[:space:]]*=[[:space:]]*"\(.*\)"/\1/p' crates/policy-server/Cargo.toml) | ||
| TAG_VERSION=$(echo ${{ github.ref_name }} | sed 's/v//') | ||
| TAG_VERSION=$(echo "$GIT_REF_NAME" | sed 's/v//') |
There was a problem hiding this comment.
TAG_VERSION=$(echo "$GIT_REF_NAME" | sed 's/v//') removes all occurrences of v, not just a leading v (e.g., a tag like v1.0.0-preview would become 1.0.0-preie). If the intent is only to strip a leading v, prefer a more exact operation (e.g., shell parameter expansion or a sed anchored to ^v).
| TAG_VERSION=$(echo "$GIT_REF_NAME" | sed 's/v//') | |
| TAG_VERSION="${GIT_REF_NAME#v}" |
| GIT_REF_NAME: ${{ github.ref_name }} | ||
| run: | | ||
| echo TAG_NAME=$(echo ${{ github.ref_name }}) >> $GITHUB_ENV | ||
| echo TAG_NAME=$(echo "$GIT_REF_NAME") >> $GITHUB_ENV |
There was a problem hiding this comment.
The Retrieve tag name step uses echo TAG_NAME=$(echo "$GIT_REF_NAME") >> $GITHUB_ENV. The nested echo is redundant, and unquoted writes to $GITHUB_ENV can behave unexpectedly if the value ever contains characters that need escaping. Prefer writing the env var with a single, properly quoted/printf’d line.
| echo TAG_NAME=$(echo "$GIT_REF_NAME") >> $GITHUB_ENV | |
| printf 'TAG_NAME=%s\n' "$GIT_REF_NAME" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Ok, it's a good to have. But I think we can keep this for a future
.github/workflows/e2e-tests.yml
Outdated
|
|
||
| - name: "Install kwctl" | ||
| uses: kubewarden/github-actions/kwctl-installer@main | ||
| uses: kubewarden/github-actions/kwctl-installer@f301a7874dd642510fff54a89e4329881bf871ef # main |
There was a problem hiding this comment.
The kwctl-installer action is pinned to a commit SHA but the trailing comment says # main, which reads like it’s still tracking the mutable main branch. Consider updating the comment to the action’s release/tag (if available) or removing it to avoid confusion during future security reviews.
| uses: kubewarden/github-actions/kwctl-installer@f301a7874dd642510fff54a89e4329881bf871ef # main | |
| uses: kubewarden/github-actions/kwctl-installer@f301a7874dd642510fff54a89e4329881bf871ef |
7502774 to
61515d6
Compare
Fix all the warnings reported by Zizmor. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com> Assisted-by: Github copilot
Description
Adds a new CI to run Zizmor linter and fix all the warning reported by it
Fix #1534