Repository navigation
Forget idle scanner IPs in the brute-force detector - #71
Conversation
BruteDetector pruned timestamps inside each IP's slice but never deleted the IP, and never pruned fired at all. On an internet-facing host every scanner IP that ever failed once stayed in memory for the life of the process, which runs for months under MemoryMax=128M. Record now sweeps at most once per Window and drops every IP whose newest attempt has left the window, together with its fired time. That cannot change a decision: those attempts would be filtered out on the next Record anyway, and fired is never later than the newest attempt, so the IP is already outside its quiet period. GeoResolver's cache is also capped at 1024 entries, and starts over when full. The alert engine's per-key maps are left for a separate change. Refs eliau2005#37
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe SSH watcher now periodically removes stale brute-detector state and limits the GeoResolver lookup cache to 1,024 entries. Tests cover state removal and retention, repeat detection after stale state is removed, and cache size. ChangesBrute Detector State
GeoResolver Cache Bound
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The state bounds appear mergeable after normal checks; no outstanding issue is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes reduce accumulated memory without adding privileges or new access paths. The current event loop preserves detection behavior, but pruning introduces a timestamp-ordering assumption that could discard active evidence if the detector receives out-of-order calls. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Add Carrot credits or activate Agent usage billing to use Autopilot |
|
@coderabbitai review |
✅ Action performedReview finished.
|
eliau2005
left a comment
eliau2005
left a comment
There was a problem hiding this comment.
Thanks for this. The pruning logic is correct for the current caller, and the tests are good. There's one thing I'd like changed before merging.
prune treats times[len(times)-1] as the IP's newest attempt. That only holds while Record is called in chronological order. Today it is: the only caller passes time.Now() serially from authLoop. But Record takes an arbitrary at and doesn't enforce ordering. If an older timestamp is ever appended after a newer one (replayed or backfilled log lines, a future concurrent caller, tests), the sweep can drop an IP that still has attempts inside the window. It also drops that IP's fired entry, which can delay a threshold alert or allow an early re-fire. Both are changes from the pre-PR behaviour that the doc comment says can't happen.
Could you make prune independent of ordering? Either:
- compute the max timestamp in the slice instead of taking the last element (cheap, since slices are at most about
Thresholdlong), or - clamp in
Recordsoattempts[ip]stays sorted.
The first option is the smaller change. A small test that records an out-of-order timestamp across a sweep would lock it in.
Minor, optional: the second loop in prune (deleting fired entries with no attempts) can't find anything, since fired[ip] is only set after attempts[ip]. Feel free to drop it.
Record does not require chronological order, so prune must use the max timestamp rather than the last slice element. Drop the redundant fired cleanup loop. Add an out-of-order sweep regression test.
eliau2005
left a comment
eliau2005
left a comment
There was a problem hiding this comment.
Looks good. Max-timestamp pruning plus the out-of-order regression test is exactly what I was after. I pushed a one-line gofmt fix (a stray blank line in the test file) to your branch, so nothing more is needed from you.
|
Thanks a lot for this, @MrBeldum! It's a clean fix with a clear write-up, and you turned the review around quickly. I really appreciate the contribution. If you find StatixAgent useful, a ⭐ on the repo would mean a lot and helps others find it. |
2 participants
What & why
This handles the
sshwatchpart of #37: per-IP state that grows without limit on an internet-facing host.BruteDetectorRecordtrimmed old timestamps inside each IP's slice but never deleted the IP, and nothing ever prunedfired. Every scanner IP that failed once stayed in memory for the life of the process. The agent runs for months underMemoryMax=128M, so on a bare VPS that is slow, unbounded heap growth.Recordsweeps at most once perWindowand drops every IP whose newest attempt has left the window, along with itsfiredentry.Recordanyway.firedis never later than the newest attempt, so a forgotten IP is already outside its quiet period and may fire again, exactly as before.Record, so the agent loop needs no new goroutine or wiring.GeoResolverLeft for a separate PR: pruning
alert.Engine'slastFired/active/streakmaps.Eventtakes its cooldown per call, so expiring those keys safely needs a small design choice (store the expiry with the key, or move per-IP dedupe into the detector). I didn't want to bundle that here.Refs #37
How it was tested
TestBruteDetectorForgetsIdleIPsrecords 5,000 scanner IPs plus one IP that crosses the threshold. One new attempt an hour later leaves 1 tracked IP and 0firedentries. Without the fix the same test reports 5002 and 1. It then checks that the forgotten attacker fires again on a new burst.TestBruteDetectorKeepsActiveIPsruns a sweep while an IP is mid-burst and checks that it still fires on its third attempt. It then runs another sweep during the quiet period and checks that the ongoing attack doesn't fire again.TestGeoResolverCacheIsBoundedruns 1,034 lookups against anhttptestserver and checks the cache stays at or under the cap.TestBruteDetectorpasses unchanged.go test -race ./...,go vet ./...andgofmt -l .are clean.Checklist
go test ./...passesgo vet ./...is cleangofmt-formattedSummary by CodeRabbit