workflows: some zizmor issues fixes #2641

wainersm · 2025-10-17T17:55:31Z

Converted all workflows to checkout sources with persist-credentials: false
Removed a bunch of template injection warns

Ran "daily e2e test" in https://github.com/wainersm/cc-cloud-api-adaptor/actions/runs/18598214127 and apparently not issues caused by these changes.

zizmor flags that code checkout with persist credentials is a security risk. Default behavior of actions/checkout is to always persist so zizmor has complained about that in a lot of our workflows. We don't need to persist the credentials, hence, explicitly setting `persist-credentials: false` in all workflows that checkout code. Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

Fix a lot of template injection issues flagged by zizmor on the github workflows. The fix pattern is to export the input/matrix/secret/var in form of environment variables for the scripts. Assisted-by: `zizmor --fix` and Cursor Signed-off-by: Wainer dos Santos Moschetta <[email protected]>

wainersm · 2025-10-17T17:57:11Z

@mkulke the azure-e2e-test workflow is almost completely free of template injection issues. I ran out of energy to fix the one related with the calls to format.

@stevenhorsman @ldoktor please reviews :)

mkulke · 2025-10-18T07:35:31Z

@mkulke the azure-e2e-test workflow is almost completely free of template injection issues. I ran out of energy to fix the one related with the calls to format.

@stevenhorsman @ldoktor please reviews :)

Hey @wainersm, where did you see the zizmor warnings/errors? zizmor in the CI seems to be content w/ the workflows, see here.

Are our settings too relaxed? If so, we probably should adjust them also in this PR? Looking at the changes. I'm not sure what template injection issues are being addressed. Usually an injection can happen if you parse untrusted strings, and when you do this (in our current configuration) zizmor complains about, which is good.

strings that are in github.secrets/vars are usually not untrusted, they are controlled by the repo owner (and zizmor can tell the difference), but well, you can argue... however e.g. this is not about injection at all:

The string is expanded within the yaml somewhere at github, there is nothing that would be injected from either trusted or untrusted sources.

So, I'm on the fence, if this is considered good practice nowadays, let's do it. But in any case, this should be enforced by the CI.

wainersm · 2025-10-20T12:59:25Z

@mkulke the azure-e2e-test workflow is almost completely free of template injection issues. I ran out of energy to fix the one related with the calls to format.
@stevenhorsman @ldoktor please reviews :)

Hey @wainersm, where did you see the zizmor warnings/errors? zizmor in the CI seems to be content w/ the workflows, see here.

Hi @mkulke !

