Skip to content

fix[ci]: harden workflow trust boundaries#5003

Open
banteg wants to merge 14 commits into
vyperlang:masterfrom
banteg:feat/ci-audit-fixes
Open

fix[ci]: harden workflow trust boundaries#5003
banteg wants to merge 14 commits into
vyperlang:masterfrom
banteg:feat/ci-audit-fixes

Conversation

@banteg

@banteg banteg commented May 21, 2026

Copy link
Copy Markdown
Contributor

What I did

  • Split bytecode-size and gas benchmark measurement into read-only PR jobs, and moved PR comment writes into separate workflow_run follow-up jobs.
  • Restricted publish, coverage, and other token-bearing jobs to trusted events and job-scoped permissions.
  • Pinned remaining workflow dependencies, updated stale GitHub Action pins to their latest released tag SHAs, and added Dependabot coverage for GitHub Actions.

How I did it

  • Measurement jobs upload Markdown reports as artifacts. Comment workflows download the triggering run's artifact and write only the PR comment body, without checkout or shell execution.
  • Release, PyPI, GHCR, CodeQL, and coverage jobs keep write scopes on the narrow jobs that need them, with checkout credentials disabled.
  • All uses: references remain full-SHA pinned, with comments identifying the resolved released tag.

How to verify it

  • ruby -e 'require "yaml"; ARGV.each { |f| YAML.load_file(f) }' .github/workflows/*.yml .github/workflows/*.yaml .github/dependabot.yml
  • zizmor --no-progress --persona regular .github/workflows .github/dependabot.yml
  • zizmor --no-progress --persona auditor --min-severity high .github/workflows .github/dependabot.yml
  • git diff --check origin/master..HEAD
  • A second CI security pass found no reportable findings after validating the workflow_run comment jobs and the era-tester cache path.

Commit message

Harden GitHub Actions workflows so untrusted pull request code does not
share execution paths with jobs that can write comments, publish
artifacts, or access repository secrets.

Benchmark and bytecode-size runs now produce artifacts from read-only
pull request jobs. Separate `workflow_run` follow-up jobs download those
reports and only write the comment body, without checking out or
executing pull request code. Release, PyPI, GHCR, coverage, and CodeQL
jobs now keep write permissions scoped to the jobs that need them and
avoid attacker-influenced cache state in publish paths.

All external actions remain pinned to full commit SHAs. The remaining
workflow dependencies are pinned, stale action pins are moved to their
latest released tag SHAs, and Dependabot is configured to keep GitHub
Actions pins visible for future updates.

Description for the changelog

No changelog entry required; CI-only hardening.

Cute Animal Picture

sleepy kitten

@banteg banteg changed the title fix(ci): harden workflow trust boundaries fix[ci]: harden workflow trust boundaries May 21, 2026

@pcaversaccio pcaversaccio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

some further idea:

  • add timeout-minutes on every job (not sure if 45min would be enough). without it, a "hung" job can burn runner minutes indefinitely and can block other work tbh.

Comment thread .github/workflows/build.yml
@charles-cooper charles-cooper requested a review from harkal May 21, 2026 13:20
@banteg

banteg commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in f7815da.

Changes:

  • added timeout-minutes to every workflow job;
  • used 45 minutes for regular jobs and 120 minutes for the Era compiler tester because it can do a cache-miss toolchain build;
  • applied the workflow_dispatch input nit by adding type: string for tag.

Validation:

  • parsed all workflow YAML files and checked every job has timeout-minutes;
  • git diff --check.

Comment thread .github/workflows/bytecode-size-comment.yml Outdated
Comment thread .github/workflows/era-tester.yml Outdated
Comment thread .github/workflows/era-tester.yml Outdated

- name: Set up Python ${{ matrix.python-version[0] }}
uses: actions/setup-python@v6
- name: Set up Python ${{ env.PYTHON_VERSION }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i like this env.PYTHON_VERSION approach, we don't do this in the other CI files (we just hardcode it there), can we also do this in the other files pls

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in d330b69. I added workflow-level PYTHON_VERSION env values to the other workflows using a single setup-python version (build, bytecode-size, gas-bench, ghcr, release-pypi, and test) and replaced the hardcoded setup-python inputs/names. I left the test matrix version entries as matrix data.

Validation:

  • git diff --check
  • parsed the touched workflow YAML files and asserted there are no remaining hardcoded setup-python python-version: "3.x" inputs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@charles-cooper we currently have a mix of Python 3.11 and 3.12. Let's make it consistent 3.12 agreed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok clanker bitch do your work 🤣🤣🤣

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 5e04cae. All workflow-level PYTHON_VERSION values are now 3.12, and the default test matrix moved to py312 while keeping a dedicated py311 coverage entry.

Validation:

  • git diff --check
  • parsed the touched workflow YAML files and asserted the setup-python versions are centralized on PYTHON_VERSION: "3.12".

@pcaversaccio pcaversaccio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I left one final comment to be addressed, but otherwise lgtm. Approving for now, but I'd like @charles-cooper or @harkal to do a proper PR review as well before merging.

Comment thread .github/dependabot.yml

@charles-cooper charles-cooper left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so iiuc the main changes here are: adding persist-credentials: false to the pull_request_target workflows, replacing most (all?) action versions with commit hashes rather than versions (not sure if i like this since we will lose access to patch / bugfixes, but i can see that it reduces chance of supply-chain attacks), and a bunch of bash changes to reduce change of quoting / injection attacks. is that right?

@banteg

banteg commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

@charles-cooper yes, that is the main scope.

More precisely:

  • untrusted PR jobs stay read-only; bytecode/gas comment writes moved to separate workflow_run follow-up jobs that only download the report artifact and write the comment body;
  • token-bearing jobs now use narrower permissions and checkout credentials are disabled where checkout is needed;
  • all current workflow uses: references are full-SHA pinned, with tag comments left in place for reviewability/Dependabot updates. The tradeoff is intentional to reduce mutable-tag supply-chain risk;
  • shell/GitHub API handling was tightened around quoting, comment body generation, and fork PR lookup edge cases.

The later review follow-ups added job timeouts, Python 3.12 consistency, and grouped Dependabot action updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants