From 0adbe96540b6d536d802a88a602aa73ac3f7aed5 Mon Sep 17 00:00:00 2001 From: Louis Parkin Date: Wed, 5 Aug 2026 16:12:17 +0200 Subject: [PATCH] STAC-25540: keep the private-index credential off the pull request path The previous revision confined the GitLab package registry credential to a single step and claimed that reaching it would require editing the workflow. That claim was wrong. A `pull_request` run executes the pull request's own copy of the workflow *and* of every script it calls, so a pull request could rewrite fetch_private_wheels.sh, reorder the steps, or add one of its own. Confining a secret inside pull-request-controlled code is hardening, not a boundary. So the private-index suites no longer run on pull requests at all. They run on push, tag and workflow_dispatch events, whose contents are reviewed before they reach the release branch, and where the secret is therefore not exposed to unreviewed code. * select-checks.sh moves private-index suites to a new `deferred_private_checks` output on pull requests, so the matrix is empty rather than gated, and the reason is reported. * check-tests-private-index is conditioned on the event, replacing the fork guard that is now redundant. * fetch_private_wheels.sh refuses to run on a pull_request event, so the rule survives an edit to the workflow condition. * ci-success reports the deferral, so a green pull request never implies vsphere was covered when it was not. The cost is that vsphere is verified on the release branch rather than on the pull request that changes it. That is accepted knowingly: the suite changes a handful of times a year, and the alternatives cost more than they return. The durable fix is to stop needing the private index -- VMware now publishes this SDK to public PyPI under renamed packages -- which deletes the credential, the job and this gap together. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/scripts/select-checks.sh | 54 ++++++++--- .github/workflows/checks-tests.yml | 127 +++++++++++++++---------- .setup-scripts/fetch_private_wheels.sh | 30 ++++-- 3 files changed, 139 insertions(+), 72 deletions(-) diff --git a/.github/scripts/select-checks.sh b/.github/scripts/select-checks.sh index 1b844442..6efeed1a 100755 --- a/.github/scripts/select-checks.sh +++ b/.github/scripts/select-checks.sh @@ -21,17 +21,25 @@ # * push / workflow_dispatch run everything (GitLab: `master_branch`, # `release_branch`). # -# Writes two arrays to $GITHUB_OUTPUT for `fromJson()` in a matrix: -# checks -- suites that need no credentials -# private_checks -- suites that install from the private GitLab PyPI index +# Writes three arrays to $GITHUB_OUTPUT for `fromJson()` in a matrix: +# checks -- suites that need no credentials +# private_checks -- suites that install from the private GitLab PyPI +# index, and are cleared to run on this event +# deferred_private_checks -- private-index suites withheld from this event +# (always empty outside pull requests) # # The split is a security boundary, not a convenience. The credential-free suites -# run on GitHub-hosted runners with no secrets in scope at all, so a fork PR can -# run them safely. The private-index suites need a registry password, and any test -# code executing alongside it could read it, so they run in a separate job that -# downloads the private packages into a local wheelhouse and destroys the -# credential before the suite starts (STAC-25463 review, STAC-25540). Keeping them -# in one matrix would hand that credential to every suite. +# run with no secrets in scope at all. The private-index suites need a registry +# password, so they are kept in a separate job -- and, on pull requests, are not +# run at all (STAC-25540, second review pass). +# +# That last part is the whole point, so it is worth stating plainly: a +# `pull_request` run executes the pull request's own copy of the workflow and of +# every script it calls. Hardening the job cannot keep a determined pull request +# away from a secret the run is holding -- it can always edit the thing that holds +# it. The only run that cannot leak the credential is a run that never receives +# it, so these suites are deferred to push, tag and workflow_dispatch events, +# whose contents are reviewed before they reach the release branch. set -euo pipefail @@ -74,8 +82,12 @@ CHECKS=( # mirrored into the StackVista package registry and needs authentication. # # Everything not listed here is credential-free and must stay that way: adding a -# suite to this list moves it behind a manual approval gate, and removing the -# need for the private index is always the better fix. +# suite to this list stops it running on pull requests altogether, and removing +# the need for the private index is always the better fix. For vsphere that fix +# looks reachable -- VMware now publishes the SDK to public PyPI under renamed +# packages (vmware-vapi-runtime, vmware-vapi-common-client, pyvmomi) and ships +# the NSX/VMC wheels from its own public index -- so this list should shrink to +# nothing once the pin is modernised. PRIVATE_INDEX_CHECKS=( vsphere ) @@ -110,7 +122,7 @@ is_private_index() { emit() { local -a selected=("$@") - local -a public=() private=() + local -a public=() private=() deferred=() local check for check in ${selected[@]+"${selected[@]}"}; do if is_private_index "${check}"; then @@ -120,17 +132,33 @@ emit() { fi done - local public_json private_json + # Pull requests do not run the private-index suites at all (STAC-25540, second + # review pass). See the security-boundary note at the top of this file: a + # `pull_request` run executes the pull request's own copy of the workflow and + # scripts, so the credential can only be protected by withholding it. These + # suites run on the release branch instead, where the code has been reviewed. + if [ "${EVENT_NAME}" = "pull_request" ] && [ "${#private[@]}" -gt 0 ]; then + deferred=("${private[@]}") + private=() + fi + + local public_json private_json deferred_json public_json=$(to_json ${public[@]+"${public[@]}"}) private_json=$(to_json ${private[@]+"${private[@]}"}) + deferred_json=$(to_json ${deferred[@]+"${deferred[@]}"}) { echo "checks=${public_json}" echo "private_checks=${private_json}" + echo "deferred_private_checks=${deferred_json}" } >>"${GITHUB_OUTPUT}" echo "Selected credential-free suites: ${public_json}" echo "Selected private-index suites: ${private_json}" + if [ "${deferred_json}" != "[]" ]; then + echo "Deferred private-index suites: ${deferred_json}" + echo "::notice title=Private-index suites do not run on pull requests::${deferred_json} resolve only against the private package registry. Pull requests are deliberately given no credential to reach it, so these suites run on ${BASE_REF:-the release branch} after merge." + fi } # Anything that is not a pull request is a full run. On the release branch the diff --git a/.github/workflows/checks-tests.yml b/.github/workflows/checks-tests.yml index 5a48c84f..a2495c5d 100644 --- a/.github/workflows/checks-tests.yml +++ b/.github/workflows/checks-tests.yml @@ -71,33 +71,46 @@ name: Check tests # least-privilege by construction; this repo's *publishing* role is still # deferred, per the note above. # -# Scoping it to a single step was not enough on its own, because the old setup -# script wrote it to ~/.netrc and the suite's own test code runs afterwards in -# the same job and could read it. So the suites that need it are split into -# their own job, `check-tests-private-index`, and that job now downloads the -# private packages into a local wheelhouse and deletes the credential before any -# repository code runs (STAC-25540). The remaining suites never see it. +# No pull request ever receives it. `pull_request` runs execute the pull +# request's own copy of this workflow and of every script it calls, so a run +# that holds a secret can always be made to disclose it -- by editing the +# fetching script, reordering steps, or adding one. A repository secret and +# pull-request-controlled code cannot be arranged into a boundary. The suites +# that need this credential therefore do not run on pull requests at all; they +# run on push, tag and workflow_dispatch events, whose contents are reviewed +# before reaching the release branch. See `check-tests-private-index`. # -# This replaced the `private-package-index` GitHub Environment, whose required -# reviewers released the credential only after a human approved each run. It -# worked, but SHARED_PATHS covers the CI files, so it fired on roughly one -# commit in six and blocked authors on their own pull requests for no gain the -# wheelhouse does not also provide. +# Within those runs the credential is still confined to a single step +# (STAC-25540): the script writes ~/.netrc, downloads one fixed package set into +# a local wheelhouse, deletes the netrc, and points pip at the wheelhouse, so +# the suite and its dependency tree install with nothing to authenticate +# against. The predecessor left the netrc readable for the rest of the job. That +# is defence in depth, not the boundary -- the boundary is the event condition +# above. # -# Residual, and deliberately left alone here: the underlying secrets are still -# repo- and org-level, so a pull request that edits this workflow could add a -# new reference outside that step. That change is visible in the diff and is -# what review and the Zizmor audit are for. Making them environment-only secrets -# would remove even that path, but the org-level password is shared with other -# repos (stackstate-agent among them), so it needs its own change. +# Two earlier answers to the same review finding are recorded here so they are +# not re-proposed. A `private-package-index` GitHub Environment with required +# reviewers did gate the credential, but SHARED_PATHS covers the CI files, so it +# fired on roughly one commit in six and blocked authors on their own pull +# requests -- while only ever constraining people who already have write access. +# Prefetching the wheelhouse on a trusted event and passing it to pull requests +# through the Actions cache also works, but any pull request can read a cache, +# and a pull request can only restore one from its base branch. +# +# The durable fix is to stop needing the index: VMware now publishes this SDK to +# public PyPI under renamed packages (vmware-vapi-runtime, +# vmware-vapi-common-client, pyvmomi) and serves the NSX/VMC wheels from its own +# public index, so modernising the pin removes the credential, this job and the +# pull-request coverage gap in one change. # # RUNNERS # Everything runs on GitHub-hosted runners. The suites are pure-Python and need # no Docker daemon, so the self-hosted docker-public pool bought nothing while # costing real isolation: fork PRs had to be excluded from it, which in turn -# meant a fork could never produce a CI verdict. On hosted runners forks run the -# full credential-free matrix, and only the private-index job keeps a fork guard -# -- forks receive no secrets in any case. This also removes the question of +# meant a fork could never produce a CI verdict. On hosted runners forks run +# exactly the same matrix as any other pull request -- the private-index job is +# off the pull-request path entirely, so no fork-specific guard is needed for +# it any more. This also removes the question of # pulling upstream images across the self-hosted NAT: the BCI reference is # direct, from a public registry, on infrastructure that is meant to reach it. # The phase-2 Docker suites will need a runner with a daemon; that decision @@ -156,16 +169,17 @@ env: jobs: select-checks: name: Select check suites to run - # Runs for forks too. Every job in this workflow now runs on GitHub-hosted - # runners with no secrets in scope, except the private-index suites, which are - # isolated behind a protected environment below. There is therefore nothing a - # fork branch can reach here, and blocking forks outright would leave them - # unable to satisfy branch protection at all (STAC-25463 review). + # Runs for forks too. Every job that runs on a pull request does so with no + # secrets in scope at all -- the one job that uses a credential does not run + # on pull requests (see `check-tests-private-index`). There is therefore + # nothing a fork branch can reach here, and blocking forks outright would + # leave them unable to satisfy branch protection at all (STAC-25463 review). runs-on: ubuntu-latest timeout-minutes: 10 outputs: checks: ${{ steps.select.outputs.checks }} private_checks: ${{ steps.select.outputs.private_checks }} + deferred_private_checks: ${{ steps.select.outputs.deferred_private_checks }} image: ${{ steps.image.outputs.ref }} steps: - name: Check out repository @@ -307,28 +321,33 @@ jobs: # credential at all: vsphere pins a package that resolves solely from the # private GitLab Package Registry. # - # The credential is confined to a single step (STAC-25540). It is written, - # used for one fixed pip download, and destroyed before any repository code - # runs; the suite then installs from the resulting local wheelhouse with - # nothing to authenticate against. That replaces the protected GitHub - # Environment this job used to sit behind, which answered the same STAC-25463 - # review finding by requiring a human to release every run -- and, because - # SHARED_PATHS covers the CI files, fired on roughly one commit in six and - # blocked authors on their own pull requests. + # This job does not run on pull requests (STAC-25540, second review pass). + # + # An earlier revision ran it on pull requests with the credential confined to + # a single step, and claimed that reaching it would require editing this + # workflow. That claim was wrong, and the review was right to call it: a + # `pull_request` run executes the pull request's own copy of the workflow AND + # of every script it calls, so a pull request could rewrite the fetch script, + # reorder these steps, or simply add a step of its own. Repository secrets + # plus pull-request-controlled code do not make a security boundary, however + # carefully the code in between is written. # - # The fork guard stays here, and only here. A fork PR gets no Actions secrets - # on a public repository, so it could not fetch the wheel in any case; it - # skips this job and still gets a full credential-free verdict from the other - # suites, so nothing silently reports green without tests. + # Confining the credential to one step is still worth doing and is still done + # -- it keeps the password away from the suite's dependency tree, which needed + # no malice at all to read it -- but it is hardening, not a boundary. The + # boundary is this condition: the run simply never receives the secret. # - # Residual risk, stated plainly: a pull request that edits this workflow can - # still reach the secret, because `pull_request` runs the PR's own copy of the - # workflow. That is true of every secret in every repository and is contained - # by review and branch protection, not by CI. What changed is that the - # credential is no longer readable by ordinary test code and its dependency - # tree, which needed no workflow edit at all. + # The cost is that vsphere is verified on the release branch rather than on + # the pull request that changes it. That is a real gap, accepted knowingly: + # the suite's own directory changes a handful of times a year, and the + # alternatives all cost more than they return right now. Sharing a prefetched + # wheelhouse through the Actions cache would work, but a pull request run can + # only restore caches from its base branch, and any pull request can read + # them. The durable fix is to stop needing the private index at all: VMware + # now publishes this SDK to public PyPI under renamed packages, so modernising + # the pin deletes this job, its credential and this trade-off together. if: >- - ${{ (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) + ${{ github.event_name != 'pull_request' && needs.select-checks.outputs.private_checks != '[]' }} needs: select-checks runs-on: ubuntu-latest @@ -475,7 +494,7 @@ jobs: CHECK_TESTS_PRIVATE: ${{ needs.check-tests-private-index.result }} SELECTED_CHECKS: ${{ needs.select-checks.outputs.checks }} SELECTED_PRIVATE_CHECKS: ${{ needs.select-checks.outputs.private_checks }} - IS_FORK: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository }} + DEFERRED_PRIVATE_CHECKS: ${{ needs.select-checks.outputs.deferred_private_checks }} run: | set -euo pipefail status=0 @@ -512,15 +531,17 @@ jobs: ;; esac - # The private-index matrix has one extra legitimate skip: fork PRs never - # receive the credential, by design. They still get a full verdict from - # the credential-free suites above, so this cannot hide an empty run. - printf ' %-24s %s (selected: %s, fork: %s)\n' \ - "check-tests-private" "${CHECK_TESTS_PRIVATE}" "${SELECTED_PRIVATE_CHECKS}" "${IS_FORK}" + # The private-index matrix legitimately skips whenever the selector + # chose nothing for this event. On pull requests that is always: those + # suites are deferred rather than selected, because the run holds no + # credential to reach the private index with. The deferral is reported + # so a green pull request never quietly implies vsphere was covered. + printf ' %-24s %s (selected: %s, deferred: %s)\n' \ + "check-tests-private" "${CHECK_TESTS_PRIVATE}" "${SELECTED_PRIVATE_CHECKS}" "${DEFERRED_PRIVATE_CHECKS}" case "${CHECK_TESTS_PRIVATE}" in success) ;; skipped) - if [ "${SELECTED_PRIVATE_CHECKS}" != "[]" ] && [ "${IS_FORK}" != "true" ]; then + if [ "${SELECTED_PRIVATE_CHECKS}" != "[]" ]; then echo "::error title=Selected suites never ran::check-tests-private-index was skipped while ${SELECTED_PRIVATE_CHECKS} was selected." status=1 fi @@ -531,6 +552,10 @@ jobs: ;; esac + if [ "${DEFERRED_PRIVATE_CHECKS}" != "[]" ]; then + echo "::notice title=Not covered by this run::${DEFERRED_PRIVATE_CHECKS} need the private package registry and do not run on pull requests. They run on the release branch after merge." + fi + if [ "${status}" -ne 0 ]; then exit 1 fi diff --git a/.setup-scripts/fetch_private_wheels.sh b/.setup-scripts/fetch_private_wheels.sh index f62319df..f138ce66 100755 --- a/.setup-scripts/fetch_private_wheels.sh +++ b/.setup-scripts/fetch_private_wheels.sh @@ -7,17 +7,31 @@ # self-host it in the GitLab Package Registry only because that org was private; # public PyPI now serves a 0.0.1 placeholder squatting the name. # -# The predecessor, setup_artifact_registry.sh, left a 0600 ~/.netrc in place for -# the remainder of the job, so every later step -- the tox environment, the -# suite's own tests, their transitive dependencies -- could read the password. -# Test code is PR-authored, which made that credential effectively readable by -# whoever opened the pull request (STAC-25463 review, P1). +# This script only ever runs on events whose contents have been reviewed -- push, +# tag and workflow_dispatch. It does NOT run on pull requests, and the guard below +# enforces that independently of the workflow, because a pull request can edit the +# workflow as freely as it can edit this file. That is the actual protection for +# the credential; everything else here is defence in depth (STAC-25540, second +# review pass). # -# Here the credential exists only for the duration of one pip invocation whose -# package set is fixed below, and pip is then pointed at the resulting wheelhouse -# so the rest of the job resolves offline with nothing to authenticate against. +# The defence in depth still matters. The predecessor, setup_artifact_registry.sh, +# left a 0600 ~/.netrc in place for the remainder of the job, so every later step +# -- the tox environment, the suite's own tests, their transitive dependencies -- +# could read the password. That needed no malice from anyone. Here the credential +# exists only for the duration of one pip invocation whose package set is fixed +# below, and pip is then pointed at the resulting wheelhouse so the rest of the +# job resolves offline with nothing to authenticate against. set -euo pipefail +# A pull request must never reach the registry password, and must not be able to +# arrange for this script to fetch it one. The workflow already declines to run +# the job on pull requests; this is the same rule stated where it cannot be +# removed by editing a YAML condition. +if [ "${GITHUB_EVENT_NAME:-}" = "pull_request" ]; then + echo "::error title=Refusing to fetch on a pull request::${0##*/} handles the private registry credential and must not run on pull_request events; the private-index suites run on the release branch instead." + exit 1 +fi + WHEELHOUSE_ARG="${1:-}" if [ -z "${WHEELHOUSE_ARG}" ]; then echo "usage: ${0##*/} " >&2