Repository navigation
fix(procmon): group audit records with go-libaudit's reassembler - #503
Merged
Merged
Conversation
Audit events were kept in ProcessMonitor.Events forever. send() on a connected socket is audited as sendto without an address, so those events never became ready and were never released, and the agent grew by about 1.4 GiB per minute on a localhost-streaming workload (harden-runner#709). Under the same load the netlink socket overflowed (ENOBUFS), receive returned and audit monitoring stopped for the rest of the job. Use go-libaudit's Reassembler, which Auditbeat uses, with Auditbeat's defaults: at most 50 events in flight, a 2 second timeout and maintenance every 500 ms. Every event is delivered once, complete or evicted, so nothing is kept after delivery. The first qualifying PATH is kept, as before. Like Auditbeat, receive stops only when the client is closed: other errors, including ENOBUFS, are counted and receiving continues, and records with an invalid header are dropped. Sequence gaps and records whose fields cannot be parsed are counted and logged on powers of two. Bump go-libaudit from v2.3.2 to v2.6.2: v2.3.2 panics in parseSockaddr on a SOCKADDR record shorter than expected (elastic/go-libaudit#152), which stops the agent. This replaces the event map and idle sweep from #501, reverted in the previous commit.
step-security-bot-int
left a comment
step-security-bot-int
left a comment
There was a problem hiding this comment.
Please find StepSecurity AI-CodeWise code comments below.
Code Comments
go.mod
- [High]Pin dependencies to fixed patch versions to avoid introducing breaking changes
Upgrading the dependency github.com/elastic/go-libaudit/v2 from v2.3.2 to v2.6.2 skips multiple patch versions which could introduce breaking changes or new vulnerabilities. According to OWASP dependency management best practices, it is safer to upgrade incrementally and pin to exact versions to ensure compatibility and security. Update the go.mod to explicitly require the precise desired patch version after testing each upgrade incrementally, e.g. v2.3.3, v2.4.x, etc., before moving to v2.6.2, and perform thorough testing before committing to the latest version. - [Medium]Validate the compatibility of updated third-party libraries through integration tests
Upgrading third-party dependencies without verification may cause integration issues or runtime failures. This is aligned with the principle from the Google Testing Blog and semantic versioning guidelines emphasizing the need for testing when upgrading dependencies. Develop or enhance existing integration tests that cover functionalities dependent on github.com/elastic/go-libaudit/v2, and run them after the dependency upgrade to detect any incompatibility early. - [Medium]Check for known vulnerabilities in the updated dependency versions before upgrading
According to NIST SP 800-161 and OWASP Dependency-Check recommendations, all dependencies, especially those with elevated priviledge like libaudit, should be scanned for known vulnerabilities before use. Use a dependency vulnerability scanner (e.g., OWASP Dependency-Check, Snyk) against version v2.6.2 of github.com/elastic/go-libaudit/v2 and evaluate results prior to upgrading. - [Low]Document dependency upgrades in changelogs or commit messages
Maintaining a detailed changelog for dependencies is a documented best practice from the Keep a Changelog guideline. It improves traceability and auditing for security and maintenance purposes. Add a changelog entry or update the commit message describing this specific dependency upgrade and the reasons for it. - [Low]Use semantic versioning care when specifying dependency updates in go.mod
Go modules support semantic versioning whereby patch-level upgrades are generally backward compatible. Being explicit about minor and patch version changes avoids unintentional upgrades that may break compatibility. This is recommended by the official Go Modules documentation. Ensure the version upgrade in go.mod follows semantic versioning rules and explicitly specifies the full semantic version number without using wildcards or ranges.
go.sum
procmon.go
- [High]Avoid shared mutable state without proper synchronization or use atomic operations
The original code used a map (Events) protected by a mutex to handle concurrency. The new code removes the mutex and shared map, using a streaming approach instead. This prevents data races. Ensuring thread-safe access to shared mutable state is crucial; improper synchronization leads to race conditions and data corruption (see Go race detector and Effective Go concurrency principles). Ensure that either synchronization primitives like mutexes or atomic operations protect shared mutable state. In this new design, the auditStream receives complete events from the reassembler and dispatches them, which avoids shared mutable state in ProcessMonitor, adhering to concurrency best practices. - [High]Use atomic counters for metrics updated concurrently
The updated code replaces integer metrics fields such as receiveOverflow, sequenceGaps, etc., with atomic.Int64 variables. Using atomic operations avoids races and false sharing, providing safe concurrent increments and reads (per Go's sync/atomic package guidelines and concurrency patterns). Use atomic variables (atomic.Int64) and atomic methods like Add and Load for shared counters accessed by multiple goroutines. - [Medium]Log loss events on powers-of-two increments to prevent flooding logs during sustained loss
The noteLoss function increments a counter and logs only when the count reaches a new power of two. This reduces log spam during continuous loss conditions, which helps maintain log usefulness and performance (logging best practices recommend throttling high-frequency logs). Implement noteLoss function as shown using bits.Len64 to detect power-of-two increments and log only at those increments. - [Medium]Check for unrealistic or invalid counts before processing sequence gaps
In EventsLost, counts less than or equal to zero or extremely large values (likely due to underflow) are ignored. This prevents erroneous metrics updates and false alarms (defensive programming principle). Add input validation checking that count is positive and less than 1<<24 before updating counters in EventsLost. - [Medium]Group related constants using iota or grouped const blocks with explicit types
The new constants for the reassembler (MaxInFlight, Timeout, Maintain) are in a grouped const block with proper types and descriptive comments. This improves readability and maintainability (Go Lint and Effective Go style). Define related constants in a single block with appropriate types and explicit naming. - [Medium]Replace complex map key access with helper functions for better readability and maintainability
The getValue helper function wraps repeated map lookup code, simplifying the main code and reducing duplication, following DRY principles and improving code clarity. Use getValue(key string, eventMap map[string]interface{}) string helper to abstract map lookups. - [Low]Use structured comments for exported types and functions
The updated code has comments explaining new structs and functions in idiomatic Go style, which helps tools like godoc and improves developer understanding. Add comments above type and function declarations describing their behavior and intended usage. - [Low]Remove unnecessary locking when state is immutable or confined to a single goroutine
The removal of mutex.Lock()/Unlock() around event modifications is safe in context of the new design where events are constructed within a stream of messages passed synchronously. Avoiding unnecessary locks reduces complexity and potential deadlocks. Update addRecord method to operate on event data without locking since no concurrent access occurs at this stage. - [Low]Use consistent naming conventions and formatting for struct fields
The code reformat for Event struct fields aligns naming conventions and grouping, improving readability and maintainability consistent with Go standards. Align struct field definitions and group related fields logically. - [Low]Avoid unnecessary panics or logging in library functions; communicate errors via return values when feasible
The code uses noteLoss to log errors about dropped messages but does not panic, promoting robustness and error handling best practices. Use counters and controlled logging for error conditions without panicking.
procmon_linux.go
- [High]Avoid launching goroutines without limits or context cancellation
The original code launches a new goroutine for each event handled (go eventHandler.HandleEvent(event)) without limits or coordination, potentially leading to excessive resource consumption. A more controlled concurrency approach, such as using worker pools or bounded goroutine spawning, is recommended as per Go concurrency best practices described in 'Concurrency Patterns' (https://go.dev/doc/effective_go#concurrency). Refactor event handling to use a worker pool or bounded channel to control the number of concurrent goroutines, or pass a context for cancellation to cleanly shut down processing. In the given patch, this is partially addressed by using a dispatch function (possibly overridable), which can be implemented as a controlled queue internally. - [High]Handle errors robustly and avoid ignoring critical failures
The patch changes error handling to specifically check for syscall.EBADF (bad file descriptor) to return an error, and treats other errors with a delay and retry except ENOBUFS, demonstrating improved and explicit error classification. This matches best practice to handle transient vs fatal errors separately (https://blog.golang.org/errors-are-values). Continue to clearly differentiate error cases, and ensure permanent errors cause function termination while transient errors are retried with appropriate backoff. The patch correctly implements this with selective handling and 10ms sleep on other errors. - [High]Properly manage lifecycle of background tasks with synchronization
The patch introduces a background goroutine running maintainReassembler() and uses a WaitGroup plus a done channel to ensure graceful shutdown and cleanup. This aligns with best practices for goroutine lifecycle management described in Effective Go and Go concurrency patterns (https://go.dev/blog/pipelines). Keep the use of done channels and WaitGroups to synchronize background goroutines, and ensure that defer closes channels and waits appropriately to avoid goroutine leaks. - [Medium]Avoid tight loops without delays when handling temporary errors to prevent CPU spinning
The patch adds a 10ms delay (time.Sleep(10 * time.Millisecond)) when encountering non-ENOBUFS errors to prevent busy waiting loops. This demonstrates applying backoff to retry loops, minimizing CPU usage under error conditions (https://www.oreilly.com/library/view/go-programming-language/9780134190570/ch13.html). Maintain this delay or consider an exponential backoff approach when handling transient errors to improve resource efficiency. - [Medium]Use explicit error wrapping and unwrapping to preserve context
The patch uses errors.Wrap and errors.Is from the pkg/errors or Go 1.13 style errors package, improving error traceability and checking. This practice is recommended in the official Go error handling proposal (https://go.dev/blog/go1.13-errors). Continue wrapping errors when returning, and use errors.Is and errors.As to detect error types instead of comparing raw error strings. - [Medium]Encapsulate complex resource cleanup logic in defer functions to improve reliability
Using a defer function to close the done channel, wait on the WaitGroup, and close the reassembler reduces the chance of resource leaks, following Go’s idiomaticdeferusage for cleanup as per Effective Go (https://golang.org/doc/effective_go#defer). Maintain the defer-based coordinated cleanup approach for all opened resources and background tasks. - [Low]Improve clarity by removing commented-out code and unused legacy code
The original code had a comment and unused code for direct event handling and map initialization which the patch replaces with a cleaner reassembler-based approach. Removing leftover dead code and commentary improves maintainability and clarity, a principle advocated by Clean Code (Robert C. Martin). Remove legacy commented code completely once replaced by new logic to keep the codebase clean and easier to understand. - [Low]Use well-defined constants instead of magic numbers for time durations and limits
The patch introduces constants (reassemblerMaxInFlight,reassemblerTimeout,reassemblerMaintain) to configure reassembler behavior, improving readability and maintainability, consistent with Clean Code principles (https://clean-code-developer.com/blog/7-good-practices-in-golang). Define and use named constants for durations and limits so that they can be easily tuned and understood. - [Low]Avoid performing blocking operations directly in hot loops where possible
By using a reassembler to buffer and process events asynchronously, the patch reduces the amount of blocking or CPU-intensive processing done inline in the receive loop, aligning with efficient IO handling practices. Structure code to delegate heavier or blocking work to separate components with buffering or concurrency controls. - [Low]Provide clear comments explaining concurrency and error handling logic
The patch adds comments describing the behavior and purpose of the maintainReassembler function and error handling rationale, improving code comprehensibility, which is recommended in effective software engineering practices. Continue to add thoughtful comments for complex concurrency and error handling code paths to aid future maintainers.
procmon_linux_test.go
- [High]Use synchronization primitives to protect shared state in concurrent tests
The test functions dispatch events from multiple goroutines. To avoid race conditions and ensure memory visibility, shared mutable state must be protected using sync.Mutex or other synchronization mechanisms. The Go race detector and official Go concurrency best practices recommend this to ensure data consistency. Use a mutex to guard access to the 'dispatched' slice and any other shared variables modified concurrently, as done by adding a sync.Mutex 'mu' to lock/unlock when modifying 'dispatched' or other shared variables. - [High]Check and assert all relevant side effects in tests for correctness
Tests should fully assert all expected state changes, including counts of handled errors, dropped records, and dispatched events, to prevent silent failures and regressions. This aligns with best practices in testing from sources such as 'Testing in Go' by Dave Cheney. Add assertions for counts such as receiveOverflow, receiveErrors, invalidRecords, and the contents and length of dispatched events, like in the updated test where p.receiveOverflow.Load(), p.receiveErrors.Load(), p.invalidRecords.Load(), and dispatched events are checked. - [Medium]Use time-bound polling in tests to verify asynchronous conditions
When testing asynchronous event delivery or concurrent processes, polling with a timeout helps avoid hanging tests and flaky results. This is advised by Go testing best practices to wait for expected conditions with deadlines to keep tests deterministic. Use loops that periodically check the condition with a timeout boundary (e.g., using time.Now plus time.Sleep) to wait for asynchronous callbacks or events, as seen in the improved tests that wait for events to be dispatched up to a deadline. - [Medium]Add detailed failure messages to test assertions
Tests with clear, descriptive failure messages improve debugging efficiency by giving precise information about what failed and expected versus actual values. Effective test failure messages are recommended by testing documentation across languages. Ensure calls to t.Fatalf and similar include clear messages with expected and actual values, e.g., 'expected 2 overflows counted, got %d', 'expected [79 80 81] after the timeout, got %v', as done in the diff. - [Medium]Separate concerns via naming and structuring for clarity in tests
Tests should be clearly named to reflect their purpose and behavior, in line with Go testing conventions and documentation guidelines. Clear test names improve understanding and maintenance. Rename tests to more descriptive names that explain their intent, e.g., from TestReceiveContinuesAfterOverflow to TestReceiveKeepsGoingUntilTheClientIsClosed, emphasizing when the receive loop terminates. - [Low]Return expected sentinel errors for specific control flows
Tests rely on specific errors to detect termination conditions correctly. Returning expected sentinel errors like syscall.EBADF on client closure or blocking simulation helps tests verify correct stopping behavior. This aligns with Go error handling best practices. Use the injected channel blocking in fakeNetlinkStep to simulate client closure by returning syscall.EBADF in Receive when 'step.block' is closed, as implemented in the patch. - [Low]Add comments for non-obvious test logic and special conditions
Detailed comments explaining reasons for unusual or intermediate test logic help future maintainers understand test intentions and expected behaviors, aiding long-term maintainability. Recommended in effective code documentation guidelines. Add comments before complex test sections describing purpose, such as why certain messages are blocked or why timeouts are used to deliver events, as done in the improved tests. - [Low]Use explicit channel closing to signal event completion in tests
Using channels to signal and synchronize test progress steps is a good practice in concurrent tests, allowing better control over asynchronous operations and preventing race conditions. Add channels like 'release', 'inCallback', and 'finishCallback' to coordinate test steps and signal transitions, as shown in the patch. - [Low]Use fmt.Sprintf or formatting functions to construct dynamic test data
Using formatted strings for dynamic test messages improves readability, flexibility, and maintainability, following Go idioms for clean test code. Use fmt.Sprintf to construct audit records with sequence numbers and fields substituting variable values instead of hard-coded strings, as applied in the patch. - [Low]Implement additional helper functions to reduce duplication in test setup
Extracting repetitive patterns into helper functions reduces code duplication and clarifies intent in test setup and teardown, improving maintainability, as advised by testing best practices. Create helper functions like 'connect' that generate repeated sets of fakeNetlinkSteps with parametrized inputs, minimizing duplication as shown.
procmon_test.go
- [High]Use sync.Mutex consistently to protect shared state in recordingDispatch
The recordingDispatch struct uses a sync.Mutex to protect its events slice, which is correct. This follows the Go concurrency best practices documented in https://golang.org/doc/effective_go#concurrency. Consistent locking ensures safe concurrent access. No code change needed; just ensure the current pattern (locking in dispatch and dispatched methods) remains consistent. - [High]Avoid race conditions in ProcessMonitor when accessing Events map during event preparation and sweeping
The original code used a map[int]*Event in ProcessMonitor likely without external synchronization. The new tests and usage show concurrent access (e.g., PrepareEvent, and Maintain operations). According to 'Go Memory Model' (https://golang.org/ref/mem) and Go's race detector guidelines, access to shared mutable maps must be synchronized to avoid data races. Wrap accesses to ProcessMonitor.Events with sync.Mutex or sync.RWMutex or convert Events to a concurrent-safe structure like sync.Map. Without this, concurrent event handling can cause race conditions or panics. - [High]Handle error cases clearly when parsing critical fields like socket addresses
The code attempts to parse sockaddr fields; bad or incomplete input should be logged and counted but must not cause panics or incorrect event dispatch. This adherence to robust input validation is in line with OWASP Input Validation Cheat Sheet (https://cheatsheetseries.owasp.org/cheatsheets/Input_Validation_Cheat_Sheet.html) and Go best practices for error handling (https://go.dev/doc/effective_go#error_handling). Continue to increment the recordErrors counter on field parsing failures and prevent event dispatch if critical fields are not valid, as already implemented in tests like TestReassemblyHandlesShortSocketAddresses. - [Medium]Use reflect.DeepEqual only when necessary and clearly document intent
In TestReassemblyDispatchesDetectionsOnce, reflect.DeepEqual is used to compare slices for equality, which is correct but can be inefficient or cause issues if fields change. According to Effective Go (https://go.dev/doc/effective_go#comparisons) and community best practices, prefer explicit comparisons or testify/assert packages for tests. Consider replacing reflect.DeepEqual in tests with explicit slice comparisons or testing frameworks' assertion helpers for better clarity and maintenance. - [Medium]Ensure all newly added tests include proper cleanup and verify no goroutine leaks
Tests that spin up components like libaudit.Reassembler should verify cleanup to prevent resource leaks and impact in test suites. This aligns with Go testing best practices described in 'Testing' (https://golang.org/pkg/testing/) and general resource management principles. Explicitly call r.Close() in all tests that create a libaudit.Reassembler instance, as currently done in TestReassemblyDispatchesDetectionsOnce, and verify no leftover goroutines or resources remain. - [Medium]Use typed constants where appropriate to improve code maintainability and readability
Constants such as reassemblerMaxInFlight and reassemblerTimeout are used but not shown as typed constants in the snippet. Typed constants help catch type errors and clarify intent (https://go.dev/doc/effective_go#constants). Define constants with explicit types (e.g., 'const reassemblerMaxInFlight int = 50') if not already done, to increase type safety and clarity. - [Low]Improve error message consistency and clarity in tests
Test error messages should be consistent and explicit for easier debugging and traceability. This principle is suggested in Go testing guidelines (https://blog.golang.org/examples) and general testing practices. Uniformly use either t.Fatalf or t.Errorf with clear messages including actual vs expected values. Also consider including the test case name for easier identification. - [Low]Avoid commented out or dead code to improve readability
The diff shows some commented-out legacy code in tests. Removing or cleaning these out improves readability and reduces confusion. This aligns with Clean Code principles (Robert C. Martin) and Effective Go recommendations. Delete commented-out code blocks or legacy code snippets when they are no longer necessary or replaced by new test logic. - [Low]Add comments to clarify the purpose and expected state after complex test scenarios
Several tests perform non-trivial sequences of audit record injections. Adding comments about expected states helps maintainers understand the intent and avoid regressions. This practice follows Effective Go's advice on commenting code appropriately. Add brief comments before key test steps describing what is being tested and expected results, especially in tests like TestReassemblyDropsSendWithoutAddress and TestReassemblyGivesUpOnEventsAfterTimeout. - [Low]Prefer using table-driven tests with clear naming for subtests for better scalability and clarity
The use of t.Run and table-driven tests is good, but adding descriptive subtest names especially for parameterized inputs (like in TestReassemblyHandlesShortSocketAddresses) aids diagnostics and future additions. This is recommended in Go testing patterns (https://github.com/golang/go/wiki/TableDrivenTests). Use descriptive fmt.Sprintf subtest names that include input parameters clearly in TestReassemblyHandlesShortSocketAddresses and other similar tests.
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.
go-libaudit v2.6.2 pins github.com/elastic/go-licenser in rule/tools.go behind the tools build tag. It is not built into the agent, but go mod vendor needs its checksum.
step-security-bot-int
left a comment
step-security-bot-int
left a comment
There was a problem hiding this comment.
Please find StepSecurity AI-CodeWise code comments below.
Code Comments
go.sum
- [High]Avoid using dependency versions with known security vulnerabilities
It is important to regularly update dependencies to versions that have security patches applied. Using outdated dependencies can expose your project to known vulnerabilities. This aligns with the OWASP Dependency-Check best practice to use up-to-date libraries to minimize risk. Review the changelogs of all updated dependencies and verify that the versions used do not contain known security issues. If a newer secure version is available, update go.mod to point to those versions instead of the intermediate ones. Ensure to remove multiple versions to avoid conflicts. - [High]Keep dependencies in go.mod consistent and avoid redundant multiple version entries
Having multiple versions of the same module in go.mod increases the risk of dependency confusion and maintenance overhead. The Go Modules wiki recommends keeping dependencies tidy and consolidated to one version per module. Consolidate dependency versions so that only one version per module is referenced in the go.mod file. For example, instead of having "golang.org/x/mod" at versions v0.4.2 and v0.5.1 simultaneously, keep only the latest stable version required. - [Medium]Use explicit semantic versioning for clarity
Specifying explicit semantic version tags for dependencies instead of commit hashes or pseudo-version can improve maintainability and traceability. Semantic versioning is widely recommended in Go modules (per golang.org docs) as best practice. Change module references in go.mod to use explicit semantic versions (e.g., v1.4.0) rather than pseudoversions or commit hashes to clarify the exact dependency versions and simplify upgrades. - [Medium]Regularly clean up unused dependencies
Unused dependencies can lead to bloated binaries and increased attack surface. The official Go Modules documentation encourages running 'go mod tidy' to remove unused dependencies. Run 'go mod tidy' to automatically prune unused dependencies from go.mod and go.sum, keeping dependency management clean and efficient. - [Medium]Ensure compatibility of updated dependencies
Upgrading dependencies without verifying compatibility may break the build or runtime behavior. Semantic Versioning principles recommend testing compatibility when major versions update. After updating versions in go.mod, run full test suites and validation to confirm the updated dependencies do not break functionality. - [Low]Pin module versions to avoid unintentional upgrades
Explicitly pinning dependency versions in go.mod ensures reproducible builds. The Go Modules design encourages pinning versions for this reason. Use explicit versions without floating tags or ranges, to ensure builds are deterministic. - [Low]Add comments about why specific dependencies were updated or added
Adding context in go.mod or associated documentation improves maintainability and understanding for future maintainers. This is a common best practice in software engineering for documentation clarity. Add comments near updated dependencies to document reasoning, such as security patches addressed or feature additions. - [Low]Verify licenses of added dependencies comply with project policies
The addition of new dependencies should be reviewed against project licensing policy to avoid legal issues. This aligns with best practices from the Open Source Initiative and legal compliance guidelines. Check licenses of newly added dependencies (e.g., elastic/go-licenser v0.4.1) and confirm compatibility with project licensing requirements.
procmon.go
- [High]Avoid data races by using proper synchronization for shared mutable state
The original ProcessMonitor struct used a map and mutex to manage events with multiple goroutines, but in the refactored version, atomic counters are used without mutex for other fields. Ensure that all shared mutable state accessed concurrently is either made thread-safe by using atomic operations or protected by mutexes. Without proper synchronization, data races and undefined behavior can occur. This aligns with Go's official documentation on concurrency and data race prevention (https://golang.org/doc/articles/race_detector). Review all shared mutable state in ProcessMonitor and use sync.Mutex or sync.RWMutex for complex data structures like maps. Continue to use atomic.Int64 for simple counters. For example, protect the Events map with a mutex if it remains in use or redesign to avoid concurrent writes. - [High]Properly handle large or invalid input values to prevent erroneous application states
In the EventsLost function, a count greater than 1<<24 is silently ignored as invalid to avoid underflows, per comment. Skipping without logging or alerting could hide issues. According to robust error handling principles (https://golang.org/doc/effective_go#errors), unexpected input values should be logged or handled to aid debugging. Add logging or warnings when the count parameter is out of expected range to highlight potential issues. For example, log a warning for counts > 1<<24 to indicate possible data inconsistency. - [Medium]Avoid accumulation of large in-memory data structures without bounds
The original code kept events in a map with a pendingEventWindow bound to limit memory. In the refactor, events are processed immediately from the reassembler, but if upstream sources produce many events without EOE, that may cause memory growth. According to best practices in Go and event processing systems (https://www.oreilly.com/library/view/go-programming-blueprints/9781786468949/), it's important to bound resource usage for robustness. Implement limits or timeouts on event buffering even after refactoring to ensure memory use cannot grow without bound. Drop or process stale events aggressively. - [Medium]Use consistent and clear logging level strategies to avoid log flooding
NoteLoss logs when counters reach a new power-of-two to avoid log flooding, which aligns with best practices for monitoring under high-frequency error conditions (https://cloud.google.com/architecture/best-practices-for-logging-in-go#avoid_flooding). This is good, but ensure that this policy is applied consistently across all counters and situations. Continue using the power-of-two logging threshold mechanism systematically and document it clearly as a logging policy for all loss or error counters. - [Medium]Document concurrency guarantees and expectations clearly for public and internal methods
The refactored auditStream type processes audit events; methods like ReassemblyComplete are intended to be called concurrently or serialized? According to Go conventions (Effective Go) and GoDoc best practices, clarifying concurrency assumptions in comments helps maintainers avoid misuse and subtle bugs. Add comments to public types and methods (e.g., auditStream.ReassemblyComplete) describing if they are safe for concurrent use or require external synchronization. - [Low]Avoid unnecessary type assertions and conversions for improved code clarity and performance
In addRecord, the code formats eventMap values to string using fmt.Sprintf("%v", nameType). A type switch or assertion may be clearer and possibly more efficient (https://golang.org/ref/spec#Type_assertions). Replace fmt.Sprintf calls with direct type assertions or conversions when the expected type is known, improving clarity and performance. - [Low]Use idiomatic Go naming conventions consistently across the codebase
Struct field names like SentForProcessing use mixed capitalization. While it is idiomatic, ensure uniform naming (e.g., SentForProcessing vs. SentForProcessing) across the project, and possibly use abbreviations only when clear. This aligns with Effective Go naming guidelines (https://golang.org/doc/effective_go#names). Review and refactor field and method names to ensure consistency and idiomatic style throughout. - [Low]Remove commented out or unused code to keep the codebase clean
The patch removes several pieces of commented and unused code like the Events map and related locking. Keeping such dead code can confuse developers and lead to errors (https://go.dev/blog/organizing-go-code#clean). Fully remove any commented out code rather than leaving it as comments to maintain clarity. - [Low]Add unit tests covering new or refactored functionality to ensure correctness
Code refactoring modifies how audit events are processed and dispatched. Testing best practices recommend adding or updating unit and integration tests to verify behavior (https://golang.org/doc/code#Testing). Implement or update unit tests for auditStream methods (e.g., ReassemblyComplete, EventsLost) and ProcessMonitor methods to prevent regressions. - [Low]Add usage comments for exported types and functions to improve code maintainability
Some public types and functions like auditStream and noteLoss have limited explanatory comments. Documenting their purpose following GoDoc conventions enhances readability and maintainability (https://blog.golang.org/godoc-documenting-go-code). Add descriptive comments on exported symbols explaining their responsibilities and usage.
procmon_linux.go
-
[High]Avoid launching goroutines without tracking their lifecycle or error handling
The original code launched a goroutine per event with 'go eventHandler.HandleEvent(event)' without any control. According to Effective Go and concurrency best practices (Go blog: 'Go Concurrency Patterns'), untracked goroutines can leak or cause race conditions. The new approach wraps event handling inside a dispatch function that can be customized but still launches in a goroutine by default. This enables better control and extensibility. Use a dispatch function that can be replaced or managed. For example:dispatch := p.dispatchEvent
if dispatch == nil {
dispatch = func(event *Event) { go eventHandler.HandleEvent(event) }
}
Pass the events to this dispatch instead of calling goroutines directly from the 'receive' method.
-
[High]Use a reassembler for audit messages to handle multi-part events and maintain event integrity
The patch adds 'libaudit.NewReassembler' and pushes raw audit messages into the reassembler instead of parsing each message individually. This follows libaudit's recommended approach for processing audit events that may be fragmented. This prevents misinterpretation or loss of multi-part audit events as outlined in Elastic's Auditbeat implementation. Create a reassembler and push raw messages to it:reassembler, err := libaudit.NewReassembler(maxInFlight, timeout, handler)
if err != nil {
return err
}
defer reassembler.Close()for {
rawEvent, err := r.Receive(false)
if err != nil {
// handle error
}
err := reassembler.Push(rawEvent.Type, rawEvent.Data)
if err != nil {
// handle error
}
} -
[High]Implement maintenance of the reassembler to drop expired or stalled events regularly
The patch adds the 'maintainReassembler' function which periodically calls 'Reassembler.Maintain()' to give up on events that never arrive completely in time. This is analogous to Auditbeat's logic. Without this, incomplete audit events could block processing indefinitely. This approach aligns with robust event streaming system best practices. Introduce a goroutine that ticks periodically and calls 'reassembler.Maintain()' until stopped:func maintainReassembler(r *libaudit.Reassembler, done <-chan struct{}) {
ticker := time.NewTicker(reassemblerMaintain)
defer ticker.Stop()
for {
select {
case <-done:
return
case <-ticker.C:
_ = r.Maintain()
}
}
} -
[High]Handle audit receive errors more precisely, differentiating between recoverable and terminal errors
The patch refines error handling on Receive by returning immediately on EBADF (bad file descriptor) error which indicates closed client, but for other errors, such as ENOBUFS, it logs and continues. The prior code only handled overflow errors. This aligns with syscalls and error handling best practices to improve robustness (Go syscall docs). In the receive loop, handle errors like:if errors.Is(err, syscall.EBADF) {
return errors.Wrap(err, "receive failed")
}
p.noteReceiveError(err)
if !errors.Is(err, syscall.ENOBUFS) {
time.Sleep(10 * time.Millisecond)
}
continue -
[Medium]Use synchronization primitives to safely manage goroutine lifecycles and resource cleanup
The patch introduces a sync.WaitGroup to manage the 'maintainReassembler' goroutine lifecycle, ensuring it finishes before the 'receive' function returns and resources are closed. This avoids race conditions during shutdown. This is consistent with Go concurrency best practices (Go sync package documentation). Use a WaitGroup like:var maintaining sync.WaitGroup
maintaining.Add(1)
go func() {
defer maintaining.Done()
maintainReassembler(reassembler, done)
}()defer func() {
close(done)
maintaining.Wait()
reassembler.Close()
}() -
[Medium]Avoid global variables or uninitialized maps by initializing required maps in the function scope carefully
The original code commented out 'p.Events = make(map[int]*Event)', implying potential removal without clear replacement. Instead, the patch initializes multiple maps inside 'eventHandler'. This ensures maps are controlled and scoped properly, reducing risk of nil map panics or concurrency issues, following effective Go initialization best practices. Initialize maps like:eventHandler.ProcessConnectionMap = make(map[string]bool)
eventHandler.ProcessFileMap = make(map[string]bool)
eventHandler.SourceCodeMap = make(map[string][]*Event)
eventHandler.ProcessMap = make(map[string]*Process)
eventHandler.FileOverwriteCounterMap = make(map[string]int) -
[Medium]Use constants or variables for magic numeric values to improve maintainability
The code uses ranges like 1300-2999 for valid audit messages and time.Sleep(10 * time.Millisecond) hardcoded. These should be defined as constants with meaningful names, which aids readability and maintainability per Go style guidelines. Define constants:const (
minAuditMessageType = 1300
maxAuditMessageType = 2999
sleepOnErrorDuration = 10 * time.Millisecond
)// then use these in conditions and sleeps
-
[Low]Use error wrapping consistently with descriptive messages for better diagnostics
The patch replaces error returns with 'errors.Wrap' and adds context like 'create reassembler' or 'receive failed'. According to Dave Cheney's error handling best practices, adding context improves troubleshooting. Wrap errors when returning, e.g.:return errors.Wrap(err, "create reassembler")
return errors.Wrap(err, "receive failed")
-
[Low]Use defer to ensure resources like ticker.Stop() to avoid resource leaks
In maintainReassembler, the patch defers ticker.Stop(), following Go best practices to ensure that resources are released when no longer needed (Go documentation on time.Ticker). Use:defer ticker.Stop()
-
[Low]Centralize logging or counters for dropped or invalid records to facilitate monitoring
The patch marks invalid or dropped audit messages with 'noteLoss' along with reasons. Monitoring such metrics follows security best practices (OWASP Logging Guidance) to help detect issues in event processing pipelines. Add calls like:noteLoss(&p.invalidRecords, 1, fmt.Sprintf("dropped audit record with an invalid header: %v", err))
Maintain counters or logs in a thread-safe manner.
procmon_linux_test.go
- [High]Avoid Data Race on Shared Variables in Concurrent Code
The test code appends to a shared slice (such as 'dispatched' and 'pids') concurrently from different goroutines without adequate synchronization. According to the Go Memory Model (https://golang.org/ref/mem), unsynchronized access to shared variables leads to data races and undefined behavior. Use a mutex (sync.Mutex) to guard all accesses (reads and writes) to shared variables. The code already uses a mutex 'mu' for appending 'dispatched' and 'pids'; ensure that any read access (e.g., in tests) also acquires the mutex to avoid race conditions. - [High]Use Context or Channel-based Cancellation Instead of Blocking Receives Without Timeout
The method 'Receive' blocks on 'step.block' channel indefinitely without a timeout or context cancellation. This can cause unit tests to hang if the release channel is not closed. According to idiomatic Go concurrency patterns (https://blog.golang.org/context), long blocking operations should be interruptible. Add a timeout or use a context with cancellation to the blocking receive on 'step.block' to avoid potential indefinite blocking during tests or runtime. - [Medium]Check Errors with 'errors.Is' for Comparisons Instead of Direct Equality
The tests compare errors directly to specific syscall errors like 'syscall.EBADF' and 'syscall.ENOBUFS'. Per best practices in Go error handling (https://blog.golang.org/go1.13-errors), use 'errors.Is' to unwrap and compare errors instead of direct equality which may fail with wrapped errors. Replace direct error comparisons with the standard 'errors.Is' function to ensure proper error identification. - [Medium]Avoid Hardcoding Magic Numbers and String Literals in Tests
There are many hardcoded values such as audit message strings, sequence numbers, and timeouts in the tests. Per clean code practices (Robert C. Martin, 'Clean Code'), magic numbers reduce readability and maintainability. Define constants or variables for repeated literals like timeouts, audit sequence numbers, and common strings to improve readability and facilitate future updates. - [Medium]Use Table-Driven Tests for Repetitive Test Scenarios
The tests have multiple similar scenarios to test connectivity events with small differences. Per Go testing best practices (https://dave.cheney.net/2019/05/07/prefer-table-driven-tests), table-driven tests reduce code duplication and improve clarity. Refactor the multiple connect scenarios into a table-driven test with clearly labeled cases to reduce duplication. - [Low]Use Descriptive Variable Names for Channels
Channels like 'release' and 'finishCallback' are not descriptively named, which can confuse maintenance developers. Effective naming improves code understandability (https://golang.org/doc/effective_go#names). Rename channels to more expressive names reflecting their purpose, such as 'blockReleaseChan' or 'callbackDoneChan'. - [Low]Include Comments Explaining Complex Test Logic
The test code involves intricate synchronization and non-obvious timing-dependent behaviors but lacks sufficient explanatory comments. Good commenting, as stated in Effective Go, helps future maintainers. Add clear comments explaining why certain waits, channel closes, and ordering are necessary in the tests. - [Low]Close Channels to Signal End of Transmission
In some tests, channels such as 'release' are closed to unblock receivers, which is good. In others, similar synchronization might benefit from channel closing as an explicit signal instead of other methods. According to Go idioms, closing channels is a standard way to broadcast completion. Ensure all channels used for signaling completion are closed rather than relying on sending dummy values or other mechanisms. - [Low]Avoid Sleeping in Tests to Wait for Asynchronous Events
The tests use sleeps (e.g., time.Sleep(10 * time.Millisecond)) in polling loops which can make tests flaky or slow. Effective Go suggests using synchronization primitives or channels instead of sleeping. Use synchronization mechanisms such as channels with timeouts or WaitGroups to wait for asynchronous test events instead of polling with sleep. - [Low]Use Test Helper Functions to Reduce Boilerplate
Testing code has repeated setup patterns and repeated code blocks. Per Go testing best practices and to improve maintainability, helper functions should be used to reduce duplication. Extract common setup and repeated steps into reusable test helper functions.
procmon_test.go
- [High]Avoid data races by protecting shared mutable state
The recordingDispatch struct uses a mutex to protect access to its events slice. This is a correct practice following the Go Memory Model and the authoritative Go blog on concurrency (https://go.dev/blog/share). This pattern should be consistently applied to all shared mutable state to avoid potential race conditions. Continue using the mutex in recordingDispatch struct as implemented. Review other mutable shared states for proper synchronization and apply similar locking as needed. - [High]Use atomic operations for performance-critical counters
The countingStream uses atomic.Int64 for counting delivered events which is efficient for frequent increments and avoids lock contention. This practice aligns with the golang.org/x/sync/atomic documentation and effective concurrency patterns (https://pkg.go.dev/sync/atomic). Maintain usage of atomic.Int64 for counters such as delivered, sequenceGaps, recordErrors, receiveOverflow, and receiveErrors. Review any other counters and replace with atomic types if appropriate. - [Medium]Validate inputs thoroughly and handle parsing errors safely
In TestReassemblyHandlesShortSocketAddresses, unusual or malformed saddr fields are tested, recognizing that earlier go-libaudit versions panicked on short input (elastic/go-libaudit#152). Proper error handling and validation avoids crashes and undefined states, consistent with secure coding principles (CERT C Secure Coding Standard SEI CERT). Continue validating all inputs to network address parsing and increment error counters on failure. Ensure defensive code guards against malformed inputs and never panics. - [Medium]Avoid dispatching incomplete or invalid events
The tests ensure that events missing essential fields (like missing SOCKADDR for sendto) are not dispatched. This follows good event consistency and correctness principles, recommended in event-driven architecture best practices (https://martinfowler.com/articles/201701-event-driven.html). Keep and enforce logic where only fully prepared and valid events are dispatched downstream. Filter events lacking essential data fields appropriately. - [Medium]Ensure tests use stable and predictable timestamps for reproducibility
The tests create fixed timestamps for audit events, comparing expected values using time.Unix with hardcoded epoch seconds. Stable deterministic timestamps improve test reliability (https://testing.googleblog.com/2015/01/testing-on-toilet-stable-tests.html). Continue using fixed, deterministic timestamps in tests. Avoid external dependencies such as current time or time.Now() calls in tests unless mocked. - [Low]Write expressive test helper functions to improve readability and maintainability
Helper functions like pushRecords, newTestReassembler, syscallRecord, proctitleRecord, and eoeRecord help create readable and reusable test code, aligning with testing best practices (https://golang.org/doc/effective_go#testing). Maintain use of helper functions and extend them if repetitive patterns increase. Ensure helper functions contain clear comments and consistent naming. - [Low]Keep error counters for observability and debugging
The ProcessMonitor tracks recordErrors, sequenceGaps, receiveOverflow, and receiveErrors. Counting such events improves monitoring and observability as recommended by 'Effective Go' and well-known logging strategies (https://12factor.net/logs). Continue maintaining and incrementing counters on error conditions. Add structured logging around error increments if applicable to improve diagnostics. - [Low]Use canonical naming alignments for tests and variables
The revised code uses consistent naming like 'TestReassembly...' for tests that exercise audit reassembly and dispatch. Consistent naming improves codebase navigability and adheres to Go testing conventions (https://golang.org/pkg/testing/). Adopt consistent naming for tests, variables, and helpers throughout the codebase, making intent obvious and searchability enhanced. - [Low]Use deferred unlocking and closing to ensure resource safety
The recordingDispatch.dispatch method properly uses defer to unlock the mutex ensuring correctness even if panics occur, aligning with Go idiomatic best practices (https://golang.org/doc/effective_go#defer). Similarly, tests defer closing reassembler resources. Continue to use defer immediately after locking or allocating resources. Review code for missing defer pairs and add them to ensure proper resource cleanup in all execution paths. - [Low]Check for and ignore impossible or invalid event gaps
The implementation ignores impossible sequence gaps (e.g., zero or max uint32) to avoid incorrect metrics and logs. Input validation and sanity checks reduce false positives and aligns with robust system design principles (https://www.us-cert.gov/bsi/articles/best-practices/secure-design). Maintain validation in EventsLost to discard invalid parameters. Add comments explaining the logic to aid future maintainers.
go.mod
- [Medium]Pin dependency versions explicitly and avoid using indirect dependencies without confirmed necessity
The patch adds a new indirect dependencygithub.com/elastic/go-licenser v0.4.1. Indirect dependencies should be managed carefully because they can introduce transitive vulnerabilities or break builds if updated without explicit oversight. According to the Go Modules best practices documented by the Go team (https://blog.golang.org/using-go-modules), dependencies should be explicit when they are needed directly. Review the necessity of addinggithub.com/elastic/go-licenseras a direct or indirect dependency. If the package is needed directly, add it as a direct require entry with a specific version and import statement in the source code. Otherwise, remove it to avoid unnecessary dependencies. - [Low]Update dependency versions regularly and ensure compatibility with your codebase
The patch updatesgithub.com/elastic/go-libaudit/v2from v2.3.2 to v2.6.2. It is a good practice to keep dependencies up to date to benefit from security patches and performance improvements, in line with recommendations from the OWASP Dependency-Check project (https://owasp.org/www-project-dependency-check/). However, ensure thorough testing for compatibility before upgrading. Run regression and integration tests after upgrading dependencies to detect any breaking changes. If regression tests fail, freeze at the previously known working version or address compatibility issues accordingly.
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 was referenced Oct 30, 2026
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.
Fixes the agent memory growth in step-security/harden-runner#709, and replaces the approach from #501, which this PR reverts.
Problem
ProcessMonitor.Eventsgot an entry for every audit sequence, and nothing removed them. The audit rules include-S sendto -S sendmsgfor every process, andsend()on a connected socket is audited assendtowith noSOCKADDRrecord, so those events never became ready and stayed in the map. On a workload where short-lived processes stream over localhost TCP, the agent grew by about 1.4 GiB per minute until the runner ran out of memory.Under the same load the netlink socket overflowed (
ENOBUFS),receivereturned, and audit monitoring stopped for the rest of the job while the agent kept running.#501 fixed both with a hand-written event map that retires events on
EOEand an idle sweep. This PR reverts it and uses go-libaudit'sReassemblerinstead, which Auditbeat uses in production.Changes
libaudit.NewReassemblerusing Auditbeat's defaults: at most 50 events in flight, a 2 second timeout, andMaintain()every 500 ms. Every event is delivered once, complete or evicted, so nothing is kept after delivery. Only events the handler can use are dispatched, as before. When severalPATHrecords qualify (for example a rename), the first one is kept, as before.EBADF). Other errors, includingENOBUFS, are counted and receiving continues, with a 10 ms pause for errors other thanENOBUFS. Records with an invalid header are dropped instead of stopping monitoring. The maintenance goroutine is stopped before the final flush.parseSockaddron aSOCKADDRrecord shorter than expected (Check socket address size elastic/go-libaudit#152), which stops the agent. v2.6.2 returns an error instead, and also opens the netlink socket withSOCK_CLOEXEC. Parsing of the fields the agent uses, theReassemblerand the audit rule building are unchanged between the two versions.Testing
Reassembler:connect(IPv4 and IPv6), addressedsendto,execve, file write and rename are each dispatched exactly once with their fields; asendtowithout an address is not dispatched; 10,000 incomplete events leave at most 50 in flight; timed-out events are delivered by the maintenance goroutine whilereceiveis blocked;receivekeeps going throughENOBUFS, another error and an invalid record, stops onEBADF, and waits for a running maintenance callback before returning; short socket addresses do not panic. Each behaviour was mutation-checked against the tests.go test -race ./...on Linux with Go 1.27.3: all pass, skippingTest_addAuditRules, which needs iptables and root and also fails onint. No test outside procmon changes result.sendtoevent, as with fix(procmon): retire audit events on EOE and keep receiving on overflow #501.ubuntu-latest, with a workload of short-lived processes streaming over localhost TCP for 8 minutes:int(#501)Not covered
Agent CPU stays near one core on this workload, because every
send()is still audited and parsed. Reducing that is a separate change.