I ran zizmor .github/workflows/* -p locally to get a list of complains, where -p is pedantic. I just realized our zizmor CI workflow is configured with auditor persona which should produce less warnings.

The report that you pointed here is green but it generated a report. I think it will only raise a fail if the changes in the PR touches a block of code that contains a warning. In other words, our zizmor workflow is configured to catch new issues on the code base.

Are our settings too relaxed? If so, we probably should adjust them also in this PR? Looking at the changes. I'm not sure what template injection issues are being addressed. Usually an injection can happen if you parse untrusted strings, and when you do this (in our current configuration) zizmor complains about, which is good.

Sorry I didn't share the actual rule: https://docs.zizmor.sh/audits/#template-injection I'm addressing.

Basically zizmor complains any use of ${{ }} within run step because templates are expanded in the shell script without any verification, so potentially a user could inject malicious code in the script.

It complains even when it is read values from repo owner controlled variables, for example, var and secret that I don't see how it could be exploited but still zizmor complains. Likewise, values from matrix are also flagged as potential template injecting vectors.

strings that are in github.secrets/vars are usually not untrusted, they are controlled by the repo owner (and zizmor can tell the difference), but well, you can argue... however e.g. this is not about injection at all:

The string is expanded within the yaml somewhere at github, there is nothing that would be injected from either trusted or untrusted sources.

This is the warning about jitter can be found in the same report that you shared above, in https://github.com/confidential-containers/cloud-api-adaptor/actions/runs/18603338548/job/53046857607#step:3:2852

So, I'm on the fence, if this is considered good practice nowadays, let's do it. But in any case, this should be enforced by the CI.

I see some options:

defer the fix of these warnings for when we change a block of code that contains it
proactively fix the warnings but only these that represent a real risk
proactively fix everything

IMO, we should pursue (2) but sometimes it's easier to just fix everything (3) (although you increase chances of breaking workflows...)

I can split this PR in two if that helps. At begin I wanted to address the persist-credentials warnings as I discussed with @ldoktor and @stevenhorsman in #2607 (comment) , which the first commit, but then I think I got too happy because it was Friday and started fixing the warnings about the template stuff :)

mkulke · 2025-10-20T13:24:24Z

@mkulke the azure-e2e-test workflow is almost completely free of template injection issues. I ran out of energy to fix the one related with the calls to format.
@stevenhorsman @ldoktor please reviews :)

Hey @wainersm, where did you see the zizmor warnings/errors? zizmor in the CI seems to be content w/ the workflows, see here.

Hi @mkulke !

I ran zizmor .github/workflows/* -p locally to get a list of complains, where -p is pedantic. I just realized our zizmor CI workflow is configured with auditor persona which should produce less warnings.

thx for explaining, @wainersm. yeah it probably makes sense to split the commit: I'm not sure we have to use -pedantic, If we run zizmor with the default, the injections problems are still caught:

...
error[template-injection]: code injection via template expansion
  --> .github/workflows/webhook_image.yaml:87:38
   |
73 |         run: |
   |         ^^^ this run block
74 |           tags="${{ inputs.image_tags }}"
...
86 |             echo "::group::Push latest"
87 |             make docker-push IMG=${{ inputs.registry }}/peer-pods-webhook:latest
   |                                      ^^^^^^^^^^^^^^^ may expand into attacker-controllable code
   |
   = note: audit confidence → High
   = note: this finding has an auto-fix

there are already a lot of findings with the default setting

208 findings (100 suppressed, 93 fixable): 0 unknown, 10 informational, 11 low, 46 medium, 41 high

maybe it makes sense if we fix those and then it would match the settings that we enforce in the CI.

mkulke · 2025-10-20T13:28:48Z

I can do the azure workflow files (I'll open a separate PR)

wainersm · 2025-10-20T15:24:50Z

Hi @mkulke ,

Sending 1st commit in a separated PR: #2643

I agree, the default zizmor configuration (with auditor persona) should catch all the security-related issues, that matters most I think.

stevenhorsman

The changes look okay to me, but impact the workflows, so any chance we can run what we can as an upstream branch to check them before merging?

stevenhorsman · 2025-10-22T15:27:56Z

.github/workflows/webhook_image.yaml

          fi
+        env:
+          IMAGE_TAGS: ${{ inputs.image_tags }}
+          REGISTRY: ${{ inputs.registry }}


Can we get the new line back?

stevenhorsman · 2025-10-22T15:29:12Z

action lint is also unhappy with some of the changes

ldoktor · 2025-10-27T06:43:06Z

.github/workflows/azure-e2e-test.yml

+        AZURE_RESOURCE_GROUP: ${{ vars.AZURE_RESOURCE_GROUP }}
      run: |
-        NODE_RG="$(az aks show -g ${{ vars.AZURE_RESOURCE_GROUP }} -n "$CLUSTER_NAME" --query nodeResourceGroup -o tsv)"
+        NODE_RG="$(az aks show -g ${AZURE_RESOURCE_GROUP} -n "$CLUSTER_NAME" --query nodeResourceGroup -o tsv)"


There is an unrelated previously existing inconsistency in $CLUSTER_NAME -> ${CLUSTER_NAME}. Could you address that as a new commit (or do we not care?)

ldoktor · 2025-10-27T06:45:54Z

.github/workflows/azure-e2e-test.yml

    - name: Create peerpod subnet
      env:
        AZURE_LOCATION: ${{ matrix.parameters.location }}
+        AZURE_RESOURCE_GROUP: ${{ vars.AZURE_RESOURCE_GROUP }}


The AZURE_RESOURCE_GROUP repeats quite often, wouldn't it be better to move it to workflow env?

ldoktor · 2025-10-27T06:53:42Z

.github/workflows/build.yaml

-          fi
+          make build
+        env:
+          RELEASE_BUILD: ${{ matrix.type == 'release' && true || false }}


This should work (according to AI) but wouldn't it be safer to quote the "true" and "false" since we expect string? IMO it'd be more explicit.

ldoktor

Overall looks good, thanks for the separation and nice commit descriptions :-). I left a few little notes, but only the @stevenhorsman's extra line and shellcheck/gha workflow issues seem important (perhaps even the true->"true" but not super important).

There are quite a few changes so it'd indeed be nice to commit it to a new tag to get full ci execution before merging (although git revert works as well ;-) )

ldoktor · 2025-10-27T07:02:22Z

.github/workflows/caa_build_and_push_per_arch.yaml

-          dev_tags=${{ inputs.dev_tags }}
-          release_tags=${{ inputs.release_tags }}
+          dev_tags=${DEV_TAGS}
+          release_tags=${RELEASE_TAGS}


The shellcheck is right, there are several places lacking quotation like here...

wainersm added 2 commits October 17, 2025 13:01

wainersm requested a review from a team as a code owner October 17, 2025 17:55

wainersm added the CI Issues related to CI workflows label Oct 17, 2025

wainersm mentioned this pull request Oct 20, 2025

workflows: do not persist credentials on checkout #2643

Merged

stevenhorsman approved these changes Oct 22, 2025

View reviewed changes

ldoktor reviewed Oct 27, 2025

View reviewed changes

workflows: some zizmor issues fixes #2641

Are you sure you want to change the base?

workflows: some zizmor issues fixes #2641

Uh oh!

Conversation

wainersm commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wainersm commented Oct 17, 2025

Uh oh!

mkulke commented Oct 18, 2025

Uh oh!

wainersm commented Oct 20, 2025

Uh oh!

mkulke commented Oct 20, 2025

Uh oh!

mkulke commented Oct 20, 2025

Uh oh!

wainersm commented Oct 20, 2025

Uh oh!

stevenhorsman left a comment

Choose a reason for hiding this comment

Uh oh!

stevenhorsman Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

stevenhorsman commented Oct 22, 2025

Uh oh!

ldoktor Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

ldoktor Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

ldoktor Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

ldoktor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldoktor Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wainersm commented Oct 17, 2025 •

edited

Loading

ldoktor left a comment •

edited

Loading