Repository navigation
Skip parsing audit events that cannot be detections - #505
Open
venkata-stepsecurity wants to merge 2 commits into
Open
venkata-stepsecurity wants to merge 2 commits into
venkata-stepsecurity wants to merge 2 commits into
Conversation
An event made only of SYSCALL, PROCTITLE and EOE records has no address, path, working directory or arguments, so isEventReady is always false for it. Check the record types before parsing and return early for these. On a busy runner most events are sends on connected sockets, which have no SOCKADDR record. Parsing them was most of the agent's CPU.
…st on unparsed fixtures
step-security-bot-int
left a comment
There was a problem hiding this comment.
Please find StepSecurity AI-CodeWise code comments below.
Code Comments
procmon.go
- [High]Add unit tests for hasDetectionRecord function to verify correctness
According to standards like the ISTQB and best practices in software engineering manuals such as those from IEEE, new functionality introduced in code should be complemented with comprehensive unit tests to ensure the feature behaves as expected and to prevent regressions. Implement unit tests with various AuditMessage slices covering cases with only SYSCALL, PROCTITLE, EOE, and with other records to verify that hasDetectionRecord accurately identifies detection records. - [High]Add input parameter validation for the msgs slice in ReassemblyComplete and hasDetectionRecord
Defensive programming principles as described in authoritative sources like "Clean Code" by Robert C. Martin recommend validating inputs to functions to avoid potential nil pointer dereferences or unexpected behaviors. Check if msgs is nil or empty at the start of hasDetectionRecord and ReassemblyComplete, and handle those cases appropriately (e.g., treat nil or empty slice as having no detection record and return early). - [Medium]Document concurrency assumptions for the auditStream methods
Go projects often have concurrent processing, and understanding whether methods like ReassemblyComplete or EventsLost are safe for concurrent invocation is critical for maintainability and correctness, as emphasized in the Go concurrency patterns documentation from golang.org. Add comments about concurrency guarantees or add synchronization mechanisms if these methods are expected to be called concurrently to avoid data races. - [Medium]Avoid unnecessary iteration by exiting early in hasDetectionRecord when possible
The current implementation already returns early upon finding a detection record which is good; maintaining this pattern aligns with best practices for efficient loop termination described in effective Go and other style guides. No code change needed, but emphasize this practice in comments for future maintainers. - [Low]Use explicit named return values for boolean functions to improve readability
According to Go style recommendations, named return values can improve clarity in short functions by indicating what the output represents. Modify hasDetectionRecord signature to: func hasDetectionRecord(msgs []*auparse.AuditMessage) (hasRecord bool) { ... } and use return statements accordingly. - [Low]Ensure consistent error handling/logging when ReassemblyComplete returns early
Best practices per Google's Go Style guide suggest logging or error handling if an event is skipped to aid in debugging and maintenance. Optionally add debug level logging before returning early in ReassemblyComplete indicating no detection record was found. - [Low]Separate concerns by extracting event building logic from the iteration
The Single Responsibility Principle (from SOLID principles outlined by Robert C. Martin) suggests decomposing functions with multiple responsibilities into smaller focused functions. Extract the event population logic inside the loop into a separate function likepopulateEventFromMsg(event *Event, msg *auparse.AuditMessage).
procmon_test.go
- [High]Add comments to clarify the purpose of test functions and key test cases
The test functions such as TestHasDetectionRecord, TestEventsWithoutDetectionRecordsAreNeverReady, and TestReassemblySkipsParsingEventsWithoutDetectionRecords lack contextual comments explaining their purpose and rationale. According to the Go best practices (https://go.dev/doc/effective_go#commentary), comments are important for maintainability and for helping future developers understand test intent clearly. Add descriptive comments above each test function and complex test cases to explain what they are testing and why certain inputs/outputs are expected. - [High]Use table-driven tests with t.Run to improve test isolation and reporting
The TestHasDetectionRecord test uses a table-driven format but does not use t.Run subtests, which limits granularity of test failure reporting. As recommended by the Go testing package documentation (https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks), using t.Run enhances test clarity and helps isolate individual test failures. Refactor the loop in TestHasDetectionRecord to use t.Run(tt.name, func(t *testing.T) {...}) to run each test case as a subtest. - [Medium]Check error handling thoroughly in tests and fail fast with context
In TestEventsWithoutDetectionRecordsAreNeverReady, the test calls t.Fatalf immediately upon any parsing error, but it would be more helpful to report which event and record caused the failure. This improves debugging and is aligned with best testing practices (https://testing.googleblog.com/2013/05/testing-on-toilet-flaky-tests.html). Improve error messages in test failures by including detailed event and record indices or descriptions. Consider using t.Errorf with continue or t.FailNow appropriately. - [Medium]Avoid hardcoding magic numbers or literal values that lack context
Values like syscall numbers (e.g., 60, 44) and audit event IDs (e.g., 65) appear throughout the tests without explanation. According to Clean Code principles (Robert C. Martin), magic numbers should be replaced with named constants to improve readability and maintainability. Define constants or enums for syscall numbers and audit record IDs with descriptive names and use them in the tests. - [Medium]Use helper functions to reduce repetition and improve readability in tests
Some setup code like building []*auparse.AuditMessage slices is repetitive. The presence of helper functions improves test code clarity and reduces duplication as recommended in Effective Go (https://go.dev/doc/effective_go#tests). Create helper functions to assemble audit message slices or parse auditRecords into messages to improve DRYness. - [Low]Verify consistent use of error variables and checks for parse errors in tests
In adding records to events, errors are only checked partially. According to Go error-handling best practices (https://github.com/golang/go/wiki/ErrorHandling), errors should be handled explicitly and consistently especially in test code to avoid false positives. Confirm that all parsing errors and failure scenarios are checked and cause test failures where appropriate. - [Low]Use table-driven tests consistently across all tests for uniformity
While TestHasDetectionRecord uses a table-driven approach, other tests like TestEventsWithoutDetectionRecordsAreNeverReady don't use it consistently for all event sets. Consistency in testing style improves codebase maintainability. Consider converting other similar tests to table-driven form with descriptive test case names. - [Low]Explicitly document assumptions on audit record fields in test cases
Test cases involve assumptions, e.g., certain audit records imply detection status. Documenting these assumptions explicitly helps others understand test coverage and design (IEEE Software Maintenance recommendations). Add inline comments or docstrings to test cases describing why each input should produce the expected output.
Feedback
We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On a runner with heavy network traffic, most audit events are sends on connected sockets: a
sendto/sendmsgwith no destination address, so there is no SOCKADDR record. The agent parses every record of every event into a map (go-libauditToMapStr), then drops these events inisEventReadybecause they have no address.A CPU profile of the agent on a workload streaming many small sends over localhost TCP showed:
ToMapStr;Change
auditStream.ReassemblyCompletechecks the record types before parsing. If every record is SYSCALL, PROCTITLE or EOE, the event is not parsed. A detection's address, path, working directory and arguments come from the SOCKADDR, PATH, CWD and EXECVE records, so these events were already discarded. Events with any other record type are parsed as before.Field errors in skipped events are no longer counted in
recordErrors.No change to audit rules or kernel settings.
Testing
hasDetectionRecordfor each event shape;go mod vendor,go mod verify,go vetandgo test -raceon Linux with Go 1.27.3.TestRunandTest_addAuditRulesfail in a container on both this branch andint, because they need a real runner.On both builds, at start, middle and end of the run, all detection probes were reported: HTTPS connections, a UDP
sendto, and the source file overwrite.On typical jobs (npm, Go, Python and Docker builds), the int agent already uses 0.1 to 2 CPU-s per job.
This is independent of #504, which changes the kernel backlog settings. With both applied, the same workload ran at 930 tests/min, with the agent at 54% of a core.