Fix execs - Githubissues

amirmalka commented 7 months ago

User description

Application profile fixes:

Args will not be sorted
Each exec (path + args) combination is unique.

Rule Manager:

Removed args sorting
Pod to pod communication is ignored (temporary) since the NN object does not fill the DNS string.

Overview

Type

Enhancement, Bug fix

Description

Refactored exec tracking in ApplicationProfileManager to use slices instead of sets for efficiency and better management.
Updated related utility functions and tests to align with the new data structure for exec tracking.
Enhanced rule processing in R0001UnexpectedProcessLaunched by removing unnecessary sorting.
Added handling to ignore in-cluster DNS requests in R0005UnexpectedDomainRequest.
Introduced a new utility function CalculateSHA256FileExecHash for generating unique identifiers for execs, with corresponding tests.

Changes walkthrough

Relevant files

Enhancement

applicationprofile_manager.go `Refactor exec tracking from set to slice for efficiency` pkg/applicationprofilemanager/v1/applicationprofile_manager.go Changed the data structure for tracking execs from `mapset.Set[string]` to `[]string` to handle execs more efficiently. Modified methods to adapt to the new execs data structure, including `saveProfile`, `ReportFileExec`, and error handling sections. Updated initialization and cleanup of exec tracking structures in various lifecycle methods.	+28/-20
r0001_unexpected_process_launched.go `Simplify exec comparison in process event rule` pkg/ruleengine/v1/r0001_unexpected_process_launched.go Removed sorting of exec args which is now unnecessary due to new exec handling logic.	+0/-3
r0005_unexpected_domain_request.go `Enhance domain request rule to ignore in-cluster DNS` pkg/ruleengine/v1/r0005_unexpected_domain_request.go - Added a check to ignore in-cluster DNS requests.	+7/-1
applicationprofile.go `Update utility functions for new execs data structure` pkg/utils/applicationprofile.go Updated `CreateCapabilitiesPatchOperations` and `EnrichApplicationProfileContainer` to support the new execs data structure.	+17/-10
utils.go `Add utility function for exec identification` pkg/utils/utils.go Added a new utility function `CalculateSHA256FileExecHash` to generate unique identifiers for execs.	+13/-5

Tests

applicationprofile_manager_test.go `Update tests for new exec tracking logic` pkg/applicationprofilemanager/v1/applicationprofile_manager_test.go Added tests to verify the handling of duplicate and unique exec reports. Adjusted assertions to reflect the new data structure and handling logic for execs.	+19/-2
utils_test.go `Implement tests for new exec hash utility function` pkg/utils/utils_test.go Added tests for the new `CalculateSHA256FileExecHash` utility function.	+42/-0

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-free[bot] commented 7 months ago

