Skip to content

Stop privileged workflows from checking out artifact-selected SHAs - #1248

Merged
cgruver merged 1 commit into
dora-metrics:mainfrom
etsauer:cursor/fix-artifact-sha-checkout
Aug 27, 2026
Merged

Stop privileged workflows from checking out artifact-selected SHAs#1248
cgruver merged 1 commit into
dora-metrics:mainfrom
etsauer:cursor/fix-artifact-sha-checkout

Conversation

@etsauer

@etsauer etsauer commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Linked Issues

related to Red Hat PSIRTSUPT-22553 / BOOST-VPD-2026-044

Description

Privileged workflow_run jobs in test-images.yml and integration-tests.yml checked out whatever SHA the untrusted producer artifact supplied as branch_ref. A fork pull request can put a different fork-network commit in that artifact and bypass actions/checkout's unsafe-checkout guard, then run with Quay and other secrets.

These consumers now check out github.event.workflow_run.head_sha, skip unless the triggering repository is this repo, and disable persisted git credentials after checkout.

Testing Instructions

  • Confirm same-repo PRs and pushes that trigger Trigger test images still run Test images manager and Integration Tests, and that checkout uses the workflow_run head SHA (not branch_ref from the artifact).
  • Confirm a fork PR no longer reaches the privileged create-images / integration-test jobs (job skipped by the head_repository condition).
  • Follow the standard PR testing process as applicable; these files are GitHub Actions workflows and are not covered by in-repo unit tests.

Made with Cursor

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2026
@openshift-ci

openshift-ci Bot commented Aug 26, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@etsauer

etsauer commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Sandbox test results

workflow_run consumers always execute the YAML from the default branch, so these checks were run on a throwaway copy of this branch rather than against dora-metrics/pelorus itself:

Static review

  • Consumers no longer read branch_ref from the artifact or pass it to actions/checkout.
  • Privileged jobs are gated with pr_type != 'closed' && github.event.workflow_run.head_repository.full_name == github.repository.
  • Checkout uses ref: ${{ github.event.workflow_run.head_sha }} and persist-credentials: false.
  • The producer still uploads branch_ref; consumers ignore it.

Happy path (same-repo push)

Pushed 751514b to test-images-verify-1248 on the sandbox.

Workflow Result
Trigger test images #33080874792 success
Test images manager #33081168831 setup succeeded; create-images ran (not skipped)
Integration Tests (same workflow_run) setup succeeded; integration-test ran (not skipped)

Checkout logged ref: 751514b25c8d35bd76059303ad00ed14c286f498 with persist-credentials: false, matching github.event.workflow_run.head_sha. Quay login failed on dummy credentials, which is expected.

Negative path (fork PR)

Opened sandbox PR #11 from etsauer-acme/pelorus-pr1248-sandbox (harmless exporters/README.md comment). Producer pr_type was synchronize, not closed.

Workflow Result
Trigger test images #33082136722 success (unprivileged producer)
Test images manager #33082167176 setup succeeded; create-images skipped
Integration Tests #33082167193 setup succeeded; integration-test skipped

Skip is from the head_repository gate (etsauer-acme/... vs etsauer/...), not from a failed setup job. Privileged jobs did not reach Checkout or Quay/Jira.

Verdict

Pass for the change in this PR: same-repo triggers still check out workflow_run.head_sha and run privileged jobs; fork PRs skip those jobs.

Residual risk (not a failure of this PR)

  • setup still downloads an untrusted artifact and still trusts pr_type / pr_number / commit_hash from it. The new repository gate is what stops fork PRs from using secrets.
  • Same-repo branches (maintainers, Dependabot) still get Quay credentials and a write GITHUB_TOKEN on create-images.
  • Fork contributors will no longer get auto-published test images or integration tests. That matches the PSIRT “trusted commits only” guidance; worth an explicit maintainer ack before merge.
  • Broader PSIRT follow-ups remain: split untrusted builds from publish, tighten token permissions, rotate Quay secrets.

A smoke test on dora-metrics/pelorus is only meaningful after this YAML is on main.

Fork PRs could place an arbitrary fork-network SHA in the producer artifact and bypass actions/checkout's unsafe-checkout guard.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Eric Sauer <esauer@redhat.com>
@etsauer
etsauer force-pushed the cursor/fix-artifact-sha-checkout branch from 751514b to 947123a Compare August 27, 2026 14:39
@etsauer
etsauer marked this pull request as ready for review August 27, 2026 14:41
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 27, 2026
@openshift-ci
openshift-ci Bot requested review from cgruver and mpryc August 27, 2026 14:41
@openshift-ci

openshift-ci Bot commented Aug 27, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgruver

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2026
@cgruver

cgruver commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2026
@cgruver
cgruver merged commit 7403445 into dora-metrics:main Aug 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants