cubeops: key the login rate limiter on the real peer IP, and bound the failure map

[dwin-gharibi]

Closes #1377.

Motivation

clientIP read X-Forwarded-For directly, so the limiter's bucket key was fully attacker-controlled:
rotating the header gave a fresh full-quota bucket per request and the 5-per-minute limit never fired.
Varying only leading whitespace on a single address had the same effect. Reading the header by hand also
bypassed gin's SetTrustedProxies machinery, so there was no notion of which proxies are allowed to
assert a client IP.

Separately, the failure map was unbounded: pruning only ever touched the key being accessed, and there was
no sweeper, so N distinct header values meant N permanent entries.

What this changes

1. internal/auth/ratelimit.go — clientIP delegates to c.ClientIP(). gin resolves the client IP
against the configured trusted-proxy set, so a forwarded header is honoured only when it comes from a
proxy the operator has declared. Untrusted callers are keyed on their real TCP peer address.

2. internal/config/config.go — new TrustedProxies []string (trusted_proxies in YAML,
CUBE_OPS_TRUSTED_PROXIES as a comma-separated env var), defaulting to empty.

3. internal/server/server.go — r.SetTrustedProxies(s.cfg.TrustedProxies) during router
construction. An invalid entry is logged and the router falls back to trusting nothing, which is the safe
direction.

4. internal/auth/ratelimit.go — bounded map. When the map reaches sweepThreshold (4096) entries,
recordFailure sweeps every key and drops those whose window has expired. Combined with (1), the key
space is now the set of real peer IPs seen within the window, which traffic bounds naturally.

No comment changes.

Deployment impact — please read before merging

This changes behaviour for anyone running CubeOps behind a reverse proxy. gin's default is to trust
all proxies; the new default trusts none. Concretely:

• Direct exposure (no proxy): improvement, no action needed. Spoofing no longer works.
• Behind nginx/LB, trusted_proxies unset: every request is attributed to the proxy's IP, so all
  clients share one bucket and 5 failures from anyone briefly blocks logins for everyone. Operators
  must set CUBE_OPS_TRUSTED_PROXIES (or trusted_proxies) to the proxy address/CIDR.

I chose the fail-closed default deliberately — the alternative (default to trusting all proxies) keeps the
bypass. The Helm chart and one-click templates should set trusted_proxies to the ingress/CubeProxy CIDR
as a follow-up; I did not touch deployment manifests here to keep this PR to one component.

Testing

New: CubeOps/internal/auth/ratelimit_xff_test.go, driving the real LoginRateLimit() middleware and
markLoginFailure through a gin router.

• TestRotatingForwardedForCannotBypassTheLimiter — 40 attempts from one peer with 40 different header
  values; the limiter must still block.
• TestWhitespacePaddedForwardedForCannotBypassTheLimiter — the padding variant.
• TestLimiterStillFiresWithoutForwardedFor — baseline, so a broken test cannot pass vacuously.
• TestDistinctPeerIPsGetIndependentBuckets — one peer being blocked must not block another.
• TestTrustedProxyForwardedForIsHonoured — with the proxy in SetTrustedProxies, two clients behind it
  are still limited independently. This is the case that would break if the fix had simply ignored the
  header.
• TestSweepBoundsTheFailureMap — the map stays at or below sweepThreshold.

Red/green — with clientIP reverted to the header-reading version (keeping the rest so the package still
compiles):

--- FAIL: TestRotatingForwardedForCannotBypassTheLimiter
    ratelimit_xff_test.go:68: rotating X-Forwarded-For bypassed the limiter entirely
--- FAIL: TestWhitespacePaddedForwardedForCannotBypassTheLimiter
    ratelimit_xff_test.go:89: whitespace-padded X-Forwarded-For bypassed the limiter

with the fix:

$ go test ./internal/auth/
ok  github.com/tencentcloud/CubeSandbox/CubeOps/internal/auth  2.336s

$ go test ./...
11 packages ok, 0 failures

CI gates checked locally:

• gofmt -l ./internal — clean (fmt-check).
• go build ./... — clean.
• go test ./... — 11 ok (unit-test-check → make cubeops-test).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

The sweep is never exercised by this test. The generated key only varies two octets (i%10 and (i/10)%10), so the loop produces at most 100 distinct IPs (10.9.0.0–10.9.9.9). len(l.failures) never reaches sweepThreshold (4096), so the sweepLocked branch in recordFailure never runs, and the final size > sweepThreshold assertion passes vacuously — the test would pass even if sweepLocked were deleted entirely. To actually exercise the sweep, generate unique keys (e.g. fmt.Sprintf("10.0.%d.%d", i>>8, i&0xff)), and assert the map is reduced after the sleep / that the sweep ran.

[cubesandboxbot]

sweepThreshold is a sweep trigger, not a cap. The sweep only removes expired entries, so during an ongoing distributed attack (all entries live, within the window) the map keeps growing past 4096 and every subsequent recordFailure pays a full O(map) sweep while holding l.mu — CPU + lock-contention amplification under exactly the attack this limiter exists to blunt. The PR description's claim that "the map stays at or below sweepThreshold" holds only once entries expire. Consider evicting when the map exceeds the threshold (not just sweeping), or document that this bounds steady-state memory rather than peak.

