Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ask catalog for package, rather than type asserting #1857

Merged
merged 4 commits into from May 10, 2024

Conversation

willmurphyscode
Copy link
Contributor

@willmurphyscode willmurphyscode commented May 10, 2024

TODO

  • tests
  • research fixing in Syft instead - nothing to be done; grype wasn't using Syft's API very well.

Explanation

Basically, when we're cataloging we remove some packages by ownership overlap, (e.g. if you're on redhat and do yum install python3-urllib, grype will drop the PyPI package urllib in favor of the RPM python3-urllib, because we have better match information for the distro package than for PyPI, e.g. because RedHad backported a fix to an old version of the python code or something).

We do this:

   		from, ok := r.From.(pkg.Package)
		if ok && excludePackage(comprehensiveDistroFeed, p, from) {
			continue
		}

Which seems fine, however, some relationships have a From of type *pkg.Package, not type pkg.Package (pointer vs struct). This causes the type assertion in grype to always fail, resulting FPs.

The underlying reason is that the syft scanner does this:

		parent := catalog.Package(parentID)
		child := catalog.Package(childID)

when building the relationship. And catalog.Package(id) returns a *Package.

However, when we're decoding an SBOM, the relationship builder looks over catalog.Sorted(), which is a []Package.

We can compensate for this in grype by trying to type assert to both pkg.Package and *pkg.Package. I think that's the easier way, and I feel like the "right" way (making Syft either always put a pointer in the To/From field of a relationship or never do so) might end up breaking the type signature of something exported, which we wanted to avoid in 1.0. Grype PR soon, but definitely open to feedback on the approach.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode willmurphyscode changed the title fix: test for ptr relationship member fix: ask catalog for package, rather than type asserting May 10, 2024
@willmurphyscode willmurphyscode marked this pull request as ready for review May 10, 2024 14:46
Signed-off-by: Will Murphy <will.murphy@anchore.com>
grype/pkg/package_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as is, but possible test improvement suggestion

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@willmurphyscode willmurphyscode enabled auto-merge (squash) May 10, 2024 15:12
@willmurphyscode willmurphyscode merged commit 5ac483a into main May 10, 2024
10 checks passed
@willmurphyscode willmurphyscode deleted the fix-remove-by-overlap branch May 10, 2024 15:20
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.

None yet

2 participants