int : don't net-conn event for sinkhole-ip and enabled setting to protect agent from getting killed

[h0x0er]

No description provided.

[step-security-bot-int]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

agent.go

• [High]Avoid Evaluating Flags or Configuration Outside of Intended Context
  The variable globalFlags is assigned at the top of the Run function, but the feature flags may be dynamic and expected to be refreshed periodically or synced to reflect the current state. Reading the flags once and using it throughout can lead to stale values and unexpected behavior, violating principles of consistency (Microsoft Secure DevOps guidelines). Refactor the code to fetch globalFlags at the time of usage inside the EnforceKillBlock function or design GetGlobalFeatureFlags() to always return the current flags, possibly through a thread-safe mechanism or observer pattern. For example, inside EnforceKillBlock, call GetGlobalFeatureFlags() instead of relying on a locally cached variable.
• [High]Avoid Defining Anonymous Function Inline when it only Returns a Computed Boolean
  The use of an immediately invoked function expression (IIFE) for setting EnforceKillBlock to a boolean value is unnecessarily complex and may be confusing. This violates clean code principles described by Robert C. Martin and Go idioms (Effective Go). Replace the inline anonymous function with direct boolean expression evaluation. For example, set EnforceKillBlock: !GetGlobalFeatureFlags().DisableKillEnforcement instead of the current IIFE.
• [Medium]Avoid Global Variable Side Effects and Use Dependency Injection
  The function GetGlobalFeatureFlags() is called globally without explicit dependency injection, which can reduce testability and increase coupling. According to Go best practices and general software engineering principles (Effective Go, Clean Architecture by Robert C. Martin). Inject the global flags or a configuration provider explicitly into the Run function and pass down as needed, rather than calling a global getter function. For example, pass the flags as a parameter or encapsulate them in a context.
• [Medium]Check Error Handling after Calling External API getGlobalBlocklist
  The variable err is assigned from apiclient.getGlobalBlocklist() but never checked for errors before passing response to NewGlobalBlocklist. Ignoring errors violates robust error handling principles (Go Effective Error Handling, OWASP API Security). Add an if statement checking if err != nil after the call, log or handle the error accordingly before continuing. For example:

if err != nil {
WriteLog(fmt.Sprintf("Error fetching global blocklist: %v", err))
return err
}

• [Medium]Avoid Splitting Log Statements Unnecessarily
  There is a call to WriteLog("initialized global feature flags") followed by WriteLog("\n") which can be combined to a single call to improve log atomicity and clarity, following logging best practices (The Log: What every software engineer should know about real-time data's unifying abstraction). Replace these two calls with a single call: WriteLog("initialized global feature flags\n")
• [Low]Document Function Purpose and Side Effects Clearly
  The Run function has numerous responsibilities (initializing flags, fetching blocklists, setting enforcement policy). Lack of comments describing the high-level intent makes maintenance harder (Effective Go, Clean Code). Add explanatory comments at the start of the Run function to clarify its responsibilities and side effects.
• [Low]Use Explicit Boolean Return Rather Than Ternary-Like Return
  In the EnforceKillBlock function, the if statement could be simplified. Currently:

if globalFlags.DisableKillEnforcement {
return false
}
return true

can be replaced by return !globalFlags.DisableKillEnforcement which is more concise and less error-prone (Effective Go). Simplify the function containing EnforceKillBlock to:

EnforceKillBlock: !globalFlags.DisableKillEnforcement

• [Low]Ensure Naming Conventions Use Clear Naming
  Variable name globalBlocklistResponse may be confused for a raw HTTP response, but it’s passed directly to NewGlobalBlocklist. Naming should reflect type or usage accurately (Go style guidelines). Use a name such as rawBlocklistData or globalBlocklistData depending on the type returned by apiclient.getGlobalBlocklist().

global_feature_flags.go

• [High]Avoid using negative boolean flag names to enhance code readability and prevent misunderstandings
  Negative boolean flags like 'DisableKillEnforcement' can lead to confusion about the flag's effect, increasing the chance of introducing bugs. According to the Google Style Guide and Clean Code principles, boolean variable names should be positive and clearly convey their purpose. Rename 'DisableKillEnforcement' to a positive boolean name, such as 'EnableKillEnforcement', and invert the logic wherever it is used in the codebase to maintain the same behavior.
• [Medium]Add JSON field tags with 'omitempty' to all boolean fields that are optional to reduce payload size
  The newly added 'DisableKillEnforcement' field has the 'omitempty' tag, which is good practice to omit false values when marshaling to JSON. However, existing boolean fields like 'EnableArmour' and 'EnableCustomDetectionRules' lack this tag, potentially increasing unnecessary payload size. The Go encoding/json documentation recommends tagging optional fields with 'omitempty' to avoid sending default zero values. Add 'omitempty' to JSON tags for optional boolean fields, e.g., change EnableArmour bool json:"enable_armour"`` to EnableArmour bool json:"enable_armour,omitempty"`.
• [Low]Document the purpose and effect of new feature flags inline with code comments
  Adding a field without descriptive comments reduces code maintainability. Effective self-documenting code or comments are recommended in authoritative Go documentation and clean code guides to aid future developers in understanding the intent and usage of fields. Add a comment above 'DisableKillEnforcement' explaining what it controls and any nuances in behavior, e.g.:
  // DisableKillEnforcement disables enforcement of kill commands for agents.

netmon.go

• [High]Avoid launching unnecessary goroutines for non-blocking logging operations by using synchronous calls or buffered channels
  The code launches a goroutine for WriteLog, which may lead to unbounded goroutine creation if many packets are dropped in quick succession. This can cause resource exhaustion and instability. According to "Effective Go" (https://golang.org/doc/effective_go#goroutines), goroutines are lightweight but not free, and unbounded goroutine creation should be avoided. Replace the asynchronous call to WriteLog with a synchronous call or refactor to use a dedicated logging worker with a buffered channel to serialize log entries without spawning excessive goroutines.

Example synchronous call:

WriteLog(logMessage)

Or use a buffered channel and a single goroutine to handle log writes.

• [Medium]Remove duplicate calls to netMonitor.ApiClient.sendNetConnection to improve code clarity and prevent unintended side effects
  The original code calls sendNetConnection unconditionally before checking if the IP address equals the sinkhole IP, then calls it again inside the conditional. This means the method may be called twice unnecessarily, causing duplicate network connection records. Clean code principles (Robert C. Martin's Clean Code book) advocate avoiding duplicate code and side effects. Move the call to sendNetConnection inside the conditional block to avoid duplicate calls and ensure it's only sent once when applicable.

Example fix:

if ipv4Address != StepSecuritySinkHoleIPAddress {
    netMonitor.ApiClient.sendNetConnection(...)
    // other logic
}

and remove the earlier call.

• [Medium]Add error handling for the ApiClient.sendNetConnection call to manage potential failures
  The call to sendNetConnection does not check for errors or return values. Ignoring errors may cause missed detections or inconsistencies. According to Go best practices (Effective Go: Errors), errors should be handled explicitly. Modify sendNetConnection to return an error, and add error handling, logging or retries as necessary.

Example:

err := netMonitor.ApiClient.sendNetConnection(...)
if err != nil {
    log.Printf("Failed to send net connection: %v", err)
}

• [Medium]Avoid redundant or unnecessary string concatenations for logging messages
  Building the logMessage with nested fmt.Sprintf can be simplified. Repeated string concatenations introduce minor overhead and reduce readability. Clean code encourages simplicity and clarity. Use conditional logging to reduce string concatenations or use strings.Builder for efficient concatenations if needed.

Example fix:

logMessage := "ip address dropped: " + ipv4Address
if reason != "" {
    logMessage += ", reason: " + reason
}

• [Low]Use structured logging instead of unstructured string concatenation for better log parsing and analysis
  Constructing log messages as formatted strings limits ability to process logs automatically. According to best practices (https://12factor.net/logs), structured logs improve observability. Use a structured logging package, e.g. logrus or zap, passing fields as key-value pairs.

Example:

log.WithFields(log.Fields{
    "ip": ipv4Address,
    "reason": reason,
}).Info("ip address dropped")

• [Low]Document assumptions about constants like StepSecuritySinkHoleIPAddress
  The significance of StepSecuritySinkHoleIPAddress is not explained. Providing comments improves maintainability and clarity. The Go Code Review Comments (https://github.com/golang/go/wiki/CodeReviewComments) encourage clear documentation. Add comments explaining the purpose of StepSecuritySinkHoleIPAddress and why it is excluded from certain logic.

Example:

// StepSecuritySinkHoleIPAddress is used to catch and block malicious DNS requests; traffic to this IP is handled elsewhere.
if ipv4Address != StepSecuritySinkHoleIPAddress {
    ...
}

• [Low]Use constants or enum types for well-known status or tool name strings
  The use of literal strings like "Dropped" or Unknown for Tool.Name/Tool.SHA256 can lead to errors or inconsistencies. Defining constants enhances maintainability and prevents typos. This is recommended in Go best practices and clean code. Define constants for "Dropped", Unknown names, and use them consistently.

Example:

const (
    StatusDropped = "Dropped"
    ToolNameUnknown = "Unknown"
    ToolSHA256Unknown = "Unknown"
)

and use:

status == StatusDropped
Tool{Name: ToolNameUnknown, SHA256: ToolSHA256Unknown}

• [Low]Ensure that concurrent calls to WriteAnnotation are safe or properly synchronized
  The go routine launching WriteAnnotation does not indicate if writes are thread-safe. According to Go concurrency best practices, functions called concurrently should be concurrency safe. Verify WriteAnnotation is concurrency safe or protect it using synchronization primitives, e.g., mutexes, or implement a worker queue for annotations.

Example fix:

Add concurrency safe queue or use sync.Mutex if needed.

• [Low]Add unit tests covering the conditional branches related to dropping packets and sinkhole IP handling
  No tests are shown for this critical network monitoring flow. Adding unit tests improves reliability and conform with testing best practices (Go Testing FAQ). Implement unit tests mocking ApiClient, verifying sendNetConnection is called correctly and that logs and annotations are generated for dropped packets and sinkhole IP exceptions.

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.

[Copilot]

Pull request overview

This PR adjusts telemetry and enforcement behavior in the agent by suppressing dropped net-connection events for the StepSecurity sinkhole IP, and by introducing a global feature flag to disable Armour “kill” enforcement.

Changes:

• Suppress sendNetConnection + logs/annotations for dropped traffic destined to StepSecuritySinkHoleIPAddress.
• Add disable_kill_enforcement to global feature flags.
• Wire the new flag into Armour config via EnforceKillBlock.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
netmon.go	Avoids emitting dropped net-connection events for sinkhole-IP traffic (DNS block covers it).
global_feature_flags.go	Adds DisableKillEnforcement global flag for server-controlled behavior.
agent.go	Reads global flags and configures Armour’s EnforceKillBlock based on DisableKillEnforcement.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[step-security-bot-int]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

agent.go

• [High]Ensure that the initialization and usage of 'globalFlags' is performed before any asynchronous code or goroutines use it
  The variable 'globalFlags' is initialized and then used immediately to set 'EnforceKillBlock'. If 'Run' spawns goroutines or asynchronous operations that rely on this flag, it is critical to ensure that 'globalFlags' is fully initialized and immutable (or properly synchronized) to prevent race conditions. According to the Go race detector and concurrency best practices from Effective Go (https://golang.org/doc/effective_go#concurrency), shared state must be properly synchronized. Ensure that 'globalFlags' is fully initialized before passing it to any goroutines. If 'globalFlags' is to be accessed concurrently, consider making it immutable or synchronizing access through mutexes or using concurrent-safe constructs.
• [High]Validate the 'globalFlags' returned by GetGlobalFeatureFlags before usage
  The code calls GetGlobalFeatureFlags() and immediately accesses DisableKillEnforcement without verifying that 'globalFlags' is non-nil or that DisableKillEnforcement is properly set. This can lead to nil pointer dereference or unexpected behavior. Defensive coding to check returned values can prevent runtime panics (refer to Go error handling best practices by Dave Cheney - https://dave.cheney.net/practical-go/presentations/qcon-china.html). Add validation logic after retrieving 'globalFlags' to ensure it is not nil and all required fields are valid before usage. For example, check if globalFlags == nil, and if so, handle the error appropriately or provide a safe default.
• [Medium]Add comments explaining the purpose and usage of 'EnforceKillBlock' and the logic behind '!globalFlags.DisableKillEnforcement'
  Code clarity and maintainability improve with meaningful comments especially when introducing boolean flags whose naming might be confusing. The flag 'EnforceKillBlock' being set to the inverse of 'DisableKillEnforcement' can confuse developers. The Clean Code principle from Robert C. Martin (Uncle Bob) suggests meaningful names and explanatory comments. Add a comment before setting EnforceKillBlock explaining the logic and what the flag controls, to help future maintainers understand the intent.
• [Medium]Avoid using negated boolean property names (e.g., DisableKillEnforcement) leading to double negatives
  The use of boolean flags with negative terms like 'Disable' can lead to confusion when used in conditionals (e.g., !DisableKillEnforcement). This can reduce readability. The Clean Code guideline recommends positive naming to improve readability. Refactor 'DisableKillEnforcement' to a positively named flag like 'EnableKillEnforcement' and adjust logic accordingly to remove negations.
• [Low]Check for error handling after calling GetGlobalFeatureFlags() if that function can possibly return an error or invalid state
  The code assumes the call to GetGlobalFeatureFlags is always successful. If that function returns an error or indicates failure in any way, this should be handled properly. Robust error handling is recommended by Go official guidelines to increase system reliability (https://golang.org/doc/effective_go#errors). Modify GetGlobalFeatureFlags to return an error alongside the flags or handle error cases appropriately before using returned flags.
• [Low]Consider moving 'globalFlags' declaration closer to its usage for improved code locality
  Currently, 'globalFlags' is declared near the top of the 'Run' function, but only used deeper inside. Keeping variable declarations near their usage improves code clarity and reduces cognitive load (Clean Code practice). Move the line 'globalFlags := GetGlobalFeatureFlags()' to just before the assignment to EnforceKillBlock where it is used.

global_feature_flags.go

• [High]Avoid using 'omitempty' on boolean fields as it can lead to unintended behavior in JSON serialization
  The 'omitempty' tag on boolean fields causes the field to be omitted when false, which may lead to inconsistent behavior during deserialization or when default values are expected. According to the Go JSON encoding documentation (https://pkg.go.dev/encoding/json), applying 'omitempty' on booleans should be done cautiously. Remove the 'omitempty' tag from the DisableKillEnforcement boolean field to ensure explicit true/false representation in JSON serialization:

DisableKillEnforcement bool `json:"disable_kill_enforcement"`

• [Medium]Add documentation comments to all exported struct fields to improve code maintainability and clarity
  Exported fields should have proper documentation comments following GoDoc conventions. This aids future maintainers and consumers of the API as described in Go's effective Go guide (https://go.dev/doc/effective_go#commentary). Add descriptive comments above the newly added DisableKillEnforcement field:

// DisableKillEnforcement determines whether kill enforcement is disabled.
DisableKillEnforcement bool `json:"disable_kill_enforcement"`

netmon.go

• [High]Prevent premature unlocking of mutex which can cause race conditions
  Unlocking the mutex before the end of the critical section risks concurrent access to shared data structures. According to the Go concurrency principles, mutex locks should be held exactly for the duration of access to shared memory to avoid race conditions (Go Blog: 'Go Concurrency Patterns: Timing out, moving on'). Move the call to netMutex.Unlock() to occur only after all operations on the shared map (ipAddresses) are complete. Avoid unlocking the mutex early before returning; if early returns are needed, use defer to ensure unlocking happens properly.
• [High]Avoid returning early while holding a mutex lock
  Returning early from a function while a mutex is still locked leaves the mutex in a locked state, causing potential deadlocks. Best practice from Go's sync package usage suggests deferring Unlock right after Lock to guarantee eventual unlock in all code paths. Instead of unlocking and returning when 'found' is true, place 'defer netMonitor.netMutex.Unlock()' immediately after locking to ensure the mutex is properly unlocked upon return.
• [Medium]Use defer for unlocking mutexes immediately after locking
  Using defer immediately after acquiring a lock ensures that the mutex is always released, even if the function returns early or encounters an error. This is a widely recommended practice documented in Go's sync package and effective concurrency handling. Replace explicit Unlock calls spread throughout the function with a single 'defer netMonitor.netMutex.Unlock()' placed right after netMutex.Lock(). Remove all other explicit Unlock calls within the function.
• [Medium]Clarify duplicated code to avoid redundancy and improve maintainability
  The block logging dropped traffic and sending network connection data is duplicated between conditional branches, which can lead to maintenance challenges. Per Clean Code principles (Robert C. Martin), duplication should be minimized. Consolidate logging and ApiClient.sendNetConnection calls outside of nested if conditions where possible, or abstract the functionality into a separate helper function called in both cases.
• [Medium]Avoid potential panics by validating inputs before use
  The function uses IPv4 addresses and ports in formatted strings without explicit validation. According to Go best practices and security principles (CWE-20), input validation should precede logic that depends on them to prevent malformed or malicious values. Add input validation checks for 'ipv4Address' and 'port' before caching or using them in formatting to ensure they meet expected syntactic and semantic criteria.
• [Low]Improve code readability by defining constants for repeated string literals
  Strings like "Dropped" and "": repeatedly used string literals can be defined as constants. This improves readability and avoids typos. This is aligned with Clean Code naming recommendations. Define constants for string literals such as 'Dropped' and use these constants throughout the code instead of hardcoded strings.
• [Low]Avoid launching goroutines inside critical sections
  Launching goroutines (e.g., go WriteLog) while holding a mutex can cause unexpected blocking if the goroutine tries to acquire the same lock or causes increased contention. Go best practices advise moving goroutine launches outside of mutex-protected code to reduce lock duration. Unlock the mutex before launching goroutines for WriteLog and WriteAnnotation to minimize mutex hold time.
• [Low]Add comments explaining the purpose and side effects of the function
  While the function handles network packets, a clear comment on its purpose, side effects, and thread-safety assumptions aids maintainability and readability, a recommended practice per Effective Go. Add a comment above 'handlePacket' explaining its role, the use of locking, and what shared variables it modifies.

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.