ci: resolve zizmor findings in github action workflows

[siddu-os]

This PR fixes 7 template injection vulnerabilities across 2 workflow files identified by zizmor security scanner.

Problem:
The workflows were directly interpolating user-controlled inputs into shell commands without proper sanitization. This creates a security risk where malicious input containing shell metacharacters could potentially be used for command injection attacks.

Vulnerable Code Locations
podvm_mkosi.yaml (6 issues):
Line 174: image=${{ inputs.registry }}/podvm-generic-fedora - Direct interpolation in shell variable
Line 178: image=${image}-${{ inputs.arch }} - Direct interpolation in shell variable
Line 183: if [ -n "${{ inputs.image_tag }}" ] && [ "${{ inputs.image_tag }}" != "${tag}" ] - Direct interpolation in conditional (2 occurrences)
Line 184: oras push "${image}:${{ inputs.image_tag }}" - Direct interpolation in oras command
Line 221: echo "image=${{ inputs.registry }}/podvm-docker-image-${arch}:${tag}" - Direct interpolation in output variable

e2e_libvirt.yaml (1 issue):
Line 112: oras pull ${{ inputs.podvm_image }} - Direct interpolation in oras command

Solution
Following GitHub Actions security best practices, all template expressions have been moved to environment variables in the env: block. GitHub Actions automatically escapes environment variable values, preventing injection attacks.

Changes Made
podvm_mkosi.yaml:

Added env: block:
env:
REGISTRY: ${{ inputs.registry }}
ARCH: ${{ inputs.arch }}
IMAGE_TAG: ${{ inputs.image_tag }}

Replaced template expressions:
${{ inputs.registry }} → ${REGISTRY} (2 occurrences)
${{ inputs.arch }} → ${ARCH} (1 occurrence)
${{ inputs.image_tag }} → ${IMAGE_TAG} (3 occurrences)

e2e_libvirt.yaml:
Added env: block:
env:
PODVM_IMAGE: ${{ inputs.podvm_image }}

Replaced template expression:
${{ inputs.podvm_image }} → ${PODVM_IMAGE}

References
Zizmor Documentation: https://docs.zizmor.sh/audits/#template-injection

Security Impact
This change eliminates potential command injection vulnerabilities while maintaining the same functionality. The workflows will continue to work exactly as before, but with improved security posture.

[stevenhorsman]

LGTM. Thanks

[mkulke]

Can we make sure that this is is enforced continuously by the CI?

[stevenhorsman]

Can we make sure that this is is enforced continuously by the CI?

Are you proposing that we set advanced-security: false in the workflow, so that issues found are printed in the console (and I think then the job fails), rather than being raised in the github security tab?

[mkulke]

Can we make sure that this is is enforced continuously by the CI?

Are you proposing that we set advanced-security: false in the workflow, so that issues found are printed in the console (and I think then the job fails), rather than being raised in the github security tab?

not sure, don't we have a zizmor workflow that processes all workflows? it feels a bit odd that we manually address individual problems, what if the next person refactors something and re-introduces a template injection?

[stevenhorsman]

Can we make sure that this is is enforced continuously by the CI?

Are you proposing that we set advanced-security: false in the workflow, so that issues found are printed in the console (and I think then the job fails), rather than being raised in the github security tab?

not sure, don't we have a zizmor workflow that processes all workflows? it feels a bit odd that we manually address individual problems, what if the next person refactors something and re-introduces a template injection?

We do have a zizmor workflow - as I said the default behaviour of it is to just report into https://github.com/confidential-containers/cloud-api-adaptor/security/code-scanning?query=is%3Aopen+branch%3Amain+tool%3Azizmor, which hides things rather, so we could switch as I suggested to get a failure to show up on the CI job if there are issues. We will have quite a lot of failures there as we seem to have ~75 issues at the moment, but if it means we clean them up more, it would be a good thing.

[mkulke]

Can we make sure that this is is enforced continuously by the CI?

Are you proposing that we set advanced-security: false in the workflow, so that issues found are printed in the console (and I think then the job fails), rather than being raised in the github security tab?

not sure, don't we have a zizmor workflow that processes all workflows? it feels a bit odd that we manually address individual problems, what if the next person refactors something and re-introduces a template injection?

We do have a zizmor workflow - as I said the default behaviour of it is to just report into https://github.com/confidential-containers/cloud-api-adaptor/security/code-scanning?query=is%3Aopen+branch%3Amain+tool%3Azizmor, which hides things rather, so we could switch as I suggested to get a failure to show up on the CI job if there are issues. We will have quite a lot of failures there as we seem to have ~75 issues at the moment, but if it means we clean them up more, it would be a good thing.

oof, I see. that would be more red ci, but yeah, I don't think it makes sense to keep openeing PRs for individual files. usually those are fixable quickly and we could assign an agent to the issue.

[stevenhorsman]

Can we make sure that this is is enforced continuously by the CI?

Are you proposing that we set advanced-security: false in the workflow, so that issues found are printed in the console (and I think then the job fails), rather than being raised in the github security tab?

not sure, don't we have a zizmor workflow that processes all workflows? it feels a bit odd that we manually address individual problems, what if the next person refactors something and re-introduces a template injection?

We do have a zizmor workflow - as I said the default behaviour of it is to just report into https://github.com/confidential-containers/cloud-api-adaptor/security/code-scanning?query=is%3Aopen+branch%3Amain+tool%3Azizmor, which hides things rather, so we could switch as I suggested to get a failure to show up on the CI job if there are issues. We will have quite a lot of failures there as we seem to have ~75 issues at the moment, but if it means we clean them up more, it would be a good thing.

oof, I see. that would be more red ci, but yeah, I don't think it makes sense to keep openeing PRs for individual files. usually those are fixable quickly and we could assign an agent to the issue.

Ok, so I guess the next step is to switch the zizmor reporting and see what shows up?

[stevenhorsman]

#3119 shows the output of switching the reporting into the job, rather than github

[stevenhorsman]

@siddu-os - do you think you can use Bob to fix all the resolve template injection issues in this PR please and then we can cut down on the number of PRs and reviews required as Magnus suggests?

[siddu-os]

@siddu-os - do you think you can use Bob to fix all the resolve template injection issues in this PR please and then we can cut down on the number of PRs and reviews required as Magnus suggests?

Sure, @stevenhorsman. I'll coordinate with the team and make sure all the remaining zizmor template injection fixes are included in this PR rather than split across multiple PRs. Thank you.

[stevenhorsman]

What about the issues in webhook_image.yaml? Sorry - I missed that Rafsal's PR was merged that fixed that. I should have highlighted that csi-wrapper isn't important as it's being deleted in #3105.

[siddu-os]

What about the issues in webhook_image.yaml? Sorry - I should have highlighted that csi-wrapper isn't important as it's being deleted in #3105

okay, let me remove - csi_wrapper_images.yaml changes and webhook_image.yaml issues are already fixed with the PR #3101 and PR is merged.

[stevenhorsman]

@siddu-os it looks like there are still multiple code injection via template expansion issues in the zizmor report. Can you take a look please

[mkulke]

this one might be tough to resolve.

Error: e2e_on_pull.yaml:8: use of fundamentally insecure workflow trigger: pull_request_target is almost always used insecurely

I think we need to scrutinize the workflows split them accordingly.

name: e2e tests

on:
  ...
  pull_request_target:
    types:
      # This workflow will be run if the pull request is labeled test_e2e_libvirt,
      # so adding 'labeled' to the list of activity types.
      #
      - opened
      - synchronize
      - reopened
      - labeled
    branches:
      - 'main'

concurrency:
  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
  cancel-in-progress: true

permissions: {}

jobs:
  authorize:
    runs-on: ubuntu-24.04
    if: |
     contains(github.event.pull_request.labels.*.name, 'test_e2e_aws') ||
     contains(github.event.pull_request.labels.*.name, 'test_e2e_libvirt') ||
     contains(github.event.pull_request.labels.*.name, 'test_e2e_byom')
    steps:
      - run: "true"
  e2e:
    uses: ./.github/workflows/e2e_run_all.yaml
    needs: [authorize]
    ...

libvirt doesn't need any environment from the repo to run the tests. they should be able to launch on a fork. I think the same is true for byom, they don't need pull_request_target.

AWS will require secrets from the main repo, s390x will require special runners, so they would be in a group that have to use pull_request_target.

Currently AFAIU s390x e2e tests are also covered by the test_e2e_libvirt flag, I suggest adding a discrete label test_e2e_s390x.

[stevenhorsman]

LGTM as long is it doesn't clash with other changes. I think it will be blocked by #3130 which fixes the govulncheck issue though. Thanks

[mkulke]

@siddu-os: needs a rebase, if you feel the pull_request_target error is too complex to fix for you we can leave that error unresolved and address it in a separate PR. but it would be good to address all other warnings and notices that zizmor currently flags

[stevenhorsman]

Thanks for all the changes, but we need the job names to match the ids for backwards compatibility

[stevenhorsman]

Can you update the names to match the ids please so webhook in this case, otherwise the test pre-req and required tests will break.

[siddu-os]

All zizmor warnings and notices are now addressed with job names matching their IDs.
Thanks for the review.

[siddu-os]

needs a rebase, if you feel the pull_request_target error is too complex to fix for you we can leave that error unresolved and address it in a separate PR. but it would be good to address all other warnings and notices that zizmor currently flags

I agree this should be addressed separately. All other zizmor findings are now resolved - Thanks a lot for the review.