Closed amitschendel closed 7 months ago
PR Description updated to latest commit (https://github.com/kubescape/node-agent/commit/360356bc012c8b2c5dadaa1d1b5a60ea8f0d79be)
⏱️ Estimated effort to review [1-5] | 4, because the PR involves substantial changes across multiple files, including core functionality related to process tree handling and alert generation. The changes impact various exporters and the malware manager, requiring careful review of logic, potential side effects, and integration with existing systems. |
🧪 Relevant tests | No |
🔍 Possible issues | Possible Bug: The removal of certain fields like `isPart_of_image` and `path` in alert structures may lead to loss of important debugging information unless adequately replaced or handled elsewhere. |
Performance Concern: The recursive process tree creation could potentially be expensive in terms of computation, especially if the process tree is deep or if this operation is performed frequently. | |
🔒 Security concerns | No |
Category | Suggestions |
Enhancement |
Add a check to ensure that a valid parent process ID is being stored.___ **Consider handling the case whereshim.PPID might be zero, which could indicate that the parent process does not exist or an error in fetching the process details. This can prevent potential issues when setting the containerIdToShimPid .**
[pkg/malwaremanager/v1/malware_manager.go [92]](https://github.com/kubescape/node-agent/pull/252/files#diff-39e99490e457120b984db82ca98c0e5227942707d1e37245ffd2287079ad89afR92-R92)
```diff
-mm.containerIdToShimPid.Set(notif.Container.Runtime.ContainerID, uint32(shim.PPID))
+if shim.PPID != 0 {
+ mm.containerIdToShimPid.Set(notif.Container.Runtime.ContainerID, uint32(shim.PPID))
+} else {
+ logger.L().Warning("No valid PPID found for container", helpers.String("containerID", notif.Container.Runtime.ContainerID))
+}
```
|
Enhance error logging by including the process ID in the debug message.___ **Use a more specific error message when failing to get the process status, including theprocess ID.** [pkg/utils/utils.go [672]](https://github.com/kubescape/node-agent/pull/252/files#diff-81ddbadfb415ccbb9c7af84f11668d1aa5e53c34025bf86d4702f16b4e42f045R672-R672) ```diff -logger.L().Debug("Failed to get process status", helpers.String("error", err.Error())) +logger.L().Debug("Failed to get process status for PID", helpers.String("pid", strconv.Itoa(proc.PID)), helpers.String("error", err.Error())) ``` | |
Improve UID and GID parsing by iterating over all available IDs.___ **Consider using a loop to handle multiple UIDs and GIDs instead of only using the secondelement.** [pkg/utils/utils.go [676-686]](https://github.com/kubescape/node-agent/pull/252/files#diff-81ddbadfb415ccbb9c7af84f11668d1aa5e53c34025bf86d4702f16b4e42f045R676-R686) ```diff -uid64, err := strconv.ParseUint(status.UIDs[1], 10, 32) -if err != nil { - return nil, nil +for _, uidStr := range status.UIDs { + uid64, err := strconv.ParseUint(uidStr, 10, 32) + if err != nil { + continue + } + uid = uint32(uid64) + break } -uid = uint32(uid64) -gid64, err := strconv.ParseUint(status.GIDs[1], 10, 32) -if err != nil { - return nil, nil +for _, gidStr := range status.GIDs { + gid64, err := strconv.ParseUint(gidStr, 10, 32) + if err != nil { + continue + } + gid = uint32(gid64) + break } -gid = uint32(gid64) ``` | |
Enhance the reliability of the
___
**Consider checking for the existence of the | |
Implement structured logging for better log management.___ **Use structured logging instead offmt.Sprintf for constructing log messages to enhance log management and analysis.** [pkg/ruleengine/v1/r0001_unexpected_process_launched.go [122]](https://github.com/kubescape/node-agent/pull/252/files#diff-ac1a37fd641e5d6211b057e4cbc91e8e5d6fad1ebdf030ad944240e92c48bffeR122-R122) ```diff -fmt.Sprintf("Unexpected process launched: %s in: %s", execPath, execEvent.GetContainer()), +log.WithFields(log.Fields{"execPath": execPath, "container": execEvent.GetContainer()}).Info("Unexpected process launched") ``` | |
Use distinct hash values for clarity in test scenarios.___ **To improve test clarity and maintainability, consider adding more detailed mock data orusing distinct values for different hash fields unless they are intentionally identical for the test scenario.** [pkg/exporters/http_exporter_test.go [173-175]](https://github.com/kubescape/node-agent/pull/252/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R173-R175) ```diff -MD5Hash: "testmalwarehash", -SHA1Hash: "testmalwarehash", -SHA256Hash: "testmalwarehash", +MD5Hash: "uniqueMD5Hash", +SHA1Hash: "uniqueSHA1Hash", +SHA256Hash: "uniqueSHA256Hash", ``` | |
Bug |
Ensure error handling is performed before using the result of a function that can fail.___ **It is recommended to handle the error fromutils.GetProcessStat before proceeding to use shim to avoid potential nil pointer dereferences if shim is nil due to an error.**
[pkg/malwaremanager/v1/malware_manager.go [88-92]](https://github.com/kubescape/node-agent/pull/252/files#diff-39e99490e457120b984db82ca98c0e5227942707d1e37245ffd2287079ad89afR88-R92)
```diff
shim, err := utils.GetProcessStat(int(notif.Container.Pid))
if err != nil {
logger.L().Warning("RuleManager - failed to get shim process", helpers.Error(err))
-} else {
- mm.containerIdToShimPid.Set(notif.Container.Runtime.ContainerID, uint32(shim.PPID))
+ return
}
+mm.containerIdToShimPid.Set(notif.Container.Runtime.ContainerID, uint32(shim.PPID))
```
|
Prevent potential out-of-range errors by checking if the slice is not empty.___ **Consider handling the case where `event.Args` is empty to prevent out-of-range errors.** [pkg/utils/utils.go [554-557]](https://github.com/kubescape/node-agent/pull/252/files#diff-81ddbadfb415ccbb9c7af84f11668d1aa5e53c34025bf86d4702f16b4e42f045R554-R557) ```diff -if len(event.Args) > 1 { +if len(event.Args) > 0 { return event.Args[1:] } return []string{} ``` | |
Improve error handling by returning the actual error instead of nil.___ **Replace the hardcoded error return with `err` to provide more context about the failure.** [pkg/utils/utils.go [678]](https://github.com/kubescape/node-agent/pull/252/files#diff-81ddbadfb415ccbb9c7af84f11668d1aa5e53c34025bf86d4702f16b4e42f045R678-R678) ```diff -return nil, nil +return nil, err ``` | |
Performance |
Implement caching for file hash calculations to improve function efficiency.___ **To improve the efficiency of theenrichMalwareResult function, consider caching the results of file hash calculations and file size retrievals to avoid recalculating them multiple times for the same files.** [pkg/malwaremanager/v1/malware_manager.go [209-215]](https://github.com/kubescape/node-agent/pull/252/files#diff-39e99490e457120b984db82ca98c0e5227942707d1e37245ffd2287079ad89afR209-R215) ```diff -md5hash, err := utils.CalculateMD5FileHash(hostPath) -if err != nil { - logger.L().Debug("Failed to calculate md5 hash for file", helpers.Error(err)) - md5hash = "" +if baseRuntimeAlert.MD5Hash == "" && hostPath != "" { + if cachedHash, found := hashCache.Get(hostPath); found { + baseRuntimeAlert.MD5Hash = cachedHash.(string) + } else { + md5hash, err := utils.CalculateMD5FileHash(hostPath) + if err != nil { + logger.L().Debug("Failed to calculate md5 hash for file", helpers.Error(err)) + md5hash = "" + } else { + hashCache.Set(hostPath, md5hash) + } + baseRuntimeAlert.MD5Hash = md5hash + } } -baseRuntimeAlert.MD5Hash = md5hash ``` |
Maintainability |
Refactor repeated hash calculation code into a separate function for better maintainability.___ **Refactor the repeated code for hash calculation and file size retrieval into separatefunctions to improve code maintainability and reduce redundancy.** [pkg/malwaremanager/v1/malware_manager.go [209-215]](https://github.com/kubescape/node-agent/pull/252/files#diff-39e99490e457120b984db82ca98c0e5227942707d1e37245ffd2287079ad89afR209-R215) ```diff -md5hash, err := utils.CalculateMD5FileHash(hostPath) -if err != nil { - logger.L().Debug("Failed to calculate md5 hash for file", helpers.Error(err)) - md5hash = "" +func calculateFileHash(hashFunc func(string) (string, error), hostPath string) string { + hash, err := hashFunc(hostPath) + if err != nil { + logger.L().Debug("Failed to calculate file hash", helpers.Error(err)) + return "" + } + return hash } -baseRuntimeAlert.MD5Hash = md5hash +baseRuntimeAlert.MD5Hash = calculateFileHash(utils.CalculateMD5FileHash, hostPath) ``` |
Refactor repetitive code into a function.___ **Refactor the alert label creation to a separate function to improve code readability andmaintainability.** [pkg/exporters/alert_manager.go [82-83]](https://github.com/kubescape/node-agent/pull/252/files#diff-40568094a80f334c1148d539cedecc426f810a6e32e5eeab274195f4c80693d5R82-R83) ```diff -"pid": fmt.Sprintf("%d", failedRule.GetRuntimeProcessDetails().ProcessTree.PID), -"ppid": fmt.Sprintf("%d", failedRule.GetRuntimeProcessDetails().ProcessTree.PPID), +"pid": createLabel("pid", failedRule), +"ppid": createLabel("ppid", failedRule), ``` | |
Verify the impact of removing
___
**The removal of | |
Best practice |
Use a constructor function for initializing
___
**To ensure the integrity of the |
Use pointers for optional struct fields.___ **Consider initializing theInfectedPID field using a pointer to ensure that it can be omitted if not applicable, which is a common practice in Go for optional struct fields.** [pkg/ruleengine/v1/r0007_kubernetes_client_executed.go [97]](https://github.com/kubescape/node-agent/pull/252/files#diff-c7f3697d600957bfc81d25f2f4f3ad77f03a09775a62f1a878856621f88ad653R97-R97) ```diff -InfectedPID: event.Pid, +InfectedPID: &event.Pid, ``` | |
Add safety checks for container fields in test structures.___ **Consider checking for nil or default values before using theContainerID and ContainerName fields to avoid potential runtime panics or logic errors in tests.** [pkg/exporters/http_exporter_test.go [107-108]](https://github.com/kubescape/node-agent/pull/252/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R107-R108) ```diff -ContainerID: "testcontainerid", -ContainerName: "testcontainer", +ContainerID: "testcontainerid", // Add nil or default value checks +ContainerName: "testcontainer", // Add nil or default value checks ``` | |
Possible issue |
Add nil checks before accessing struct fields.___ **To avoid potential nil pointer dereference, add a check to ensureevent.Runtime is not nil before accessing event.Runtime.ContainerID .**
[pkg/ruleengine/v1/r1005_fileless_execution.go [91]](https://github.com/kubescape/node-agent/pull/252/files#diff-ef0155bc510f47092b9799316b90b72f6eda3347ee96da897e27fe9961f06792R91-R91)
```diff
-ContainerID: event.Runtime.ContainerID,
+ContainerID: safeGetContainerID(event.Runtime),
```
|
Ensure safe handling of pointers when assigning string values.___ **Ensure that theSize field in BasicRuntimeAlert is properly handled as a pointer since it is being assigned from a string variable sizeStr . This could lead to potential runtime errors if not handled correctly.** [pkg/exporters/http_exporter_test.go [172]](https://github.com/kubescape/node-agent/pull/252/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R172-R172) ```diff -Size: &sizeStr, +Size: &sizeStr, // Ensure sizeStr is appropriately defined and not nil before dereferencing ``` | |
Verify and correct hash values to reflect accurate test data.___ **SinceMD5Hash , SHA1Hash , and SHA256Hash are all assigned the same value "testmalwarehash", consider if this is intentional or a mistake. If they should be different, correct the values accordingly.** [pkg/exporters/http_exporter_test.go [173-175]](https://github.com/kubescape/node-agent/pull/252/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R173-R175) ```diff -MD5Hash: "testmalwarehash", -SHA1Hash: "testmalwarehash", -SHA256Hash: "testmalwarehash", +MD5Hash: "testMD5Hash", +SHA1Hash: "testSHA1Hash", +SHA256Hash: "testSHA256Hash", ``` | |
Security |
Sanitize log data to prevent injection attacks.___ **Ensure that theStructuredData fields such as comm , uid , and gid are properly sanitized or validated to prevent log injection attacks.** [pkg/exporters/syslog_exporter.go [77-81]](https://github.com/kubescape/node-agent/pull/252/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R77-R81) ```diff -"comm": failedRule.GetRuntimeProcessDetails().ProcessTree.Comm, -"uid": fmt.Sprintf("%d", failedRule.GetRuntimeProcessDetails().ProcessTree.Uid), +"comm": sanitize(failedRule.GetRuntimeProcessDetails().ProcessTree.Comm), +"uid": sanitize(fmt.Sprintf("%d", failedRule.GetRuntimeProcessDetails().ProcessTree.Uid)), ``` |
Summary:
Summary:
User description
Overview
Type
Enhancement, Bug fix
Description
ProcessTree
structure across various components for enhanced process detail tracking.ProcessTree
.ProcessTree
for better process context in alerts.ProcessTree
structure usage.ProcessTree
creation and management.Changes walkthrough
31 files
alert_manager.go
Integrate ProcessTree structure and streamline alert summaries
pkg/exporters/alert_manager.go
ProcessTree
structure forprocess details.
IsPartOfImage
in malware alerts.csv_exporter.go
Refactor CSV Exporter to Use ProcessTree Structure
pkg/exporters/csv_exporter.go
ProcessTree
for process details.http_exporter.go
Enhance HTTP Exporter with ProcessTree Structure
pkg/exporters/http_exporter.go
ProcessTree
into HTTP alerts for detailed processinformation.
fields.
stdout_exporter.go
Update Stdout Exporter to Log ProcessTree Details
pkg/exporters/stdout_exporter.go - Updated stdout exporter to log new `ProcessTree` structure.
syslog_exporter.go
Integrate ProcessTree in Syslog Exporter Logs
pkg/exporters/syslog_exporter.go - Updated syslog exporter to include `ProcessTree` details in logs.
malware_manager_interface.go
Update Malware Manager Interface to Use ProcessTree
pkg/malwaremanager/malware_manager_interface.go
ProcessTree
instead ofRuntimeAlertProcessDetails
.clamav.go
Refactor ClamAV Integration to Use ProcessTree
pkg/malwaremanager/v1/clamav/clamav.go
ProcessTree
for storing process details in malwaredetection.
malware_manager.go
Enhance Malware Result Enrichment with ProcessTree
pkg/malwaremanager/v1/malware_manager.go
using
ProcessTree
.malwareresult.go
Update MalwareResult to Use ProcessTree for Process Details
pkg/malwaremanager/v1/types/malwareresult.go
MalwareResult
to useProcessTree
for detailed processinformation.
ruleengine_interface.go
Update Rule Engine Interface to Use ProcessTree
pkg/ruleengine/ruleengine_interface.go
ProcessTree
for process details.failureobj.go
Update GenericRuleFailure to Use ProcessTree
pkg/ruleengine/v1/failureobj.go
GenericRuleFailure
to useProcessTree
for storing processdetails.
helpers.go
Remove Outdated Helper Functions in Rule Engine
pkg/ruleengine/v1/helpers.go
r0001_unexpected_process_launched.go
Update Rule for Unexpected Process Launch to Use ProcessTree
pkg/ruleengine/v1/r0001_unexpected_process_launched.go
ProcessTree
for capturing unexpected processlaunches.
r0002_unexpected_file_access.go
Update Rule for Unexpected File Access to Use ProcessTree
pkg/ruleengine/v1/r0002_unexpected_file_access.go
ProcessTree
for capturing unexpected file accessdetails.
r0003_unexpected_system_call.go
Update Rule for Unexpected System Call to Use ProcessTree
pkg/ruleengine/v1/r0003_unexpected_system_call.go
ProcessTree
for capturing unexpected system calls.r0004_unexpected_capability_used.go
Update Rule for Unexpected Capability Usage to Use ProcessTree
pkg/ruleengine/v1/r0004_unexpected_capability_used.go
ProcessTree
for capturing unexpected capabilityusage.
r0005_unexpected_domain_request.go
Update Rule for Unexpected Domain Request to Use ProcessTree
pkg/ruleengine/v1/r0005_unexpected_domain_request.go
ProcessTree
for capturing unexpected domainrequests.
r0006_unexpected_service_account_token_access.go
Update Rule for Unexpected Service Account Token Access to Use
ProcessTree
pkg/ruleengine/v1/r0006_unexpected_service_account_token_access.go
ProcessTree
for capturing unexpected serviceaccount token access.
r0007_kubernetes_client_executed.go
Update Rule for Execution of Kubernetes Client to Use ProcessTree
pkg/ruleengine/v1/r0007_kubernetes_client_executed.go
ProcessTree
for capturing execution of Kubernetesclient.
r1000_exec_from_malicious_source.go
Update Rule for Execution from Malicious Sources to Use ProcessTree
pkg/ruleengine/v1/r1000_exec_from_malicious_source.go
ProcessTree
for capturing execution from malicioussources.
r1001_exec_binary_not_in_base_image.go
Update Rule for Execution of Binaries Not in Base Image to Use
ProcessTree
pkg/ruleengine/v1/r1001_exec_binary_not_in_base_image.go
ProcessTree
for capturing execution of binariesnot in the base image.
r1002_load_kernel_module.go
Update Rule for Kernel Module Loading to Use ProcessTree
pkg/ruleengine/v1/r1002_load_kernel_module.go
ProcessTree
for capturing kernel module loading.r1003_malicious_ssh_connection.go
Update Rule for Malicious SSH Connections to Use ProcessTree
pkg/ruleengine/v1/r1003_malicious_ssh_connection.go
ProcessTree
for capturing malicious SSHconnections.
r1004_exec_from_mount.go
Update Rule for Execution from Mount Points to Use ProcessTree
pkg/ruleengine/v1/r1004_exec_from_mount.go
ProcessTree
for capturing execution from mountpoints.
r1005_fileless_execution.go
Update Rule for Fileless Execution to Use ProcessTree
pkg/ruleengine/v1/r1005_fileless_execution.go
ProcessTree
for capturing fileless executions.r1006_unshare_system_call.go
Update Rule for Unshare System Call to Use ProcessTree
pkg/ruleengine/v1/r1006_unshare_system_call.go
ProcessTree
for capturingunshare
system calls.r1007_xmr_crypto_mining.go
Update Rule for XMR Crypto Mining to Use ProcessTree
pkg/ruleengine/v1/r1007_xmr_crypto_mining.go
ProcessTree
for capturing XMR crypto miningactivities.
r1008_crypto_mining_domain.go
Update Rule for Crypto Mining Domain Communication to Use ProcessTree
pkg/ruleengine/v1/r1008_crypto_mining_domain.go
ProcessTree
for capturing crypto mining domaincommunications.
r1009_crypto_mining_port.go
Update Rule for Crypto Mining Related Port Activities to Use
ProcessTree
pkg/ruleengine/v1/r1009_crypto_mining_port.go
ProcessTree
for capturing crypto mining relatedport activities.
rule_manager.go
Enhance Rule Manager to Enrich Rule Failures with ProcessTree
pkg/rulemanager/v1/rule_manager.go
details using
ProcessTree
.utils.go
Add Utility Functions for ProcessTree Handling
pkg/utils/utils.go
ProcessTree
structures.
5 files
alert_manager_test.go
Update tests for new ProcessTree structure in alert manager
pkg/exporters/alert_manager_test.go
ProcessTree
structure in processdetails.
csv_exporter_test.go
Update CSV Exporter Tests for ProcessTree Integration
pkg/exporters/csv_exporter_test.go - Updated CSV exporter tests to use the new `ProcessTree` structure.
http_exporter_test.go
Update HTTP Exporter Tests for New Process Details Handling
pkg/exporters/http_exporter_test.go
ProcessTree
.stdout_exporter_test.go
Update Tests for Stdout Exporter ProcessTree Logging
pkg/exporters/stdout_exporter_test.go - Updated tests to check for new process tree logging.
syslog_exporter_test.go
Update Syslog Exporter Tests for ProcessTree Logging
pkg/exporters/syslog_exporter_test.go