build: mitigate potential script injection in promote workflow#7166
Conversation
Pass promote workflow inputs through environment variables instead of
interpolating them directly into shell commands with ${{ }}. Direct
interpolation allows shell metacharacters in input values to be
evaluated, which is a script injection risk in a workflow that has
access to publishing credentials.
Signed-off-by: Jared Watts <jbw976@gmail.com>
📝 WalkthroughWalkthroughA GitHub Actions workflow refactoring that replaces direct input interpolations with environment variables (VERSION, CHANNEL, and PRE_RELEASE) across multiple promotion steps, maintaining existing control flow while improving variable handling and shell parsing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/promote.yml (1)
65-67: Consider centralizing repeated input env mapping at job scope.Would you be open to defining
VERSIONandCHANNELonce underjobs.promote.env? It would reduce duplication and lower the chance of step-to-step drift later.♻️ Suggested refactor
jobs: promote: runs-on: ubuntu-24.04 + env: + VERSION: ${{ inputs.version }} + CHANNEL: ${{ inputs.channel }} steps: @@ - name: Promote Images to DockerHub if: env.DOCKER_USR != '' - env: - VERSION: ${{ inputs.version }} - CHANNEL: ${{ inputs.channel }} run: nix run .#promote-images -- crossplane/crossplane "$VERSION" "$CHANNEL" @@ - name: Promote Images to Upbound if: env.UPBOUND_MARKETPLACE_PUSH_ROBOT_USR != '' - env: - VERSION: ${{ inputs.version }} - CHANNEL: ${{ inputs.channel }} run: nix run .#promote-images -- xpkg.upbound.io/crossplane/crossplane "$VERSION" "$CHANNEL" @@ - name: Promote Images to GitHub Container Registry - env: - VERSION: ${{ inputs.version }} - CHANNEL: ${{ inputs.channel }} run: nix run .#promote-images -- ghcr.io/crossplane/crossplane "$VERSION" "$CHANNEL" @@ - name: Promote Artifacts to S3 if: env.AWS_USR != '' env: AWS_ACCESS_KEY_ID: ${{ secrets.AWS_USR }} AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_PSW }} AWS_DEFAULT_REGION: us-east-1 - VERSION: ${{ inputs.version }} - CHANNEL: ${{ inputs.channel }} PRE_RELEASE: ${{ inputs.pre-release }}Also applies to: 72-74, 78-80, 89-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/promote.yml around lines 65 - 67, Move the repeated environment variable mappings for VERSION and CHANNEL up to the job-level by defining them under jobs.promote.env so all steps inherit them; update/remove the per-step env blocks (the ones currently setting VERSION: ${{ inputs.version }} and CHANNEL: ${{ inputs.channel }} in multiple steps) to avoid duplication and drift, ensuring the job named "promote" (and any steps referencing VERSION/CHANNEL) use the centralized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/promote.yml:
- Around line 65-67: Move the repeated environment variable mappings for VERSION
and CHANNEL up to the job-level by defining them under jobs.promote.env so all
steps inherit them; update/remove the per-step env blocks (the ones currently
setting VERSION: ${{ inputs.version }} and CHANNEL: ${{ inputs.channel }} in
multiple steps) to avoid duplication and drift, ensuring the job named "promote"
(and any steps referencing VERSION/CHANNEL) use the centralized values.
haarchri
left a comment
haarchri
left a comment
There was a problem hiding this comment.
LGTM - are we backporting to our active releases ?
|
it makes sense to backport yes, but since previous versions use earthly i don't think the backport would go smoothly. i'll probably open a PR on v2.1 and then backport that PR to the other previous branches... |
|
I've opened #7167 for 2.1 earthly based promote workflow and added backport labels to that for v2.0 and v1.20. |
|
Successfully created backport PR for |
2 participants
Description of your changes
We did some repo auditing after @ytsarev shared this article about other CNCF projects github actions workflows being exploited: https://www.stepsecurity.io/blog/hackerbot-claw-github-actions-exploitation
Most of the findings for improvements are related to setting default minimal permissions for workflows, which is covered in this ongoing PR:
This PR updates the
promote.ymlworkflow to minimize a potential script injection attack.We now pass promote workflow inputs through environment variables instead of interpolating them directly into shell commands with
${{ }}. Direct interpolation allows shell metacharacters in input values to be evaluated, which is a script injection risk in a workflow that has access to publishing credentials.Note that this was not a readily exploitable vector, because the promote workflow can only be manually executed by maintainers. This path would be viable only if a maintainer's credential first became compromised.
I have:
./nix.sh flake checkto ensure this PR is ready for review.Added or updated unit tests.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Followed the API promotion workflow if this PR introduces, removes, or promotes an API.Need help with this checklist? See the cheat sheet.