Add OpenVEX report generation via govulncheck#9767
Conversation
📝 WalkthroughWalkthroughAdds OpenVEX generation and publishing: a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as Makefile
participant Script as hack/govulncheck.sh
participant GV as govulncheck
participant CI as GitHub Actions
participant Store as Cloud Storage
Dev->>Make: make vex
Make->>Script: run with VEX_ONLY=true
Script->>GV: govulncheck -format openvex
GV-->>Script: build/cri-o.openvex.json
Script->>Make: emit path (build/cri-o.openvex.json)
Note over CI,Store: On main/release/tag
CI->>CI: run security-checks (upload vex artifact)
CI->>Store: vex-upload job uploads artifact to cri-o/artifacts
Store-->>CI: stored artifact
CI->>Script: release-notes include OpenVEX link & cosign verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert 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 |
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 (1)
hack/govulncheck.sh (1)
33-35:⚠️ Potential issue | 🔴 CriticalThe jq filter is incompatible with
govulncheck -jsonv1.1.4 output format and will silently fail to detect vulnerabilities.The govulncheck v1.1.4 JSON output is a stream of
Messageobjects where each message contains exactly one top-level field:config,progress,SBOM,osv, orfinding. The current filter assumes a.vulnerability.modules[]?structure that does not exist in this version, causingmodvulnsandlibvulnsto always be empty. This silently defeats the vulnerability detection, allowing the script to exit 0 even when vulnerabilities are present.The filter must be rewritten to iterate over streaming JSON messages and extract vulnerability data from the
findingmessages (which containosv,fixed_version, and vulnerability trace information), not from a non-existent.vulnerabilityobject.
🧹 Nitpick comments (2)
Makefile (1)
390-392:make vexruns the full vulnerability-check pipeline, not just VEX generation.The
vextarget invokes the samehack/govulncheck.shscript asverify-govulncheck. This meansmake vexwill also install system packages viaapt-get, run a secondgovulncheck -jsonpass, andexit 1if module vulnerabilities are found. A user runningmake vexto generate a local report (as documented inSECURITY.md) may not expect this behavior.Consider splitting the script or gating the verify portion behind an environment variable so
make vexonly producesbuild/cri-o.openvex.json.scripts/release-notes/release_notes.go (1)
199-228: TheFprintfwith 50+ positional%sargs is fragile and hard to audit.Every time a new artifact is added (as in this PR), someone must carefully count positions to insert new
bundleVersionargs at the right spot. A mismatch silently corrupts the rendered release notes.Consider switching to
text/templateorstrings.NewReplacerwith named placeholders (e.g.,{{.BundleVersion}}) to make the template self-documenting and less error-prone.
1a29f6e to
1b707d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 324-340: The vex-upload job currently requires security-checks to
succeed and will be skipped on failure; update the job-level if expression for
the job named vex-upload so it still runs for the target refs even when
security-checks failed but not when it was cancelled — e.g. replace the existing
if with a combined condition that preserves the ref checks and adds
needs.security-checks.result != 'cancelled' (refer to the job name vex-upload
and the needs: security-checks reference).
In `@hack/govulncheck.sh`:
- Around line 25-36: Update the second govulncheck invocation to use the modern
flag (replace the legacy "-json" with "-format json" on the govulncheck call
assigned to report) so both invocations use consistent flags, and then
consolidate the duplicate scans by running govulncheck a single time and
producing both outputs from that single run (e.g., run govulncheck once into a
temp file and write both build/cri-o.openvex.json and the JSON report from that
single result), changing the two separate "$GOPATH_BIN"/govulncheck calls to a
single invocation and using post-processing/tee to emit both formats.
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
315-320: Consider usingif: !cancelled()instead ofif: always().
if: always()will also execute when the workflow is cancelled. A more precise condition isif: !cancelled(), which runs on both success and failure but respects cancellation.Proposed change
- uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0 - if: always() + if: '!cancelled()' with: name: vex path: build/cri-o.openvex.json if-no-files-found: ignore
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9767 +/- ##
==========================================
+ Coverage 67.32% 67.46% +0.13%
==========================================
Files 210 210
Lines 29112 29112
==========================================
+ Hits 19600 19639 +39
+ Misses 7820 7778 -42
- Partials 1692 1695 +3 🚀 New features to boost your workflow:
|
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
1b707d5 to
c355bea
Compare
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/release-notes/release_notes.go (1)
199-228: The positional%sapproach with 55 arguments is extremely fragile — consider switching to named template parameters.The new lines add 4 more
bundleVersionarguments (lines 226-227) to an already massiveSprintfcall. A single miscounted%ssilently shifts every subsequent value, and this is practically impossible to audit by eye. I verified the counts appear correct for this change, but this is one-off luck at this scale.Consider refactoring to
strings.NewReplacerortext/templatewith a data struct, which would make the template self-documenting and immune to positional drift. This is pre-existing tech debt, so not blocking.
|
@cri-o/cri-o-maintainers PTAL |
| with: | ||
| name: vex | ||
| path: build/cri-o.openvex.json | ||
| if-no-files-found: ignore |
There was a problem hiding this comment.
in which case we fail to generate the file?
There was a problem hiding this comment.
It's just a safeguard if govulncheck tool fails to run.
There was a problem hiding this comment.
Shouldn't we fail the pipeline then?
There was a problem hiding this comment.
We already do. If govulncheck fails to run, the make verify-govulncheck step fails the job. The if-no-files-found: ignore only prevents a redundant error from the upload step.
Case 1: govulncheck runs, no vulns found
security-checks job vex-upload job
(all branches) (main/release/tags only)
+---------------------+ +----------------+ +------------------+ +-----------+
| make govulncheck | | upload-artifact| | download-artifact| | upload |
| | | | | | | to GCS |
| VEX file: created |--->| VEX file: |-->| VEX file: |-->| |
| Vuln check: pass | | uploaded | | downloaded | | PASS |
+---------------------+ +----------------+ +------------------+ +-----------+
Result: PASS Result: PASS
Case 2: govulncheck runs, vulns found
security-checks job vex-upload job
(all branches) (main/release/tags only, if: !cancelled())
+---------------------+ +----------------+ +------------------+ +-----------+
| make govulncheck | | upload-artifact| | download-artifact| | upload |
| | | (if: always) | | | | to GCS |
| VEX file: created |--->| VEX file: |-->| VEX file: |-->| |
| Vuln check: FAIL | | uploaded | | downloaded | | PASS |
+---------------------+ +----------------+ +------------------+ +-----------+
Result: FAIL Result: PASS (VEX still uploaded)
Case 3: govulncheck fails to install/run
security-checks job vex-upload job
(all branches) (main/release/tags only, if: !cancelled())
+---------------------+ +----------------+ +------------------+
| make govulncheck | | upload-artifact| | download-artifact|
| | | (if: always) | | |
| VEX file: none |--->| VEX file: |-->| no artifact |
| Script: FAIL | | ignored | | FAIL |
+---------------------+ +----------------+ +------------------+
Result: FAIL ^ Result: FAIL
|
if-no-files-found: ignore
prevents a redundant error here
In all failure cases the pipeline already fails. The ignore only matters in case 3 to avoid a noisy second error on the upload-artifact step. The vex-upload job uses !cancelled() so it still runs when vulns are found (case 2), ensuring the VEX report is uploaded to GCS even on failure.
|
/lgtm |
|
/override ci/prow/e2e-gcp-ovn Unrelated. |
|
@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/e2e-aws-ovn, ci/prow/e2e-gcp-ovn 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 kubernetes-sigs/prow repository. |
|
@saschagrunert: 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9767 +/- ##
==========================================
+ Coverage 67.32% 67.46% +0.13%
==========================================
Files 210 210
Lines 29112 29112
==========================================
+ Hits 19600 19639 +39
+ Misses 7820 7778 -42
- Partials 1692 1695 +3 🚀 New features to boost your workflow:
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds project-level VEX (Vulnerability Exploitability eXchange) support:
hack/govulncheck.shto generate an OpenVEX documentmake vextargetSECURITY.mdWhich issue(s) this PR fixes:
Fixes #9104
Special notes for your reviewer:
Requires a companion PR in cri-o/packaging to generate, sign, and
upload the VEX file during the release process:
cri-o/packaging#339
Does this PR introduce a user-facing change?
Summary by CodeRabbit
New Features
Documentation
Chores