Skip to content

ci: harden GitHub Actions security - #78

Merged
qibinlei merged 7 commits into
mainfrom
secure-ci
Aug 14, 2026
Merged

ci: harden GitHub Actions security#78
qibinlei merged 7 commits into
mainfrom
secure-ci

Conversation

@kratsg

@kratsg kratsg commented Aug 13, 2026

Copy link
Copy Markdown
Member

Summary

CI-security hardening pass: pin all GitHub Actions to SHAs, fix zizmor findings, add zizmor as a pre-commit hook, add a dependabot cooldown, and close a gap where fork PRs could run arbitrary code on the self-hosted runner.

Changes

  • Pin all GitHub Actions to SHAs (npx actions-up), each with a # vX.Y.Z comment. Verified every pin against the upstream tag ref.
  • Fix zizmor template-injection findings: the parse, setup-globus, and upload composite actions interpolated ${{ inputs.* }} directly into run: shell blocks. Routed all of them through env: instead.
  • Fix zizmor artipacked findings: added persist-credentials: false to every actions/checkout step (none of these jobs need to push using the checkout token).
  • Fix zizmor excessive-permissions finding: added a workflow-level permissions: contents: read to docs.yml so the build job no longer runs with default permissions (the deploy job already had its own narrower explicit grant).
  • Added zizmor --persona=pedantic as a pre-commit hook, scoped with --min-severity=low so it doesn't fail on the persona's informational-only job-naming nits (see "zizmor ignores" below for the one real ignore).
  • Dependabot: added a 7-day cooldown to the github-actions group, switched it to monthly, and renamed the group key from the deprecated actions alias to github-actions.
  • .pre-commit-config.yaml: froze every hook to its current tag's SHA (prek auto-update --freeze --cooldown-days 7); no hook had a release outside the cooldown window, so no versions moved. Also set autoupdate_schedule: monthly on the existing ci: block.
  • Closed a self-hosted-runner / fork-PR gap: the rucio job in uchicago.yml already skipped itself for pull_request events from forks, but every other job on arc-runner-set-uchicago (including test-actions in ci.yml) had no such guard — any fork could open a PR and have its branch's shell scripts execute directly on the lab's self-hosted infrastructure. Applied the same github.event.pull_request.head.repo.fork == false guard everywhere that job pattern is used.

Major version changes

None. actions-up moved every action reference to a SHA of the same tag it was already on (e.g. actions/checkout@v7v7.0.1 pinned by SHA); no major bumps were available/applicable. prek auto-update likewise found no hook releases outside the 7-day cooldown, so no pre-commit hook moved versions either.

One version note (not major, but worth flagging): prefix-dev/setup-pixi in .github/actions/parse/action.yml was on v0.9.3 while ci.yml/docs.yml already used v0.10.1; actions-up brought it in line with the other two call sites at v0.10.1 (SHA-pinned).

Held back

  • pixi.toml: exclude-newer cooldown — the skill's default step 6 asks for a 7-day exclude-newer cooldown on pixi-managed dependencies. exclude-newer was only added to pixi in v0.67.0, but this repo pins pixi-version: v0.49.0 in three places (ci.yml, docs.yml, .github/actions/parse/action.yml). Adding the setting now would very likely break every pixi run/pixi install invocation in CI against an old pixi that doesn't recognize the field. Skipped; bumping pixi itself to ≥0.67.0 is a separate, non-security change that deserves its own PR and testing.

zizmor ignores

  • dangerous-triggers on semantic-pr-check.yml (pull_request_target): this workflow never checks out the PR's code — there's no actions/checkout step at all — and only reads the PR title via the GitHub API through amannn/action-semantic-pull-request. pull_request_target is used deliberately so PRs from forks still get a statuses: write-capable token to report the check; a plain pull_request trigger is forced read-only for fork PRs and would silently fail to post the status, defeating the point of validating first-time contributors' PR titles. Documented in .github/zizmor.yml.

Verification

  • uvx zizmor --persona=pedantic --min-severity=low .github → clean (0 low/medium/high findings; the informational job-naming findings are filtered by design and are not security-relevant).
  • uvx prek run --all-files → all hooks pass, including the newly added zizmor hook.
  • Every SHA pin verified against git ls-remote for its upstream tag ref before committing.

kratsg added 6 commits August 13, 2026 13:06
Assisted-by: Claude (Anthropic)
zizmor flagged direct ${{ inputs.* }} interpolation inside run: blocks in
the parse, setup-globus, and upload composite actions as code-injection
risk. Route inputs through env vars instead so shell expansion cannot be
influenced by template expansion.

Assisted-by: Claude (Anthropic)
- Set persist-credentials: false on every actions/checkout step; none of
  these jobs need the git token to persist past the job.
- Add an explicit contents: read workflow-level permissions block to
  docs.yml so the build job no longer runs with default (broad)
  permissions; the deploy job keeps its own narrower explicit grant.

Assisted-by: Claude (Anthropic)
- Add zizmor as a pre-commit hook (pedantic persona) so CI-security
  findings surface locally, matching the check now run in CI.