[cubesandboxbot]

This is a behavior change for every handler, not just the login limiter: gin's default (used before this PR) trusts all proxies and honors X-Forwarded-For; SetTrustedProxies with an empty list trusts none. All in-repo deployments proxy CubeOps through nginx that sets X-Forwarded-For (Helm chart deploy/kubernetes/chart/templates/ops.yaml + WebUI nginx, one-click deploy/one-click/webui/nginx.conf), and none set CUBE_OPS_TRUSTED_PROXIES/trusted_proxies. So with the default config, every login is keyed on the nginx pod IP: 5 failed attempts from anyone block logins for all users behind that proxy for a minute, and requestLogger's CallerIP now logs the proxy IP. The PR body acknowledges this, but the chart/one-click templates and config.example.yaml should ship with this change (or as a tracked release blocker) so the fail-closed default doesn't land as a regression.

[cubesandboxbot]

Review of PR #1378 — cubeops: key the login rate limiter on the real peer IP, and bound the failure map

AI-generated review. Not a human approval.

Overall

The core fix is correct and well-motivated. Replacing the hand-rolled X-Forwarded-For parsing with gin's c.ClientIP() and a fail-closed SetTrustedProxies (default: trust none) genuinely closes the attacker-controlled bucket-key bypass: a client that isn't a declared proxy can no longer rotate the header to get a fresh full-quota bucket, and gin's trusted-proxy machinery is what decides when a header is honourable. The bounded-map sweep/eviction logic is also correct — I traced the invariants:

• The map never exceeds sweepThreshold (4096): at each recordFailure, if len >= 4096, expired keys are swept; if still >= 4096, the oldest ~2048 are evicted, then the new failure is appended, so the high-water mark stays at 4096.
• Slices stay time-ordered (appends are monotonic via time.Now()'s monotonic clock), so fails[len(fails)-1] is correctly the most-recent failure for eviction ordering.
• No nil-slice panics on the recordFailure path even when the key was just evicted (nil[:0] + append works).
• TestExpiredEntriesAreSweptOnceTheThresholdIsReached is timing-robust (40 ms sleep vs a 20 ms window).

The new XFF regression tests (TestRotatingForwardedForCannotBypassTheLimiter, TestWhitespacePaddedForwardedForCannotBypassTheLimiter, TestLimiterStillFiresWithoutForwardedFor, TestDistinctPeerIPsGetIndependentBuckets, TestTrustedProxyForwardedForIsHonoured) are well-designed and would fail against the old clientIP implementation. I do not see a remaining spoofing bypass: with no trusted proxies, c.ClientIP() returns the real TCP peer, which an off-path attacker cannot forge.

Findings below, most relevant first.

Findings

1. TestEvictionKeepsTheMostRecentAttackers never exercises the eviction path (CONFIRMED, test coverage)

sweepThreshold is 4096. The test fills sweepThreshold-6 = 4090 keys, adds 5 for recent (map = 4095), and then calls recordFailure(distinctIP(sweepThreshold)) at line 195 with len(l.failures) == 4095 < sweepThreshold — so neither sweepLocked nor evictOldestLocked runs, and the isBlocked(recent) assertion on line 197 checks a map that was never evicted. The test would pass even if evictOldestLocked were replaced with a delete-everything implementation, so the "most recent attackers are preserved" property it claims to verify is unverified. TestLiveEntriesAreEvictedSoTheMapStaysBounded does verify boundedness, but not the eviction policy. To hit the branch, the map must be at >= sweepThreshold at the entry of a recordFailure call (e.g. fill to sweepThreshold-5, add the recent failures to reach 4096, then record one more distinct IP to trigger sweep/evict) and then assert recent is still blocked.

2. Default deployment ships with a shared login bucket behind any proxy (callout, availability)

With trusted_proxies empty everywhere in this PR (Helm values.yaml, env.example, cubeops-start.sh), the default All-in-One and Helm-chart deployments (WebUI nginx → CubeOps) key the limiter on the proxy's single source IP: every client shares one bucket. An unauthenticated caller can then lock out all logins behind that proxy for a minute with 5 failed attempts — and repeat indefinitely to keep logins blocked. This is the documented, deliberate fail-closed trade-off (it correctly stops the XFF bypass, which is the worse outcome), but since this PR touches the deployment manifests anyway, wiring the actual ingress/CubeProxy CIDR (or at least the All-in-One bridge gateway) for the default path would avoid shipping a known availability regression as the default. At minimum, the upgrade notes should call this out loudly.

3. validateTrustedProxies doesn't trim entries; the env-var path does (CONFIRMED, low)

splitAndTrim trims comma-separated CUBE_OPS_TRUSTED_PROXIES entries, but validateTrustedProxies passes YAML entries straight to net.ParseCIDR/net.ParseIP. A YAML entry such as - " 10.0.0.1 " is rejected with "not an IP address or CIDR block" while the identical value via the env var is accepted. Trimming in the validator (or reusing splitAndTrim) would make the two paths consistent.

4. gin expands a bare 127.0.0.1 trusted proxy to the whole 127.0.0.0/8 (note)

gin's prepareTrustedCIDRs special-cases loopback addresses into a /8 rather than a /32, so trusted_proxies: ["127.0.0.1"] trusts every loopback source, not just 127.0.0.1. Practical risk is low (only the local host can source loopback traffic), and in All-in-One mode the WebUI nginx connects from the Docker bridge gateway rather than loopback, so 127.0.0.1 isn't the right entry there anyway — but the config docs could note this so operators aren't surprised.

Minor notes (no action required)

• The eviction policy can drop a currently-blocked IP's key (one that went quiet after hitting the limit) once the map is under sustained pressure from ≥4096 distinct live IPs, forgetting its failures. This is the intended bounded-memory trade-off under a distributed attack and doesn't meaningfully change the security posture (an attacker with that many IPs doesn't need to reset the victim's block).
• PR description mentions TestSweepBoundsTheFailureMap, but the shipped test is split into TestExpiredEntriesAreSweptOnceTheThresholdIsReached and TestLiveEntriesAreEvictedSoTheMapStaysBounded — cosmetic drift only.
• SetTrustedProxies in server.go is defensive dead code given Load() already validated the entries; the nil fallback is the safe direction, so no change needed.

[cubesandboxbot]

Medium — this default re-opens the bypass this PR fixes, for any peer on a private range.

gin treats any peer whose TCP source address falls in the trusted set as a proxy and honors its X-Forwarded-For verbatim. In the chart, CubeOps is a ClusterIP service, so the peers that can reach it are not just the webui nginx pod but every pod in the cluster — including tenant sandbox pods, which are the untrusted workload in this product (and in a default cluster there is no NetworkPolicy to stop them). For such a peer, sending X-Forwarded-For: <anything> keys each request to a fresh rate-limit bucket — the exact bypass the PR fixes. The same applies to the one-click default (127.0.0.1,::1,172.16.0.0/12), where sandbox containers on the Docker bridge can reach the host's 0.0.0.0:3010 directly.

Consider defaulting trustedProxies to empty (fail closed) and requiring the operator to pin the actual ingress/pod CIDR, and/or adding a NetworkPolicy so tenant pods can't reach the ops ClusterIP at all. The comment already acknowledges narrowing this — but it ships as the default, so the shipped behavior is "trust all private networks", which materially weakens the security guarantee of the fix.

[cubesandboxbot]

Low — this is well-formedness-only validation; 0.0.0.0/0 and ::/0 pass and silently restore "trust everyone".

net.ParseCIDR("0.0.0.0/0") and ::/0 both succeed, so either value passes this check and then makes gin trust every proxy — the pre-fix behavior — undoing the fix while passing config validation. Note also that gin's documented "*" sentinel for the same thing is rejected here with a confusing "not an IP address or CIDR block" error. Consider rejecting explicit trust-all entries (or at least documenting that 0.0.0.0/0/::/0 are equivalent to "*" and must not be used), and mentioning "*" in the error message.

[cubesandboxbot]

The eviction path is never exercised here. sweepThreshold is 4096; this fill loop leaves the map at sweepThreshold-6 = 4090 entries, the 5 recent failures bring it to 4095, and the final recordFailure(distinctIP(sweepThreshold)) at line 195 runs with len(l.failures) == 4095 < sweepThreshold — so both sweepLocked and evictOldestLocked are skipped, and the isBlocked(recent) assertion on line 197 checks a map that was never evicted.

As written, this test would pass even if evictOldestLocked were replaced with a delete-everything implementation, so the "most recent attackers are preserved" property it claims to verify is unverified. (TestLiveEntriesAreEvictedSoTheMapStaysBounded verifies the map stays bounded but not the eviction policy.)

To actually hit the branch, the map needs to be at >= sweepThreshold at the entry of a recordFailure call — e.g. fill to sweepThreshold-5, add the recent failures to reach 4096, then record one more distinct IP to trigger sweep/evict, and assert recent is still blocked.

[cubesandboxbot]

Worth an explicit release callout: with trusted_proxies empty everywhere in this PR (Helm chart, one-click templates, systemd script), the default All-in-One and Helm deployments (WebUI nginx → CubeOps, all requests sharing one source IP) key the login limiter on a single proxy IP — every client shares one bucket. An unauthenticated caller can then block all logins behind that proxy for a minute with 5 failed attempts, and repeat indefinitely.

The fail-closed default is the right call and correctly stops the XFF bypass, but since this PR already touches the deployment manifests, wiring the actual ingress/CubeProxy CIDR (or at least the All-in-One bridge gateway) for the default path would avoid shipping a known availability regression; at minimum the upgrade/breaking-change notes should state this loudly.

[cubesandboxbot]

validateTrustedProxies passes entries straight to net.ParseCIDR/net.ParseIP without trimming, while the env-var path (splitAndTrim) trims. A YAML entry like - " 10.0.0.1 " is therefore rejected with "not an IP address or CIDR block" even though the identical value via CUBE_OPS_TRUSTED_PROXIES would be accepted. Trimming here (or reusing splitAndTrim) would make the two config paths consistent.