diff --git a/cmd/server/config.go b/cmd/server/config.go index c4308dae9..5388ca27d 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -279,6 +279,10 @@ type Config struct { // so browsers enforce same-origin policy. Set to ["*"] to allow all origins. CORSAllowedOrigins []string `json:"corsAllowedOrigins,omitempty"` + // WebSocket carries the /ws transport limits from #1794. Omitted entirely + // means: rate limit at its default, connection cap off, no deny list. + WebSocket *WebSocketConfig `json:"webSocket,omitempty"` + DebugAffinity bool `json:"debugAffinity,omitempty"` // MapDarkTileProvider selects the default dark-mode basemap provider for @@ -619,6 +623,81 @@ func LoadConfig(baseDirs ...string) (*Config, error) { return cfg, nil // defaults } +// WebSocketConfig holds the /ws transport limits (#1794). These sit behind +// the CheckOrigin allowlist (#1793) and catch non-browser clients, which can +// omit or forge Origin. +type WebSocketConfig struct { + // MaxConnsPerIP caps concurrent /ws connections from one client address. + // DEFAULT 0, meaning OFF, and that is a deliberate departure from the + // value floated on #1794. + // + // A cap of 5 is safe only when one address means one household. It does + // not on mobile: carrier-grade NAT puts thousands of unrelated subscribers + // behind a single public IPv4, so a low cap would refuse real visitors on + // phones while barely inconveniencing a scraper that can rent more + // addresses. Operators who know their audience can set it; we must not + // pick it for them. + MaxConnsPerIP int `json:"maxConnsPerIP,omitempty"` + + // UpgradesPerMinPerIP caps handshakes per minute from one client address. + // DEFAULT 30, on. Unlike the connection cap this is safe under CGNAT: a + // real client upgrades a handful of times per minute even while + // reconnecting, so 30 leaves ordinary traffic untouched while flattening + // the reconnect loop that makes scrapers expensive. + UpgradesPerMinPerIP *int `json:"upgradesPerMinPerIP,omitempty"` + + // TrustedProxies lists the addresses or CIDRs of reverse proxies whose + // X-Forwarded-For may be believed. REQUIRED for the limits to do anything + // behind nginx/Caddy/Traefik/ingress: without it every visitor shares the + // proxy's address, so the limits are skipped and a warning is logged. + // Never list an address you do not control; anyone reaching the server + // from a trusted address can name any client IP they like. + TrustedProxies []string `json:"trustedProxies,omitempty"` + + // Deny blocks addresses outright at the upgrade, before the handshake. + // Accepts both bare addresses ("1.2.3.4") and CIDRs ("1.2.3.0/24"). + // Applies even when clients cannot be told apart, because it is an + // explicit instruction rather than an inference. + Deny []string `json:"deny,omitempty"` +} + +// WSMaxConnsPerIP returns the configured concurrent-connection cap, 0 = off. +func (c *Config) WSMaxConnsPerIP() int { + if c.WebSocket == nil || c.WebSocket.MaxConnsPerIP < 0 { + return 0 + } + return c.WebSocket.MaxConnsPerIP +} + +// WSUpgradesPerMinPerIP returns the upgrade-rate cap, defaulting to 30 when +// unset. A pointer distinguishes "not configured" (use the default) from an +// explicit 0, which an operator uses to turn the limit off. +func (c *Config) WSUpgradesPerMinPerIP() int { + if c.WebSocket == nil || c.WebSocket.UpgradesPerMinPerIP == nil { + return 30 + } + if v := *c.WebSocket.UpgradesPerMinPerIP; v > 0 { + return v + } + return 0 +} + +// WSTrustedProxies returns the configured reverse-proxy addresses, if any. +func (c *Config) WSTrustedProxies() []string { + if c.WebSocket == nil { + return nil + } + return c.WebSocket.TrustedProxies +} + +// WSDeny returns the configured deny entries, if any. +func (c *Config) WSDeny() []string { + if c.WebSocket == nil { + return nil + } + return c.WebSocket.Deny +} + func (c *Config) applyListLimitsDefaults() { if c.ListLimits == nil { c.ListLimits = &ListLimitsConfig{} diff --git a/cmd/server/main.go b/cmd/server/main.go index 1c31a277a..7928f73c2 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -349,6 +349,8 @@ func main() { // WebSocket hub hub := NewHub() hub.SetAllowedOrigins(cfg.CORSAllowedOrigins) + hub.ConfigureLimits(cfg.WSMaxConnsPerIP(), cfg.WSUpgradesPerMinPerIP(), + cfg.WSTrustedProxies(), cfg.WSDeny()) // #1794 hub.upgrader.EnableCompression = cfg.WSCompressionEnabled() // HTTP server diff --git a/cmd/server/routes.go b/cmd/server/routes.go index e0e1801d8..e1312b91e 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -889,8 +889,10 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { uptime := time.Since(s.startedAt).Seconds() wsClients := 0 + var wsDeny, wsRate, wsConnCap int64 if s.hub != nil { wsClients = s.hub.ClientCount() + wsDeny, wsRate, wsConnCap = s.hub.limits.counts() // #1794; nil-safe } // Real packet store stats @@ -962,8 +964,9 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { P95Ms: round(percentile(sortedPauses, 0.95), 1), P99Ms: round(percentile(sortedPauses, 0.99), 1), }, - Cache: cs, - WebSocket: WebSocketStatsResp{Clients: wsClients}, + Cache: cs, + WebSocket: WebSocketStatsResp{Clients: wsClients, + RejectedDeny: wsDeny, RejectedRate: wsRate, RejectedConnCap: wsConnCap}, PacketStore: HealthPacketStoreStats{ Packets: pktCount, EstimatedMB: pktEstMB, diff --git a/cmd/server/types.go b/cmd/server/types.go index c24b5341b..b3d81c7df 100644 --- a/cmd/server/types.go +++ b/cmd/server/types.go @@ -586,6 +586,13 @@ type PerfCacheStats struct { type WebSocketStatsResp struct { Clients int `json:"clients"` + // #1794: upgrade rejections since boot, split by cause so an operator can + // tell a deny-list hit from a client that is merely reconnecting too fast. + // omitempty: an install with no limits configured sends none of these + // rather than three permanent zeroes. + RejectedDeny int64 `json:"rejectedDeny,omitempty"` + RejectedRate int64 `json:"rejectedRate,omitempty"` + RejectedConnCap int64 `json:"rejectedConnCap,omitempty"` } type HealthPacketStoreStats struct { diff --git a/cmd/server/websocket.go b/cmd/server/websocket.go index ba4adc90d..a8b177305 100644 --- a/cmd/server/websocket.go +++ b/cmd/server/websocket.go @@ -17,7 +17,8 @@ type Hub struct { mu sync.RWMutex clients map[*Client]bool upgrader websocket.Upgrader - allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) + allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) + limits *wsLimiter // #1794: per-IP caps and deny list; nil allows everything } // SetAllowedOrigins configures the exact-match origin allowlist consulted by @@ -78,6 +79,32 @@ type Client struct { conn *websocket.Conn send chan []byte closeOnce sync.Once + // release returns this connection's slot to the per-IP concurrent cap + // (#1794). Always non-nil so Unregister needs no nil check; it is a no-op + // when limits are disabled. + release func() +} + +// ConfigureLimits installs the #1794 transport limits. Called once at +// startup, before the listener binds, so no upgrade can race a half-built +// limiter. Passing zero values for both caps leaves only the deny list +// active, which is a legitimate configuration. +func (h *Hub) ConfigureLimits(maxConnsPerIP, upgradesPerMin int, trustedProxies, deny []string) { + l := newWSLimiter() + l.maxConnsPerIP = maxConnsPerIP + l.upgradesPerMin = upgradesPerMin + l.trustedProxies = parseCIDRList(trustedProxies, "webSocket.trustedProxies") + l.denyNets = parseCIDRList(deny, "webSocket.deny") + h.mu.Lock() + h.limits = l + h.mu.Unlock() + if len(l.denyNets) > 0 { + log.Printf("[ws] deny list active: %d network(s)", len(l.denyNets)) + } + if len(l.trustedProxies) == 0 && (maxConnsPerIP > 0 || upgradesPerMin > 0) { + log.Printf("[ws] per-IP limits configured with no trustedProxies: they apply only to " + + "directly-connected clients. Behind a reverse proxy, set webSocket.trustedProxies.") + } } func NewHub() *Hub { @@ -112,6 +139,12 @@ func (h *Hub) Unregister(c *Client) { c.closeOnce.Do(func() { close(c.send) }) } h.mu.Unlock() + // #1794: outside the lock — release takes the limiter's own mutex, and + // holding two locks in one order here and the other order elsewhere is how + // deadlocks are made. Idempotent, so a double Unregister cannot over-credit. + if c.release != nil { + c.release() + } log.Printf("[ws] client disconnected (%d total)", h.ClientCount()) } @@ -151,15 +184,33 @@ func (h *Hub) Broadcast(msg interface{}) { // ServeWS handles the WebSocket upgrade and runs the client. func (h *Hub) ServeWS(w http.ResponseWriter, r *http.Request) { + // #1794: decide BEFORE the upgrade. Rejecting afterwards would already + // have allocated the connection and completed the handshake, which is the + // resource this is meant to protect. + ok, reason, key, release := h.limits.allow(r) + if !ok { + status := http.StatusForbidden + if reason == wsRejectRate { + // 429 tells a well-behaved client to back off; 403 would read as + // "never come back" for what is a temporary refusal. + status = http.StatusTooManyRequests + } + log.Printf("[ws] reject ip=%s reason=%s", key, reason) + http.Error(w, "websocket upgrade refused", status) + return + } + conn, err := h.upgrader.Upgrade(w, r, nil) if err != nil { log.Printf("[ws] upgrade error: %v", err) + release() // the slot was charged above and this connection never happened return } client := &Client{ - conn: conn, - send: make(chan []byte, 256), + conn: conn, + send: make(chan []byte, 256), + release: release, } h.Register(client) diff --git a/cmd/server/ws_limits.go b/cmd/server/ws_limits.go new file mode 100644 index 000000000..9638b9e33 --- /dev/null +++ b/cmd/server/ws_limits.go @@ -0,0 +1,274 @@ +package main + +// Issue #1794: per-IP limits and a deny list for the /ws upgrade, as defence +// in depth behind the CheckOrigin allowlist from #1793. +// +// CheckOrigin only stops browsers. A Go, Python or curl client can omit the +// Origin header entirely or forge one, connect, and sit in the hub. These +// limits work on the transport instead: who is connecting, how often, and how +// many at once. +// +// THE THING THAT MAKES THIS DANGEROUS TO SHIP NAIVELY, stated up front: +// most CoreScope installs run behind nginx, Caddy, Traefik or a k8s ingress +// (cdn_detection.go says so in as many words). For those, r.RemoteAddr is the +// proxy, 127.0.0.1 for every visitor on earth. A per-IP cap keyed on that +// address protects nobody; it counts the whole internet as one client and +// hands the sixth legitimate browser tab a 403. That is a self-inflicted +// outage, not hardening. +// +// So enforcement is conditional on being able to tell clients apart: +// - X-Forwarded-For is honoured ONLY when the request arrives from an +// address the operator listed in trustedProxies. Otherwise it is an +// attacker-supplied header, and trusting it would let anyone forge a +// fresh source IP per connection, which is worse than no limit at all. +// - If the peer looks like a local reverse proxy and no trustedProxies is +// configured, clients cannot be distinguished, so the per-IP limits are +// SKIPPED and one warning is logged telling the operator what to set. +// The deny list still applies, because an explicit block is the +// operator's own instruction rather than an inference. +// +// Not shipped: a default deny list of hosting-provider ranges. It was +// proposed on the issue (44 CIDRs for one VPS host, after a single scraper +// was seen). Blanket-blocking a provider by default breaks legitimate +// operators who host there, is undiscoverable by the person locked out, and +// ages badly as ranges are reassigned. Operators who want it can configure it. + +import ( + "log" + "net" + "net/http" + "strings" + "sync" + "sync/atomic" + "time" +) + +// wsUpgradeWindow is the span the upgrade budget is counted over. One minute +// matches the config field name (upgradesPerMinPerIP) so an operator reading +// the config can predict the behaviour without reading this file. +const wsUpgradeWindow = time.Minute + +// wsBucketIdleGC is how long an idle per-IP record is kept before collection. +// Longer than the window, so a client that pauses briefly cannot earn a fresh +// budget by being forgotten. +const wsBucketIdleGC = 10 * time.Minute + +// wsRejectReason is the label used both in the log line and in the +// /api/stats counters, so an operator can match one to the other. +type wsRejectReason string + +const ( + wsRejectDeny wsRejectReason = "deny" + wsRejectRate wsRejectReason = "rate" + wsRejectConnCap wsRejectReason = "conncap" +) + +type wsIPState struct { + conns int + upgrades []time.Time // timestamps inside the current window + lastSeenAt time.Time +} + +// wsLimiter holds the parsed configuration and the live per-IP state. A nil +// *wsLimiter allows everything, so a server that never configures limits +// behaves exactly as it did before this change. +type wsLimiter struct { + mu sync.Mutex + + maxConnsPerIP int // 0 disables the concurrent-connection cap + upgradesPerMin int // 0 disables the upgrade-rate limit + + trustedProxies []*net.IPNet + denyNets []*net.IPNet + + state map[string]*wsIPState + + rejectedDeny atomic.Int64 + rejectedRate atomic.Int64 + rejectedConnCap atomic.Int64 + + // warnedIndistinct fires the "set trustedProxies" warning once, rather + // than on every upgrade from behind an unconfigured proxy. + warnedIndistinct sync.Once +} + +func newWSLimiter() *wsLimiter { + return &wsLimiter{state: make(map[string]*wsIPState)} +} + +// parseCIDRList turns operator strings into networks. A bare address is +// accepted and treated as a single-host network, because "1.2.3.4" is what an +// operator naturally writes and silently ignoring it would be the worst +// possible failure mode for a deny list. An unparseable entry is logged and +// skipped rather than aborting startup: one typo must not take the server +// down, but it must not pass unnoticed either. +func parseCIDRList(entries []string, what string) []*net.IPNet { + out := make([]*net.IPNet, 0, len(entries)) + for _, raw := range entries { + e := strings.TrimSpace(raw) + if e == "" { + continue + } + if _, n, err := net.ParseCIDR(e); err == nil { + out = append(out, n) + continue + } + if ip := net.ParseIP(e); ip != nil { + bits := 32 + if ip.To4() == nil { + bits = 128 + } + out = append(out, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) + continue + } + log.Printf("[ws] WARNING: %s entry %q is neither an IP nor a CIDR, ignored", what, e) + } + return out +} + +func ipInAny(ip net.IP, nets []*net.IPNet) bool { + if ip == nil { + return false + } + for _, n := range nets { + if n.Contains(ip) { + return true + } + } + return false +} + +// clientIP resolves the address the limits are counted against, and reports +// whether that address actually identifies a client. +// +// distinct is false when the peer is a loopback or private address and no +// trustedProxies is configured: the request has almost certainly crossed a +// reverse proxy whose X-Forwarded-For we are not allowed to believe, so every +// visitor looks identical and per-IP limits would punish the wrong people. +func (l *wsLimiter) clientIP(r *http.Request) (ip net.IP, distinct bool) { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + host = r.RemoteAddr + } + peer := net.ParseIP(strings.TrimSpace(host)) + if peer == nil { + return nil, false + } + if ipInAny(peer, l.trustedProxies) { + // First hop is the client; later hops were appended by intermediaries + // we have no reason to trust individually. + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + first := strings.TrimSpace(strings.Split(xff, ",")[0]) + if fwd := net.ParseIP(first); fwd != nil { + return fwd, true + } + } + // A trusted proxy that forwarded no XFF still identifies itself. + return peer, true + } + if peer.IsLoopback() || peer.IsPrivate() || peer.IsLinkLocalUnicast() { + return peer, false + } + return peer, true +} + +// allow decides whether an upgrade may proceed. On rejection it returns the +// reason. On success it returns a release func the caller MUST invoke when +// the connection ends, so the concurrent-connection count comes back down. +func (l *wsLimiter) allow(r *http.Request) (ok bool, reason wsRejectReason, key string, release func()) { + noop := func() {} + if l == nil { + return true, "", "", noop + } + ip, distinct := l.clientIP(r) + + // The deny list is an explicit operator instruction, so it applies even + // when clients cannot be told apart. Behind an unconfigured proxy it + // simply never matches, which is visible rather than silently wrong. + if ipInAny(ip, l.denyNets) { + l.rejectedDeny.Add(1) + return false, wsRejectDeny, ip.String(), noop + } + if l.maxConnsPerIP <= 0 && l.upgradesPerMin <= 0 { + return true, "", "", noop + } + if !distinct { + l.warnedIndistinct.Do(func() { + log.Printf("[ws] WARNING: per-IP limits are configured but every request arrives from %s, "+ + "so clients cannot be told apart and the limits are NOT enforced. "+ + "Set webSocket.trustedProxies to your reverse proxy's address to enable them.", ip) + }) + return true, "", "", noop + } + + key = ip.String() + now := time.Now() + + l.mu.Lock() + defer l.mu.Unlock() + l.gcLocked(now) + + st := l.state[key] + if st == nil { + st = &wsIPState{} + l.state[key] = st + } + st.lastSeenAt = now + + if l.upgradesPerMin > 0 { + cutoff := now.Add(-wsUpgradeWindow) + kept := st.upgrades[:0] + for _, t := range st.upgrades { + if t.After(cutoff) { + kept = append(kept, t) + } + } + st.upgrades = kept + if len(st.upgrades) >= l.upgradesPerMin { + l.rejectedRate.Add(1) + return false, wsRejectRate, key, noop + } + } + if l.maxConnsPerIP > 0 && st.conns >= l.maxConnsPerIP { + l.rejectedConnCap.Add(1) + return false, wsRejectConnCap, key, noop + } + + // Both budgets have room, so charge them together: a rejected upgrade + // must never consume rate budget for a connection it did not get. + if l.upgradesPerMin > 0 { + st.upgrades = append(st.upgrades, now) + } + st.conns++ + + var once sync.Once + return true, "", key, func() { + once.Do(func() { + l.mu.Lock() + defer l.mu.Unlock() + if s := l.state[key]; s != nil && s.conns > 0 { + s.conns-- + s.lastSeenAt = time.Now() + } + }) + } +} + +// gcLocked drops records with no live connections and no recent activity, so +// the map cannot grow without bound when the server is scanned from many +// addresses. Caller must hold l.mu. +func (l *wsLimiter) gcLocked(now time.Time) { + for k, st := range l.state { + if st.conns == 0 && now.Sub(st.lastSeenAt) > wsBucketIdleGC { + delete(l.state, k) + } + } +} + +// counts reports the rejection tallies for /api/stats. +func (l *wsLimiter) counts() (deny, rate, connCap int64) { + if l == nil { + return 0, 0, 0 + } + return l.rejectedDeny.Load(), l.rejectedRate.Load(), l.rejectedConnCap.Load() +} diff --git a/cmd/server/ws_limits_test.go b/cmd/server/ws_limits_test.go new file mode 100644 index 000000000..cf025770f --- /dev/null +++ b/cmd/server/ws_limits_test.go @@ -0,0 +1,296 @@ +package main + +// Tests for the #1794 /ws transport limits. The five cases the issue lists as +// TDD requirements are marked with the exact wording from the issue body, so a +// reviewer can match them off without reading the implementation. + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func req(remoteAddr, xff string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/ws", nil) + r.RemoteAddr = remoteAddr + if xff != "" { + r.Header.Set("X-Forwarded-For", xff) + } + return r +} + +func limiter(maxConns, perMin int, trusted, deny []string) *wsLimiter { + l := newWSLimiter() + l.maxConnsPerIP = maxConns + l.upgradesPerMin = perMin + l.trustedProxies = parseCIDRList(trusted, "trusted") + l.denyNets = parseCIDRList(deny, "deny") + return l +} + +// "Deny CIDR match → 403, hub count unchanged." +func TestWSDeny_CIDRMatchRejects(t *testing.T) { + l := limiter(0, 0, nil, []string{"203.0.113.0/24"}) + ok, reason, _, _ := l.allow(req("203.0.113.7:1234", "")) + if ok || reason != wsRejectDeny { + t.Fatalf("denied CIDR: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectDeny) + } + if ok, _, _, _ := l.allow(req("203.0.114.7:1234", "")); !ok { + t.Error("an address outside the denied range must still connect") + } + if d, _, _ := l.counts(); d != 1 { + t.Errorf("rejectedDeny = %d, want 1", d) + } +} + +func TestWSDeny_BareAddressIsAcceptedAsSingleHost(t *testing.T) { + // Operators write "1.2.3.4", not "1.2.3.4/32". Silently ignoring the bare + // form would be the worst possible failure for a deny list: it looks + // configured and blocks nothing. + l := limiter(0, 0, nil, []string{"198.51.100.9"}) + if ok, _, _, _ := l.allow(req("198.51.100.9:5000", "")); ok { + t.Error("bare address in the deny list must block") + } + if ok, _, _, _ := l.allow(req("198.51.100.10:5000", "")); !ok { + t.Error("the neighbouring address must not be blocked") + } +} + +func TestWSDeny_UnparseableEntryIsSkippedNotFatal(t *testing.T) { + // One typo must not take the deny list, or the server, down. + nets := parseCIDRList([]string{"not-an-ip", "203.0.113.0/24"}, "deny") + if len(nets) != 1 { + t.Fatalf("parsed %d networks, want 1 (the valid entry)", len(nets)) + } +} + +// "6th conn from same IP with cap=5 → 403." +func TestWSConnCap_SixthConnectionRejected(t *testing.T) { + l := limiter(5, 0, nil, nil) + for i := 1; i <= 5; i++ { + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Fatalf("connection %d of 5 was rejected", i) + } + } + ok, reason, _, _ := l.allow(req("203.0.113.5:1", "")) + if ok || reason != wsRejectConnCap { + t.Fatalf("6th connection: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectConnCap) + } + if _, _, c := l.counts(); c != 1 { + t.Errorf("rejectedConnCap = %d, want 1", c) + } +} + +func TestWSConnCap_ReleaseFreesTheSlot(t *testing.T) { + l := limiter(1, 0, nil, nil) + _, _, _, release := l.allow(req("203.0.113.5:1", "")) + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); ok { + t.Fatal("cap=1 must refuse the second concurrent connection") + } + release() + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Error("after release the slot must be reusable") + } +} + +func TestWSConnCap_ReleaseIsIdempotent(t *testing.T) { + // Unregister can run twice for one client; double-crediting would let an + // IP accumulate slots it never gave back. + l := limiter(1, 0, nil, nil) + _, _, _, release := l.allow(req("203.0.113.5:1", "")) + release() + release() + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Fatal("first reconnect must be allowed") + } + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); ok { + t.Error("cap=1 must still hold: the double release must not have credited two slots") + } +} + +// "31 upgrades/min from same IP → 31st rejected." +func TestWSRate_ThirtyFirstUpgradeRejected(t *testing.T) { + l := limiter(0, 30, nil, nil) + for i := 1; i <= 30; i++ { + if ok, _, _, _ := l.allow(req("203.0.113.8:1", "")); !ok { + t.Fatalf("upgrade %d of 30 was rejected", i) + } + } + ok, reason, _, _ := l.allow(req("203.0.113.8:1", "")) + if ok || reason != wsRejectRate { + t.Fatalf("31st upgrade: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectRate) + } + if _, r, _ := l.counts(); r != 1 { + t.Errorf("rejectedRate = %d, want 1", r) + } +} + +func TestWSRate_BudgetIsPerAddress(t *testing.T) { + l := limiter(0, 2, nil, nil) + l.allow(req("203.0.113.1:1", "")) + l.allow(req("203.0.113.1:1", "")) + if ok, _, _, _ := l.allow(req("203.0.113.1:1", "")); ok { + t.Fatal("the first address should be out of budget") + } + if ok, _, _, _ := l.allow(req("203.0.113.2:1", "")); !ok { + t.Error("a different address must have its own budget") + } +} + +func TestWSRate_OldAttemptsLeaveTheWindow(t *testing.T) { + l := limiter(0, 2, nil, nil) + key := "203.0.113.3" + l.state[key] = &wsIPState{ + upgrades: []time.Time{time.Now().Add(-2 * wsUpgradeWindow), time.Now().Add(-90 * time.Second)}, + lastSeenAt: time.Now(), + } + if ok, _, _, _ := l.allow(req(key+":1", "")); !ok { + t.Error("attempts older than the window must not count against the budget") + } +} + +func TestWSRate_RejectedUpgradeDoesNotConsumeBudget(t *testing.T) { + // A refusal must not push the client further into debt, or a client that + // keeps retrying could never recover even once its window cleared. + l := limiter(1, 5, nil, nil) + l.allow(req("203.0.113.4:1", "")) // takes the only conn slot, 1 upgrade used + for i := 0; i < 3; i++ { + l.allow(req("203.0.113.4:1", "")) // all rejected on the conn cap + } + l.mu.Lock() + used := len(l.state["203.0.113.4"].upgrades) + l.mu.Unlock() + if used != 1 { + t.Errorf("upgrades charged = %d, want 1 (rejections must not be charged)", used) + } +} + +// "Trusted proxy + XFF spoofing → rate counted against the XFF IP, not the proxy." +func TestWSClientIP_TrustedProxyUsesForwardedFor(t *testing.T) { + l := limiter(0, 1, []string{"10.0.0.0/8"}, nil) + if ok, _, key, _ := l.allow(req("10.0.0.2:1", "203.0.113.20, 10.0.0.2")); !ok || key != "203.0.113.20" { + t.Fatalf("key = %q (ok=%v), want the first XFF hop 203.0.113.20", key, ok) + } + // Budget of 1 is now spent for that client, but a different client behind + // the SAME proxy must be unaffected. This is the whole point of honouring + // XFF: one visitor must not exhaust the budget for everyone else. + if ok, _, _, _ := l.allow(req("10.0.0.2:1", "203.0.113.21, 10.0.0.2")); !ok { + t.Error("a second client behind the same trusted proxy must have its own budget") + } + if ok, _, _, _ := l.allow(req("10.0.0.2:1", "203.0.113.20, 10.0.0.2")); ok { + t.Error("the first client's budget should still be spent") + } +} + +// "Untrusted RemoteAddr + XFF spoofing → XFF ignored, rate counted against RemoteAddr." +func TestWSClientIP_UntrustedPeerIgnoresForwardedFor(t *testing.T) { + l := limiter(0, 1, []string{"10.0.0.0/8"}, nil) + // A public peer that is not a configured proxy. If XFF were believed here, + // an attacker would mint a fresh identity per request and the limit would + // be worthless. + if ok, _, key, _ := l.allow(req("203.0.113.30:1", "198.51.100.1")); !ok || key != "203.0.113.30" { + t.Fatalf("key = %q (ok=%v), want the peer address, not the forged header", key, ok) + } + if ok, _, _, _ := l.allow(req("203.0.113.30:1", "198.51.100.2")); ok { + t.Error("changing the forged header must not buy a fresh budget") + } +} + +func TestWSClientIP_TrustedProxyWithoutForwardedForUsesPeer(t *testing.T) { + l := limiter(0, 5, []string{"10.0.0.0/8"}, nil) + if _, _, key, _ := l.allow(req("10.0.0.2:1", "")); key != "10.0.0.2" { + t.Errorf("key = %q, want the proxy's own address when it forwards nothing", key) + } +} + +// The safety property this design exists for: behind an unconfigured reverse +// proxy every visitor shares one address, so enforcing a per-IP cap would +// refuse real users. Limits must be skipped, not applied. +func TestWSLimits_SkippedWhenClientsCannotBeToldApart(t *testing.T) { + l := limiter(1, 1, nil, nil) // caps of 1, and no trustedProxies + for i := 0; i < 10; i++ { + if ok, reason, _, _ := l.allow(req("127.0.0.1:1", "203.0.113.9")); !ok { + t.Fatalf("request %d refused (%s): loopback peer with no trustedProxies must not be limited", i, reason) + } + } + if _, r, c := l.counts(); r != 0 || c != 0 { + t.Errorf("rejections rate=%d conncap=%d, want 0 and 0", r, c) + } +} + +func TestWSLimits_PrivatePeerAlsoCountsAsIndistinct(t *testing.T) { + l := limiter(1, 1, nil, nil) + for i := 0; i < 5; i++ { + if ok, _, _, _ := l.allow(req("192.168.1.10:1", "")); !ok { + t.Fatalf("request %d refused: a private peer with no trustedProxies must not be limited", i) + } + } +} + +// The deny list is an explicit operator instruction, so unlike the caps it +// still applies when clients cannot be told apart. +func TestWSDeny_AppliesEvenWhenIndistinct(t *testing.T) { + l := limiter(0, 0, nil, []string{"127.0.0.1"}) + if ok, reason, _, _ := l.allow(req("127.0.0.1:1", "")); ok || reason != wsRejectDeny { + t.Errorf("ok=%v reason=%q, want the deny list to apply regardless", ok, reason) + } +} + +func TestWSLimits_NilLimiterAllowsEverything(t *testing.T) { + // A Hub built without ConfigureLimits must behave exactly as before. + var l *wsLimiter + ok, _, _, release := l.allow(req("203.0.113.99:1", "")) + if !ok { + t.Fatal("a nil limiter must allow the upgrade") + } + release() // must not panic + if d, r, c := l.counts(); d != 0 || r != 0 || c != 0 { + t.Errorf("nil limiter counts = %d/%d/%d, want zeroes", d, r, c) + } +} + +func TestWSLimits_IdleStateIsCollected(t *testing.T) { + // Without collection, scanning the server from many addresses would grow + // the map forever. + l := limiter(0, 5, nil, nil) + l.state["203.0.113.50"] = &wsIPState{conns: 0, lastSeenAt: time.Now().Add(-2 * wsBucketIdleGC)} + l.state["203.0.113.51"] = &wsIPState{conns: 1, lastSeenAt: time.Now().Add(-2 * wsBucketIdleGC)} + l.mu.Lock() + l.gcLocked(time.Now()) + _, idleKept := l.state["203.0.113.50"] + _, liveKept := l.state["203.0.113.51"] + l.mu.Unlock() + if idleKept { + t.Error("an idle record with no connections should be collected") + } + if !liveKept { + t.Error("a record with a live connection must never be collected") + } +} + +func TestWSConfig_Defaults(t *testing.T) { + // The connection cap is deliberately OFF by default: carrier-grade NAT + // puts many unrelated subscribers behind one address, so a low cap would + // refuse real visitors on mobile networks. + var c Config + if got := c.WSMaxConnsPerIP(); got != 0 { + t.Errorf("default maxConnsPerIP = %d, want 0 (off)", got) + } + if got := c.WSUpgradesPerMinPerIP(); got != 30 { + t.Errorf("default upgradesPerMinPerIP = %d, want 30", got) + } + zero := 0 + c.WebSocket = &WebSocketConfig{UpgradesPerMinPerIP: &zero} + if got := c.WSUpgradesPerMinPerIP(); got != 0 { + t.Errorf("explicit 0 = %d, want 0: an operator must be able to turn the rate limit off", got) + } + five := 5 + c.WebSocket = &WebSocketConfig{UpgradesPerMinPerIP: &five, MaxConnsPerIP: 3} + if got := c.WSUpgradesPerMinPerIP(); got != 5 { + t.Errorf("configured rate = %d, want 5", got) + } + if got := c.WSMaxConnsPerIP(); got != 3 { + t.Errorf("configured cap = %d, want 3", got) + } +} diff --git a/config.example.json b/config.example.json index 7e2aa3ae7..5bb7cbf0f 100644 --- a/config.example.json +++ b/config.example.json @@ -35,6 +35,16 @@ "_comment_ingestorStats": "Ingestor publishes a 1-Hz stats snapshot consumed by the server's /api/perf/io and /api/perf/write-sources endpoints (#1120). Path is configured via the CORESCOPE_INGESTOR_STATS environment variable on the INGESTOR process. Default: /tmp/corescope-ingestor-stats.json. The writer uses O_NOFOLLOW + 0o600, so a pre-planted symlink in /tmp cannot be used to clobber an arbitrary file. SECURITY: in shared-tmp environments (multi-tenant hosts), point CORESCOPE_INGESTOR_STATS at a private directory like /var/lib/corescope/ingestor-stats.json that only the corescope user can write to.", "corsAllowedOrigins": [], "_comment_corsAllowedOrigins": "Cross-origin allowlist for embed scenarios (#1369) AND for /ws WebSocket upgrades (#1793 — CSWSH defense per OWASP WebSocket Security Cheat Sheet). Exact-match origins, e.g. [\"https://blog.example.com\", \"https://embed.example.com\"]. Same-origin requests (Origin host == request Host) and non-browser clients (no Origin header) are always allowed for /ws. When empty (default), no Access-Control-* headers are sent and browsers enforce same-origin; cross-origin /ws upgrades are rejected. When non-empty, only the listed origins receive CORS headers, and Access-Control-Allow-Methods is limited to GET, HEAD, OPTIONS (the cross-domain surface is read-only — same-origin admin writes are unaffected). Use [\"*\"] to allow any origin for CORS XHR (NOT recommended for write-capable deployments); note that \"*\" is deliberately NOT honored for /ws upgrades — list explicit origins to permit cross-origin WebSocket clients (OWASP guidance: avoid wildcards in CSWSH defense). Operators can override per-deployment with the CORS_ALLOWED_ORIGINS environment variable (comma-separated). No credentialed CORS is enabled. To embed the map or channels pages cross-domain, add the embedding origin here and use the URL pattern '/#/map?embed=1' or '/#/channels?embed=1' — embed mode hides the top-nav, bottom-nav, and side drawer for full-bleed iframe rendering.", + "webSocket": { + "maxConnsPerIP": 0, + "_comment_maxConnsPerIP": "#1794 - concurrent /ws connections allowed from one client address. 0 = off, and that is the shipped default ON PURPOSE: carrier-grade NAT puts thousands of unrelated mobile subscribers behind one public IPv4, so a low cap refuses real visitors while a scraper simply rents more addresses. Set it only if you know your audience is not behind shared NAT.", + "upgradesPerMinPerIP": 30, + "_comment_upgradesPerMinPerIP": "#1794 - handshakes per minute from one client address. On by default. Safe under CGNAT: a real client upgrades a handful of times per minute even while reconnecting, so 30 leaves ordinary traffic alone while flattening a scraper reconnect loop. Set 0 to disable.", + "trustedProxies": [], + "_comment_trustedProxies": "#1794 - addresses or CIDRs of YOUR reverse proxy. REQUIRED for the per-IP limits to do anything behind nginx/Caddy/Traefik/ingress: without it every visitor shares the proxy's address, so the limits are skipped and one warning is logged at startup. X-Forwarded-For is believed only from these addresses, because anyone else can forge it. Never list an address you do not control.", + "deny": [], + "_comment_deny": "#1794 - addresses or CIDRs refused at the /ws upgrade, before the handshake. Accepts both \"1.2.3.4\" and \"1.2.3.0/24\". Applies even behind an unconfigured proxy, since it is your explicit instruction. Rejections are counted in /api/stats under websocket.rejectedDeny." + }, "https": { "cert": "/path/to/cert.pem", "key": "/path/to/key.pem",