- Add a 7-day cooldown to the github-actions dependabot updates group,
  switch it to monthly to match the pre-commit bot's cadence, and rename
  the group key from the deprecated actions alias to github-actions.
- Set autoupdate_schedule: monthly on the existing pre-commit ci: block.
- Add .github/zizmor.yml with a documented ignore for the
  dangerous-triggers finding on semantic-pr-check.yml's
  pull_request_target trigger (no checkout step, PR-title-only, needed
  for a write-scoped token on fork PRs).

Assisted-by: Claude (Anthropic)
prek auto-update --freeze --cooldown-days 7 found no releases outside the
cooldown window, so every hook stays on its current version, now pinned by
SHA. Also tune the new zizmor hook to --min-severity=low so it doesn't
fail on the pedantic persona's informational-only job-naming findings.

Assisted-by: Claude (Anthropic)
The rucio job already guarded against running on arc-runner-set-uchicago
for pull_request events from forks, but every other job on that
self-hosted runner (test-actions in ci.yml, and all uchicago.yml jobs
besides rucio) had no such guard. Any fork could open a PR and have its
branch's shell scripts execute directly on the lab's self-hosted
infrastructure. Apply the same fork check consistently to close the gap.

Assisted-by: Claude (Anthropic)
@kratsg
kratsg marked this pull request as ready for review August 13, 2026 18:33
Comment thread .github/actions/parse/action.yml
@qibinlei

Copy link
Copy Markdown
Collaborator

Regression: host field silently becomes the literal string ${NODE_NAME} instead of the real hostname

Where: .github/actions/parse/action.yml (the new HOST: ${{ inputs.host }} / --host "$HOST" pattern) and every caller in .github/workflows/uchicago.yml that passes host: ${NODE_NAME} (11 call sites: rucio, evnt-native, evnt-el9, evnt-centos7, truth3-native, truth3-el9, truth3-centos7, coffea, eventloop-columnar, eventloop-standard, fastframes).

Why it breaks: before this PR, ${{ inputs.host }} was substituted as literal text directly into the run: script source, so bash actually saw --host "${NODE_NAME}" in the script and expanded it against the runner's real $NODE_NAME OS env var at execution time. After moving to env: HOST: ${{ inputs.host }} + --host "$HOST" (the injection-safety fix this PR makes for inputs.job, inputs.log-file, etc.), HOST's value is just the inert 12-character string ${NODE_NAME} — bash does not re-parse an environment variable's value as further shell syntax, so it gets passed through unexpanded.

Verified directly:

$ NODE_NAME=actual-runner-hostname bash -c 'echo --host "${NODE_NAME}"'
--host actual-runner-hostname     # old template-then-execute behavior
$ HOST='${NODE_NAME}' bash -c 'echo --host "$HOST"'
--host ${NODE_NAME}               # new behavior: literal, wrong

Impact: every Kibana benchmark payload produced by the uchicago.yml jobs will record the literal string ${NODE_NAME} in the host field instead of the actual runner hostname, and this will fail silently since the parse/upload steps use continue-on-error.

Suggested fix (in .github/workflows/uchicago.yml):

  1. Add a step after checkout in each job to promote the runner's real NODE_NAME OS env var into the GitHub Actions env context — self-hosted runners don't auto-populate env.* from arbitrary OS env vars, they have to be exported explicitly:
    - name: Export node name
      run: echo "NODE_NAME=$NODE_NAME" >> "$GITHUB_ENV"
  2. Change all 11 occurrences of host: ${NODE_NAME} to host: ${{ env.NODE_NAME }}.

This also brings uchicago.yml in line with docs/workflows/benchmarks.md:79 and docs/workflows/development.md:138, which already document host: ${{ env.NODE_NAME }} as the intended pattern — uchicago.yml and docs/workflows/parsing.md just never matched that (a pre-existing inconsistency this PR's refactor turned into an active bug).

Moving the parse composite action's inputs to env vars (to fix
zizmor's template-injection findings) stopped the ${NODE_NAME} host
value from being re-expanded as shell syntax: HOST's value became the
inert literal string "${NODE_NAME}" instead of the runner's real
hostname. Export the runner's NODE_NAME into GITHUB_ENV after checkout
in each job and pass it as host: env.NODE_NAME (the GHA expression),
matching the pattern already documented in docs/workflows/benchmarks.md
and development.md.

Reported-by: qibinlei
Assisted-by: Claude (Anthropic)
@kratsg

kratsg commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

Thanks for catching this, @qibinlei — confirmed and fixed in 8539980. Added an "Export node name" step (echo "NODE_NAME=$NODE_NAME" >> "$GITHUB_ENV") after checkout in all 11 affected jobs and switched host: ${NODE_NAME} to host: ${{ env.NODE_NAME }}, matching the pattern already documented in benchmarks.md/development.md. Re-verified zizmor (still clean) and prek (all hooks pass) after the change.

@qibinlei
qibinlei merged commit d86d628 into main Aug 14, 2026
33 checks passed
@qibinlei
qibinlei deleted the secure-ci branch August 14, 2026 02:10
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.

2 participants