Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hlsl] Pin hlsl-test-all resusable workflow to main branch #122518

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tstellar
Copy link
Collaborator

This will cause each hlsl test workflow to load the hlsl-test-all file from the main branch instead of from the source branch of the PR.

PROs:

  • We can constrain use of the self-hosted Offload Runners to the hlsl-test-all workflow.
  • This will protect the runners from "Script Kiddie" attacks where someone submits a PR with a malicious workflow to many repositories at once.

CONs:

  • This will not protect the Offload Runners from someone submitting a PR that modifies the LLVM source to execute malicious code when built.
  • It will not be possible to test changes to the hlsl-test-all workflow in a PR. We would need to set up some other process for doing this e.g. a special branch name that can be pushed to to test changes.

This will cause each hlsl test workflow to load the hlsl-test-all file
from the main branch instead of from the source branch of the PR.

PROs:
 * We can constrain use of the self-hosted Offload Runners to the
   hlsl-test-all workflow.
 * This will protect the runners from "Script Kiddie" attacks where
   someone submits a PR with a malicious workflow to many repositories
   at once.

CONs:
 * This *will not* protect the Offload Runners from someone submitting a
   PR that modifies the LLVM source to execute malicious code when built.
 * It will not be possible to test changes to the hlsl-test-all workflow
   in a PR.  We would need to set up some other process for doing this
   e.g. a special branch name that can be pushed to to test changes.
@tstellar
Copy link
Collaborator Author

I need some help thinking through the PROs and CONs, I'm not sure I got them all.

@tstellar tstellar force-pushed the hlsl-runner-restrictions branch from 755a824 to ae4befe Compare January 10, 2025 20:15
@tstellar tstellar requested a review from carlocab January 10, 2025 20:15
@tstellar tstellar marked this pull request as ready for review January 10, 2025 20:18
@llvmbot
Copy link
Member

llvmbot commented Jan 10, 2025

@llvm/pr-subscribers-github-workflow

Author: Tom Stellard (tstellar)

Changes

This will cause each hlsl test workflow to load the hlsl-test-all file from the main branch instead of from the source branch of the PR.

PROs:

  • We can constrain use of the self-hosted Offload Runners to the hlsl-test-all workflow.
  • This will protect the runners from "Script Kiddie" attacks where someone submits a PR with a malicious workflow to many repositories at once.

CONs:

  • This will not protect the Offload Runners from someone submitting a PR that modifies the LLVM source to execute malicious code when built.
  • It will not be possible to test changes to the hlsl-test-all workflow in a PR. We would need to set up some other process for doing this e.g. a special branch name that can be pushed to to test changes.

Full diff: https://github.com/llvm/llvm-project/pull/122518.diff

1 Files Affected:

  • (modified) .github/workflows/hlsl-matrix.yaml (+1-1)
diff --git a/.github/workflows/hlsl-matrix.yaml b/.github/workflows/hlsl-matrix.yaml
index c63a32acd2b3e0..e0185d19637ee3 100644
--- a/.github/workflows/hlsl-matrix.yaml
+++ b/.github/workflows/hlsl-matrix.yaml
@@ -23,7 +23,7 @@ jobs:
         runs-on:
           - hlsl-macos
 
-    uses: ./.github/workflows/hlsl-test-all.yaml
+    uses: llvm/llvm-project/.github/workflows/hlsl-test-all.yaml@main
     with:
       SKU: hlsl-macos
       TestTarget: check-hlsl-clang-mtl # TODO: This target changes based on SKU

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

I’m good with these tradeoffs. I don’t expect this workflow to change too often in significant ways, and since it is only a small part of the community depending on it we can make do with the challenges testing it.

Thank you @tstellar for your help getting this working!

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Trade-offs look right to me. Thanks!

@boomanaiden154
Copy link
Contributor

@llvm-beanz How is the runner setup? Is it physical hardware on a network with anything sensitive? A cloud VM somewhere? If it's trivial to nuke from orbit and can't touch anything anyone would care about, I think that changes the security posture versus if those things aren't true.

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

Successfully merging this pull request may close these issues.

5 participants