Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/e2e-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ jobs:
e2e-vmaas:
uses: ./.github/workflows/e2e.yml
with:
test-suite: ${{ github.event.inputs.test-suite || 'vmaas' }}
test-filter: ${{ github.event.inputs.test-filter || '' }}
test-suite: ${{ inputs.test-suite || 'vmaas' }}
test-filter: ${{ inputs.test-filter || '' }}
85 changes: 57 additions & 28 deletions .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ on:
required: false
type: string
default: "vmaas-helm"
image-overrides:
description: "JSON object mapping Helm value paths to image refs, e.g. '{\"service.images.service\":\"ghcr.io/osac-project/fulfillment-service:pr-123\"}'"
required: false
type: string
default: "{}"
installer-ref:
description: "Branch/ref of osac-installer to clone for the deploy step"
required: false
type: string
default: "main"
outputs:
result:
description: "Test result: PASSED or FAILED"
Expand Down Expand Up @@ -110,8 +120,11 @@ jobs:
printf '%s' "${AAP_LICENSE}" | base64 -d > "$RUNNER_TEMP/aap-license.zip"
chmod 600 "$RUNNER_TEMP/aap-license.zip"

- name: Ensure cluster-tool is configured
- name: Prepare cluster environment
env:
FLAVOR_IMAGE: ${{ inputs.flavor-image }}
run: |
# --- Ensure cluster-tool is configured ---
CT_CONFIG="${HOME}/.config/cluster-tool"
ROOT_CONFIG="/root/.config/cluster-tool"
if [[ -f "${CT_CONFIG}/servers.json" ]] && [[ -f "${CT_CONFIG}/state.json" ]]; then
Expand All @@ -125,10 +138,7 @@ jobs:
exit 1
fi

- name: Pull or update cluster flavor
env:
FLAVOR_IMAGE: ${{ inputs.flavor-image }}
run: |
# --- Pull or update cluster flavor ---
# Try non-blocking exclusive lock to pull/update the flavor.
# If another job is already pulling, skip — the boot step's
# shared lock will wait for it to finish.
Expand All @@ -144,8 +154,7 @@ jobs:
exit "${rc}"
fi

- name: Clean up stale clone resources
run: |
# --- Clean up stale clone resources ---
# Destroy any leftover clone from a previous run attempt, then
# remove orphaned Linux bridges that survive virsh net-destroy
# (e.g. from interrupted teardowns). Only targets bridges matching
Expand All @@ -170,8 +179,6 @@ jobs:

echo "Cluster clone '${CLONE_NAME}' booted."

- name: Fix kubeconfig ownership
run: |
# cluster-tool boot runs as root (via sudo), so both ~/.kube and
# the kubeconfig are owned by root. Copy to RUNNER_TEMP so the
# runner user owns it — podman needs this to relabel for :Z mounts.
Expand All @@ -184,10 +191,18 @@ jobs:
E2E_IMAGE: "osac-e2e-tests:${{ github.run_id }}"
run: podman build -t "${E2E_IMAGE}" -f Containerfile .

- name: Wait for KubeVirt
- name: Deploy OSAC
env:
KUBECONFIG: ${{ runner.temp }}/kubeconfig
VALUES_FILE: values/vmaas-ci/values.yaml
INSTALLER_NAMESPACE: ${{ inputs.namespace }}
INSTALLER_VM_TEMPLATE: ${{ inputs.vm-template }}
INSTALLER_CLUSTER_TEMPLATE: ${{ inputs.cluster-template }}
AAP_LICENSE_FILE: ${{ runner.temp }}/aap-license.zip
INSTALLER_REF: ${{ inputs.installer-ref }}
IMAGE_OVERRIDES: ${{ inputs.image-overrides }}
run: |
export KUBECONFIG="$RUNNER_TEMP/kubeconfig"

# --- Wait for KubeVirt ---
echo "Waiting for KubeVirt to be Available..."
for attempt in $(seq 1 60); do
AVAILABLE=$(oc get hyperconverged kubevirt-hyperconverged -n openshift-cnv \
Expand All @@ -206,15 +221,7 @@ jobs:
exit 1
fi

- name: Deploy OSAC
env:
KUBECONFIG: ${{ runner.temp }}/kubeconfig
VALUES_FILE: values/vmaas-ci/values.yaml
INSTALLER_NAMESPACE: ${{ inputs.namespace }}
INSTALLER_VM_TEMPLATE: ${{ inputs.vm-template }}
INSTALLER_CLUSTER_TEMPLATE: ${{ inputs.cluster-template }}
AAP_LICENSE_FILE: ${{ runner.temp }}/aap-license.zip
run: |
# --- Deploy ---
# Ensure helm and yq are available
if ! command -v helm &>/dev/null; then
curl -fsSL https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 | bash
Expand All @@ -226,7 +233,7 @@ jobs:
fi

INSTALLER_DIR="$RUNNER_TEMP/osac-installer"
git clone --recurse-submodules --depth 1 \
git clone --recurse-submodules --depth 1 --branch "${INSTALLER_REF}" \
https://github.com/osac-project/osac-installer.git "${INSTALLER_DIR}"
cd "${INSTALLER_DIR}"

Expand All @@ -238,6 +245,29 @@ jobs:
# runs helm upgrade, and waits with health monitoring.
python3 scripts/refresh-after-snapshot.py

# Apply image overrides via helm upgrade --set flags.
# Callers pass a JSON object mapping Helm value paths to image refs,
# e.g. '{"service.images.service":"ghcr.io/repo:pr-42"}'.
# If empty or '{}', this is a no-op.
if [[ -n "${IMAGE_OVERRIDES}" ]] && [[ "${IMAGE_OVERRIDES}" != "{}" ]]; then
echo "Applying image overrides..."
SET_ARGS=()
for key in $(echo "${IMAGE_OVERRIDES}" | jq -r 'keys[]'); do
value=$(echo "${IMAGE_OVERRIDES}" | jq -r --arg k "${key}" '.[$k]')
SET_ARGS+=("--set" "${key}=${value}")
echo " --set ${key}=${value}"
done

yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml
helm dependency update charts/osac/
helm upgrade osac charts/osac/ \
--namespace "${INSTALLER_NAMESPACE}" \
--values "${VALUES_FILE}" \
"${SET_ARGS[@]}" \
--timeout 10m \
--wait
fi
Comment on lines +248 to +269

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Argument injection and word-splitting risks in Helm override parsing.

The current approach has two issues:

  1. Argument injection via keys: A malicious JSON key like {"--values /etc/passwd": "x"} would produce --set --values /etc/passwd=x, allowing arbitrary Helm flags to be injected.

  2. Word splitting on values: Since ${SET_ARGS} is expanded unquoted (line 259), values containing spaces break into separate arguments, causing incorrect Helm behavior.

Consider using an array to safely accumulate arguments:

🔒 Proposed fix using bash array for safe argument handling
          if [[ -n "${IMAGE_OVERRIDES}" ]] && [[ "${IMAGE_OVERRIDES}" != "{}" ]]; then
            echo "Applying image overrides..."
-           SET_ARGS=""
-           for key in $(echo "${IMAGE_OVERRIDES}" | jq -r 'keys[]'); do
-             value=$(echo "${IMAGE_OVERRIDES}" | jq -r --arg k "${key}" '.[$k]')
-             SET_ARGS="${SET_ARGS} --set ${key}=${value}"
-             echo "  --set ${key}=${value}"
+           SET_ARGS=()
+           while IFS= read -r key; do
+             value=$(echo "${IMAGE_OVERRIDES}" | jq -r --arg k "${key}" '.[$k]')
+             # Validate key doesn't start with - to prevent argument injection
+             if [[ "${key}" == -* ]]; then
+               echo "ERROR: Invalid override key '${key}' - keys cannot start with -" >&2
+               exit 1
+             fi
+             SET_ARGS+=(--set "${key}=${value}")
+             echo "  --set ${key}=${value}"
-           done
+           done < <(echo "${IMAGE_OVERRIDES}" | jq -r 'keys[]')

            yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml
            helm dependency update charts/osac/
            helm upgrade osac charts/osac/ \
              --namespace "${INSTALLER_NAMESPACE}" \
              --values "${VALUES_FILE}" \
-             ${SET_ARGS} \
+             "${SET_ARGS[@]}" \
              --timeout 10m \
              --wait
          fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/e2e.yml around lines 241 - 262, The SET_ARGS variable is
built by string concatenation, which causes argument injection risks (malicious
keys could inject arbitrary Helm flags) and word-splitting issues (values with
spaces break into separate arguments). Convert SET_ARGS from a string to a bash
array by initializing it as SET_ARGS=() at the start, then in the loop where you
currently append to SET_ARGS="${SET_ARGS} --set ${key}=${value}", instead append
array elements using SET_ARGS+=("--set" "${key}=${value}"), and finally when
passing it to the helm upgrade command on the line with --set flags, expand it
safely as "${SET_ARGS[@]}" instead of the unquoted ${SET_ARGS}.

Source: Coding guidelines


- name: Run E2E tests
id: test
env:
Expand Down Expand Up @@ -274,9 +304,12 @@ jobs:
pytest ${TEST_ARGS} ${FILTER_ARGS} \
--junitxml=/tmp/junit.xml -v

- name: Destroy cluster clone
- name: Teardown
if: always()
env:
E2E_IMAGE: "osac-e2e-tests:${{ github.run_id }}"
run: |
# --- Destroy cluster clone ---
echo "Destroying clone '${CLONE_NAME}'..."
sudo python3 /usr/local/bin/cluster-tool destroy "${CLONE_NAME}" 2>&1 || true

Expand All @@ -288,11 +321,7 @@ jobs:
sudo ip link delete "${br}" 2>/dev/null || true
done

- name: Clean up
if: always()
env:
E2E_IMAGE: "osac-e2e-tests:${{ github.run_id }}"
run: |
# --- Clean up temporary files ---
rm -f "$RUNNER_TEMP/pull-secret.json" "$RUNNER_TEMP/aap-license.zip" "$RUNNER_TEMP/kubeconfig"
rm -f "${HOME}/.config/containers/auth.json"
rm -rf "$RUNNER_TEMP/osac-installer"
Expand Down
Loading