Add filters to package cataloger by jonasagx · Pull Request #1021 · anchore/syft

jonasagx · 2022-05-31T21:16:52Z

This PR adds filters so a package without name or version doesn't go in the list of all discovered packages. Two catalogers were modified:

generic cataloger: used by most specific catalogers, acts as a central-ish way of guaranteeing packages have a name.
python package cataloger: doesn't use the generic cataloger, therefore needed its own package filter.

This PR addresses items 1, 2 and 3 from #780 (comment).

Integration and units tests were added to validate behavior.

Fix: #780

Signed-off-by: Jonas Xavier jonasx@anchore.com

jonasagx · 2022-05-31T23:30:24Z

SPDX doesn't require package version, while CycloneDX does.
We should be compatible with both formats and only require package name.

stevespringett · 2022-06-01T05:52:54Z

As of CDX v1.4, component version is optional, not required.

luhring

Thanks for getting this started! This isn't ready for merging just yet.

At the highest level, we need to figure out the best place to filter out unacceptable packages. As implemented currently, this filtering happens fairly downstream in the process. This is good in that there's a central place where filtering happens, but it's not good in that it means unusable packages are flowing through more parts of the logic. This makes the code a bit trickier to debug and makes it easier to miss problems — such as that we haven't yet accounted for package relationships properly (I commented on this below). So I think we need to figure out a better place to do this filtering.

I see that you've added CLI tests, but it's not yet clear to me why these tests needed to be created at the "CLI" level. I've asked about this in a code comment below.

The PR description says that you've added integration tests, but I don't see new integration tests. Is there more code to check into this branch? (I do see new test fixture files added.)

See the other code comments for the rest of my thoughts — although some may become moot depending on how the implementation ends up pivoting.

Let me know if you have any questions!

luhring · 2022-06-01T00:45:01Z

+type filterPkgFn func(pkg.Package) bool
+
+func filterOutPkg(p pkg.Package, filters ...filterPkgFn) bool {
+	for _, f := range filters {
+		if f(p) {
+			return true
+		}
+	}
+	return false
+}
+
+func hasEmptyName(p pkg.Package) bool {
+	return p.Name == ""
+}


If I'm understanding correctly, we just want to test to see if a package has a name, is that right? If so, it seems like we don't need all these pieces, and we could just use the p.Name == "" directly above.

Correct, this code no longer exists. Generic and Python Package catalogers do the equivalent filtering.

luhring · 2022-06-01T00:46:31Z

+			if filterOutPkg(p, hasEmptyName) {
+				continue
+			}


If we're going to discard this package from processing, we should do this check as early as possible. Putting the check here is problematic for a couple of reasons:

We're doing work above this point (e.g. creating CPEs) that might ultimately be wasted. So this would point to at least moving this check to the top of the for ... packages loop. But more importantly,

This implementation doesn't deal with relationships that may have been created for packages that we'll end up excluding from the catalog, so we may end up with relationships that reference packages that don't exist, which is a big problem. We generate relationships here in this loop, but catalogers also can return relationships. So this filtering probably needs to be done at least as upstream as in the catalogers themselves, so that we never generate relationships for packages we're going to discard.

I addressed items 1 and 2 with the generic cataloger and the python package cataloger. In the generic cataloger I remove all relationships From and To a dropped package.

So this filtering probably needs to be done at least as upstream as in the catalogers themselves, so that we never generate relationships for packages we're going to discard.

Having package and relationships dropped by the generic cataloger still a (centralized) last barrier for invalid packages.

luhring · 2022-06-01T00:47:32Z

@@ -0,0 +1 @@
+Name: broken-2.0.0


Why is this test fixture called "broken..."? It looks like this fixture is intended to lead to a correct cataloging of a package, is that right?

Also, I think it's a bit confusing to reason about this entry in the Syft output, since this name actually includes a version-looking piece of data. I'd suggest dropping the -2.0.0, so it's really clear that this is a case where we did not find any version information.

Thanks for pointing that out. I moved it to the integration tests folder of fixtures, under a new file: no-version.

jonasagx · 2022-06-02T00:23:39Z

Thank you for the review @luhring!

At the highest level, we need to figure out the best place to filter out unacceptable packages. As implemented currently, this filtering happens fairly downstream in the process. This is good in that there's a central place where filtering happens, but it's not good in that it means unusable packages are flowing through more parts of the logic. This makes the code a bit trickier to debug and makes it easier to miss problems — such as that we haven't yet accounted for package relationships properly (I commented on this below). So I think we need to figure out a better place to do this filtering.

I added package filtering validation to the generic cataloger, used by most specific catalogers. I added code removing impacted relationships.

The Python Package Cataloger doesn't use the generic cataloger, I had to change it to filter out packages without name. This cataloger doesn't gather relationships so the change is pretty simple.

The PR description says that you've added integration tests, but I don't see new integration tests. Is there more code to check into this branch? (I do see new test fixture files added.)

I removed the cli tests in favor of unit and integration tests. Before I had added only integration test fixtures, sorry for the confusion.

jonasagx · 2022-06-02T00:35:00Z

 			pkgs = append(pkgs, *p)
 		}
 	}
 	return pkgs, nil, nil


Notice this cataloger doesn't return relationships

jonasagx · 2022-06-02T02:04:40Z

Other than the Python Package Cataloger I've found a couple others that don't depend on generic cataloger:

deb.NewDpkgdbCataloger() requires version and name to make a package 👍🏼
rpmdb.NewRpmdbCataloger() doesn't enforce non-empty name, but it depends on data from rpmDBs, I don't know how reliable they are 🤞🏼
golang.NewGoModuleBinaryCataloger() relies on go mod path, which is always present 👍🏼

luhring

This is getting closer... 👍

I left some specific comments, but I'll also provide some high-level feedback here:

There's still an integration test being leveraged here, but again, we don't need an integration test here — we can just use a unit test. Using a unit test is preferable wherever possible.
We should have a central function that checks is a package is viable (i.e. has a name). A function like this is defined in the python package, but it should be defined instead in a central place, and then used by all the catalogers, so that we're applying the same criteria to packages from all catalogers. I saw your comment about some of the catalogers maybe being okay without this check, but I'd strongly encourage the use of this check for all code paths, so that we can centrally control what the requirements are for a viable package. It's not clear to me that the assumptions about other catalogers like RPM and Go are sufficient here.

luhring · 2022-06-02T15:46:34Z

 			continue
 		}

+		pkgIDsForRemoval := make([]artifact.Identifiable, 0)


nit: Using the var pkgIDsForRemoval []artifact.Identifiable syntax for statements like this is a bit cleaner, and also has the advantage of avoiding a wasted array allocation in the event that no items are ever appended to this slice.

(this comment can apply to other similar lines like this added in this PR — but again, this is just a "nit" comment)

luhring · 2022-06-02T15:48:26Z

 			p.SetID()
+			// doing it here so all packages have an ID,
+			// IDs are used to remove relationships
+			if p.Name == "" {


Since we have a need for this check across multiple catalogers — both the generic cataloger and others — we should extract this check out as a centralized function that can be used by any catalogers that need it

I moved the validation to a func in the pkg package (:P words...), which is then reused by other catalogers.

luhring · 2022-06-02T15:52:04Z

 	return packages, relationships, nil
 }

+func removePkgsFromRelationships(pkgs []artifact.Identifiable, relationships []artifact.Relationship) []artifact.Relationship {


We can make the name removePkgsFromRelationships more clear. This function isn't removing packages, it's removing relationships.

Additionally, the logic here isn't specific to packages, since you've used the type artifact.Identifiable. I think it's a good choice to use this type, since all you need are IDs. 👍

I'd suggest a function name like removeRelationshipsWithArtifactIDs, and I'd suggest renaming the pkgs parameter to something like artifactsToExclude.

Good call, I renamed the function and param with your suggestions.

luhring · 2022-06-02T15:56:13Z

+	for _, r := range relationships {
+		for _, p := range pkgs {
+			if r.To.ID() == p.ID() || r.From.ID() == p.ID() {
+				goto skip
+			}
+		}
+		cleanedRelationships = append(cleanedRelationships, r)
+	skip:
+	}


I think we can simplify and optimize this implementation. Specifically, we can avoid the nested for loops and the goto. And we can also avoid looping through all packages for every single relationship.

My suggestion here is to use a map to store the artifact IDs to exclude, so that we can determine whether a given ID is in the set with an O(1) lookup. You have two options for doing this: 1) you could build the map inside this function, using the given []artifact.Identifiable` value, or 2) you could just ask the function caller for this map in the first place (instead of a slice). Given how this function is called above, I'd lean toward option 2, since it looks like it'd be just as easy to construct this set as a map as it is now to construct it as a slice.

I agree, I implemented option 2.

luhring · 2022-06-02T15:56:26Z

 	}
 }
+
+func Test_removePkgsFromRelationships(t *testing.T) {


Nice testing here! 👍

luhring · 2022-06-02T15:57:11Z

 	return pkgs, nil, nil
 }

+func pkgHasName(p *pkg.Package) bool {


This is the kind of function that should exist centrally for all catalogers to leverage, as opposed to defined just in the python package.

Separately, this function takes a pointer (*pkg.Package) but then doesn't check for a nil pointer condition, which could lay a trap for future developers to enable a nil pointer dereference panic. I'd suggest adjusting this function signature so that it doesn't take a pointer (it would just take a pkg.Package), since the caller is already checking for p != nil and could just as easily pass a dereferenced pkg.Package to this function.

The new pkg validation check for nil values as well.

This PR adds filters so a package without name or version doesn't go in the list of all discovered packages. Integration and cli tests were added to validate the feature. Signed-off-by: Jonas Xavier <jonasx@anchore.com>

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

also removes cli tests in favor of integration and unit tests Signed-off-by: Jonas Xavier <jonasx@anchore.com>

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

jonasagx · 2022-06-02T23:13:13Z

I saw your comment about some of the catalogers maybe being okay without this check, but I'd strongly encourage the use of this check for all code paths, so that we can centrally control what the requirements are for a viable package. It's not clear to me that the assumptions about other catalogers like RPM and Go are sufficient here.

I've added pkg validation to deb/rpm/golang catalogers, I expanded the unit tests to ensure correct dropping of invalid packages.

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

luhring

I think we're close... a couple of things to clear up

luhring · 2022-06-03T01:47:05Z

-
 	globParsers := map[string]ParserFn{
-		"**/a-path.txt": parser,
+		"**/*.txt": parser, // this glob should capture all files, including the empty one


Can you explain this change? I want to understand your thinking here

Yes, this glob (**/*.txt) will find a file named empty.txt, that will turn into an invalid package, and relationship, later on dropped from the final list. The previous glob (**/a-path.txt) wouldn't find empty.txt.

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

luhring

Looks great, thanks!

* main: (70 commits) fix: add php catalogers to all catalogers (anchore#1065) feat: add use-all-catalogers flag (anchore#1050) Updates parsing of `yarn.lock` to use `resolved` URLs that are pulled from yarn and npm registries (anchore#926) remove OSS Meetup message (anchore#1057) add pom.xml cataloger (anchore#1055) Add support for CBL-Mariner distroless images (anchore#1045) Add catalogers configuration (anchore#1038) add template output (anchore#1051) update stereoscope to latest version (anchore#1052) update zip_read_closer to incorporate zip64 support (anchore#1041) Add pacman (alpm) parser support (anchore#943) Update of README.md (anchore#1027) bump cosign to v1.9.0 to resolve reporting of GHSA-66x3-6cw3-v5gj (anchore#1025) add workflows to test new project automation (anchore#1023) improve LanguageByName and add unit tests (anchore#1034) Read Description from dpkg status files (anchore#996) Add announcement for Anchore OSS Virtual Meetup (anchore#1033) add main module field to go bin metadata (anchore#1026) Add filters to package cataloger (anchore#1021) change draft to false for release process (anchore#1016) ... Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>

* Add filters to package cataloger This PR adds filters so a package without name or version doesn't go in the list of all discovered packages. Integration and cli tests were added to validate the feature. Signed-off-by: Jonas Xavier <jonasx@anchore.com> * add nolint:funlen to cataloger/catalog.go Signed-off-by: Jonas Xavier <jonasx@anchore.com> * don't require package version Signed-off-by: Jonas Xavier <jonasx@anchore.com> * add package filtering to generic and python cataloger also removes cli tests in favor of integration and unit tests Signed-off-by: Jonas Xavier <jonasx@anchore.com> * drop nolint:funlen Signed-off-by: Jonas Xavier <jonasx@anchore.com> * check for no-removal operation Signed-off-by: Jonas Xavier <jonasx@anchore.com> * remove unused fixtures Signed-off-by: Jonas Xavier <jonasx@anchore.com> * rename no-version file to hide semantic version Signed-off-by: Jonas Xavier <jonasx@anchore.com> * drop integration tests and add pkg func for validation Signed-off-by: Jonas Xavier <jonasx@anchore.com> * python cataloger use global pkg validation Signed-off-by: Jonas Xavier <jonasx@anchore.com> * check for valid packages on deb/go/rpm catalogers Signed-off-by: Jonas Xavier <jonasx@anchore.com> * update rpm cataloger after rebase Signed-off-by: Jonas Xavier <jonasx@anchore.com> * nit with pointers Signed-off-by: Jonas Xavier <jonasx@anchore.com> * simpler use of package validation Signed-off-by: Jonas Xavier <jonasx@anchore.com> * remmove double pkg validations Signed-off-by: Jonas Xavier <jonasx@anchore.com> * rename func param to artifactsToExclude Signed-off-by: Jonas Xavier <jonasx@anchore.com> * add test for relationships and bug fix Signed-off-by: Jonas Xavier <jonasx@anchore.com> * feedback changes Signed-off-by: Jonas Xavier <jonasx@anchore.com>

jonasagx marked this pull request as draft May 31, 2022 21:16

jonasagx mentioned this pull request May 31, 2022

Drop whiteout/opaque files and warn when pkg lacks Name/Version #1017

Closed

jonasagx marked this pull request as ready for review May 31, 2022 22:05

luhring suggested changes Jun 1, 2022

View reviewed changes

jonasagx commented Jun 2, 2022

View reviewed changes

jonasagx requested a review from luhring June 2, 2022 02:08

luhring suggested changes Jun 2, 2022

View reviewed changes

jonasagx added 12 commits June 2, 2022 15:01

Add filters to package cataloger

bc72dff

This PR adds filters so a package without name or version doesn't go in the list of all discovered packages. Integration and cli tests were added to validate the feature. Signed-off-by: Jonas Xavier <jonasx@anchore.com>

add nolint:funlen to cataloger/catalog.go

b6de780

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

don't require package version

58418c7

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

add package filtering to generic and python cataloger

dd2c0a7

also removes cli tests in favor of integration and unit tests Signed-off-by: Jonas Xavier <jonasx@anchore.com>

drop nolint:funlen

64e8928

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

check for no-removal operation

a91173a

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

remove unused fixtures

07bdb32

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

rename no-version file to hide semantic version

746aa26

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

drop integration tests and add pkg func for validation

1d807ec

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

python cataloger use global pkg validation

8917ace

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

check for valid packages on deb/go/rpm catalogers

a789772

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

update rpm cataloger after rebase

838c328

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

jonasagx force-pushed the add-catalog-package-filters branch from e50c421 to 838c328 Compare June 2, 2022 22:07

jonasagx added 3 commits June 2, 2022 15:48

nit with pointers

96b502f

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

simpler use of package validation

bb654d6

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

remmove double pkg validations

9f0aa05

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

rename func param to artifactsToExclude

8b4706b

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

luhring reviewed Jun 3, 2022

View reviewed changes

add test for relationships and bug fix

8e5848b

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

jonasagx requested a review from luhring June 3, 2022 04:32

feedback changes

631045f

Signed-off-by: Jonas Xavier <jonasx@anchore.com>

luhring approved these changes Jun 3, 2022

View reviewed changes

luhring merged commit caff672 into anchore:main Jun 3, 2022

Conversation

jonasagx commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasagx commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevespringett commented Jun 1, 2022

Uh oh!

luhring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasagx Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonasagx commented Jun 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasagx commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luhring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasagx Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasagx Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasagx commented Jun 2, 2022

Uh oh!

luhring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luhring left a comment

Choose a reason for hiding this comment

jonasagx commented May 31, 2022 •

edited

Loading

jonasagx commented May 31, 2022 •

edited

Loading

jonasagx Jun 2, 2022 •

edited

Loading

jonasagx commented Jun 2, 2022 •

edited

Loading

jonasagx Jun 2, 2022 •

edited

Loading

jonasagx Jun 2, 2022 •

edited

Loading