diff --git a/web/job/check_client_ip_job.go b/web/job/check_client_ip_job.go index 312a8eee..6439252f 100644 --- a/web/job/check_client_ip_job.go +++ b/web/job/check_client_ip_job.go @@ -35,6 +35,25 @@ var job *CheckClientIpJob const defaultXrayAPIPort = 62789 +// ipStaleAfterSeconds controls how long a client IP kept in the +// per-client tracking table (model.InboundClientIps.Ips) is considered +// still "active" before it's evicted during the next scan. +// +// Without this eviction, an IP that connected once and then went away +// keeps sitting in the table with its old timestamp. Because the +// excess-IP selector sorts ascending ("oldest wins, newest loses") to +// protect the original/current connections, that stale entry keeps +// occupying a slot and the IP that is *actually* currently using the +// config gets classified as "new excess" and banned by fail2ban on +// every single run — producing the continuous ban loop from #4077. +// +// 30 minutes is chosen so an actively-streaming client (where xray +// emits a fresh `accepted` log line whenever it opens a new TCP) will +// always refresh its timestamp well within the window, but a client +// that has really stopped using the config will drop out of the table +// in a bounded time and free its slot. +const ipStaleAfterSeconds = int64(30 * 60) + // NewCheckClientIpJob creates a new client IP monitoring job instance. func NewCheckClientIpJob() *CheckClientIpJob { job = new(CheckClientIpJob) @@ -202,6 +221,31 @@ func (j *CheckClientIpJob) processLogFile() bool { return shouldCleanLog } +// mergeClientIps combines the persisted (old) and freshly observed (new) +// IP-with-timestamp lists for a single client into a map. An entry is +// dropped if its last-seen timestamp is older than staleCutoff. +// +// Extracted as a helper so updateInboundClientIps can stay DB-oriented +// and the merge policy can be exercised by a unit test. +func mergeClientIps(old, new []IPWithTimestamp, staleCutoff int64) map[string]int64 { + ipMap := make(map[string]int64, len(old)+len(new)) + for _, ipTime := range old { + if ipTime.Timestamp < staleCutoff { + continue + } + ipMap[ipTime.IP] = ipTime.Timestamp + } + for _, ipTime := range new { + if ipTime.Timestamp < staleCutoff { + continue + } + if existingTime, ok := ipMap[ipTime.IP]; !ok || ipTime.Timestamp > existingTime { + ipMap[ipTime.IP] = ipTime.Timestamp + } + } + return ipMap +} + func (j *CheckClientIpJob) checkFail2BanInstalled() bool { cmd := "fail2ban-client" args := []string{"-h"} @@ -310,16 +354,9 @@ func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.Inboun json.Unmarshal([]byte(inboundClientIps.Ips), &oldIpsWithTime) } - // Merge old and new IPs, keeping the latest timestamp for each IP - ipMap := make(map[string]int64) - for _, ipTime := range oldIpsWithTime { - ipMap[ipTime.IP] = ipTime.Timestamp - } - for _, ipTime := range newIpsWithTime { - if existingTime, ok := ipMap[ipTime.IP]; !ok || ipTime.Timestamp > existingTime { - ipMap[ipTime.IP] = ipTime.Timestamp - } - } + // Merge old and new IPs, evicting entries that haven't been + // re-observed in a while. See mergeClientIps / #4077 for why. + ipMap := mergeClientIps(oldIpsWithTime, newIpsWithTime, time.Now().Unix()-ipStaleAfterSeconds) // Convert back to slice and sort by timestamp (oldest first) // This ensures we always protect the original/current connections and ban new excess ones. diff --git a/web/job/check_client_ip_job_test.go b/web/job/check_client_ip_job_test.go new file mode 100644 index 00000000..8bd1a73b --- /dev/null +++ b/web/job/check_client_ip_job_test.go @@ -0,0 +1,77 @@ +package job + +import ( + "reflect" + "testing" +) + +func TestMergeClientIps_EvictsStaleOldEntries(t *testing.T) { + // #4077: after a ban expires, a single IP that reconnects used to get + // banned again immediately because a long-disconnected IP stayed in the + // DB with an ancient timestamp and kept "protecting" itself against + // eviction. Guard against that regression here. + old := []IPWithTimestamp{ + {IP: "1.1.1.1", Timestamp: 100}, // stale — client disconnected long ago + {IP: "2.2.2.2", Timestamp: 1900}, // fresh — still connecting + } + new := []IPWithTimestamp{ + {IP: "2.2.2.2", Timestamp: 2000}, // same IP, newer log line + } + + got := mergeClientIps(old, new, 1000) + + want := map[string]int64{"2.2.2.2": 2000} + if !reflect.DeepEqual(got, want) { + t.Fatalf("stale 1.1.1.1 should have been dropped\ngot: %v\nwant: %v", got, want) + } +} + +func TestMergeClientIps_KeepsFreshOldEntriesUnchanged(t *testing.T) { + // Backwards-compat: entries that aren't stale are still carried forward, + // so enforcement survives access-log rotation. + old := []IPWithTimestamp{ + {IP: "1.1.1.1", Timestamp: 1500}, + } + got := mergeClientIps(old, nil, 1000) + + want := map[string]int64{"1.1.1.1": 1500} + if !reflect.DeepEqual(got, want) { + t.Fatalf("fresh old IP should have been retained\ngot: %v\nwant: %v", got, want) + } +} + +func TestMergeClientIps_PrefersLaterTimestampForSameIp(t *testing.T) { + old := []IPWithTimestamp{{IP: "1.1.1.1", Timestamp: 1500}} + new := []IPWithTimestamp{{IP: "1.1.1.1", Timestamp: 1700}} + + got := mergeClientIps(old, new, 1000) + + if got["1.1.1.1"] != 1700 { + t.Fatalf("expected latest timestamp 1700, got %d", got["1.1.1.1"]) + } +} + +func TestMergeClientIps_DropsStaleNewEntries(t *testing.T) { + // A log line with a clock-skewed old timestamp must not resurrect a + // stale IP past the cutoff. + new := []IPWithTimestamp{{IP: "1.1.1.1", Timestamp: 500}} + got := mergeClientIps(nil, new, 1000) + + if len(got) != 0 { + t.Fatalf("stale new IP should have been dropped, got %v", got) + } +} + +func TestMergeClientIps_NoStaleCutoffStillWorks(t *testing.T) { + // Defensive: a zero cutoff (e.g. during very first run on a fresh + // install) must not over-evict. + old := []IPWithTimestamp{{IP: "1.1.1.1", Timestamp: 100}} + new := []IPWithTimestamp{{IP: "2.2.2.2", Timestamp: 200}} + + got := mergeClientIps(old, new, 0) + + want := map[string]int64{"1.1.1.1": 100, "2.2.2.2": 200} + if !reflect.DeepEqual(got, want) { + t.Fatalf("zero cutoff should keep everything\ngot: %v\nwant: %v", got, want) + } +}