PR Description updated to latest commit (https://github.com/kubescape/node-agent/commit/4de6593d6054995abff1c9d42e10a6139fb25300)

codiumai-pr-agent-free[bot] commented 7 months ago

PR Review

⏱️ Estimated effort to review [1-5]	3, because the PR involves changes in data structures and their management, which requires a detailed understanding of the existing system and the implications of these changes. The changes are spread across multiple files and include modifications to core functionalities like exec tracking and rule processing, which are critical for the application's performance and correctness.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The removal of sorting for `execEvent.Args` in `R0001UnexpectedProcessLaunched` and `execCall.Args` in the application profile might lead to inconsistencies in exec matching if the args are not always provided in the same order. This could potentially allow unexpected processes to be launched without being detected.
🔍 Possible issues	Data Integrity: The change from sets to slices for exec tracking might introduce issues with duplicate exec entries if not handled properly. The PR seems to address this by using a hash function to generate unique identifiers for execs, but this needs thorough testing to ensure no duplicates are missed and no false unique entries are created.
🔒 Security concerns	No

✨ Review tool usage guide:

**Overview:** The `review` tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be [added](https://pr-agent-docs.codium.ai/tools/review/#general-configurations) by configuring the tool. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on any PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L23) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ``` See the review [usage page](https://pr-agent-docs.codium.ai/tools/review/) for a comprehensive guide on using this tool.

codiumai-pr-agent-free[bot] commented 7 months ago

PR Code Suggestions

Category	Suggestions
Possible issue	Add nil check before using the `Range` method on `toSaveExecs` to prevent nil pointer dereference. ___ Consider checking for nil before using `toSaveExecs.Range` to avoid potential nil pointer dereference. [pkg/applicationprofilemanager/v1/applicationprofile_manager.go [456-461]](https://github.com/kubescape/node-agent/pull/265/files#diff-fc815317651e17975c117749e7661127dbcde82fd9d4d36ebc76cb5b09b3c54eR456-R461) ```diff -toSaveExecs.Range(func(uniqueExecIdentifier string, v []string) bool { - if !am.toSaveExecs.Get(watchedContainer.K8sContainerID).Has(uniqueExecIdentifier) { - am.toSaveExecs.Get(watchedContainer.K8sContainerID).Set(uniqueExecIdentifier, v) - } - return true -}) +if toSaveExecs != nil { + toSaveExecs.Range(func(uniqueExecIdentifier string, v []string) bool { + if !am.toSaveExecs.Get(watchedContainer.K8sContainerID).Has(uniqueExecIdentifier) { + am.toSaveExecs.Get(watchedContainer.K8sContainerID).Set(uniqueExecIdentifier, v) + } + return true + }) +} ```
Enhancement	Standardize the data structures for execs storage to improve type safety and code clarity. ___ Use a consistent data structure for `savedExecs` and `toSaveExecs` to ensure type safety and avoid confusion. [pkg/applicationprofilemanager/v1/applicationprofile_manager.go [46-51]](https://github.com/kubescape/node-agent/pull/265/files#diff-fc815317651e17975c117749e7661127dbcde82fd9d4d36ebc76cb5b09b3c54eR46-R51) ```diff -savedExecs maps.SafeMap[string, maps.SafeMap[string, []string]] -toSaveExecs maps.SafeMap[string, maps.SafeMap[string, []string]] +savedExecs maps.SafeMap[string, maps.SafeMap[string, []string]] +toSaveExecs maps.SafeMap[string, maps.SafeMap[string, []string]] ```
Enhancement	Use a configurable parameter for domain suffix checks to enhance flexibility. ___ Replace the hardcoded domain suffix with a configurable parameter to enhance flexibility and maintainability. [pkg/ruleengine/v1/r0005_unexpected_domain_request.go [71-72]](https://github.com/kubescape/node-agent/pull/265/files#diff-fe45fd0bea6a18b7edee9a1e285e10ce183b9ae962a0d5e441c4bfbd33ab47afR71-R72) ```diff -if strings.HasSuffix(domainEvent.DNSName, "svc.cluster.local.") { +if strings.HasSuffix(domainEvent.DNSName, config.ClusterDomainSuffix) { return nil } ```
Maintainability	Refactor execs setting logic into a method to reduce duplication and improve maintainability. ___ Instead of manually checking and setting each exec, consider using a method that encapsulates this logic to reduce code duplication and improve maintainability. [pkg/applicationprofilemanager/v1/applicationprofile_manager.go [456-461]](https://github.com/kubescape/node-agent/pull/265/files#diff-fc815317651e17975c117749e7661127dbcde82fd9d4d36ebc76cb5b09b3c54eR456-R461) ```diff -toSaveExecs.Range(func(uniqueExecIdentifier string, v []string) bool { - if !am.toSaveExecs.Get(watchedContainer.K8sContainerID).Has(uniqueExecIdentifier) { - am.toSaveExecs.Get(watchedContainer.K8sContainerID).Set(uniqueExecIdentifier, v) - } - return true -}) +toSaveExecs.Range(am.safeSetExecs(watchedContainer.K8sContainerID)) ```
Maintainability	Refactor repetitive patch operations into a loop or function for better maintainability. ___ Consider using a loop or a function to handle repetitive patch operations to enhance code maintainability and reduce errors. [pkg/utils/utils.go [328-336]](https://github.com/kubescape/node-agent/pull/265/files#diff-81ddbadfb415ccbb9c7af84f11668d1aa5e53c34025bf86d4702f16b4e42f045R328-R336) ```diff -existingPatch = append(existingPatch, - PatchOperation{ +patchOps := []PatchOperation{ + { Op: "replace", Path: "/metadata/annotations/" + EscapeJSONPointerElement(helpersv1.StatusMetadataKey), Value: string(watchedContainer.status), }, - PatchOperation{ + { Op: "replace", Path: "/metadata/annotations/" + EscapeJSONPointerElement(helpersv1.CompletionMetadataKey), + } +} +existingPatch = append(existingPatch, patchOps...) ```
Bug	Properly initialize new `maps.SafeMap` instances to prevent runtime errors. ___ Ensure that the new `maps.SafeMap` instances are properly initialized to avoid runtime panics due to nil map operations. [pkg/applicationprofilemanager/v1/applicationprofile_manager.go [277]](https://github.com/kubescape/node-agent/pull/265/files#diff-fc815317651e17975c117749e7661127dbcde82fd9d4d36ebc76cb5b09b3c54eR277-R277) ```diff -am.toSaveExecs.Set(watchedContainer.K8sContainerID, new(maps.SafeMap[string, []string])) +newExecMap := new(maps.SafeMap[string, []string]) +newExecMap.Init() // Assuming Init is a method to properly initialize the map +am.toSaveExecs.Set(watchedContainer.K8sContainerID, newExecMap) ```
Performance	Optimize the setting of execs in the map by using a method that checks and sets atomically. ___ Use a more efficient method for setting execs in the `toSaveExecs` map to avoid unnecessary checks and improve performance. [pkg/applicationprofilemanager/v1/applicationprofile_manager.go [456-461]](https://github.com/kubescape/node-agent/pull/265/files#diff-fc815317651e17975c117749e7661127dbcde82fd9d4d36ebc76cb5b09b3c54eR456-R461) ```diff toSaveExecs.Range(func(uniqueExecIdentifier string, v []string) bool { - if !am.toSaveExecs.Get(watchedContainer.K8sContainerID).Has(uniqueExecIdentifier) { - am.toSaveExecs.Get(watchedContainer.K8sContainerID).Set(uniqueExecIdentifier, v) - } + am.toSaveExecs.Get(watchedContainer.K8sContainerID).SetIfAbsent(uniqueExecIdentifier, v) return true }) ```
Best practice	Check for errors when writing to a hash to ensure data integrity. ___ Ensure that the `hash.Write` method's error is checked to handle potential failures in hash computation. [pkg/utils/utils.go [458]](https://github.com/kubescape/node-agent/pull/265/files#diff-81ddbadfb415ccbb9c7af84f11668d1aa5e53c34025bf86d4702f16b4e42f045R458-R458) ```diff -hash.Write([]byte(fmt.Sprintf("%s;%v", path, args))) +if _, err := hash.Write([]byte(fmt.Sprintf("%s;%v", path, args))); err != nil { + return "", err +} ```
Best practice	Add logging for ignored in-cluster communications to improve traceability. ___ Consider adding a debug or info log statement before returning nil for better traceability of the decision-making process. [pkg/ruleengine/v1/r0005_unexpected_domain_request.go [71-72]](https://github.com/kubescape/node-agent/pull/265/files#diff-fe45fd0bea6a18b7edee9a1e285e10ce183b9ae962a0d5e441c4bfbd33ab47afR71-R72) ```diff if strings.HasSuffix(domainEvent.DNSName, "svc.cluster.local.") { + log.Info("Ignoring in-cluster communication for domain:", domainEvent.DNSName) return nil } ```

✨ Improve tool usage guide:

**Overview:** The `improve` tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on a PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L78) related to the improve tool (`pr_code_suggestions` section), use the following template: ``` /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_code_suggestions] some_config1=... some_config2=... ``` See the improve [usage page](https://pr-agent-docs.codium.ai/tools/improve/) for a comprehensive guide on using this tool.