From 472b5d68a665bd0b366d1025911c63643ca882ec Mon Sep 17 00:00:00 2001 From: Carlos Panato Date: Thu, 17 Sep 2026 10:40:35 +0200 Subject: [PATCH 1/3] fix: do not let a stale cache directory suppress signature verification `ct.sh` treated the existence of the cache directory as proof that a verified install had happened, and interpolated unvalidated version strings into filesystem paths and into `$GITHUB_PATH` / `$GITHUB_ENV`. Three related problems followed from that. 1. `mkdir -p "${cache_dir}"` ran before the download and verification, so a run that failed verification left an empty directory behind. Every later run on the same runner then took the cache-hit path, skipped `cosign` entirely, and executed whatever was at `${cache_dir}/ct`. 2. Because `${version}` was interpolated straight into `cache_dir`, a value containing `../` resolved outside `$RUNNER_TOOL_CACHE` altogether. Pointing it at an existing attacker-controlled directory made the cache-hit path fire on the first run: `cosign` was never invoked, the script exited 0, and that directory was prepended to `$GITHUB_PATH` for every subsequent step. 3. A newline in a version string added extra lines to the `$GITHUB_ENV` file, which the runner parses one `KEY=VALUE` per line, allowing arbitrary environment variables (`LD_PRELOAD`, say) to be set for later steps. Fixes: - Validate all three version inputs against a strict version pattern, rejecting path separators and newlines at the point of entry. - Gate the cache-hit path on `[[ ! -x "${cache_dir}/ct" ]]` rather than on the directory existing, so a partially populated cache no longer counts as a verified install. - Download, verify and extract into a `mktemp -d` staging directory and publish to `${cache_dir}` only once all three succeed, with an EXIT trap removing the staging directory on every path. A failed run now leaves nothing behind that a later run could reuse. - Add `--fail` to the download so an HTTP error is reported as a download failure instead of being saved as the "tarball" and resurfacing as a misleading signature-verification error. Residual, and accepted: an executable `ct` pre-seeded at the validated cache path still short-circuits verification. That requires write access to `$RUNNER_TOOL_CACHE`, which means code already running on the runner, and it is the same trust model the `actions/setup-*` tool cache uses. Verified with a harness using fake curl/cosign/uv: - traversal version -> rejected, exit 1 (was: exit 0, cosign never invoked) - newline version -> rejected, exit 1, $GITHUB_ENV untouched - empty cached dir -> cosign now invoked and install completes (was: exit 127, cosign never invoked) - failed verification -> exit 1, zero entries left under the tool cache, $GITHUB_PATH untouched, no staging dir leaked - clean install -> exit 0, correct $GITHUB_PATH, `ct version` runs - unknown version -> "Unable to download", not "Unable to validate" shellcheck, `bash -n` and `zsh -n` all clean. Signed-off-by: Carlos Panato --- ct.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/ct.sh b/ct.sh index f597260..a616dcb 100755 --- a/ct.sh +++ b/ct.sh @@ -8,6 +8,28 @@ DEFAULT_CHART_TESTING_VERSION=3.14.0 DEFAULT_YAMLLINT_VERSION=1.33.0 DEFAULT_YAMALE_VERSION=6.0.0 +# Set once a download is staged, so cleanup() can remove it on any exit path. +staging_dir= + +cleanup() { + if [[ -n "${staging_dir}" ]]; then + rm -rf "${staging_dir}" + fi +} + +# Version strings are interpolated into filesystem paths and into the +# $GITHUB_PATH / $GITHUB_ENV files, so restrict them to characters that can +# neither traverse directories nor add extra lines to those files. +validate_version() { + local flag="$1" + local value="$2" + + if [[ ! "${value}" =~ ^[0-9]+(\.[0-9]+)*([-+][A-Za-z0-9.]+)?$ ]]; then + echo "ERROR: '${flag}' must be a version number, got: '${value}'" >&2 + exit 1 + fi +} + show_help() { cat << EOF Usage: $(basename "$0") @@ -22,8 +44,14 @@ main() { local yamllint_version="${DEFAULT_YAMLLINT_VERSION}" local yamale_version="${DEFAULT_YAMALE_VERSION}" + trap cleanup EXIT + parse_command_line "$@" + validate_version '-v|--version' "${version}" + validate_version '--yamllint-version' "${yamllint_version}" + validate_version '--yamale-version' "${yamale_version}" + install_chart_testing } @@ -88,23 +116,45 @@ install_chart_testing() { local cache_dir="${RUNNER_TOOL_CACHE}/ct/${version}/${arch}" local venv_dir="${cache_dir}/venv" - if [[ ! -d "${cache_dir}" ]]; then - mkdir -p "${cache_dir}" - + # Only treat the cache as populated when the binary itself is present. An + # empty or partially populated directory -- left behind by an earlier run + # that failed after mkdir but before extraction -- must never suppress + # signature verification. + if [[ ! -x "${cache_dir}/ct" ]]; then echo "Installing chart-testing v${version}..." - CT_CERT=https://github.com/helm/chart-testing/releases/download/v${version}/chart-testing_${version#v}_linux_${arch}.tar.gz.pem - CT_SIG=https://github.com/helm/chart-testing/releases/download/v${version}/chart-testing_${version#v}_linux_${arch}.tar.gz.sig + local ct_cert="https://github.com/helm/chart-testing/releases/download/v${version}/chart-testing_${version}_linux_${arch}.tar.gz.pem" + local ct_sig="https://github.com/helm/chart-testing/releases/download/v${version}/chart-testing_${version}_linux_${arch}.tar.gz.sig" + + # Stage everything outside the cache, and publish to ${cache_dir} only + # after the download, signature verification and extraction have all + # succeeded. This keeps a failed run from leaving anything behind that + # a later run could mistake for a verified install. + staging_dir="$(mktemp -d)" + + # --fail so an HTTP error is reported as a download failure rather than + # being saved as the "tarball" and surfacing later as a bogus + # signature-verification error. + if ! curl --fail --retry 5 --retry-delay 1 -sSLo "${staging_dir}/ct.tar.gz" \ + "https://github.com/helm/chart-testing/releases/download/v${version}/chart-testing_${version}_linux_${arch}.tar.gz"; then + echo "ERROR: Unable to download chart-testing version: v${version}" >&2 + exit 1 + fi - curl --retry 5 --retry-delay 1 -sSLo ct.tar.gz "https://github.com/helm/chart-testing/releases/download/v${version}/chart-testing_${version#v}_linux_${arch}.tar.gz" - if ! cosign verify-blob --certificate "${CT_CERT}" --signature "${CT_SIG}" \ + if ! cosign verify-blob --certificate "${ct_cert}" --signature "${ct_sig}" \ --certificate-identity "https://github.com/helm/chart-testing/.github/workflows/release.yaml@refs/heads/main" \ - --certificate-oidc-issuer "https://token.actions.githubusercontent.com" ct.tar.gz; then + --certificate-oidc-issuer "https://token.actions.githubusercontent.com" "${staging_dir}/ct.tar.gz"; then echo "ERROR: Unable to validate chart-testing version: v${version}" >&2 exit 1 fi - tar -xzf ct.tar.gz -C "${cache_dir}" - rm -f ct.tar.gz + mkdir -p "${staging_dir}/extracted" + tar -xzf "${staging_dir}/ct.tar.gz" -C "${staging_dir}/extracted" + + # Safe because validate_version has already rejected anything that + # could make ${cache_dir} point outside ${RUNNER_TOOL_CACHE}. + rm -rf "${cache_dir}" + mkdir -p "$(dirname "${cache_dir}")" + mv "${staging_dir}/extracted" "${cache_dir}" echo 'Creating virtual Python environment...' export UV_LINK_MODE=copy From ebc159f30510233140ed2cedd7d2350927be0175 Mon Sep 17 00:00:00 2001 From: Carlos Panato Date: Thu, 17 Sep 2026 10:47:00 +0200 Subject: [PATCH 2/3] test: cover ct.sh failure paths The existing jobs in test-action.yml only exercise successful installs. That covers a broken install loudly, but leaves every failure path unguarded -- and those are the ones that regress silently. A change that stopped invoking cosign, or that let a leftover cache directory count as a verified install, would keep all three jobs green. Add tests/ct_test.sh, which stubs curl, cosign and uv so the control flow can be exercised without network access, and a workflow to run it. Real cosign and real downloads stay covered by the end-to-end jobs in test-action.yml, so stubbing them here does not leave a gap. Covered: - hostile version strings (path traversal, absolute path, command substitution, semicolon, embedded newline) are rejected before reaching a filesystem path or the runner files - a v-prefixed version and a prerelease version are still accepted - a leftover cache directory does not suppress verification - a failed verification leaves nothing under the tool cache and does not write to $GITHUB_PATH - the staging directory is removed on failure - a download error is reported as a download error, not as a signature-verification error - a successful install publishes the binary, $GITHUB_PATH and CT_CONFIG_DIR - a missing $RUNNER_TOOL_CACHE is an error The suite is deliberately checked against both versions of the script: 13/13 pass on this branch, and 8 of the 13 fail on the unhardened script for the expected reasons, so the tests demonstrably exercise the fix rather than merely passing alongside it. The job lives in a new workflow file rather than as another job in test-action.yml to avoid colliding with the other in-flight changes to that file. Signed-off-by: Carlos Panato --- .github/workflows/test-ct-sh.yaml | 30 +++ tests/ct_test.sh | 313 ++++++++++++++++++++++++++++++ 2 files changed, 343 insertions(+) create mode 100644 .github/workflows/test-ct-sh.yaml create mode 100755 tests/ct_test.sh diff --git a/.github/workflows/test-ct-sh.yaml b/.github/workflows/test-ct-sh.yaml new file mode 100644 index 0000000..dfae576 --- /dev/null +++ b/.github/workflows/test-ct-sh.yaml @@ -0,0 +1,30 @@ +name: test-ct-sh + +on: + pull_request: + push: + branches: [main] + +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: {} + +jobs: + test_ct_sh: + runs-on: ubuntu-latest + permissions: + contents: read # Clone the repository + + name: ct.sh failure-path tests + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Run ct.sh tests + run: tests/ct_test.sh + + - name: Shellcheck + run: shellcheck ct.sh tests/ct_test.sh diff --git a/tests/ct_test.sh b/tests/ct_test.sh new file mode 100755 index 0000000..4c12d6d --- /dev/null +++ b/tests/ct_test.sh @@ -0,0 +1,313 @@ +#!/usr/bin/env bash + +# Failure-path tests for ct.sh. +# +# curl, cosign and uv are stubbed so the script's control flow can be exercised +# without network access. The happy path against the real tools is already +# covered end to end by the jobs in .github/workflows/test-action.yml; what is +# tested here is everything that fails, because those paths fail silently -- +# a regression that stops invoking cosign would otherwise leave CI green. + +set -o errexit +set -o nounset +set -o pipefail + +CT_SH="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/ct.sh" + +passes=0 +failures=0 + +pass() { + printf 'ok - %s\n' "$1" + passes=$((passes + 1)) +} + +fail() { + printf 'FAIL - %s\n %s\n' "$1" "$2" >&2 + failures=$((failures + 1)) +} + +# Fresh sandbox per test: stub PATH entry, tool cache, and the runner files +# that ct.sh appends to. +setup() { + workdir="$(mktemp -d)" + stubdir="${workdir}/bin" + mkdir -p "${stubdir}" "${workdir}/cache" "${workdir}/tmp" + + export RUNNER_TOOL_CACHE="${workdir}/cache" + export GITHUB_PATH="${workdir}/github_path" + export GITHUB_ENV="${workdir}/github_env" + # mktemp -d in ct.sh honours TMPDIR, so staging dirs land here and leaks + # are observable. + export TMPDIR="${workdir}/tmp" + : > "${GITHUB_PATH}" + : > "${GITHUB_ENV}" + + # A real tarball, so tar and the post-extract steps behave normally. + mkdir -p "${workdir}/payload/etc" + printf '#!/bin/sh\necho "Version: v3.14.0"\n' > "${workdir}/payload/ct" + chmod +x "${workdir}/payload/ct" + echo 'schema' > "${workdir}/payload/etc/chart_schema.yaml" + tar -czf "${workdir}/release.tar.gz" -C "${workdir}/payload" . + + stub_curl_ok + stub_cosign 0 + printf '#!/bin/sh\nexit 0\n' > "${stubdir}/uv" + chmod +x "${stubdir}/uv" +} + +teardown() { + rm -rf "${workdir}" +} + +stub_curl_ok() { + cat > "${stubdir}/curl" < "${stubdir}/curl" + chmod +x "${stubdir}/curl" +} + +stub_cosign() { + cat > "${stubdir}/cosign" <> "${workdir}/cosign.log" +exit $1 +EOF + chmod +x "${stubdir}/cosign" +} + +cosign_invoked() { + [[ -f "${workdir}/cosign.log" ]] +} + +# Runs ct.sh with the stubs first on PATH. Never aborts the suite: the exit +# status is what most of these tests assert on. +run_ct() { + set +o errexit + PATH="${stubdir}:${PATH}" bash "${CT_SH}" "$@" > "${workdir}/output" 2>&1 + rc=$? + set -o errexit +} + +output() { + cat "${workdir}/output" +} + +#----------------------------------------------------------------------------- +# A version string that is not a plain version number must be rejected before +# it can reach a filesystem path or the $GITHUB_PATH / $GITHUB_ENV files. +#----------------------------------------------------------------------------- + +test_rejects_hostile_versions() { + local name value + while IFS='|' read -r name value; do + [[ -n "${name}" ]] || continue + setup + # Precondition for the traversal case: a previous run leaves ct/ behind. + mkdir -p "${RUNNER_TOOL_CACHE}/ct" + run_ct --version "${value}" + + if [[ ${rc} -eq 0 ]]; then + fail "rejects ${name}" "expected non-zero exit, got 0" + elif ! output | grep -q 'must be a version number'; then + fail "rejects ${name}" "expected a validation error, got: $(output | tail -1)" + elif cosign_invoked; then + fail "rejects ${name}" "cosign should not have been reached" + elif [[ -s "${GITHUB_PATH}" || -s "${GITHUB_ENV}" ]]; then + fail "rejects ${name}" "runner files were written to" + else + pass "rejects ${name}" + fi + teardown + done <<'CASES' +path traversal|../../../../tmp/evil +absolute path|/tmp/evil +command substitution|3.14.0$(id) +semicolon|3.14.0; id +CASES + + # Newline kept out of the heredoc above, which is line-oriented. + setup + run_ct --version "$(printf '3.14.0\nLD_PRELOAD=/tmp/evil.so')" + if [[ ${rc} -eq 0 ]]; then + fail "rejects embedded newline" "expected non-zero exit, got 0" + elif grep -q 'LD_PRELOAD' "${GITHUB_ENV}"; then + fail "rejects embedded newline" "injected a variable into \$GITHUB_ENV" + else + pass "rejects embedded newline" + fi + teardown +} + +# The action passes v-prefixed versions (test-action.yml uses 'v3.8.0'), so the +# leading v must still be stripped and accepted. +test_accepts_v_prefixed_version() { + setup + run_ct --version v3.14.0 + if [[ ${rc} -ne 0 ]]; then + fail "accepts v-prefixed version" "exit ${rc}: $(output | tail -1)" + else + pass "accepts v-prefixed version" + fi + teardown +} + +test_accepts_prerelease_version() { + setup + run_ct --version 3.14.0-rc.1 + if [[ ${rc} -ne 0 ]] && output | grep -q 'must be a version number'; then + fail "accepts prerelease version" "rejected 3.14.0-rc.1" + else + pass "accepts prerelease version" + fi + teardown +} + +#----------------------------------------------------------------------------- +# A directory left behind by an earlier failed run must not be mistaken for a +# verified install. +#----------------------------------------------------------------------------- + +test_stale_cache_dir_does_not_skip_verification() { + setup + # Exactly what a run that died after mkdir but before extraction leaves. + mkdir -p "${RUNNER_TOOL_CACHE}/ct/3.14.0/amd64" + run_ct --version 3.14.0 + + if ! cosign_invoked; then + fail "stale cache dir does not skip verification" \ + "cosign was never invoked (verification silently skipped)" + elif [[ ${rc} -ne 0 ]]; then + fail "stale cache dir does not skip verification" "exit ${rc}: $(output | tail -1)" + else + pass "stale cache dir does not skip verification" + fi + teardown +} + +test_failed_verification_leaves_nothing_reusable() { + setup + stub_cosign 1 + run_ct --version 3.14.0 + + local leftovers + leftovers="$(find "${RUNNER_TOOL_CACHE}" -mindepth 1 | wc -l | tr -d ' ')" + + if [[ ${rc} -eq 0 ]]; then + fail "failed verification leaves nothing reusable" "expected non-zero exit, got 0" + elif ! output | grep -q 'Unable to validate chart-testing version'; then + fail "failed verification leaves nothing reusable" \ + "expected the validation error, got: $(output | tail -1)" + elif [[ "${leftovers}" != "0" ]]; then + fail "failed verification leaves nothing reusable" \ + "${leftovers} entries left under the tool cache" + elif [[ -s "${GITHUB_PATH}" ]]; then + fail "failed verification leaves nothing reusable" "\$GITHUB_PATH was written to" + else + pass "failed verification leaves nothing reusable" + fi + teardown +} + +test_staging_dir_is_always_cleaned_up() { + setup + stub_cosign 1 + run_ct --version 3.14.0 + + local leaked + leaked="$(find "${TMPDIR}" -mindepth 1 -maxdepth 1 | wc -l | tr -d ' ')" + if [[ "${leaked}" != "0" ]]; then + fail "staging dir is always cleaned up" "${leaked} staging dir(s) left in TMPDIR" + else + pass "staging dir is always cleaned up" + fi + teardown +} + +#----------------------------------------------------------------------------- +# A download failure must not be reported as a signature problem. +#----------------------------------------------------------------------------- + +test_download_failure_is_distinct_from_verification_failure() { + setup + stub_curl_http_error + run_ct --version 3.14.0 + + if [[ ${rc} -eq 0 ]]; then + fail "download failure is reported as such" "expected non-zero exit, got 0" + elif ! output | grep -q 'Unable to download chart-testing version'; then + fail "download failure is reported as such" \ + "expected a download error, got: $(output | tail -1)" + else + pass "download failure is reported as such" + fi + teardown +} + +#----------------------------------------------------------------------------- +# The success path must still install and publish the tool. +#----------------------------------------------------------------------------- + +test_successful_install() { + setup + run_ct --version 3.14.0 + + local ct_bin="${RUNNER_TOOL_CACHE}/ct/3.14.0/amd64/ct" + if [[ ${rc} -ne 0 ]]; then + fail "successful install" "exit ${rc}: $(output | tail -1)" + elif ! cosign_invoked; then + fail "successful install" "cosign was not invoked" + elif [[ ! -x "${ct_bin}" ]]; then + fail "successful install" "ct binary missing at ${ct_bin}" + elif ! grep -qx "${RUNNER_TOOL_CACHE}/ct/3.14.0/amd64" "${GITHUB_PATH}"; then + fail "successful install" "cache dir was not added to \$GITHUB_PATH" + elif ! grep -q '^CT_CONFIG_DIR=' "${GITHUB_ENV}"; then + fail "successful install" "CT_CONFIG_DIR was not exported" + else + pass "successful install" + fi + teardown +} + +test_missing_tool_cache_is_an_error() { + setup + export RUNNER_TOOL_CACHE="${workdir}/does-not-exist" + run_ct --version 3.14.0 + if [[ ${rc} -eq 0 ]]; then + fail "missing tool cache is an error" "expected non-zero exit, got 0" + else + pass "missing tool cache is an error" + fi + teardown +} + +main() { + test_rejects_hostile_versions + test_accepts_v_prefixed_version + test_accepts_prerelease_version + test_stale_cache_dir_does_not_skip_verification + test_failed_verification_leaves_nothing_reusable + test_staging_dir_is_always_cleaned_up + test_download_failure_is_distinct_from_verification_failure + test_successful_install + test_missing_tool_cache_is_an_error + + printf '\n%d passed, %d failed\n' "${passes}" "${failures}" + [[ ${failures} -eq 0 ]] +} + +main "$@" From 72b57df9e63be6b94fdf8dd1f434d0d1d8814781 Mon Sep 17 00:00:00 2001 From: Carlos Panato Date: Thu, 17 Sep 2026 10:52:51 +0200 Subject: [PATCH 3/3] test: stop leaking TMPDIR between test cases setup() exported TMPDIR into the sandbox directory that teardown() then deleted, so the next test case called `mktemp -d` with TMPDIR pointing at a path that no longer existed. This passed locally and failed on the runner because of a platform difference: GNU mktemp honours TMPDIR for its default template and fails when the directory is missing, while the BSD mktemp on macOS ignores TMPDIR and always uses /tmp. Only the first test case ran in CI. Scope TMPDIR to the ct.sh invocation in run_ct() instead, so the staging directory is still observable without the harness's own mktemp calls depending on it. Verified by emulating GNU mktemp semantics through a PATH wrapper: the previous commit reproduces the CI failure after the first test case, and this version reports 13/13 under both GNU and BSD semantics. The differential against the unhardened script is unchanged at 8 of 13 failing, so the suite still exercises the fix. Signed-off-by: Carlos Panato --- tests/ct_test.sh | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/ct_test.sh b/tests/ct_test.sh index 4c12d6d..0c69286 100755 --- a/tests/ct_test.sh +++ b/tests/ct_test.sh @@ -37,9 +37,6 @@ setup() { export RUNNER_TOOL_CACHE="${workdir}/cache" export GITHUB_PATH="${workdir}/github_path" export GITHUB_ENV="${workdir}/github_env" - # mktemp -d in ct.sh honours TMPDIR, so staging dirs land here and leaks - # are observable. - export TMPDIR="${workdir}/tmp" : > "${GITHUB_PATH}" : > "${GITHUB_ENV}" @@ -96,9 +93,15 @@ cosign_invoked() { # Runs ct.sh with the stubs first on PATH. Never aborts the suite: the exit # status is what most of these tests assert on. +# +# TMPDIR is set for this invocation only, so ct.sh's staging directory lands +# somewhere observable without leaking into the harness's own mktemp calls. +# Note GNU mktemp honours TMPDIR but BSD mktemp does not, so the staging-dir +# assertion is only meaningful on Linux, which is where CI runs it. run_ct() { set +o errexit - PATH="${stubdir}:${PATH}" bash "${CT_SH}" "$@" > "${workdir}/output" 2>&1 + TMPDIR="${workdir}/tmp" PATH="${stubdir}:${PATH}" \ + bash "${CT_SH}" "$@" > "${workdir}/output" 2>&1 rc=$? set -o errexit } @@ -229,7 +232,7 @@ test_staging_dir_is_always_cleaned_up() { run_ct --version 3.14.0 local leaked - leaked="$(find "${TMPDIR}" -mindepth 1 -maxdepth 1 | wc -l | tr -d ' ')" + leaked="$(find "${workdir}/tmp" -mindepth 1 -maxdepth 1 | wc -l | tr -d ' ')" if [[ "${leaked}" != "0" ]]; then fail "staging dir is always cleaned up" "${leaked} staging dir(s) left in TMPDIR" else