USHIFT-6920: feat: add crypto_scan auto-rebase for cbom generation#7051
USHIFT-6920: feat: add crypto_scan auto-rebase for cbom generation#7051fracappa wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fracappa: This pull request references USHIFT-6920 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 story 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fracappa The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThe PR adds a strict Bash crypto-scan workflow that stages selected MicroShift sources, extracts a scanner image with ChangesCryptographic artifact generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RebaseJob
participant CryptoScan
participant Skopeo
participant CryptoScanner
participant JQ
participant Git
RebaseJob->>CryptoScan: Run crypto scan
CryptoScan->>Skopeo: Download scanner image
Skopeo-->>CryptoScan: Provide scanner layers
CryptoScan->>CryptoScanner: Scan staged MicroShift sources
CryptoScanner-->>CryptoScan: Write CBOM JSON
CryptoScan->>JQ: Transform CBOM to SPDX 2.3
JQ-->>CryptoScan: Write SPDX JSON
RebaseJob->>Git: Stage and commit changed artifacts
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 3
🤖 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 `@scripts/auto-rebase/crypto_scan.sh`:
- Line 38: In scripts/auto-rebase/crypto_scan.sh, replace the CBOM status
command substitution at lines 38-38 with separate echo and ls commands, then
apply the same change to the SPDX status output at lines 79-79: echo the
generated-file label first and run ls -lh on the corresponding output variable
separately so failures propagate.
- Line 25: Update the EXIT trap command in crypto_scan.sh to preserve quoting
around SCAN_DIR when the deferred cleanup runs, ensuring rm -rf handles paths
containing spaces safely.
- Around line 1-5: Update the script header to use the /usr/bin/bash shebang,
remove the shellcheck disable directive, and consolidate the separate errexit,
nounset, and pipefail settings into a single set -euo pipefail command.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a283846f-6ebb-4d10-89eb-1fb49d0e47df
📒 Files selected for processing (2)
scripts/auto-rebase/crypto_scan.shscripts/auto-rebase/rebase_job_entrypoint.sh
| # findings already covered by "vendor/". | ||
| # We stage a scan directory with only the sources we want scanned. | ||
| SCAN_DIR=$(mktemp -d) | ||
| trap "rm -rf ${SCAN_DIR}" EXIT |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote variables in trap command.
The double-quoted string evaluates ${SCAN_DIR} without quotes in the resulting command, which will fail if the path contains spaces. As per coding guidelines, shell scripts must quote all variables.
Proposed fix
-trap "rm -rf ${SCAN_DIR}" EXIT
+trap 'rm -rf "${SCAN_DIR}"' EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trap "rm -rf ${SCAN_DIR}" EXIT | |
| trap 'rm -rf "${SCAN_DIR}"' EXIT |
🤖 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 `@scripts/auto-rebase/crypto_scan.sh` at line 25, Update the EXIT trap command
in crypto_scan.sh to preserve quoting around SCAN_DIR when the deferred cleanup
runs, ensuring rm -rf handles paths containing spaces safely.
Source: Coding guidelines
| "${SCANNER_IMAGE}" \ | ||
| /workspace > "${CBOM_OUTPUT}" | ||
|
|
||
| echo "CBOM generated: $(ls -lh "${CBOM_OUTPUT}")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid swallowed errors in command substitutions.
Both of these lines use command substitutions that mask failures because errexit is not inherited by default. Based on learnings, restructure to avoid the substitution so errors are caught.
scripts/auto-rebase/crypto_scan.sh#L38-L38: Replace withecho "CBOM generated:"followed byls -lh "${CBOM_OUTPUT}"on a new line.scripts/auto-rebase/crypto_scan.sh#L79-L79: Replace withecho "SPDX generated:"followed byls -lh "${SPDX_OUTPUT}"on a new line.
📍 Affects 1 file
scripts/auto-rebase/crypto_scan.sh#L38-L38(this comment)scripts/auto-rebase/crypto_scan.sh#L79-L79
🤖 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 `@scripts/auto-rebase/crypto_scan.sh` at line 38, In
scripts/auto-rebase/crypto_scan.sh, replace the CBOM status command substitution
at lines 38-38 with separate echo and ls commands, then apply the same change to
the SPDX status output at lines 79-79: echo the generated-file label first and
run ls -lh on the corresponding output variable separately so failures
propagate.
Source: Learnings
fracappa
force-pushed
the
fca/add-ushift-cbom
branch
from
d51d57d to
08c01c1
Compare
added auto-rebase crypto_scan.sh script for cbom generation. This script uses crypto-scanner-iamge to generate a CycloneDX document. It also provides a SPDX one, by implementing the mapping suggested by the prod sec team.
fracappa
force-pushed
the
fca/add-ushift-cbom
branch
from
711e3fe to
494416a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/auto-rebase/crypto_scan.sh (1)
42-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid swallowed errors in command substitutions.
Both lines use command substitutions that mask failures because
errexitis not inherited by default. Based on learnings, restructure to avoid the substitution so errors are caught.
scripts/auto-rebase/crypto_scan.sh#L42-L42: Replace withecho "CBOM generated:"followed byls -lh "${CBOM_OUTPUT}"on a new line.scripts/auto-rebase/crypto_scan.sh#L83-L83: Replace withecho "SPDX generated:"followed byls -lh "${SPDX_OUTPUT}"on a new line.🤖 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 `@scripts/auto-rebase/crypto_scan.sh` at line 42, In scripts/auto-rebase/crypto_scan.sh at lines 42-42 and 83-83, remove the ls command substitutions from the CBOM and SPDX status messages. In the CBOM generation flow, print “CBOM generated:” first, then run ls -lh against CBOM_OUTPUT on the next line; similarly, print “SPDX generated:” first, then run ls -lh against SPDX_OUTPUT on the next line so failures propagate.Source: Learnings
🤖 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 `@scripts/auto-rebase/crypto_scan.sh`:
- Line 12: Update the EXIT trap command to use single quotes around the trap
body and double-quote IMAGE_DIR, SCANNER_DIR, and SCAN_DIR within it, preventing
early expansion and safely handling paths containing spaces.
---
Duplicate comments:
In `@scripts/auto-rebase/crypto_scan.sh`:
- Line 42: In scripts/auto-rebase/crypto_scan.sh at lines 42-42 and 83-83,
remove the ls command substitutions from the CBOM and SPDX status messages. In
the CBOM generation flow, print “CBOM generated:” first, then run ls -lh against
CBOM_OUTPUT on the next line; similarly, print “SPDX generated:” first, then run
ls -lh against SPDX_OUTPUT on the next line so failures propagate.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5676ec02-b50f-4b9c-b3e5-1c493893d6e2
📒 Files selected for processing (1)
scripts/auto-rebase/crypto_scan.sh
| IMAGE_DIR=$(mktemp -d) | ||
| SCANNER_DIR=$(mktemp -d) | ||
| SCAN_DIR=$(mktemp -d) | ||
| trap "rm -rf ${IMAGE_DIR} ${SCANNER_DIR} ${SCAN_DIR}" EXIT |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote variables in trap command.
As per coding guidelines and Shellcheck (SC2064), use single quotes for the trap command and double-quote the variables inside to prevent early expansion and properly handle paths with spaces.
🐛 Proposed fix
-trap "rm -rf ${IMAGE_DIR} ${SCANNER_DIR} ${SCAN_DIR}" EXIT
+trap 'rm -rf "${IMAGE_DIR}" "${SCANNER_DIR}" "${SCAN_DIR}"' EXIT🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 12-12: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 12-12: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 12-12: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🤖 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 `@scripts/auto-rebase/crypto_scan.sh` at line 12, Update the EXIT trap command
to use single quotes around the trap body and double-quote IMAGE_DIR,
SCANNER_DIR, and SCAN_DIR within it, preventing early expansion and safely
handling paths containing spaces.
Sources: Coding guidelines, Linters/SAST tools
fracappa
force-pushed
the
fca/add-ushift-cbom
branch
from
d4b1e1d to
494416a
Compare
|
@fracappa: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
added auto-rebase crypto_scan.sh script for cbom generation. This script uses crypto-scanner-iamge to generate a CycloneDX document. It also provides a SPDX one, by implementing the mapping suggested by the prod sec team.
Summary by CodeRabbit