[CWS-1120] Merge activity dump and security profile managers #33877

YoannGh · 2025-02-10T15:29:11Z

What does this PR do?

Merge activity dump and security profile managers into a single manager responsible for handling both types of object
Refactor the activity dump and security profile data types, introducing a new Profile type that includes fields used for both activity collection (dump) and profile lookups.

Motivation

Remove the need to unmarshal every newly created activity dump to check whether it should be loaded to be used as a security profile

Describe how you validated your changes

Possible Drawbacks / Trade-offs

Additional Notes

spikat · 2025-02-13T09:59:50Z

cmd/security-agent/subcommands/runtime/activity_dump.go

 }

 func diffActivityDump(_ log.Component, _ config.Component, _ secrets.Component, args *activityDumpCliParams) error {
-	ad := dump.NewEmptyActivityDump(nil)
-	if err := ad.Decode(args.file); err != nil {
+	p := profile.New(cgroupModel.WorkloadSelector{}, nil, false, 0, nil)


Maybe we could have an helper like NewEmpty here (and on the other similar places), instead of having to call a func with a lot of variables (for code clarity). WDYT ?

spikat · 2025-02-18T09:33:31Z

pkg/security/security_profile/profile/profile.go

+		for _, existingTagName := range existingTagNames {
+			if existingTagName == tagName {
+				found = true
+				break
 			}
 		}
+		if !found {
+			p.tags = append(p.tags, tag)
+		}


Should we use slices.Contains() instead ?

spikat · 2025-02-18T09:41:34Z

pkg/security/security_profile/profile/profile.go

+	p.m.Lock()
+	defer p.m.Unlock()
+
+	imageTag := utils.GetTagValue("image_tag", p.tags)


we should not use the tag of the profile here IMHO

spikat · 2025-02-18T09:47:52Z

pkg/security/security_profile/profile/profile.go

+// getSelectorStr internal, thread-unsafe version of GetSelectorStr
+func (p *Profile) getSelectorStr() string {
+	tags := make([]string, 0, len(p.tags)+2)
+	if len(p.Metadata.ContainerID) > 0 {
+		tags = append(tags, fmt.Sprintf("container_id:%s", p.Metadata.ContainerID))
 	}
-	sp := &SecurityProfile{
-		selector:        selector,
-		eventTypes:      eventTypes,
-		versionContexts: make(map[string]*VersionContext),
-		timeResolver:    tr,
-		pathsReducer:    pathsReducer,
+	if len(p.Metadata.CGroupContext.CGroupID) > 0 {
+		tags = append(tags, fmt.Sprintf("cgroup_id:%s", p.Metadata.CGroupContext.CGroupID))
 	}
-	if selector.Tag != "" && selector.Tag != "*" {
-		sp.versionContexts[selector.Tag] = &VersionContext{
-			eventTypeState: make(map[model.EventType]*EventTypeState),
+	if len(p.tags) > 0 {
+		for _, tag := range p.tags {
+			if !strings.HasPrefix(tag, "container_id") && !strings.HasPrefix(tag, "cgroup_id") {
+				tags = append(tags, tag)
+			}
 		}
 	}
-	return sp
+	if len(tags) == 0 {
+		return "empty_selector"
+	}
+	return strings.Join(p.tags, ",")
 }


should we use the p.selector.String() instead? To avoid having to compute the selector every time

spikat · 2025-02-18T10:35:12Z

pkg/security/security_profile/profile/profile.go

-	if opts.DifferentiateArgs && input.Metadata.DifferentiateArgs {
-		p.ActivityTree.DifferentiateArgs()
-	}
+	imageTag := utils.GetTagValue("image_tag", p.tags)


we should use the imagetag of the event if any, not the profile's one

spikat · 2025-02-18T14:15:43Z

pkg/security/security_profile/profile/profile.go

+	for _, workload := range p.Instances {
+		if entry.ContainerID == workload.ContainerID {
+			return true
+		}
 	}


Not directly linked to your PR, but this looks insufficient for the new cgroup selectors

spikat · 2025-02-18T14:46:30Z

pkg/security/security_profile/directory.go

+	profileFiles := make(map[string]*profileFile)
+	for _, file := range files {
+		if !fileHasProfileExtension(file.Name()) {
+			continue
+		}
+
+		fileInfo, err := file.Info()
+		if err != nil {
+			seclog.Warnf("failed to retrieve file [%s] information: %s", file.Name(), err)
+			continue
+		}
+
+		if !fileInfo.Mode().IsRegular() {
+			continue
+		}
+
+		path := filepath.Join(directoryPath, file.Name())
+		_, ok := profileFiles[path]
+		if !ok {
+			profileFiles[path] = &profileFile{
+				path:  path,
+				mTime: fileInfo.ModTime(),
+			}
+		}
+	}
+
+	fileSlice := make([]*profileFile, 0, len(profileFiles))
+	for _, file := range profileFiles {
+		fileSlice = append(fileSlice, file)
+	}


why not creating directly a slice instead of a map (to then copy it to a slice) ?

spikat · 2025-02-18T15:10:42Z

pkg/security/security_profile/directory.go

+	// selectorToName allows finding a security profile from a given selector
+	// selector to names is a 1-to-N mapping (because multiple profiles can be created for the same selector)
+	selectorToNames map[cgroupModel.WorkloadSelector][]string
+	// namesToFiles allows finding the files associated from a given security profile name
+	// name to files is a 1-to-N mapping (because a same profile can be stored with multiple file formats)
+	nameToEntry *simplelru.LRU[string, *profileEntry]


IMHO we should really try to simplify this representation. Having a cgroup selector as key for the first map, and as value on the second is super error prone.
We need to spend some time discussing together, but a first idea to simplify it would be:

Don't bother having selector with versions. At the end, what we want is only image_name as selector (and '*' as tag/version), right?

I would also suggest to keep only the last version of a profile when persisting a new one. I don't see the point of keeping old versions of a profile (except for debug purposes?)

Ideally, if we can only keep the first map (selector to files) IMHO it would be great and would simplify by a lot the code complexity

spikat · 2025-02-18T16:43:41Z