Conversation
0738ae0 to
82134a5
Compare
|
Some of the recent vulnerabilities related to |
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
82134a5 to
bbb40c0
Compare
|
I was looking at this as well recently, and even wondered "how much to-the-letter SemVer is required here?" If we only need major.minor.patch sorting, we could even do a much more basic handling; split into 3 digits, and sort on those (cutting any pre-release, which we could either ignore, or very basic compare). That may also avoid having to use I was lazy, so asked my LLM to write-up a quick example (just to illustrate): type version struct {
major int
minor int
patch int
}
func compareVersion(a, b version) int {
if c := cmp.Compare(a.major, b.major); c != 0 {
return c
}
if c := cmp.Compare(a.minor, b.minor); c != 0 {
return c
}
return cmp.Compare(a.patch, b.patch)
}
versions := []version{
{1, 10, 0},
{1, 2, 1},
{2, 0, 0},
{1, 2, 0},
}
slices.SortFunc(versions, compareVersion)And for parsing, something like; func parseVersion(s string) (version, error) {
major, rest, ok := strings.Cut(s, ".")
if !ok {
return version{}, errors.New("invalid version")
}
minor, patch, ok := strings.Cut(rest, ".")
if !ok {
return version{}, errors.New("invalid version")
}
// Ignore pre-release/build metadata for this basic comparison.
patch, _, _ = strings.Cut(patch, "-")
patch, _, _ = strings.Cut(patch, "+")
var v version
var err error
if v.major, err = strconv.Atoi(major); err != nil {
return version{}, err
}
if v.minor, err = strconv.Atoi(minor); err != nil {
return version{}, err
}
if v.patch, err = strconv.Atoi(patch); err != nil {
return version{}, err
}
return v, nil
} |
But these are not in code that is called through NRI. Did you get positives when running |
|
Thanks for the response @thaJeztah ! I am not entirely opposed to this. If you feel strongly about this, I can take a stab at creating a simple server parser. However, I do think it's also justified to import the module here. We are using a fair amount of methods exposed by the module. We also know that Masterminds/semver isn't as broadly scoped as x/mod given that it is for semver processing only. |
Edit: ignore, this repo doesn't vendor so the actual diff isn't captured in the diffstat |
Yes; I think the vulnerabilities perspective is less relevant; most reports would be A) false positives, and B) (more relevant) this module is primarily intended as a library module adhering to MVS, so dependencies in That said, the "reduce dependencies" is something that's relevant. Overall https://github.com/golang/mod/blob/d0a27b2d4a48460806692bf5c87fc157c3c65292/go.mod#L5 require (
github.com/google/go-cmp v0.6.0
github.com/yuin/goldmark v1.4.13
golang.org/x/mod v0.41.0
golang.org/x/net v0.59.0
golang.org/x/sync v0.23.0
golang.org/x/telemetry v0.0.0-20260908163034-4bcc4b2ee518
)
require golang.org/x/sys v0.48.0 // indirectFrom that perspective, it'd be interesting to see how viable a local implementation would be (I haven't tried to do so yet); that could be a separate PR to see what the alternative could look like? (and it could be a quick draft just for exploring the option). |
Yeah, this is my reaction. I'd generally prefer a golang.org/x/* dependency over another one just because I understand its maintenance (and patching) practices. I can see the issue with
The purpose of this is negotiating versions with the runtimes, and the runtimes use more than just |
Yes, they can be ruled out as false-positives. The issue is that it takes time for us to file those VEX records, running govulncheck and inputting the findings. On its own, it's not that big a deal for sure, but we've also noticed that this work fixing VEX's, running |
|
@thaJeztah @samuelkarp Just following up. How would you like me to proceed? |
Yes, I can definitely relate to that; VEX statements are near impossible to maintain, and most scanners aren't smart enough to do a deep analysis (as Purely out of curiosity; in what situations did you end up with it flagging the vulnerability? Because any project consuming this module can update the dependency; if may be shown as an
I'm not a maintainer on NRI itself, so also slightly defer to @samuelkarp, but if you have time to give it a try, it may be worth doing a draft PR to see what changing to a local implementation would look like. While But if we can remove it entirely here (for a minimal local implementation that satisfies the requirements), that would be a win. |
Sadly, the presence of a vulnerable go module as an indirect dependency is enough to get the vuln flagged. Given the wide customer base for our projects and the wide variety of security scanners, there are many image scanners that do not take into account these nuances and just flag these vulnerabilities. To the customer/client, they just want the CVE that they see addressed either through an image patch release (with the dep updated) or a VEX filed. Yes, we could certainly update the dependency and publish a new release, but given the steady pipeline of CVEs that result in a never-ending flow of CVEs, the option of publishing patch releases to play catch up is not viable which is why we've settled with VEX filing for the CVEs which we can easily rule out as false positives. Over time, we realised that even the cost of filing VEX's add up over time. In an ideal world, I wouldn't even be creating a PR like this, but the cost of resolving false-positive/irrelevant CVEs is proving to be way higher than it should.
Understood, I'll take a stab at this. |
That's .. really odd, and IMO that's a very broken scanner. I should slightly nuance that; a scanner would be correct to look at the version of dependencies used in And (as mentioned above) a module is free to update indirect dependencies to newer versions than is resolved (and in case of vulnerabilities; expected to do so), but it's not up to library modules to "drive up" versions. At least it's in direct conflict with the core design of go modules (all centered around MVS); library modules should specify the lowest possible version. Doing otherwise would enforce all consumers to update, which not only causes more code-churn, but is also a much bigger risk; every dependency update risks introducing new bugs (and vulnerabilities).
Thanks, that's appreciated! Let me know if you need help; and don't hesitate to push an early draft even if it's not complete or "clean" yet. |
Motivation
golang.org/x/modis a pretty heavy dependency in comparison toMasterminds/semver/v3as its scope extends beyond semver. The Masterminds module on the other hand is focused on semver processing.With this change, we decrease the CVE surface further as
golang.org/x/modis known to have CVEs whereas the same cannot be said of theMasterminds/semver/v3module