OSAC-2186: Pin configAsCode.eeImage/projectGitBranch at release - #412
Conversation
|
@minmzzhang: This pull request references OSAC-2186 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe chart publishing workflow validates the extracted tag version, then updates and verifies ChangesChart publishing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish-charts.yaml:
- Around line 38-39: Prevent shell injection in the publish workflow by passing
the tag-derived version through an environment variable rather than
interpolating steps.version.outputs.version directly into shell commands.
Validate the variable against the expected release-version format before use,
then reference the validated variable in the image update on line 37 and the
eeImage and projectGitBranch updates in the sed commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0dc70a39-dec7-4360-9105-ee5f396ea013
📒 Files selected for processing (1)
.github/workflows/publish-charts.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish-charts.yaml:
- Around line 43-45: Update the chart replacement steps in the publish workflow
to fail if any expected placeholder is missing or duplicated. For each of image,
eeImage, and projectGitBranch, assert exactly one matching placeholder before
editing or verify the resulting values after editing, and terminate the workflow
on failure before packaging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c30446af-ae86-44f6-9471-7c3a5319b9ab
📒 Files selected for processing (1)
.github/workflows/publish-charts.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/publish-charts.yaml (2)
83-105: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd SAST/SCA before chart packaging.
This release workflow publishes the chart without a visible SAST/SCA gate. Add pinned scan steps that fail before
helm package.As per path instructions,
.github/workflows/**/*requires “SAST/SCA steps in pipeline.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish-charts.yaml around lines 83 - 105, Add pinned SAST and SCA scan steps to the publish workflow before the helm package command, ensuring each scan fails the job on findings and runs against the repository/chart sources as appropriate. Keep the existing version substitution and packaging flow unchanged aside from placing the security gates before packaging.Source: Path instructions
83-105: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSign the published OCI chart with Sigstore.
The chart is packaged and pushed without a
cosignsignature. Sign the pushed OCI artifact with a SHA-pinned setup action and minimally scopedid-token: writepermission.As per path instructions,
.github/workflows/**/*requires “Sign artifacts with Sigstore/cosign.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish-charts.yaml around lines 83 - 105, The publish workflow must sign the pushed OCI chart with Sigstore. Add a SHA-pinned cosign setup action, grant the workflow minimally scoped id-token: write permission, and invoke cosign to sign the published chart reference after the chart push step, using the existing VERSION-based artifact tag.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/publish-charts.yaml:
- Around line 83-105: Add pinned SAST and SCA scan steps to the publish workflow
before the helm package command, ensuring each scan fails the job on findings
and runs against the repository/chart sources as appropriate. Keep the existing
version substitution and packaging flow unchanged aside from placing the
security gates before packaging.
- Around line 83-105: The publish workflow must sign the pushed OCI chart with
Sigstore. Add a SHA-pinned cosign setup action, grant the workflow minimally
scoped id-token: write permission, and invoke cosign to sign the published chart
reference after the chart push step, using the existing VERSION-based artifact
tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 56db46cd-d84b-4e30-bc5e-4b04f1f746b7
📒 Files selected for processing (1)
.github/workflows/publish-charts.yaml
Pin configAsCode.eeImage and configAsCode.projectGitBranch to the release tag in publish-charts.yaml, matching the existing bootstrap.image substitution. Pass the version through an env var with format validation to prevent tag-injection, and verify each sed substitution landed to fail closed on placeholder drift. Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: Min Zhang <minzhang@redhat.com>
11c90bd to
93cd515
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz, minmzzhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7ad4dff
into
osac-project:main
3 participants
Summary
charts/aap/values.yamldeclaresconfigAsCode.eeImageandconfigAsCode.projectGitBranchas empty-string placeholders.bootstrap.imageis patched to the exact release version bypublish-charts.yamlbefore packaging, but these two fields were never touched by the release workflow.{{- with }}treats an empty string as falsy, the chart template omitsAAP_EE_IMAGE/AAP_PROJECT_GIT_BRANCHfrom the generated Secret whenever these are unset, and the Ansible config-as-code layer (playbooks/vars/config.yml) then falls back toghcr.io/osac-project/osac-aap:latest/main. A released chart deployed with default values therefore silently runs an unpinned execution-environment image and syncs automation content frommaininstead of the tested release.sedsubstitutions alongside the existingbootstrap.imagepatch so both fields default to the release version/tag, consistent with howbootstrap.imageis already handled. The committedvalues.yamlplaceholders are intentionally left unchanged — only the release-time patch step is extended.Hardening added in response to CodeRabbit review
steps.version.outputs.version(derived from the pushed tag'sGITHUB_REF_NAME) was interpolated directly via${{ }}template expansion into the shell script. GitHub Actions substitutes${{ }}as raw text before bash parses it, so a maliciously crafted tag name could break out of thesedargument and run arbitrary commands in this privileged release job (contents: write,packages: write). Fixed by passing the version throughenv:and validating it against^[0-9A-Za-z._-]+$before use, applied to all three substitutions (including the pre-existingbootstrap.imageline).sedexits0even when it matches zero lines, so a futurevalues.yamlformat change could silently leave a placeholder unpinned with no error. Added areplace_and_verifycheck after each substitution that fails the workflow if the expected replacement isn't found.Verified locally (not just by inspection):
projectGitUrifield untouched.sed.Test plan
v*tag (or dry-run the workflow) and confirm the packaged chart'svalues.yamlhaseeImageandprojectGitBranchpinned to the release version/tag rather than empty strings.bootstrap.imagepatch behavior is unchanged.; touch /tmp/PWNED) is rejected by the version validation instead of executing.Fixes OSAC-2186.
Summary by CodeRabbit