Skip to content

Skip parsing audit events that cannot be detections - #505

Open
venkata-stepsecurity wants to merge 2 commits into
intfrom
venkat/procmon-skip-unready-parse
Open

venkata-stepsecurity wants to merge 2 commits into
intfrom
venkat/procmon-skip-unready-parse

Conversation

@venkata-stepsecurity

Copy link
Copy Markdown
Collaborator

Problem

On a runner with heavy network traffic, most audit events are sends on connected sockets: a sendto/sendmsg with no destination address, so there is no SOCKADDR record. The agent parses every record of every event into a map (go-libaudit ToMapStr), then drops these events in isEventReady because they have no address.

A CPU profile of the agent on a workload streaming many small sends over localhost TCP showed:

  • 62% of agent CPU in ToMapStr;
  • 43% in go-libaudit's regex field parser alone;
  • 14% in netlink receive;
  • 14% in GC.

Change

auditStream.ReassemblyComplete checks 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

  • Unit tests:
    • hasDetectionRecord for each event shape;
    • a check that representative SYSCALL/PROCTITLE-only events (send, connect, execve and file syscalls, hex-encoded names) parse cleanly and are not ready when fully parsed;
    • a test that fails if the skip is removed (verified by removing it).
  • Existing reassembly and detection tests unchanged and passing.
  • go mod vendor, go mod verify, go vet and go test -race on Linux with Go 1.27.3. TestRun and Test_addAuditRules fail in a container on both this branch and int, because they need a real runner.
  • End to end on ubuntu-latest, 480 s of the same workload, against the int build at 2235fe6:
int 2235fe6 this branch
workload tests/min 250 980
agent CPU 501 CPU-s (104% of a core) 198 CPU-s (41%)
kernel backlog wait time actual 1,156,139 ms 50,382 ms
kernel lost records 0 0
agent peak RSS 51 MiB 58 MiB

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.

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.

@step-security-bot-int step-security-bot-int left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 like populateEventFromMsg(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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants