fix: exclude owned Go modules from OS packages - #3602
Conversation
Signed-off-by: Eljees <3.14hell@gmail.com>
c33f062 to
2313f9d
Compare
spiffcs
left a comment
There was a problem hiding this comment.
@Eljees thanks for the PR. Can you provide an example of an incorrect grype vulnerability report that existed before this PR and a new report that highlights the fix?
The previous version-prefix check ran before the comprehensive-distro ownership rule,
so a legitimate owned module was retained when
--> its module version differed from the distro package version.
This can leave duplicate package representations available for vulnerability matching
even though the OS package is the authoritative representation.
Let me know if I'm reading the above spec/description correctly.
If we find there a module version that is different from the distro package version then that suggests there were two modules discovered. We would have a package for both the distro owned package (distro package version) AND the other package that had a version mis match.
If you have an example of an SBOM or vulnerability report that this fixes I'd like to look at those cases to check if we SHOULD be eliminating a package here, or if it's correct to have both.
|
Thanks @spiffcs — you were right to push on this. I compared before/after on the pinned quality SBOMs with the same imported DB (candidate build vs v0.116.0), and the evidence does not support the change in its current form. Concrete counter-example from the quality corpus ( I'll narrow this to the case the issue is actually about: only exclude a go-module child when it is the parent package's own main module (e.g. the containerd rpm ↔ |
Narrow the previous blanket exclusion: an embedded Go module is only removed when it appears to be the owning distro package's main module (module path basename matches the package name, e.g. the containerd package owning github.com/containerd/containerd), or when it is the Go standard library owned by the distro's Go toolchain package. Dependency modules embedded in a third-party package's binaries stay matchable, since distro feeds do not track vulnerabilities for those. Validated against the quality-gate SBOM corpus with a shared DB: - gitlab-ce (Ubuntu; omnibus deb owning 1,836 go modules): zero match changes, where the previous revision dropped 877 matches (869 with no distro-level replacement, including CVE-2023-48795 for golang.org/x/crypto) - azurelinux3 test image: only stdlib-owned-by-golang duplicates are removed (11 of them have alias-equivalent CVE matches on the golang rpm; the rest are absent from the Azure Linux feed, which is authoritative for a comprehensive distro) Signed-off-by: Eljees <3.14hell@gmail.com>
|
Reworked in 056da3d as described above. The exclusion now only applies when the embedded Go module appears to be the owning package's own main module — the module path basename matches the package name (containerd rpm ↔ Validation against the pinned quality SBOMs with a shared DB (same binary pair, before = this branch's base):
Marking ready for review again — happy to adjust scope further if you'd prefer the stdlib case split out. |
|
@spiffcs you're reading it right, and the answer is that the original patch was wrong — it produces false negatives, not duplicate cleanup. I built this branch and its base with the same imported DB and ran both against the pinned quality-gate SBOM corpus. On the So the assumption the patch leaned on doesn't hold: 056da3d narrows it to the case the issue is actually about: an embedded Go module is only excluded when it looks like the owning package's own main module — module path basename matches the package name, e.g. the containerd rpm and Re-run on the same binary pair:
Unit tests updated — the containerd case still excludes, and a new case asserts a gitlab-ce-style dependency module is kept. Happy to split the stdlib half out if you'd rather scope it tighter. |
|
@spiffcs — I said on #3584 that I'd stop pinging, so this isn't one. It's an offer to close. Where this stands: you asked for a concrete before/after, and the honest answer was that my original patch was wrong. Measured against the pinned quality-gate SBOM corpus with a shared imported DB, it removed 1,271 go-module matches corpus-wide and added none — 1,195 with no distro-level replacement, including CVE-2023-48795 on 056da3d narrows it to the case the issue is actually about: exclude an embedded Go module only when it looks like the owning package's own main module, plus stdlib owned by the distro's Go toolchain package. On the same corpus that's zero change on gitlab-ce and only the azurelinux3 stdlib duplicates. If that scope still isn't what you want, say so and I'll close it — no explanation needed, and I won't re-open the topic. If it is what you want, it's green and mergeable and I'll rebase whenever suits you. |
noqt
left a comment
There was a problem hiding this comment.
Reviewing from NOQT at exact head 056da3df17b22cd5fd23b7b09d8a6acbc76d348d.
The rework correctly preserves embedded dependency modules, but the basename heuristic still recreates the same coverage error for third-party OS packages whose own main module matches the package name. comprehensiveDistroOwner knows only the image distro and package type; it has no repository or source provenance for the deb/rpm.
I added a reviewer-only regression with an Ubuntu image, parent deb:vault@1.16.2-1, and child go-module:github.com/hashicorp/vault@v1.16.2. Current main keeps both packages. This head drops the Go module because vault == path.Base(module), even though an Ubuntu feed cannot be assumed to cover an arbitrary third-party vault deb. That can hide the upstream Go findings the child exists to match.
Please retain the main-module child when versions differ unless there is evidence that the owning package is actually covered by the selected distro feed, and add this third-party-package boundary as a regression. If the toolchain/stdlib case has a stronger provenance signal, that can be split or narrowed independently.
The PR's focused overlap suite, affected-package vet, diff hygiene, and clean merge onto current main f6f2ce0a2293e65c97bf5468926cb0c9ddad822f all pass. The reviewer regression passes on current main and fails on this head; that false-negative boundary is the blocker.
|
@noqt I reproduced your scenario as a test case on this exact head, and you are right about the mechanics: with Before concluding that is a defect introduced here, I ran the same scenario with a The binary child is absorbed too, by the pre-existing overlap rule (version-prefix similarity plus the comprehensive-distro check). The boundary you describe — an OS package the distro feed does not actually cover absorbing its child — is a property of the existing mechanism, because the SBOM carries no repository provenance for a deb/rpm; On the specific suggestion to retain the main-module child when versions differ: that gate would also retain Where the line could actually be drawn correctly is provenance: if syft ever records the package's origin repository, gating the exclusion (both the existing binary rule and this one) on "the owning package really comes from the distro" would fix your scenario and keep the duplicate suppression. That is out of scope for grype's data today. If the maintainers prefer to be conservative here, the change can be narrowed to the |
|
You're right about the current baseline. I reproduced the binary case on clean main That changes my framing, not the merge recommendation. The provenance gap is pre-existing; this PR expands that accepted false-negative trade-off to a new package class whose differing versions previously kept it matchable. Existing binary behavior isn't evidence that the expansion is safe. Your conservative option is the right repair from NOQT's perspective: narrow this PR to the So the changes-requested verdict remains, but with the repair path narrowed to the stdlib slice you proposed. Thanks for testing the counterexample instead of hand-waving it away. |
|
Agreed on the binary case being pre-existing behaviour. Since we now agree the stdlib-owned-by-toolchain slice is the provenance-safe subset, I can push a revision narrowed to just that case whenever @spiffcs wants it — the corpus impact of that slice is the azurelinux3 stdlib/golang pairs measured earlier, and it sidesteps the third-party-package boundary entirely. Holding until the maintainers weigh in, since the main-module-basename heuristic is the part still genuinely in dispute and that call is theirs to make. |
Summary
Why
The overlap relationship already establishes that the distro package owns the embedded Go module. The previous version-prefix check ran before the comprehensive-distro ownership rule, so a legitimate owned module was retained when its module version differed from the distro package version.
This can leave duplicate package representations available for vulnerability matching even though the OS package is the authoritative representation.
Validation
git diff --checkgo test ./grype/pkgwas attempted withgolang:1.26.3; dependency downloads failed with upstream TLS handshake timeouts before the test package could run