refactor: replace hashicorp/go-version with Masterminds/semver in CVE matcher - #22013
refactor: replace hashicorp/go-version with Masterminds/semver in CVE matcher#22013davdhacs wants to merge 2 commits into
Conversation
… matcher Replace hashicorp/go-version with Masterminds/semver (already in go.mod) for version parsing and constraint checking in the CVE matcher. Key changes: - Use semver.NewVersion / semver.NewConstraint for all version operations - Rewrite getBaseVersion to use Major()/Minor()/Patch() instead of fragile strings.ReplaceAll on the prerelease suffix; return *semver.Version (no error -- three non-negative integers always produce valid semver) - Inline matchBaseVersion/matchExactVersion into MatchVersions -- they re-parsed already-parsed versions and added naming without semantic weight - Remove getConstraints helper; use semver.NewConstraint(strings.Join(...)) directly, which fixes a latent false-positive bug: the old code AND'd individually-parsed constraints in a loop, so if all parses failed the empty slice defaulted to true - Fix regex bug: [v|V] -> [vV] (inside a character class, | is literal) - Fix typo: "funcitonality" -> "functionality" - Update TestMatchVersions to test through MatchVersions (the removed helpers were internal; integration tests already cover the same paths) Generated with AI assistance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hashicorp/go-version is no longer imported directly by any StackRox code after the CVE matcher migration to Masterminds/semver. It remains as an indirect dependency via stackrox/istio-cves, stackrox/k8s-cves, and stackrox/k8s-overlay-patch. Generated with AI assistance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22013 +/- ##
==========================================
- Coverage 51.23% 51.19% -0.04%
==========================================
Files 2864 2864
Lines 178713 178681 -32
==========================================
- Hits 91560 91480 -80
- Misses 79117 79155 +38
- Partials 8036 8046 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 Build Images ReadyImages are ready for commit 567363f. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-622-g567363f558 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe CVE matcher now uses Masterminds semver for exact, base-version, and range comparisons. GKE/EKS version regexes are corrected, tests invoke the public matching API, and go-version is classified as an indirect dependency. ChangesCVE version matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
central/cve/matcher/matcher_test.go (1)
82-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the range/"block of versions" branch.
All 8 cases exercise only the single-version path (
cpeUpdate=*or a specific prerelease string). None setVersionStartIncluding/VersionEndIncluding/VersionEndExcluding, so the range-constraint logic (matcher.go lines 273-304) — including the PR's headline fix for false positives when all constraints fail to parse — has zero coverage in this suite.Consider adding cases with
VersionStartIncluding/VersionEndExcludingset, plus a case where the range bounds are unparseable, to lock in the described false-positive fix.🤖 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 `@central/cve/matcher/matcher_test.go` around lines 82 - 130, The MatchVersions test table currently covers only single-version matching and not the range-constraint branch. Extend the cases in the visible MatchVersions test to populate VersionStartIncluding/VersionEndIncluding/VersionEndExcluding for valid ranges, and add a case with unparseable range bounds that expects no match, covering the false-positive fix when all constraints fail to parse.
🤖 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 `@central/cve/matcher/matcher.go`:
- Line 247: Update the CPE version-range handling around the existing AllEmpty
guard and constraint parts construction to include VersionStartExcluding
alongside the other three range fields. Ensure entries with only
VersionStartExcluding use the range branch, and preserve that exclusive
lower-bound value when building parts for the generated constraint.
---
Nitpick comments:
In `@central/cve/matcher/matcher_test.go`:
- Around line 82-130: The MatchVersions test table currently covers only
single-version matching and not the range-constraint branch. Extend the cases in
the visible MatchVersions test to populate
VersionStartIncluding/VersionEndIncluding/VersionEndExcluding for valid ranges,
and add a case with unparseable range bounds that expects no match, covering the
false-positive fix when all constraints fail to parse.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 78163a30-805a-40c4-a560-586edc85d4a1
📒 Files selected for processing (3)
central/cve/matcher/matcher.gocentral/cve/matcher/matcher_test.gogo.mod
| // Note that cpeVersionAndUpdate can't be "*:*" in this case, since there is no info about start and end versions | ||
| // This is the case where there is just one version so check against it. | ||
| // Note that cpeVersionAndUpdate can't be "*:*" in this case, since there is no info about start and end versions. | ||
| if stringutils.AllEmpty(cpeMatch.VersionStartIncluding, cpeMatch.VersionEndIncluding, cpeMatch.VersionEndExcluding) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
VersionStartExcluding is never checked, causing mis-classification and missing lower bound.
The NVD NVDCVEFeedJSON10DefCPEMatch schema has four range fields: VersionStartIncluding, VersionStartExcluding, VersionEndIncluding, VersionEndExcluding. This code only inspects three of them:
- Line 247's
stringutils.AllEmpty(...)guard omitsVersionStartExcluding, so a CPE entry that sets onlyVersionStartExcluding(a legitimate, documented NVD pattern) is incorrectly routed into the "single version" branch instead of the range branch. - Lines 279-289 build constraint
partsfrom onlyVersionStartIncluding/VersionEndIncluding/VersionEndExcluding, so even when the range branch is correctly reached (because another field is also set), an exclusive lower bound is silently dropped from the constraint, widening the match beyond what the CVE data specifies.
This can cause both false negatives (single-version branch applied to what's actually a range) and false positives (missing lower-bound exclusion), which directly affects CVE-to-cluster/image matching correctness.
🐛 Proposed fix
- if stringutils.AllEmpty(cpeMatch.VersionStartIncluding, cpeMatch.VersionEndIncluding, cpeMatch.VersionEndExcluding) {
+ if stringutils.AllEmpty(cpeMatch.VersionStartIncluding, cpeMatch.VersionStartExcluding, cpeMatch.VersionEndIncluding, cpeMatch.VersionEndExcluding) { var parts []string
if cpeMatch.VersionStartIncluding != "" {
parts = append(parts, fmt.Sprintf(">= %s", cpeMatch.VersionStartIncluding))
}
+
+ if cpeMatch.VersionStartExcluding != "" {
+ parts = append(parts, fmt.Sprintf("> %s", cpeMatch.VersionStartExcluding))
+ }Also applies to: 273-289
🤖 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 `@central/cve/matcher/matcher.go` at line 247, Update the CPE version-range
handling around the existing AllEmpty guard and constraint parts construction to
include VersionStartExcluding alongside the other three range fields. Ensure
entries with only VersionStartExcluding use the range branch, and preserve that
exclusive lower-bound value when building parts for the generated constraint.
1 participant
Description
Replace
hashicorp/go-versionwithMasterminds/semver(v1, already in go.mod) in the CVE matcher. This consolidates two version-parsing libraries down to one.Beyond the dep swap, this change fixes several pre-existing issues found during a three-pass code review cycle:
Bug fixes:
getBaseVersionused fragilestrings.ReplaceAll(v.String(), "-"+prerelease, "")which could corrupt versions where the prerelease string appears in build metadata (e.g.,1.2.3-rc1+meta-rc1). Rewritten to useMajor()/Minor()/Patch()— structurally correct, impossible to corrupt.true(vacuous truth), incorrectly reporting a CVE match. The new code correctly returns no-match. This is security-critical code — a false positive means flagging a cluster as vulnerable when it isn't.[v|V]→[vV]— the|inside a character class matched a literal pipe character.Simplifications:
matchBaseVersionandmatchExactVersionintoMatchVersions— they were thin wrappers that re-parsed an already-parsed version string.getConstraintshelper — replaced with directsemver.NewConstraint(strings.Join(parts, ", ")).getBaseVersionreturn type from(*Version, error)to*Version— the error was impossible (fmt.Sprintf("%d.%d.%d")with non-negative integers always produces valid semver).Other:
"funcitonality"→"functionality"TestMatchVersionsto test through the publicMatchVersionsAPI instead of deleted internal helpers.hashicorp/go-versiondemoted from direct to indirect (4 transitive deps still pull it in). Reviewed through a three-pass review cycle with convergence at 99/100.User-facing documentation
No user-facing behavior changes. The false-positive bug fix is a correctness improvement but was latent (unreachable with valid NVD data due to an upstream guard).
Testing and quality
Automated testing
How I validated my change
go build ./central/cve/matcher/...passesgo test ./central/cve/matcher/...— all 10 test cases passMasterminds/semverproduces identical results for all version string patterns in the test data (standard semver, v-prefixed, prerelease, build metadata, GKE-style, 2-segment)go mod tidyconfirmshashicorp/go-versiondemoted to indirect