Skip to content

migrate to Masterminds/semver/v3 for semver processing - #317

Open
tariq1890 wants to merge 1 commit into
containerd:mainfrom
tariq1890:masterminds-semver
Open

tariq1890 wants to merge 1 commit into
containerd:mainfrom
tariq1890:masterminds-semver

Conversation

@tariq1890

@tariq1890 tariq1890 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Motivation

golang.org/x/mod is a pretty heavy dependency in comparison to Masterminds/semver/v3 as 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/mod is known to have CVEs whereas the same cannot be said of the Masterminds/semver/v3 module

@tariq1890

Copy link
Copy Markdown
Contributor Author

Some of the recent vulnerabilities related to golang.org/x/mod can be seen here

Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
@thaJeztah

Copy link
Copy Markdown
Member

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 sort.Sort, and instead use the more modern slices.Sort / slices.SortFunc

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
}

@samuelkarp

Copy link
Copy Markdown
Member

Some of the recent vulnerabilities related to golang.org/x/mod can be seen here

But these are not in code that is called through NRI. Did you get positives when running govulncheck?

@tariq1890

Copy link
Copy Markdown
Contributor Author

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.

@samuelkarp

samuelkarp commented Sep 16, 2026

Copy link
Copy Markdown
Member

golang.org/x/mod is a pretty heavy dependency in comparison to Masterminds/semver/v3

Diffstat on this PR is +76 -54, so it's adding more code than it removes.

Edit: ignore, this repo doesn't vendor so the actual diff isn't captured in the diffstat

@thaJeztah

Copy link
Copy Markdown
Member

But these are not in code that is called through NRI. Did you get positives when running govulncheck?

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 go.mod are kept at the minimum required version where possible, leaving it up to consumers of the module to pick any SemVer-compatible newer version of those dependencies if they need it (or if their code is impacted by vulnerabilities in those dependencies).

That said, the "reduce dependencies" is something that's relevant. Overall golang.org/x/* are more commonly seen as an extension of stdlib, and most consumers would already keep those dependencies up-to-date, but golang.org/x/mod is not my favorite one because of its dependency on golang.org/x/tools which is really a grab-bag of utilities that's notorious for bringing in much more indirects than desirable;

https://github.com/golang/mod/blob/d0a27b2d4a48460806692bf5c87fc157c3c65292/go.mod#L5
https://github.com/golang/tools/blob/b8a78cfad0304b579cbe066aa2c8fae831162502/go.mod#L5-L14

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 // indirect

From 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).

@samuelkarp

Copy link
Copy Markdown
Member

Overall golang.org/x/* are more commonly seen as an extension of stdlib, and most consumers would already keep those dependencies up-to-date

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 golang.org/x/tools though.

From that perspective, it'd be interesting to see how viable a local implementation would be

The purpose of this is negotiating versions with the runtimes, and the runtimes use more than just major.minor.patch versions (for example containerd uses -prerelease sections too). Although I think we're stripping -prerelease and +buildtag anyway here so maybe if we don't care about those values at all a small local implementation might be sufficient.

@tariq1890

Copy link
Copy Markdown
Contributor Author

But these are not in code that is called through NRI. Did you get positives when running govulncheck?

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 govulncheck accumulates over time, adds to the churn and tanks productivity. This PR was just created to optimise for all of this :). (just providing my rationale as to how I ended up filing this PR)

@tariq1890

Copy link
Copy Markdown
Contributor Author

@thaJeztah @samuelkarp Just following up. How would you like me to proceed?

@thaJeztah

Copy link
Copy Markdown
Member

On its own, it's not that big a deal for sure, but we've also noticed that this work fixing VEX's, running govulncheck accumulates over time, adds to the churn and tanks productivity.

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 govulncheck does - and even govulncheck sometimes fails).

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 // indirect, but Go module's "MVS" is fully centered around SemVer, so you can update to a newer version (and have the CVE (false positive) addressed). Admitted; none of the golang.org/x modules have a stable release, but generally the Go maintainers still treat them as stable releases (no breaking changes).

How would you like me to proceed?

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 golang.org/x/mod isn't the "greatest" dependency to have; it's already a dependency of most projects in the container ecosystem, so may still be preferable over a new dependency. Masterminds/semver is not an uncommon dependency, but it's slightly less common than golang.org/x dependencies.

But if we can remove it entirely here (for a minimal local implementation that satisfies the requirements), that would be a win.

@tariq1890

tariq1890 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

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 // indirect, but Go module's "MVS" is fully centered around SemVer, so you can update to a newer version (and have the CVE (false positive) addressed). Admitted; none of the golang.org/x modules have a stable release, but generally the Go maintainers still treat them as stable releases (no breaking changes).

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.

But if we can remove it entirely here (for a minimal local implementation that satisfies the requirements), that would be a win.

Understood, I'll take a stab at this.

@thaJeztah

Copy link
Copy Markdown
Member

Sadly, the presence of a vulnerable go module as an indirect dependency is enough to get the vuln flagged.

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 go.mod (including indirect dependencies listed in it), but Go modules only use a single version of a dependency in a code-base (for the same "major" version that is); any scanner that would deduct otherwise would be broken.

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).

I'll take a stab at this.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants