ROX-13794: Make node analyzer return content sets, not cpes by jvdm · Pull Request #1055 · stackrox/scanner

jvdm · 2023-01-13T18:34:09Z

Description and rationale

Populate RHEL Content Sets in the return value of certified RHEL components detection.

This will be consumed by Compliance node scanning and eventually sent to Central Scanner through scannerV1.Component to be mapped to CPEs. Analysis detection used to perform the CPE mapping on detection, and that is left for backward compatibility. Still, the mapping results in nil gracefully if CPE mapping is not available, which is the case for node scanning.

Tests

UTs.

vikin91 · 2023-01-16T11:10:12Z

We also must make sure that the GetNodeVulnerabilitiesRequest.Components from https://github.com/stackrox/scanner/blob/master/proto/scanner/api/v1/node_scan_service.proto contains the content_sets instead/additionally to CPEs.

Otherwise Central will not be able to pass the content_sets to Scanner through the GetNodeVulnerabilities API.

ghost · 2023-01-16T18:59:29Z

Images are ready for the commit at 3d1a547.

To use the images, use the tag 2.27.x-39-g3d1a547e8d.

RTann

I'm not sure we can guarantee there is a single content_manifest per node since its a culmination of all layers of an image. We knew there was one per layer, but there may be more for the image as a whole

RTann · 2023-01-17T21:06:15Z

+	var err error

 	// In the current state, RHCOS will always be handled as certified system.
 	// If no CPEs are found on RHCOS, a note needs to be added that informs users of it.


this comment may need to be adjusted since we are looking at content sets now

Yes, it may. Obviously, the comment mentions CPE, which will no longer be handled in RHCOS detection. But other aspects of the comment might still be relevant. What is your suggestion?

sorry I was referring specifically to the CPE part of this line

I reviewed this, and the comment needs to be refactored. Not only the mention of CPE. It is fixed in 3d1a547

RTann · 2023-01-17T21:07:51Z

-			logrus.WithFields(logrus.Fields{LogLayerName: name, "feature count": len(featureVersions)}).Debug("detected features")
-		}
+		logrus.WithFields(logrus.Fields{
+			LogLayerName:    name,


I'm actually not familiar with this key

Do you mean we should change it or remove it? Not clear what your suggestion is.

oh I was hoping you'd tell me what it was 😄 I just checked, and it's the word "Layer"

RTann · 2023-01-17T21:09:58Z

+	rhelfeatures := &database.RHELv2Components{
+		Dist:     namespace.Name,
+		Packages: packages,
+		// CPEs are mapped and returned with content sets for backward compatibility.


Needed for scanning images in OCR. Perhaps we can update that path, too, which would also eliminate the need to pass those over from Central -> Sensor -> Scanner in another PR, if it works out

I see where you coming from. When looking at this patch, it is natural to try to eliminate CPE mapping in detection altogether. But the handling outside RHCOS scanning might be more complex.

I intentionally haven't investigated further. Perhaps database models need to be adjusted in Central Scanner, for example. In any case, I already created an issue ROX-14414 to track this, as we should consider it in a different context.

sounds good! Yeah definitely not in the scope of this PR

RTann · 2023-01-17T21:11:02Z

+		ContentSets: contentSets,
+	}
+	if err := rpm.AnnotateComponentsWithPackageManagerInfo(files, languageComponents); err != nil {
+		logrus.WithError(err).Errorf("Failed to analyze package manager info for language components: %s", name)


I wonder if we should make this a warning since we do ignore it?

It could be a warning. What is your suggestion?

hmmm I'm thinking maybe keep it as an Error log, but maybe we also mention that we are going to continue with the analysis? That being said, this log is not new, so perhaps this is out-of-scope

RTann · 2023-01-17T21:12:41Z

 // AllRHELRequiredFiles lists all the names of the files required to identify RHEL-based releases.
 var AllRHELRequiredFiles set.StringSet

+// contentManifests set with all known directories that might contain content manifest files.
+var contentManifestDirs set.StringSet


Suggested change

// AllRHELRequiredFiles lists all the names of the files required to identify RHEL-based releases.

var AllRHELRequiredFiles set.StringSet

// contentManifests set with all known directories that might contain content manifest files.

var contentManifestDirs set.StringSet

var (

// AllRHELRequiredFiles lists all the names of the files required to identify RHEL-based releases.

var AllRHELRequiredFiles set.StringSet

// contentManifests set with all known directories that might contain content manifest files.

var contentManifestDirs set.StringSet

)

also nit: comment name should start with the variable's name

Fixed at 0ee2223

RTann · 2023-01-17T21:15:49Z

+	for _, prefix := range contentManifestDirs.AsSlice() {
+		for name, file := range files.GetFilesPrefix(prefix) {
+			if strings.HasSuffix(name, ".json") {
+				// Return the first one found, as there should be only one dir per layer/node.


nit: file not dir

Also, I'm not 100% sure we can guarantee it's 1 per node. An image can have more than one (I've seen up to three in the wild), and if a node is essentially a running container, then I think it's possible a node can have more than one.

Oh, interesting. The logic was kept the same as in the previous patch. If, in fact, there can be multiple Content Sets per image/node, we need to know how to process all these content sets.

I noticed that there is some other metadata associated with each content set file, so it might be possible that when working with multiple, we need to process them differently. Or maybe concatenating them on a list. Hopefully.

I am going to keep the original data and follow up separately.

I actually noticed that in the proposed patch we were actually returning the last content set instead of the first one. This was fixed at e096fa7

Maybe we should update this to allow for more than one content set per image layer (therefore node as well). That should not hurt image scanning results, and will allow for multiple content sets per node (maybe in a separate PR if we mark this with a TODO or something). I believe just appending each file's ContentSets into a slice is sufficient.

You bring up another point: are we sure RHCOS nodes' content_sets use the same format we are currently expecting? I'm not sure. Has this been discussed?

Maybe we should update this to allow for more than one content set per image layer (therefore, node as well).

Maybe. Another possibility is leaving the content set detection unchanged as we collect more information on the multiple contents sets scenario. For example, we need to know how RHCOS content sets are created in their builds or how multiple content sets map to packages and layers used in the final container to decide the correct approach.

Perhaps it is also unnecessary to keep this PR blocked and hold potential demos while we gather these answers. Instead, maybe it's even better to have a functioning scanning in which we can start doing E2E/manual testing and enrich our questions.

You bring up another point: are we sure RHCOS nodes' content_sets use the same format we are currently expecting?

I thought content sets have a schema. Or are you referring to RHCOS using a new version of the schema?

RTann · 2023-01-17T21:17:13Z

 		// Expected values for assertion.
-		sampleExpectedPkgs []*database.RHELv2Package
-		unexpectedPkgs     []*database.RHELv2Package
-		expectedCPEs       []string


do we test the mapping from content set to CPE in repo2cpe? If not, we should do that there, so we don't lose this test

Do you mean

scanner/pkg/repo2cpe/mapping_test.go

Line 11 in 0ee2223

func TestMapping(t *testing.T) {

RTann · 2023-01-18T00:38:13Z

+	var err error

 	// In the current state, RHCOS will always be handled as certified system.
 	// If no CPEs are found on RHCOS, a note needs to be added that informs users of it.


sorry I was referring specifically to the CPE part of this line

RTann · 2023-01-18T00:40:03Z

-			logrus.WithFields(logrus.Fields{LogLayerName: name, "feature count": len(featureVersions)}).Debug("detected features")
-		}
+		logrus.WithFields(logrus.Fields{
+			LogLayerName:    name,


oh I was hoping you'd tell me what it was 😄 I just checked, and it's the word "Layer"

RTann · 2023-01-18T00:41:09Z

+	rhelfeatures := &database.RHELv2Components{
+		Dist:     namespace.Name,
+		Packages: packages,
+		// CPEs are mapped and returned with content sets for backward compatibility.


sounds good! Yeah definitely not in the scope of this PR

RTann · 2023-01-18T00:42:30Z

+		ContentSets: contentSets,
+	}
+	if err := rpm.AnnotateComponentsWithPackageManagerInfo(files, languageComponents); err != nil {
+		logrus.WithError(err).Errorf("Failed to analyze package manager info for language components: %s", name)


hmmm I'm thinking maybe keep it as an Error log, but maybe we also mention that we are going to continue with the analysis? That being said, this log is not new, so perhaps this is out-of-scope

RTann · 2023-01-18T00:43:02Z

 		// Expected values for assertion.
-		sampleExpectedPkgs []*database.RHELv2Package
-		unexpectedPkgs     []*database.RHELv2Package
-		expectedCPEs       []string


RTann · 2023-01-18T01:01:21Z

+		Dist:     namespace.Name,
+		Packages: packages,
+		// CPEs are mapped and returned with content sets for backward compatibility.
+		CPEs:        repo2cpe.Singleton().Get(contentSets),


Just realized this may panic if it attempts to load a file and fails. It won't panic as long as REPO_TO_CPE_DIR is not defined (or is empty). Maybe we should reconsider the panic. It made sense before Scanner started to be used as a library like this, but now if we mess up, Compliance pods can panic. At the moment, it won't, but it could in the future if we aren't careful

Yeah, maybe. It depends on how the final container layout will be, how compliance consumes the Scanner library and the coupling between them. See ROX-13934, which captures this work.

vikin91 · 2023-01-18T11:29:38Z

+		if contents != nil {
+			break
+		}


So if we have multiple locations to search for - len(contentManifestDirs.AsSlice()) > 1 and we find a content in the first location contentManifestDirs[0], why are we are breaking the search here? Shouldn't we check also the other ones - i.e., contentManifestDirs[1], contentManifestDirs[2] and so on?

The current behavior would suggest that we are searching all location but we expect that the first hit is correct. Is it safe to assume that?

Shouldn't we check also the other ones - i.e., contentManifestDirs[1], contentManifestDirs[2] and so on?

We want to check all the contentManifestDirs but stop on the first file found. That is because we don't expect to find content sets in more than one directory.

Initially, this code would only search in one directory, the default directory for content sets in certified RHEL-based images. Now we are adding RHCOS, which uses a different location -- they have a limitation in their build that prevents them from writing to /root at compose time -- but we don't expect to see content sets in both locations.

The current behavior would suggest that we are searching all locations but we expect that the first hit is correct. Is it safe to assume that?

For RHCOS node scanning, we are starting to debate that assumption.

Based on the original discussion, it seems the image build would collect all repos used and populate one single file. But there are some reports of multiple content sets found "in the wild". Which might be caused by RHCOS customizations? We are not sure, but we are planning to follow up. In any case, maybe we should not try to capture that scenario without knowing more about it.

What do you think, @vikin91?

Okay this convinces me. I understand that the potential case of multiple content sets json files will be handled in a separate PR.

I haven't seen it in RHCOS, but I have in RHEL-based images, which makes me think it could always be possible in RHCOS unless told otherwise. For example, in https://catalog.redhat.com/software/containers/redhat-openjdk-18/openjdk18-openshift/58ada5701fbe981673cd6b10?container-tabs=overview I see this:

[jboss@3ae40f064f3c ~]$ ls /root/buildinfo/content_manifests/ redhat-openjdk-18-openjdk18-openshift-container-1.15-2.json rhel-server-container-7.9-848.1669829396.json

we are planning to follow up

Do we have a ticket to track this?

we are planning to follow up

https://issues.redhat.com/browse/ROX-13794

vikin91

Looks good to me!

I would love to see it in action on OpenShift to see that it works - we can make such test before of after this is merged as this feat is behind a feature flag, so I am leaving the choice to you.

jvdm · 2023-01-19T18:23:43Z

@vikin91:

I would love to see it in action on OpenShift to see that it works - we can make such test before of after this is merged as this feat is behind a feature flag, so I am leaving the choice to you.

I was planning to do that after working on the Scanner APIs. Glad you pushed towards it already. I will explore once that PR is merged.

RTann · 2023-01-20T01:05:00Z

+		if contents != nil {
+			break
+		}


I haven't seen it in RHCOS, but I have in RHEL-based images, which makes me think it could always be possible in RHCOS unless told otherwise. For example, in https://catalog.redhat.com/software/containers/redhat-openjdk-18/openjdk18-openshift/58ada5701fbe981673cd6b10?container-tabs=overview I see this:

[jboss@3ae40f064f3c ~]$ ls /root/buildinfo/content_manifests/ redhat-openjdk-18-openjdk18-openshift-container-1.15-2.json rhel-server-container-7.9-848.1669829396.json

RTann · 2023-01-20T01:06:17Z

+		if contents != nil {
+			break
+		}


we are planning to follow up

Do we have a ticket to track this?

jvdm added 4 commits January 12, 2023 11:12

Add content sets to Components proto

9e56a0b

Update rpm.go to use the new CPE mapping return

4989819

Merge 'origin/master'

a2c102b

Make node analyzer return content sets, not cpes

76cdda2

jvdm mentioned this pull request Jan 13, 2023

ROX-13794: Add content sets to Components proto #1053

Merged

Base automatically changed from jvdm/ROX-13794/proto-updates/pull to master January 16, 2023 17:33

jvdm added 2 commits January 16, 2023 09:55

Merge 'origin/master' into 'jvdm/ROX-13794/node-analyzer/pull'

25e9841

Log features if emtpy and add comment on CPE usage

181c8b5

jvdm self-assigned this Jan 16, 2023

jvdm requested review from Maddosaurus and RTann and removed request for RTann January 16, 2023 18:56

Add UT for content set retrieval, supporting RHCOS

81494b0

jvdm requested review from daynewlee and vikin91 January 16, 2023 19:36

jvdm changed the title ~~Make node analyzer return content sets, not cpes~~ ROX-13794: Make node analyzer return content sets, not cpes Jan 16, 2023

Make style lint smile

3836b82

RTann suggested changes Jan 17, 2023

View reviewed changes

jvdm added 2 commits January 17, 2023 15:47

refactor: Change package var declaration and fix doc comment

0ee2223

fix: Make sure the first content set is returned

e096fa7

jvdm requested a review from RTann January 18, 2023 00:15

RTann reviewed Jan 18, 2023

View reviewed changes

vikin91 reviewed Jan 18, 2023

View reviewed changes

fix: Update TODO comment on ROX-13906

3d1a547

stackrox deleted a comment from RTann Jan 18, 2023

jvdm requested a review from RTann January 18, 2023 19:51

jvdm requested a review from vikin91 January 18, 2023 20:45

vikin91 approved these changes Jan 19, 2023

View reviewed changes

RTann approved these changes Jan 20, 2023

View reviewed changes

jvdm merged commit cb0acb4 into master Jan 20, 2023

jvdm deleted the jvdm/ROX-13794/node-analyzer/pull branch January 20, 2023 01:12

Uh oh!

Conversation

jvdm commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and rationale

Tests

Uh oh!

vikin91 commented Jan 16, 2023

Uh oh!

ghost commented Jan 16, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RTann left a comment

Choose a reason for hiding this comment

Uh oh!

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!

jvdm Jan 18, 2023 • 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!

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!

jvdm Jan 18, 2023 • 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!

jvdm Jan 18, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

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

jvdm commented Jan 13, 2023 •

edited

Loading

ghost commented Jan 16, 2023 •

edited by ghost

Loading

jvdm Jan 18, 2023 •

edited

Loading

jvdm Jan 18, 2023 •

edited

Loading

jvdm Jan 18, 2023 •

edited

Loading