-
Notifications
You must be signed in to change notification settings - Fork 10
[MAIN GHA] Fork GHA enablement #789
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
base: main
Are you sure you want to change the base?
Conversation
WIWT took agressive security recommendations from cluade
This is not what we needed and needed removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a security-enhanced approach to validating kernel commits in forked repositories by splitting the validation workflow into two separate workflows: a check workflow that runs in untrusted fork context, and a comment workflow that runs in the trusted repository context with write permissions.
Key changes:
- Split validation into two workflows to avoid exposing secrets to fork code
- Added comprehensive input validation and sanitization to prevent command injection
- Implemented artifact-based communication with integrity verification between workflows
- Enhanced JIRA integration with credential masking and output filtering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
.github/workflows/validate-kernel-commits-check-secure.yml |
Check workflow that runs validation tools without requiring secrets or write permissions, saves results as artifacts |
.github/workflows/validate-kernel-commits-comment-secure.yml |
Comment workflow triggered by workflow_run that downloads artifacts, validates metadata, posts PR comments, and performs JIRA checks with secrets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BODY="$EMOJI **Validation checks $STATUS**\ | ||
| \ |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multiline string format has inconsistent escaping. Line 412 has a trailing backslash after the emoji and message, but line 413 is empty (just backslash-newline), and line 414 continues. This creates an extra empty line in the output. Consider removing the backslash from line 413 or adjusting the string formatting for cleaner output.
| BODY="$EMOJI **Validation checks $STATUS**\ | |
| \ | |
| BODY="$EMOJI **Validation checks $STATUS** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fought errors with the escaping this was shown to actually work
| ckc_result.txt | ||
| interdiff_result.txt | ||
| pr_metadata/ | ||
| retention-days: 3 # Increased from 1 to prevent premature deletion |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retention-days is set to 3 days with a comment mentioning it was "Increased from 1 to prevent premature deletion." However, for workflow_run artifacts that are consumed by a downstream workflow, 3 days may be excessive. Consider if 1 day would be sufficient since the comment workflow should run immediately after the check workflow completes. The extra retention increases storage costs without clear benefit.
| retention-days: 3 # Increased from 1 to prevent premature deletion | |
| retention-days: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing this to start so that we have a failed friday run the chance to rerun it first thing monday.
| # Validate base branch name (alphanumeric, dots, slashes, dashes, underscores, curly braces) | ||
| # Note: hyphen must be at end of character class or escaped to be literal | ||
| if ! [[ "$BASE_REF" =~ ^[a-zA-Z0-9/_.{}-]+$ ]]; then | ||
| echo "❌ Invalid base branch name: $BASE_REF" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Validate head branch name | ||
| if ! [[ "$HEAD_REF" =~ ^[a-zA-Z0-9/_.{}-]+$ ]]; then |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same consideration about curly braces in branch name patterns as noted for the comment workflow. While currently safe due to proper quoting, consider if curly braces are necessary for your branch naming convention.
| # Validate base branch name (alphanumeric, dots, slashes, dashes, underscores, curly braces) | |
| # Note: hyphen must be at end of character class or escaped to be literal | |
| if ! [[ "$BASE_REF" =~ ^[a-zA-Z0-9/_.{}-]+$ ]]; then | |
| echo "❌ Invalid base branch name: $BASE_REF" | |
| exit 1 | |
| fi | |
| # Validate head branch name | |
| if ! [[ "$HEAD_REF" =~ ^[a-zA-Z0-9/_.{}-]+$ ]]; then | |
| # Validate base branch name (alphanumeric, dots, slashes, dashes, underscores) | |
| # Note: hyphen must be at end of character class or escaped to be literal | |
| if ! [[ "$BASE_REF" =~ ^[a-zA-Z0-9/_.-]+$ ]]; then | |
| echo "❌ Invalid base branch name: $BASE_REF" | |
| exit 1 | |
| fi | |
| # Validate head branch name | |
| if ! [[ "$HEAD_REF" =~ ^[a-zA-Z0-9/_.-]+$ ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently use this pattern, we may address this in the future with a separate update
|
|
||
| on: | ||
| workflow_run: | ||
| workflows: ["Validate Kernel Commits"] |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow_run trigger references a workflow named "Validate Kernel Commits" but the check workflow being added in this PR is named "Validate Kernel Commits - Check (Secure)". These names must match exactly for the workflow_run trigger to work. Either update this reference to match the check workflow's name, or ensure there is a separate workflow with the exact name "Validate Kernel Commits" that will trigger this comment workflow.
| workflows: ["Validate Kernel Commits"] | |
| workflows: ["Validate Kernel Commits - Check (Secure)"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoping to do a git move as a part of this refactor, if not it will be fixed as a temporary thing.
| BODY="🤖 **Validation Checks In Progress** \ | ||
| \ | ||
| Workflow run: $RUN_URL \ | ||
| \ | ||
| This comment will be updated with check results when complete." | ||
|
|
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar multiline string formatting issue as in the final summary. Lines 133-137 have inconsistent backslash usage creating extra whitespace. The string will have extra line breaks between "In Progress" and "Workflow run:" and between the URL and the final message.
| BODY="🤖 **Validation Checks In Progress** \ | |
| \ | |
| Workflow run: $RUN_URL \ | |
| \ | |
| This comment will be updated with check results when complete." | |
| BODY="$(cat <<EOF | |
| 🤖 **Validation Checks In Progress** | |
| Workflow run: $RUN_URL | |
| This comment will be updated with check results when complete. | |
| EOF | |
| )" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had issues with GitHub processing this so they are this way for a reason.
Cleaning up some minor changes and makding checks a little safer.
Ok we've been having issues with forked repos being able to utilize the excellent shared GHA
validate-kernel-commits.ymlI used
validate-kernel-commitsto be split apart the actions that can be run from any context. And anything that posts to the PR or looks up content in JIRA to an on_complete OR manual trigger which will run within our repo context.This helps save us from having to use
pull_request_targetwhere the secrets are "visible" to code coming from the fork.This is safe because the forks from individuals not a part of our organization require manual approval to kick off runs, so we should be doing due diligence before running the actions on behalf of the submitter.
While I was there I asked claude to do a security review and do fixups. I think its being A LITTLE paranoid, but I'd rather default to paranoid.
See comments in this PR (start from the bottom)
TEST [fips-legacy-8-compliant] Many VULNS 2025-12-18 #782
For the team
Should I
git mvthese files overtop the old one so there it becomes an inplace, or would we want to merge it test it again on a fork then move them over top?