enh(security): Improve whitelisting and threshold applications#136
Merged
Conversation
Events ingested from nginx were classified and could auto-block an IP even when that IP or path was whitelisted. The whitelist was only honored by the nginx Lua cache, which is empty right after a restart, so a whitelisted source could still end up blocked. Whitelisted networks now match by CIDR containment and paths by prefix, and whitelisted traffic never generates events or blocks.
Security detection thresholds updated through the key-based config API now take effect on the running detector immediately instead of waiting for the agent to restart.
Code Review SummaryThis PR significantly improves security by implementing trusted proxy validation and improving the efficiency of the whitelisting system with an in-memory cache. 🚀 Key Improvements
💡 Minor Suggestions
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
Whitelist matching no longer reads the database and re-parses CIDR ranges for every ingested event. The cache is rebuilt after any whitelist change, so additions and removals still take effect immediately.
The capture tests send requests from inside the Docker network, so their source addresses fall in the private ranges that are whitelisted by default and are now correctly skipped during ingestion. The tests now present public client addresses through the forwarding header, and the e2e copy of the capture script resolves the real client IP from forwarded headers the same way the production template does.
nfebe
force-pushed
the
fix/security-whitelist-and-settings
branch
from
a9930c3 to
978346f
Compare
The realtime capture script read the client address straight from X-Forwarded-For and CF-Connecting-IP, so any client could spoof those headers to impersonate another address. Combined with default whitelisting of private ranges, a request could forge a whitelisted or internal source and bypass detection, evade an existing block, or get an innocent address blocked. Forwarded headers are now honored only when the connecting peer is one of the operator-configured trusted proxies, and the real client is taken as the first forwarded hop that is not itself a trusted proxy. With no proxies configured the connecting peer is always used, so forwarded headers cannot be spoofed out of the box.
Changing the trusted proxy list or Cloudflare-header trust through the config API now regenerates the capture script and reloads nginx so the new trust rules take effect immediately, instead of waiting for a manual script regeneration. If nginx is not running the value is still saved and the response reports that it was not applied.
The e2e capture script only does IPv4 CIDR matching. An IPv6 range now explicitly marks the peer untrusted rather than relying on incidental nil handling, so a forwarded header is never honored for it. The production template already matches IPv6 ranges directly.
| } | ||
| } | ||
|
|
||
| m.wlMu.Lock() |
There was a problem hiding this comment.
This write lock is acquired too late to prevent redundant database queries during high concurrency. If multiple requests see m.wlCache == nil, they will all proceed to query the database. While the existing review mentioned a 'double-check' pattern, the current implementation still performs the heavy lifting (DB fetch) before the lock.
Suggested change
| m.wlMu.Lock() | |
| m.wlMu.Lock() | |
| defer m.wlMu.Unlock() | |
| if m.wlCache != nil { | |
| return m.wlCache, nil | |
| } | |
| entries, err := m.db.GetWhitelist() | |
| if err != nil { | |
| return nil, err | |
| } | |
| cache = &whitelistCache{ips: make(map[string]struct{})} | |
| // ... (rest of parsing logic) ... | |
| m.wlCache = cache | |
| return cache, nil |
|
|
||
| result := &IngestResult{} | ||
|
|
||
| whitelisted, err := m.IsRequestWhitelisted(event.SourceIP, event.RequestPath) |
There was a problem hiding this comment.
Performing the whitelist check before the block check is correct for performance (whitelists are usually smaller). However, ensure that event.SourceIP is already normalized (e.g. trimmed) before this call to avoid bypasses using whitespace.
Suggested change
| whitelisted, err := m.IsRequestWhitelisted(event.SourceIP, event.RequestPath) | |
| sourceIP := strings.TrimSpace(event.SourceIP) | |
| whitelisted, err := m.IsRequestWhitelisted(sourceIP, event.RequestPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if whitelisted { | |
| return result, nil | |
| } |
A trusted-proxy entry containing a quote could break out of the Lua string literal it is rendered into and inject code into the nginx worker. Entries are now validated as IP or CIDR values and reduced to their canonical form before injection, and zoned IPv6 addresses (which can carry arbitrary characters) are rejected, so a malformed or hostile entry is dropped rather than executed.
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
1 participant
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.
No description provided.